On Sun, Nov 24, 2024 at 7:00 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Sun, 24 Nov 2024 at 05:44, Christian Brauner <brauner@xxxxxxxxxx> 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..
Thanks,
Amir.
