MIME-Version: 1.0
In-Reply-To: <CAHC9VhTg8mMHzdSPbpxvOQCWxuNuXzR7c6FJOg5+XGb-PYemRw@mail.gmail.com>
References: <20230116212105.1840362-1-mjguzik@gmail.com> <20230116212105.1840362-2-mjguzik@gmail.com>
 <CAHC9VhSKEyyd-s_j=1UbA0+vOK7ggyCp6e-FNSG7XVYvCxoLnA@mail.gmail.com>
 <CAGudoHF+bg0qiq+ByVpysa9t8J=zpF8=d1CqDVS5GmOGpVM9rQ@mail.gmail.com>
 <CAHC9VhTnpWKnKRu3wFTNfub_qdcDePdEXYZWOpvpqL0fcfS_Uw@mail.gmail.com>
 <CAGudoHEWQJKMS=pL9Ate4COshgQaC-fjQ2RN3LiYmdS=0MVruA@mail.gmail.com>
 <CAHC9VhSYg-BbJvNBZd3dayYCf8bzedASoidnX23_i4iK7P-WxQ@mail.gmail.com>
 <CAHk-=wiG5wdWrx2uXRK3-i31Zp416krnu_KjmBbS3BVkiAUXLQ@mail.gmail.com> <CAHC9VhTg8mMHzdSPbpxvOQCWxuNuXzR7c6FJOg5+XGb-PYemRw@mail.gmail.com>
From:   Mateusz Guzik <mjguzik@gmail.com>
Date:   Wed, 25 Jan 2023 16:00:19 +0100
Message-ID: <CAGudoHG-42ziSNT0g8asRj8iGzx-Gn=ETZuXkswER3Daov37=A@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] vfs: avoid duplicating creds in faccessat if possible
To:     Paul Moore <paul@paul-moore.com>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        viro@zeniv.linux.org.uk, serge@hallyn.com,
        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 1/24/23, Paul Moore <paul@paul-moore.com> 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.

-- 
Mateusz Guzik <mjguzik gmail.com>