Re: [PATCH] cred: clarify usage of get_cred_rcu()

[Paul Moore]

On Fri, Feb 27, 2026 at 5:04 AM Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
> On Thu, Feb 26, 2026 at 09:18:29PM -0500, Paul Moore wrote:
> > On Fri, Feb 20, 2026 at 4:19 AM Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
> > >
> > > After being confused by looking at get_cred() and get_cred_rcu(), I
> > > figured out what's going on. Thus, add some comments to clarify how
> > > get_cred_rcu() works for the benefit of others looking in the future.
> > >
> > > Note that in principle we could add an assertion that non_rcu is zero in
> > > the failure path of atomic_long_inc_not_zero().
> >
> > That would be interesting to add a WARN_ON() there and see what
> > happens.  Hopefully nothing, but one never knows ;)  Have you tried
> > this?
>
> I tried just now. I put it on an Android phone, and it did not seem to
> be triggered after a few minute of usage.
>
> I can send a patch adding it if you would like?

It would need much more testing than running it on Android for a few
minutes before I would consider merging it :)  I suspect it's probably
not worth the effort, I just thought it would be mildly interesting to
see if anything tripped the assertion.

> > > Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> > > ---
> > >  include/linux/cred.h | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> >
> > ...
> >
> > > +/*
> > > + * get_cred_rcu - Get a reference on a set of credentials under rcu
> >
> > I agree this is a bit pedantic, but it looks like the bulk of the file
> > capitalizes RCU and technically that is correct as it is an acronym.
>
> Will do.
>
> > > + * @cred: The credentials to reference
> > > + *
> > > + * Get a reference on the specified set of credentials, or %NULL if the last
> > > + * refcount has already been put.
> > > + *
> > > + * This is used to obtain a reference under an rcu read lock.
> >
> > I would suggest a different description:
> >
> > "Get a reference to the specified set of credentials and return a
> > pointer to the cred struct, or %NULL if it is not possible to obtain a
> > new reference.  After successfully taking a new reference to the
> > specified credentials, the cred struct will be marked for free'ing via
> > RCU."
>
> I actually think it's confusing to include
>
>         After successfully taking a new reference to the specified
>         credentials, the cred struct will be marked for free'ing via
>         RCU.
>
> in the documentation, because it makes it sounds like this method has
> the _rcu() suffix because it marks the struct for free'ing via RCU. But
> that is not the case. After all, get_cred() also marks it for free'ing
> via RCU.
>
> It has the _rcu() suffix because - if the cred struct is *already*
> marked for free'ing via RCU, then you are allowed to do this:
>
>         rcu_read_lock();
>         cred = get_cred_rcu(&foo->my_cred);
>         rcu_read_unlock();
>
> even if another thread might put foo->my_cred in parallel with the above
> piece of code.

To be really nit picky, the code doesn't actually enforce these usage
guidelines, so both are "allowed" in that sense.  The key difference
is that the rcu variant checks if the refcount is zero (the cred has
had its last ref dropped, but has not yet been free'd via rcu),
whereas the non-rcu variant always assumes the refcount is greater
than zero.

If you want to add a description/comment to the functions, I'd focus on that.

> > I suppose we could consider adding the zero check in the get_cred()
> > case, but even if we ignore the KCSAN barrier, it looks like the arch
> > support for the inc_not_zero() case isn't nearly as good, likely
> > resulting in more code to execute.
>
> I don't think that's necessary ...

I didn't say it was, in fact I was trying to dissuade anyone from
trying that because it will likely negatively impact performance for
minimal, if any, benefit.

-- 
paul-moore.com