Re: [PATCH for-next v2 21/25] io_uring: add IORING_SETUP_SINGLE_ISSUER

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

On 6/15/22 10:41, Hao Xu wrote:
> On 6/14/22 22:37, Pavel Begunkov wrote:
>> Add a new IORING_SETUP_SINGLE_ISSUER flag and the userspace visible part
>> of it, i.e. put limitations of submitters. Also, don't allow it together
>> with IOPOLL as we're not going to put it to good use.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>   include/uapi/linux/io_uring.h |  5 ++++-
>>   io_uring/io_uring.c           |  7 +++++--
>>   io_uring/io_uring_types.h     |  1 +
>>   io_uring/tctx.c               | 27 ++++++++++++++++++++++++---
>>   io_uring/tctx.h               |  4 ++--
>>   5 files changed, 36 insertions(+), 8 deletions(-)
>>
[...]
>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>> index 15d209f334eb..4b90439808e3 100644
>> --- a/io_uring/io_uring.c
>> +++ b/io_uring/io_uring.c
>> @@ -3020,6 +3020,8 @@ static __cold void io_ring_ctx_free(struct io_ring_ctx *ctx)
>>       io_destroy_buffers(ctx);
>>       if (ctx->sq_creds)
>>           put_cred(ctx->sq_creds);
>> +    if (ctx->submitter_task)
>> +        put_task_struct(ctx->submitter_task);
>>       /* there are no registered resources left, nobody uses it */
>>       if (ctx->rsrc_node)
>> @@ -3752,7 +3754,7 @@ static int io_uring_install_fd(struct io_ring_ctx *ctx, struct file *file)
>>       if (fd < 0)
>>           return fd;
>> -    ret = io_uring_add_tctx_node(ctx);
>> +    ret = __io_uring_add_tctx_node(ctx, false);

                                             ^^^^^^

Note this one


>>       if (ret) {
>>           put_unused_fd(fd);
>>           return ret;
>> @@ -3972,7 +3974,8 @@ static long io_uring_setup(u32 entries, struct io_uring_params __user *params)
>>               IORING_SETUP_CLAMP | IORING_SETUP_ATTACH_WQ |
>>               IORING_SETUP_R_DISABLED | IORING_SETUP_SUBMIT_ALL |
>>               IORING_SETUP_COOP_TASKRUN | IORING_SETUP_TASKRUN_FLAG |
>> -            IORING_SETUP_SQE128 | IORING_SETUP_CQE32))
>> +            IORING_SETUP_SQE128 | IORING_SETUP_CQE32 |
>> +            IORING_SETUP_SINGLE_ISSUER))
>>           return -EINVAL;
>>       return io_uring_create(entries, &p, params);
>> diff --git a/io_uring/io_uring_types.h b/io_uring/io_uring_types.h
>> index aba0f8cd6f49..f6d0ad25f377 100644
>> --- a/io_uring/io_uring_types.h
>> +++ b/io_uring/io_uring_types.h
>> @@ -241,6 +241,7 @@ struct io_ring_ctx {
>>       /* Keep this last, we don't need it for the fast path */
>>       struct io_restriction        restrictions;
>> +    struct task_struct        *submitter_task;
>>       /* slow path rsrc auxilary data, used by update/register */
>>       struct io_rsrc_node        *rsrc_backup_node;
>> diff --git a/io_uring/tctx.c b/io_uring/tctx.c
>> index 6adf659687f8..012be261dc50 100644
>> --- a/io_uring/tctx.c
>> +++ b/io_uring/tctx.c
>> @@ -81,12 +81,32 @@ __cold int io_uring_alloc_task_context(struct task_struct *task,
>>       return 0;
>>   }
>> -int __io_uring_add_tctx_node(struct io_ring_ctx *ctx)
>> +static int io_register_submitter(struct io_ring_ctx *ctx)
>> +{
>> +    int ret = 0;
>> +
>> +    mutex_lock(&ctx->uring_lock);
>> +    if (!ctx->submitter_task)
>> +        ctx->submitter_task = get_task_struct(current);
>> +    else if (ctx->submitter_task != current)
>> +        ret = -EEXIST;
>> +    mutex_unlock(&ctx->uring_lock);
>> +
>> +    return ret;
>> +}
> 
> Seems we don't need this uring_lock:
> When we create a ring, we setup ctx->submitter_task before uring fd is
> installed so at that time nobody else can enter this code.
> when we enter this code later in io_uring_enter, we just read it.

Not really, we specifically don't set it just to the ring's
creator but to the first submitter. That's needed to be able to
create a ring in one task and pass it over to another.

-- 
Pavel Begunkov