Re: [PATCH 1/3] io_uring: don't use req->work.creds for inline requests

[Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>]

hi,

> On 26/05/2020 09:43, Xiaoguang Wang wrote:
>> In io_init_req(), if uers requires a new credentials, currently we'll
>> save it in req->work.creds, but indeed io_wq_work is designed to describe
>> needed running environment for requests that will go to io-wq, if one
>> request is going to be submitted inline, we'd better not touch io_wq_work.
>> Here add a new 'const struct cred *creds' in io_kiocb, if uers requires a
>> new credentials, inline requests can use it.
>>
>> This patch is also a preparation for later patch.
> 
> What's the difference from keeping only one creds field in io_kiocb (i.e.
> req->work.creds), but handling it specially (i.e. always initialising)? It will
> be a lot easier than tossing it around.
> 
> Also, the patch doubles {get,put}_creds() for sqe->personality case, and that's
> extra atomics without a good reason.
You're right, thanks.
The original motivation for this patch is that it's just a preparation later patch
"io_uring: avoid whole io_wq_work copy for inline requests", I can use io_wq_work.func
to determine whether to drop io_wq_work in io_req_work_drop_env(), so if io_wq_work.func
is NULL, I don't want io_wq_work has a valid creds.
I'll look into whether we can just assign req->creds's pointer to io_wq_work.creds to
reduce the atomic operations.

Regards,
Xiaoguang Wang

> 
>>
>> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
>> ---
>>   fs/io_uring.c | 23 +++++++++++++++--------
>>   1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 2af87f73848e..788d960abc69 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -635,6 +635,7 @@ struct io_kiocb {
>>   	unsigned int		flags;
>>   	refcount_t		refs;
>>   	struct task_struct	*task;
>> +	const struct cred	*creds;
>>   	unsigned long		fsize;
>>   	u64			user_data;
>>   	u32			result;
>> @@ -1035,8 +1036,10 @@ static inline void io_req_work_grab_env(struct io_kiocb *req,
>>   		mmgrab(current->mm);
>>   		req->work.mm = current->mm;
>>   	}
>> -	if (!req->work.creds)
>> +	if (!req->creds)
>>   		req->work.creds = get_current_cred();
>> +	else
>> +		req->work.creds = get_cred(req->creds);
>>   	if (!req->work.fs && def->needs_fs) {
>>   		spin_lock(&current->fs->lock);
>>   		if (!current->fs->in_exec) {
>> @@ -1368,6 +1371,9 @@ static void __io_req_aux_free(struct io_kiocb *req)
>>   	if (req->flags & REQ_F_NEED_CLEANUP)
>>   		io_cleanup_req(req);
>>   
>> +	if (req->creds)
>> +		put_cred(req->creds);
>> +
>>   	kfree(req->io);
>>   	if (req->file)
>>   		io_put_file(req, req->file, (req->flags & REQ_F_FIXED_FILE));
>> @@ -5673,13 +5679,13 @@ static void __io_queue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>   again:
>>   	linked_timeout = io_prep_linked_timeout(req);
>>   
>> -	if (req->work.creds && req->work.creds != current_cred()) {
>> +	if (req->creds && req->creds != current_cred()) {
>>   		if (old_creds)
>>   			revert_creds(old_creds);
>> -		if (old_creds == req->work.creds)
>> +		if (old_creds == req->creds)
>>   			old_creds = NULL; /* restored original creds */
>>   		else
>> -			old_creds = override_creds(req->work.creds);
>> +			old_creds = override_creds(req->creds);
>>   	}
>>   
>>   	ret = io_issue_sqe(req, sqe, true);
>> @@ -5970,11 +5976,12 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
>>   
>>   	id = READ_ONCE(sqe->personality);
>>   	if (id) {
>> -		req->work.creds = idr_find(&ctx->personality_idr, id);
>> -		if (unlikely(!req->work.creds))
>> +		req->creds = idr_find(&ctx->personality_idr, id);
>> +		if (unlikely(!req->creds))
>>   			return -EINVAL;
>> -		get_cred(req->work.creds);
>> -	}
>> +		get_cred(req->creds);
>> +	} else
>> +		req->creds = NULL;
>>   
>>   	/* same numerical values with corresponding REQ_F_*, safe to copy */
>>   	req->flags |= sqe_flags;
>>
>