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

[Jens Axboe <axboe@kernel.dk>]

On 1/29/20 7:23 AM, Pavel Begunkov wrote:
>>>> 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's not though - one is dropped in that function, the other when the
request is freed. So we do need two references to it. With the proposed
change to keep the override_creds() variable local for that spot we
don't, and the get_cred() can then go.

> It could be I'm wrong with the statement above, need to recheck all this
> code to be sure.

I think you are :-)

> 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.

Agree, the NULL checks are safer and we should keep them.

Going through the rest of this thread, I'm making the following changes:

- ID must be > 0. I like that change, as we don't need an sqe flag to
  select personality then, and it also makes it obvious that id == 0 is
  just using current creds.

- Fixed the missing put_cred() in the teardown

- Use a local variable in io_submit_sqe() instead of assigning the
  creds to req->work.creds there

- Use cyclic idr allocation

I'm going to fold in as appropriate. If there are fixes needed on top of
that, let's do them separately.

-- 
Jens Axboe