Re: [RFC PATCH net-next 10/12] io_uring/opaque: add IORING_OPAQUE_CREATE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] io_uring/opaque: double fetch of sqe->addr3 in
  io_opaque_prep()
- [Medium] io_uring/opaque: active memcg kept for user copy in
  io_opaque_create_build()
--

commit 990f7cf758f745b1bd5de4e8984b2d7a6df26c8e
Author: Alex Markuze <amarkuze@redhat.com>

io_uring/opaque: add IORING_OPAQUE_CREATE

This commit adds the IORING_OPAQUE_CREATE operation to IORING_OP_OPAQUE_OBJ,
allowing applications to construct immutable opaque objects synchronously from
userspace memory.

> diff --git a/io_uring/opaque.c b/io_uring/opaque.c
> index 4610e212a8005..78ae19a6ad82f 100644
> --- a/io_uring/opaque.c
> +++ b/io_uring/opaque.c
> @@ -1884,7 +1884,6 @@ int io_opaque_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  	    READ_ONCE(sqe->buf_index) || READ_ONCE(sqe->__pad2[0]) ||
>  	    cmd > IORING_OPAQUE_AVAILABLE ||
>  	    cmd == IORING_OPAQUE_COMPACT || cmd == IORING_OPAQUE_SET_POLICY ||
> -	    cmd == IORING_OPAQUE_CREATE ||
>  	    (req->flags & (REQ_F_CQE_SKIP | REQ_F_FIXED_FILE)))
>  		return -EINVAL;
>  	op = io_opaque_req_alloc(req, store);
> @@ -1899,6 +1898,21 @@ int io_opaque_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  	op->offset = READ_ONCE(sqe->off);
>  	op->length = READ_ONCE(sqe->len);
>  	op->addr = u64_to_user_ptr(READ_ONCE(sqe->addr3));
> +	if (cmd == IORING_OPAQUE_CREATE) {
> +		u64 pointer = READ_ONCE(sqe->addr3);
> +
> +		if (op->handle || op->offset || !op->length ||
> +		    op->length > INT_MAX || !pointer)
> +			return -EINVAL;
> +		if (op->length > store->config.max_object_size ||
> +		    ALIGN((u64)op->length, PAGE_SIZE) > io_opaque_rx_limit(store))
> +			return -EMSGSIZE;
> +		if (check_add_overflow(pointer, (u64)op->length, &end) ||
> +		    pointer > ULONG_MAX || end > ULONG_MAX ||
> +		    !access_ok(op->addr, op->length))
> +			return -EFAULT;
> +		return 0;
> +	}

[Severity: Medium]
Does reading sqe->addr3 twice from the shared submission ring allow op->addr
and pointer to diverge if userspace modifies the SQE concurrently?

The non-NULL, overflow, and 32-bit truncation checks validate the second read
in pointer, whereas access_ok() and the later copy_from_user() in
io_opaque_create_build() use op->addr from the first read. Reading sqe->addr3
once into a local u64 and deriving op->addr from it would keep the checks and
copy consistent.

> @@ -2046,12 +2060,110 @@ static int io_opaque_stat(struct io_opaque_req *op)
>  	return io_opaque_stat_copy(op, &stat, sizeof(stat));
>  }
>  
> +/* Construction is synchronous under the ring lock, like READ_OBJECT copies. */
> +static int io_opaque_create_build(struct io_opaque_req *op)
> +{
> +	struct io_opaque_store *store = op->store;
> +	struct mem_cgroup *old = set_active_memcg(store->memcg);
> +	u64 done = 0;
> +	int ret = 0;
> +
> +	while (done < op->length) {
> +		u64 pages_left = DIV_ROUND_UP_ULL(op->length - done, PAGE_SIZE);
> +		struct io_opaque_extent *extent;
> +		unsigned int order;
> +		struct page *page;
> +		u32 chunk, written = 0;
> +
> +		page = io_opaque_dest_page(store, pages_left, &order);
> +		if (!page) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +		chunk = min_t(u64, op->length - done, PAGE_SIZE << order);
> +		extent = io_opaque_extent_new(store, page, 0, chunk, done, true);
> +		if (IS_ERR(extent)) {
> +			put_page(page);
> +			ret = PTR_ERR(extent);
> +			goto out;
> +		}
> +		op->create_reserved -= page_size(page);
> +		extent->backing->copied = true;
> +		list_add_tail(&extent->list, &op->data->extents);
> +		while (written < chunk) {
> +			u32 bytes = min_t(u32, chunk - written, PAGE_SIZE);
> +			unsigned long left;
> +			void *mapped;
> +
> +			mapped = kmap_local_page(page + (written >> PAGE_SHIFT));
> +			left = copy_from_user(mapped, op->addr + done + written, bytes);
> +			kunmap_local(mapped);

[Severity: Medium]
Does keeping store->memcg active across copy_from_user() charge the caller's
user page faults to store->memcg?

If copy_from_user() faults on unpopulated user pages when store->memcg
differs from the calling task's memcg (for instance on an imported store),
filemap page-cache allocations and __GFP_ACCOUNT fault allocations such as
user page tables are charged to active_memcg() (store->memcg) rather than the
faulting task's cgroup, and remain charged after the opaque object is freed.
Scoping set_active_memcg(store->memcg) around the kernel allocations (as
io_opaque_frame_import() does) would avoid charging user faults to the store.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110132006.3280986-1-amarkuze@redhat.com?part=10