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

[Alice Ryhl <aliceryhl@google.com>]

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@google.com> 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?

> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  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.

> > + */
> >  static inline const struct cred *get_cred_rcu(const struct cred *cred)
> >  {
> >         struct cred *nonconst_cred = (struct cred *) cred;
> >         if (!cred)
> >                 return NULL;
> >         if (!atomic_long_inc_not_zero(&nonconst_cred->usage))
> >                 return NULL;
> > +       /*
> > +        * If non_rcu is not already zero, then this call to get_cred_rcu() is
> > +        * probably wrong because if 'usage' goes to zero prior to this call,
> > +        * then get_cred_rcu() assumes it is freed with rcu.
> > +        *
> > +        * However, an exception to this is using get_cred_rcu() in cases where
> > +        * get_cred() would have been okay. To support that case, we do not
> > +        * check non_rcu and set it to zero regardless.
> > +        */
> 
> This is surely a matter of perspective, but the above seems a bit
> wordy, and doesn't address what I believe is the important part:
> setting non_rcu to zero means this credential will be freed
> asynchronously via RCU.  Both get_cred_rcu() and get_cred() set
> non_rcu to 0/false ... although get_cred() doesn't do the non-zero
> check before bumping the refcount.

I think that would be a good comment to add to get_cred(), but in the
case of get_cred_rcu(), it really should already be set to zero, because
otherwise

	rcu_read_lock();
	cred = get_cred_rcu(&foo->my_cred);
	rcu_read_unlock();

is illegal.

> 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. If you use get_cred() in a scenario
where it might be zero, you have a bug.

Alice