Re: [PATCH v3 3/3] io_uring/zcrx: share an ifq between rings

[Pavel Begunkov <asml.silence@gmail.com>]

On 10/28/25 14:55, David Wei wrote:
> On 2025-10-27 03:20, Pavel Begunkov wrote:
>> On 10/26/25 17:34, David Wei wrote:
>>> Add a way to share an ifq from a src ring that is real i.e. bound to a
>>> HW RX queue with other rings. This is done by passing a new flag
>>> IORING_ZCRX_IFQ_REG_SHARE in the registration struct
>>> io_uring_zcrx_ifq_reg, alongside the fd of the src ring and the ifq id
>>> to be shared.
>>>
>>> To prevent the src ring or ifq from being cleaned up or freed while
>>> there are still shared ifqs, take the appropriate refs on the src ring
>>> (ctx->refs) and src ifq (ifq->refs).
>>>
>>> Signed-off-by: David Wei <dw@davidwei.uk>
>>> ---
>>>   include/uapi/linux/io_uring.h |  4 ++
>>>   io_uring/zcrx.c               | 74 ++++++++++++++++++++++++++++++++++-
>>>   2 files changed, 76 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c
>>> index 569cc0338acb..7418c959390a 100644
>>> --- a/io_uring/zcrx.c
>>> +++ b/io_uring/zcrx.c
> [...]
>>> @@ -541,6 +541,67 @@ struct io_mapped_region *io_zcrx_get_region(struct io_ring_ctx *ctx,
>>>       return ifq ? &ifq->region : NULL;
>>>   }
>>> +static int io_share_zcrx_ifq(struct io_ring_ctx *ctx,
>>> +                 struct io_uring_zcrx_ifq_reg __user *arg,
>>> +                 struct io_uring_zcrx_ifq_reg *reg)
>>> +{
>>> +    struct io_ring_ctx *src_ctx;
>>> +    struct io_zcrx_ifq *src_ifq;
>>> +    struct file *file;
>>> +    int src_fd, ret;
>>> +    u32 src_id, id;
>>> +
>>> +    src_fd = reg->if_idx;
>>> +    src_id = reg->if_rxq;
>>> +
>>> +    file = io_uring_register_get_file(src_fd, false);
>>> +    if (IS_ERR(file))
>>> +        return PTR_ERR(file);
>>> +
>>> +    src_ctx = file->private_data;
>>> +    if (src_ctx == ctx)
>>> +        return -EBADFD;
>>> +
>>> +    mutex_unlock(&ctx->uring_lock);
>>> +    io_lock_two_rings(ctx, src_ctx);
>>> +
>>> +    ret = -EINVAL;
>>> +    src_ifq = xa_load(&src_ctx->zcrx_ctxs, src_id);
>>> +    if (!src_ifq)
>>> +        goto err_unlock;
>>> +
>>> +    percpu_ref_get(&src_ctx->refs);
>>> +    refcount_inc(&src_ifq->refs);
>>> +
>>> +    scoped_guard(mutex, &ctx->mmap_lock) {
>>> +        ret = xa_alloc(&ctx->zcrx_ctxs, &id, NULL, xa_limit_31b, GFP_KERNEL);
>>> +        if (ret)
>>> +            goto err_unlock;
>>> +
>>> +        ret = -ENOMEM;
>>> +        if (xa_store(&ctx->zcrx_ctxs, id, src_ifq, GFP_KERNEL)) {
>>> +            xa_erase(&ctx->zcrx_ctxs, id);
>>> +            goto err_unlock;
>>> +        }
>>
>> It's just xa_alloc(..., src_ifq, ...);
>>
>>> +    }
>>> +
>>> +    reg->zcrx_id = id;
>>> +    if (copy_to_user(arg, reg, sizeof(*reg))) {
>>> +        ret = -EFAULT;
>>> +        goto err;
>>> +    }
>>
>> Better to do that before publishing zcrx into ctx->zcrx_ctxs
> 
> I can only do one of the two suggestions above. No valid id until
> xa_alloc() returns, so I either split xa_alloc()/xa_store() with
> copy_to_user() in between, or I do a single xa_alloc() and
> copy_to_user() after.

Makes sense, I'd do splitting then, at least this way it's
not exposing it to the user space for a brief moment.

-- 
Pavel Begunkov