Re: [PATCH v2 2/2] vfs: avoid duplicating creds in faccessat if possible

[Mateusz Guzik]

On 1/25/23, Mateusz Guzik <mjguzik@xxxxxxxxx> wrote:
> On 1/24/23, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>> Although I'm looking at this again and realized that only
>> do_faccessat() calls access_override_creds(), so why not just fold the
>> new access_need_override_creds() logic into access_override_creds()?
>> Just have one function that takes the flag value, and returns an
>> old_cred/NULL pointer (or pass old_cred to the function by reference
>> and return an error code); that should still provide the performance
>> win Mateusz is looking for while providing additional safety against
>> out-of-sync changes.  I would guess the code would be smaller too.
>>
>
> It is unclear from the description if you are arguing for moving the new
> func into access_override_creds almost as is just put prior to existing
> code *or* mixing checks with assignments.
>
> static bool *access_override_creds(struct cred **ptr)
>         [snip]
>         if (!uid_eq(cred->fsuid, cred->uid) ||
>             !gid_eq(cred->fsgid, cred->gid))
>                 return false;
>         /* remaining checks go here as well */
>         [snip]
>
>         override_cred = prepare_creds();
>         if (!override_cred) {
>                 *ptr = NULL;
>                 return true;
>         }
>
>         override_cred->fsuid = override_cred->uid;
>         override_cred->fsgid = override_cred->gid;
>         [snip]
>
> If this is what you had in mind, I note it retains all the duplication
> except in one func body which I'm confident does not buy anything,
> provided the warning comment is added.
>
> At the same time the downside is that it uglifies error handling at the
> callsite, so I would say a net loss.
>
> Alternatively, if you want to somehow keep tests aroung assignments the
> code gets super hairy.
>
> But maybe you wanted something else?
>
> As I noted in another email this already got more discussion than it
> warrants.
>
> Addition of the warning comment makes sense, but concerns after that
> don't sound legitimate to me.
>

So I posted v3 with the comment, you are CC'ed.

I'm not going to further argue about the patch. If you want to write
your own variant that's fine with me, feel free to take my bench results
and denote they come from a similar version.

There is other stuff I want to post and unslowed access() helps making
the case. The CONFIG_INIT_ON_ALLOC_DEFAULT_ON option is enabled on
Debian, Ubuntu and Arch and as a side effect it zeroes bufs allocated
every time a path lookup is performed. As these are 4096 bytes in size
it is not pretty whatsoever and I'm confident not worth the hardening
promised by mandatory zeroing. Got patches to add exemption support for
caches to sort it out without disabling the opt.

-- 
Mateusz Guzik <mjguzik gmail.com>