Re: [PATCH 00/16] credentials guards: the easy cases

[Christian Brauner <brauner@kernel.org>]

On Mon, Nov 03, 2025 at 02:29:40PM +0100, Amir Goldstein wrote:
> On Mon, Nov 3, 2025 at 12:28 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > This converts all users of override_creds() to rely on credentials
> > guards. Leave all those that do the prepare_creds() + modify creds +
> > override_creds() dance alone for now. Some of them qualify for their own
> > variant.
> 
> Nice!
> 
> What about with_ovl_creator_cred()/scoped_with_ovl_creator_cred()?
> Is there any reason not to do it as well?

No, I don't think there is other than that the complexity of it warrants
a separate patch series.

When override_creds()/revert_creds() still was a reference count
bonanza, we struggled with two issues related to overlayfs:

(1) reference counting was sometimes very non-obvious
    (think:
    cred = get_cred(creator_cred);
    old_cred = override_cred(cred);
    put_cred(revert_creds(old_cred));
    put_cred(cred); or worse )

    and thus the credential override logic when creating files where you
    change the fsuid and you essentially override credentials _twice_
    lead to pretty twisted logic that wasn't necessarily clarified by
    the scope-based semantics.

    This problem is now resolved since my prior rework.

(2) The scope based cleanup did struggle in switch() statements that
    messed with the scope-based logic iirc. I don't have the details in
    my head right now anymore but basically this is why we originally
    punted on the original conversion so we wouldn't end up chasing bugs
    in two semantic changes done at the same time.

    I think we're ready to do (2) now.

What I tried in this series is to reduce the amount of scope switching
due to gotos and that's why I also moved some code around. It also helps
to visually clarify the guards when the scope is reduced by moving large
portions where work is done under a guard out to a helper. That's
especially true when the guard overrides credentials imho. That's
something I already aimed for during the first conversion.

> 
> I can try to clear some time for this cleanup.
> 
> For this series, feel free to add:
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> Thanks,
> Amir.
> 
> >
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> > Christian Brauner (16):
> >       cred: add {scoped_}with_creds() guards
> >       aio: use credential guards
> >       backing-file: use credential guards for reads
> >       backing-file: use credential guards for writes
> >       backing-file: use credential guards for splice read
> >       backing-file: use credential guards for splice write
> >       backing-file: use credential guards for mmap
> >       binfmt_misc: use credential guards
> >       erofs: use credential guards
> >       nfs: use credential guards in nfs_local_call_read()
> >       nfs: use credential guards in nfs_local_call_write()
> >       nfs: use credential guards in nfs_idmap_get_key()
> >       smb: use credential guards in cifs_get_spnego_key()
> >       act: use credential guards in acct_write_process()
> >       cgroup: use credential guards in cgroup_attach_permissions()
> >       net/dns_resolver: use credential guards in dns_query()
> >
> >  fs/aio.c                     |   6 +-
> >  fs/backing-file.c            | 147 ++++++++++++++++++++++---------------------
> >  fs/binfmt_misc.c             |   7 +--
> >  fs/erofs/fileio.c            |   6 +-
> >  fs/nfs/localio.c             |  59 +++++++++--------
> >  fs/nfs/nfs4idmap.c           |   7 +--
> >  fs/smb/client/cifs_spnego.c  |   6 +-
> >  include/linux/cred.h         |  12 ++--
> >  kernel/acct.c                |   6 +-
> >  kernel/cgroup/cgroup.c       |  10 ++-
> >  net/dns_resolver/dns_query.c |   6 +-
> >  11 files changed, 133 insertions(+), 139 deletions(-)
> > ---
> > base-commit: fea79c89ff947a69a55fed5ce86a70840e6d719c
> > change-id: 20251103-work-creds-guards-simple-619ef2200d22
> >
> >