Re: [PATCH v4 03/25] io_uring/kbuf: add support for kernel-managed buffer rings

[Joanne Koong <joannelkoong@gmail.com>]

On Thu, Feb 5, 2026 at 11:53 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Jan 16, 2026 at 03:30:22PM -0800, Joanne Koong wrote:
> > The implementation follows the same pattern as pbuf ring registration,
> > reusing the validation and buffer list allocation helpers introduced in
> > earlier refactoring. The IOBL_KERNEL_MANAGED flag marks buffer lists as
> > kernel-managed for appropriate handling in the I/O path.
>
> Do you have a man page or other documentation for the uapi somewhere?

No, but I will tidy up the liburing side changes I have for this and
add the documentation to liburing/man.
>
> > +int io_register_kmbuf_ring(struct io_ring_ctx *ctx, void __user *arg)
> > +{
> > +     struct io_uring_buf_reg reg;
> > +     struct io_buffer_list *bl;
> > +     int ret;
> > +
> > +     lockdep_assert_held(&ctx->uring_lock);
> > +
> > +     if (copy_from_user(&reg, arg, sizeof(reg)))
> > +             return -EFAULT;
> > +
> > +     ret = io_validate_buf_reg(&reg, 0);
> > +     if (ret)
> > +             return ret;
>
> Probably more a comment for patch 1, but wouldn't it make sense
> to combine copy from user and vaidation into a single helper?

I'll rename this function to io_copy_and_validate_buf_reg() and move
the copying into that.

>
> > +     ret = io_alloc_new_buffer_list(ctx, &reg, &bl);
> > +     if (ret)
> > +             return ret;
>
> Return the buffer list from io_alloc_new_buffer_list or an ERR_PTR
> to simplify this a bit?

Sounds good, I'll make this change.

>
> > +     ret = io_setup_kmbuf_ring(ctx, bl, &reg);
> > +     if (ret) {
> > +             kfree(bl);
> > +             return ret;
> > +     }
> > +
> > +     bl->flags |= IOBL_KERNEL_MANAGED;
>
> Should io_setup_kmbuf_ring set IOBL_KERNEL_MANAGED as it is the one
> creating the kernel managed buffers?

That's a good point, I'll move this line into io_setup_kmbuf_ring()
(and do the same for moving pbuf's IOBL_INC flag setting to
io_setup_pbuf_ring()).
>
> > +{
> > +     gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO | __GFP_NOWARN;
>
> Isn't this really a GFP_USER allocation and should account towardas the
> callers memory cgroup limit?

I think __GFP_ACCOUNT is what accounts the memory towards the caller's
mem cgroup limit and GFP_KERNEL_ACCOUNT sets that whereas GFP_USER
doesn't. GFP_USER sets __GFP_HARDWALL though which seems like it's
also useful, but elsewhere in the io uring code, GFP_KERNEL_ACCOUNT is
used for other userspace-facing allocations (eg pbufring's buffer
metadata). But maybe this should be GFP_USER | __GFP_ACCOUNT.

>
> > +     if (WARN_ON_ONCE(mr->pages || mr->ptr || mr->nr_pages))
> > +             return -EFAULT;
> > +
> > +     if (WARN_ON_ONCE(!nr_bufs || !buf_size))
> > +             return -EINVAL;
> > +
> > +     nr_pages = ((size_t)buf_size * nr_bufs) >> PAGE_SHIFT;
> > +     if (nr_pages > UINT_MAX)
> > +             return -E2BIG;
>
> This looks overflow prone, and probably should use check_mul_overflow.

Okay yeah I think you're right that the (size_t) casting only protects
against overflow on 64-bit systems but could still overflow 32-bit
systems. I'll change this to use check_mul_overflow.

Thanks,
Joanne
>