Re: [RFC PATCH 3/7] io_uring: support to register bvec buffer to specified io_uring

[Ming Lei <ming.lei@redhat.com>]

On Mon, Apr 28, 2025 at 11:28:30AM +0100, Pavel Begunkov wrote:
> On 4/28/25 10:44, Ming Lei wrote:
> > Extend io_buffer_register_bvec() and io_buffer_unregister_bvec() for
> > supporting to register/unregister bvec buffer to specified io_uring,
> > which FD is usually passed from userspace.
> > 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >   include/linux/io_uring/cmd.h |  4 ++
> >   io_uring/rsrc.c              | 83 +++++++++++++++++++++++++++---------
> >   2 files changed, 67 insertions(+), 20 deletions(-)
> > 
> > diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
> > index 78fa336a284b..7516fe5cd606 100644
> > --- a/include/linux/io_uring/cmd.h
> > +++ b/include/linux/io_uring/cmd.h
> > @@ -25,6 +25,10 @@ struct io_uring_cmd_data {
> ...
> >   	io_ring_submit_lock(ctx, issue_flags);
> > -	ret = __io_buffer_unregister_bvec(ctx, buf);
> > +	if (reg)
> > +		ret = __io_buffer_register_bvec(ctx, buf);
> > +	else
> > +		ret = __io_buffer_unregister_bvec(ctx, buf);
> >   	io_ring_submit_unlock(ctx, issue_flags);
> >   	return ret;
> >   }
> > +
> > +static int io_buffer_reg_unreg_bvec(struct io_ring_ctx *ctx,
> > +				    struct io_buf_data *buf,
> > +				    unsigned int issue_flags,
> > +				    bool reg)
> > +{
> > +	struct io_ring_ctx *remote_ctx = ctx;
> > +	struct file *file = NULL;
> > +	int ret;
> > +
> > +	if (buf->has_fd) {
> > +		file = io_uring_register_get_file(buf->ring_fd, buf->registered_fd);
> 
> io_uring_register_get_file() accesses task private data and the request
> doesn't control from which task it's executed. IOW, you can't use the
> helper here. It can be iowq or sqpoll, but either way nothing is
> promised.

Good catch!

Actually ublk uring_cmd is guaranteed to be issued from user context.

We can enhance it by failing buffer register:

	if ((current->flags & PF_KTHREAD) || (issue_flags & IO_URING_F_IOWQ))
		return -EACCESS;

> 
> > +		if (IS_ERR(file))
> > +			return PTR_ERR(file);
> > +		remote_ctx = file->private_data;
> > +		if (!remote_ctx)
> > +			return -EINVAL;
> 
> nit: this check is not needed.

OK.

> 
> > +	}
> > +
> > +	if (remote_ctx == ctx) {
> > +		do_reg_unreg_bvec(ctx, buf, issue_flags, reg);
> > +	} else {
> > +		if (!(issue_flags & IO_URING_F_UNLOCKED))
> > +			mutex_unlock(&ctx->uring_lock);
> 
> We shouldn't be dropping the lock in random helpers, for example
> it'd be pretty nasty suspending a submission loop with a submission
> from another task.
> 
> You can try lock first, if fails it'll need a fresh context via
> iowq to be task-work'ed into the ring. see msg_ring.c for how
> it's done for files.

Looks trylock is better, will take this approach by returning -EAGAIN,
and let ublk driver retry.


Thanks, 
Ming