Re: [PATCH v1 11/30] io_uring/kbuf: return buffer id in buffer selection

[Joanne Koong <joannelkoong@gmail.com>]

On Wed, Dec 3, 2025 at 1:54 PM Caleb Sander Mateos
<csander@purestorage.com> wrote:
>
> On Tue, Dec 2, 2025 at 4:36 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > Return the id of the selected buffer in io_buffer_select(). This is
> > needed for kernel-managed buffer rings to later recycle the selected
> > buffer.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> >  include/linux/io_uring/cmd.h   | 2 +-
> >  include/linux/io_uring_types.h | 2 ++
> >  io_uring/kbuf.c                | 7 +++++--
> >  3 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
> > index e1a75cfe57d9..dcc95e73f12f 100644
> > --- a/include/linux/io_uring_types.h
> > +++ b/include/linux/io_uring_types.h
> > @@ -109,6 +109,8 @@ struct io_br_sel {
> >                 void *kaddr;
> >         };
> >         ssize_t val;
> > +       /* id of the selected buffer */
> > +       unsigned buf_id;
>
> Looks like this could be unioned with val? I think val's size can be
> reduced to an int since only int values are assigned to it.
>

I'm not sure I see the advantage of this. imo I think it makes the
interface a bit more confusing, as val also represents the error code
and is logically its own separate entity from buf_id. I don't see the
struct io_br_sel being stored anywhere where it seems important to
save a few bytes, but maybe I'm missing something here.

> >  };
> >
> >
> > diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
> > index 8a94de6e530f..3ecb6494adea 100644
> > --- a/io_uring/kbuf.c
> > +++ b/io_uring/kbuf.c
> > @@ -239,6 +239,7 @@ static struct io_br_sel io_ring_buffer_select(struct io_kiocb *req, size_t *len,
> >         req->flags |= REQ_F_BUFFER_RING | REQ_F_BUFFERS_COMMIT;
> >         req->buf_index = buf->bid;
> >         sel.buf_list = bl;
> > +       sel.buf_id = buf->bid;
>
> This is userspace mapped, so probably should be using READ_ONCE() and
> reusing the value between req->buf_index and buf->bid? Looks like an
> existing bug that the reads of buf->bid and buf->addr aren't using
> READ_ONCE().

Agreed. I think that existing bug should be its own patch. Are you
planning to submit that fix? since you found that bug, I think you
should get the authorship for it, but if it's annoying for whatever
reason and you prefer not to, I can roll that up into this patchset.
EIther way, I'll rebase v2 on top of that and change this line to
"sel.buf_id = req->buf_index".

>
> >         if (bl->flags & IOBL_KERNEL_MANAGED)
> >                 sel.kaddr = (void *)buf->addr;
> >         else
> > @@ -262,10 +263,12 @@ struct io_br_sel io_buffer_select(struct io_kiocb *req, size_t *len,
> >
> >         bl = io_buffer_get_list(ctx, buf_group);
> >         if (likely(bl)) {
> > -               if (bl->flags & IOBL_BUF_RING)
> > +               if (bl->flags & IOBL_BUF_RING) {
> >                         sel = io_ring_buffer_select(req, len, bl, issue_flags);
> > -               else
> > +               } else {
> >                         sel.addr = io_provided_buffer_select(req, len, bl);
> > +                       sel.buf_id = req->buf_index;
>
> Could this cover both IOBL_BUF_RING and !IOBL_BUF_RING cases to avoid
> the additional logic in io_ring_buffer_select()?

Ah the next patch (patch 12/30: io_uring/kbuf: export
io_ring_buffer_select() [1]) exports io_ring_buffer_select() to be
directly called, so that was my motivation for having that sel.buf_id
assignment logic in io_ring_buffer_select(). I should have swapped the
ordering of those 2 patches to make that more clear, I'll do that for
v2.

[1] https://lore.kernel.org/linux-fsdevel/20251203003526.2889477-13-joannelkoong@gmail.com/

Thanks,
Joanne

>
> Best,
> Caleb
>
> > +               }
> >         }
> >         io_ring_submit_unlock(req->ctx, issue_flags);
> >         return sel;
> > --
> > 2.47.3
> >