Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230116212105.1840362-1-mjguzik@gmail.com> <20230116212105.1840362-2-mjguzik@gmail.com>
In-Reply-To: <20230116212105.1840362-2-mjguzik@gmail.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Fri, 20 Jan 2023 15:45:34 -0500
Message-ID: <CAHC9VhSKEyyd-s_j=1UbA0+vOK7ggyCp6e-FNSG7XVYvCxoLnA@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] vfs: avoid duplicating creds in faccessat if possible
To:     Mateusz Guzik <mjguzik@gmail.com>
Cc:     viro@zeniv.linux.org.uk, serge@hallyn.com,
        torvalds@linux-foundation.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Jan 16, 2023 at 4:21 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> access(2) remains commonly used, for example on exec:
> access("/etc/ld.so.preload", R_OK)
>
> or when running gcc: strace -c gcc empty.c
> % time     seconds  usecs/call     calls    errors syscall
> ------ ----------- ----------- --------- --------- ----------------
>   0.00    0.000000           0        42        26 access
>
> It falls down to do_faccessat without the AT_EACCESS flag, which in turn
> results in allocation of new creds in order to modify fsuid/fsgid and
> caps. This is a very expensive process single-threaded and most notably
> multi-threaded, with numerous structures getting refed and unrefed on
> imminent new cred destruction.
>
> Turns out for typical consumers the resulting creds would be identical
> and this can be checked upfront, avoiding the hard work.
>
> An access benchmark plugged into will-it-scale running on Cascade Lake
> shows:
> test    proc    before  after
> access1 1       1310582 2908735  (+121%)  # distinct files
> access1 24      4716491 63822173 (+1353%) # distinct files
> access2 24      2378041 5370335  (+125%)  # same file

Out of curiosity, do you have any measurements of the impact this
patch has on the AT_EACCESS case when the creds do need to be
modified?

> The above benchmarks are not integrated into will-it-scale, but can be
> found in a pull request:
> https://github.com/antonblanchard/will-it-scale/pull/36/files
>
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
>
> v2:
> - fix current->cred usage warn reported by the kernel test robot
> Link: https://lore.kernel.org/all/202301150709.9EC6UKBT-lkp@intel.com/
> ---
>  fs/open.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/fs/open.c b/fs/open.c
> index 82c1a28b3308..3c068a38044c 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -367,7 +367,37 @@ COMPAT_SYSCALL_DEFINE6(fallocate, int, fd, int, mode, compat_arg_u64_dual(offset
>   * access() needs to use the real uid/gid, not the effective uid/gid.
>   * We do this by temporarily clearing all FS-related capabilities and
>   * switching the fsuid/fsgid around to the real ones.
> + *
> + * Creating new credentials is expensive, so we try to skip doing it,
> + * which we can if the result would match what we already got.
>   */
> +static bool access_need_override_creds(int flags)
> +{
> +       const struct cred *cred;
> +
> +       if (flags & AT_EACCESS)
> +               return false;
> +
> +       cred = current_cred();
> +       if (!uid_eq(cred->fsuid, cred->uid) ||
> +           !gid_eq(cred->fsgid, cred->gid))
> +               return true;
> +
> +       if (!issecure(SECURE_NO_SETUID_FIXUP)) {
> +               kuid_t root_uid = make_kuid(cred->user_ns, 0);
> +               if (!uid_eq(cred->uid, root_uid)) {
> +                       if (!cap_isclear(cred->cap_effective))
> +                               return true;
> +               } else {
> +                       if (!cap_isidentical(cred->cap_effective,
> +                           cred->cap_permitted))
> +                               return true;
> +               }
> +       }
> +
> +       return false;
> +}

I worry a little that with nothing connecting
access_need_override_creds() to access_override_creds() there is a bug
waiting to happen if/when only one of the functions is updated.

Given the limited credential changes in access_override_creds(), I
wonder if a better solution would be to see if we could create a
light(er)weight prepare_creds()/override_creds() that would avoid some
of the prepare_creds() hotspots (I'm assuming that is where most of
the time is being spent).  It's possible this could help improve the
performance of other, similar operations that need to modify task
creds for a brief, and synchronous, period of time.

Have you done any profiling inside of access_override_creds() to see
where the most time is spent?  Looking at the code I have some gut
feelings on the hotspots, but it would be good to see some proper data
before jumping to any conclusions.

>  static const struct cred *access_override_creds(void)
>  {
>         const struct cred *old_cred;
> @@ -436,7 +466,7 @@ static long do_faccessat(int dfd, const char __user *filename, int mode, int fla
>         if (flags & AT_EMPTY_PATH)
>                 lookup_flags |= LOOKUP_EMPTY;
>
> -       if (!(flags & AT_EACCESS)) {
> +       if (access_need_override_creds(flags)) {
>                 old_cred = access_override_creds();
>                 if (!old_cred)
>                         return -ENOMEM;
> --
> 2.34.1

-- 
paul-moore.com