On Mon, Apr 28, 2025 at 3:27 AM Pavel Begunkov <asml.silence@xxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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.
The later patches only call it from task_work on the task that
submitted the ublk fetch request, so it should work. But I agree it's
a very subtle requirement that may be difficult to ensure for other
callers added in the future.
Best,
Caleb
>
> > + if (IS_ERR(file))
> > + return PTR_ERR(file);
> > + remote_ctx = file->private_data;
> > + if (!remote_ctx)
> > + return -EINVAL;
>
> nit: this check is not needed.
>
> > + }
> > +
> > + 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.
>
> > +
> > + do_reg_unreg_bvec(remote_ctx, buf, IO_URING_F_UNLOCKED, reg);
> > +
> > + if (!(issue_flags & IO_URING_F_UNLOCKED))
> > + mutex_lock(&ctx->uring_lock);
> > + }
> > +
> > + if (file)
> > + fput(file);
> > +
> > + return ret;
> > +}
> --
> Pavel Begunkov
>
