Re: [PATCH 00/26] cred: rework {override,revert}_creds()

[Christian Brauner <brauner@kernel.org>]

On Mon, Nov 25, 2024 at 01:55:25PM +0100, Amir Goldstein wrote:
> On Sun, Nov 24, 2024 at 7:00 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Sun, 24 Nov 2024 at 05:44, Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > This series does all that. Afaict, most callers can be directly
> > > converted over and can avoid the extra reference count completely.
> > >
> > > Lightly tested.
> >
> > Thanks, this looks good to me. I only had two reactions:
> >
> >  (a) I was surprised that using get_new_cred() apparently "just worked".
> >
> > I was expecting us to have cases where the cred was marked 'const',
> > because I had this memory of us actively marking things const to make
> > sure people didn't play games with modifying the creds in-place (and
> > then casting away the const just for ref updates).
> >
> > But apparently that's never the case for override_creds() users, so
> > your patch actually ended up even simpler than I expected in that you
> > didn't end up needing any new helper for just incrementing the
> > refcount on a const cred.
> >
> >  (b) a (slight) reaction was to wish for a short "why" on the
> > pointless reference bumps
> >
> > partly to show that it was thought about, but also partly to
> > discourage people from doing it entirely mindlessly in other cases.
> >
> > I mean, sometimes the reference bumps were just obviously pointless
> > because they ended up being right next to each other after being
> > exposed, like the get/put pattern in access_override_creds().
> >
> > But in some other cases, like the aio_write case, I think it would
> > have been good to just say
> >
> >  "The refcount is held by iocb->fsync.creds that cannot change over
> > the operation"
> >
> > or similar. Or - very similarly - the binfmt_misc uses "file->f_cred",
> > and again, file->f_cred is set at open time and never changed, so we
> > can rely on it staying around for the file lifetime.
> >
> > I actually don't know if there were any exceptions to this (ie cases
> > where the source of the override cred could actually go away from
> > under us during the operation) where you didn't end up removing the
> > refcount games as a result.
> 
> I was asking myself the same question.
> 
> I see that cachefiles_{begin,end}_secure() bump the refcount, but they
> mostly follow a very similar pattern to the cases that do not bump the refcount,
> so I wonder if you left this out because they were hidden in those
> inline helpers
> or because of the non-trivial case of  cachefiles_determine_cache_security()
> which replaces the 'master' cache_creds?
> 
> Other that that, I stared at the creds code in nfsd_file_acquire_local() and
> nfsd_setuser() more than I would like to admit, with lines like:
> 
>         /* discard any old override before preparing the new set */
>         put_cred(revert_creds(get_cred(current_real_cred())));
> 
> And my only conclusion was this code is complicated enough,
> so it'd better not use borrowed creds..

I actually ported cachefilesd and and nfsd in v2. I simply missed them.