Re: IORING_REGISTER_CREDS[_UPDATE]() and credfd_create()?

[Stefan Metzmacher <metze@samba.org>]

[-- Attachment #1.1: Type: text/plain, Size: 1590 bytes --]

Hi Pavel,

>>>>> @@ -3977,7 +3977,8 @@ static int io_req_defer_prep(struct io_kiocb *req,
>>>>>  		mmgrab(current->mm);
>>>>>  		req->work.mm = current->mm;
>>>>>  	}
>>>>> -	req->work.creds = get_current_cred();
>>>>> +	if (!req->work.creds)
>>>>> +		req->work.creds = get_current_cred();
>>>>>  
>>>>>  	switch (req->opcode) {
>>>>>  	case IORING_OP_NOP:
>>>>
>>>> The override_creds(personality_creds) has changed current->cred
>>>> and get_current_cred() will just pick it up as in the default case.
>>>>
>>>> This would make the patch much simpler and allows put_cred() to be
>>>> in io_put_work() instead of __io_req_aux_free() as explained above.
>>>>
>>>
>>> It's one extra get_current_cred(). I'd prefer to find another way to
>>> clean this up.
>>
>> As far as I can see it avoids a get_cred() in the IOSQE_PERSONALITY case
>> and the if (!req->work.creds) for both cases.
> 
> Great, that you turned attention to that! override_creds() is already
> grabbing a ref, so it shouldn't call get_cred() there.
> So, that's a bug.
> 
> It could be I'm wrong with the statement above, need to recheck all this
> code to be sure.
> 
> BTW, io_req_defer_prep() may be called twice for a req, so you will
> reassign it without putting a ref. It's safer to leave NULL checks. At
> least, until I've done reworking and fixing preparation paths.

Ok, but that would be already a bug in
"io_uring/io-wq: don't use static creds/mm assignments"
instead of logically being part of
"io_uring: support using a registered personality for commands"

metze


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]