Re: [PATCH v3 08/25] io_uring: add io_uring_cmd_fixed_index_get() and io_uring_cmd_fixed_index_put()

[Caleb Sander Mateos <csander@purestorage.com>]

On Thu, Jan 8, 2026 at 4:55 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Thu, Jan 8, 2026 at 12:44 PM Caleb Sander Mateos
> <csander@purestorage.com> wrote:
> >
> > On Thu, Jan 8, 2026 at 11:02 AM Caleb Sander Mateos
> > <csander@purestorage.com> wrote:
> > >
> > > On Mon, Dec 22, 2025 at 4:36 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > > >
> > > > Add two new helpers, io_uring_cmd_fixed_index_get() and
> > > > io_uring_cmd_fixed_index_put(). io_uring_cmd_fixed_index_get()
> > > > constructs an iter for a fixed buffer at a given index and acquires a
> > > > refcount on the underlying node. io_uring_cmd_fixed_index_put()
> > > > decrements this refcount. The caller is responsible for ensuring
> > > > io_uring_cmd_fixed_index_put() is properly called for releasing the
> > > > refcount after it is done using the iter it obtained through
> > > > io_uring_cmd_fixed_index_get().
> > > >
> > > > This is a preparatory patch needed for fuse-over-io-uring support, as
> > > > the metadata for fuse requests will be stored at the last index, which
> > > > will be different from the buf index set on the sqe.
> > > >
> > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > ---
> > > >  include/linux/io_uring/cmd.h | 20 +++++++++++
> > > >  io_uring/rsrc.c              | 65 ++++++++++++++++++++++++++++++++++++
> > > >  io_uring/rsrc.h              |  5 +++
> > > >  io_uring/uring_cmd.c         | 21 ++++++++++++
> > > >  4 files changed, 111 insertions(+)
> > > >
> > > > diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
> > > > index a63474b331bf..a141aaeb099d 100644
> > > > --- a/io_uring/rsrc.c
> > > > +++ b/io_uring/rsrc.c
> > > > @@ -1151,6 +1151,71 @@ int io_import_reg_buf(struct io_kiocb *req, struct iov_iter *iter,
> > > >         return io_import_fixed(ddir, iter, node->buf, buf_addr, len);
> > > >  }
> > > >
> > > > +int io_reg_buf_index_get(struct io_kiocb *req, struct iov_iter *iter,
> > > > +                        u16 buf_index, unsigned int off, size_t len,
> > > > +                        int ddir, unsigned issue_flags)
> > > > +{
> > > > +       struct io_ring_ctx *ctx = req->ctx;
> > > > +       struct io_rsrc_node *node;
> > > > +       struct io_mapped_ubuf *imu;
> > > > +       u64 addr;
> > > > +       int err;
> > > > +
> > > > +       io_ring_submit_lock(ctx, issue_flags);
> > > > +
> > > > +       node = io_rsrc_node_lookup(&ctx->buf_table, buf_index);
> > > > +       if (!node) {
> > > > +               io_ring_submit_unlock(ctx, issue_flags);
> > > > +               return -EINVAL;
> > > > +       }
> > > > +
> > > > +       node->refs++;
> > > > +
> > > > +       io_ring_submit_unlock(ctx, issue_flags);
> > > > +
> > > > +       imu = node->buf;
> > > > +       if (!imu) {
> > > > +               err = -EFAULT;
> > > > +               goto error;
> > > > +       }
> > > > +
> > > > +       if (check_add_overflow(imu->ubuf, off, &addr)) {
> > > > +               err = -EINVAL;
> > > > +               goto error;
> > > > +       }
> > > > +
> > > > +       err = io_import_fixed(ddir, iter, imu, addr, len);
> > > > +       if (err)
> > > > +               goto error;
> > > > +
> > > > +       return 0;
> > > > +
> > > > +error:
> > > > +       io_reg_buf_index_put(req, buf_index, issue_flags);
> > > > +       return err;
> > > > +}
> > > > +
> > > > +int io_reg_buf_index_put(struct io_kiocb *req, u16 buf_index,
> > > > +                        unsigned issue_flags)
> > > > +{
> > > > +       struct io_ring_ctx *ctx = req->ctx;
> > > > +       struct io_rsrc_node *node;
> > > > +
> > > > +       io_ring_submit_lock(ctx, issue_flags);
> > > > +
> > > > +       node = io_rsrc_node_lookup(&ctx->buf_table, buf_index);
> > >
> > > Hmm, I don't think it's safe to assume this node looked up by
> > > buf_index matches the one obtained in io_reg_buf_index_get(). Since
> > > the uring_lock is released between io_reg_buf_index_get() and
> > > io_reg_buf_index_put(), an intervening IORING_REGISTER_BUFFERS_UPDATE
> > > operation may modify the node at buf_index in the buf_table. That
> > > could result in a reference decrement on the wrong node, resulting in
> > > the new node being freed while still in use.
> > > Can we return the struct io_rsrc_node * from io_reg_buf_index_get()
> > > and pass it to io_reg_buf_index_put() in place of buf_index?
> >
> > Alternatively, store the struct io_rsrc_node * in the struct io_kiocb,
> > as io_find_buf_node() does? Then you wouldn't even need to call
> > io_reg_buf_index_put(); io_uring would automatically release the
> > buffer node reference upon completion of the request. The only reason
> > I can see why that wouldn't work is if a single fuse io_uring_cmd
> > needs to import multiple registered buffers. But it doesn't look like
> > that's the case from my quick skim of the fuse code.
>
> Good catch, I think I'll need to store that rsrc node pointer inside
> the fuse code then. I think we could store it inside the struct
> io_kiocb for the fuse use case but I think at an api level that ends
> up getting ugly with introducing unobvious relationships between
> io_uring_cmd_fixed_index_get() and io_uring_cmd_import_fixed().

Perhaps renaming it to something more similar
("io_uring_cmd_import_fixed_index()" or something) would help hint
that these 2 functions are related? But I'm fine with storing the
struct io_rsrc_node * externally if you'd prefer that.

Best,
Caleb