[PATCH] cred: clarify usage of get_cred_rcu()

[PATCH] cred: clarify usage of get_cred_rcu()

[Alice Ryhl]

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().

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 include/linux/cred.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/cred.h b/include/linux/cred.h
index ed1609d78cd7..95dcf5e967c7 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -213,32 +213,50 @@ * get_cred_many - Get references on a set of credentials
 static inline const struct cred *get_cred_many(const struct cred *cred, int nr)
 {
 	struct cred *nonconst_cred = (struct cred *) cred;
 	if (!cred)
 		return cred;
 	nonconst_cred->non_rcu = 0;
 	atomic_long_add(nr, &nonconst_cred->usage);
 	return cred;
 }
 
 /*
  * get_cred - Get a reference on a set of credentials
  * @cred: The credentials to reference
  *
  * Get a reference on the specified set of credentials.  The caller must
  * release the reference.  If %NULL is passed, it is returned with no action.
  *
  * This is used to deal with a committed set of credentials.
  */
 static inline const struct cred *get_cred(const struct cred *cred)
 {
 	return get_cred_many(cred, 1);
 }
 
+/*
+ * get_cred_rcu - Get a reference on a set of credentials under rcu
+ * @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.
+ */
 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.
+	 */
 	nonconst_cred->non_rcu = 0;
 	return cred;
 }
-- 
2.53.0.345.g96ddfc5eaa-goog

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

[Paul Moore]

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?

> 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.

> + * @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."

> + */
>  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 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.

>         nonconst_cred->non_rcu = 0;
>         return cred;
>  }
> --
> 2.53.0.345.g96ddfc5eaa-goog

-- 
paul-moore.com

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

[Alice Ryhl]

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

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

[Paul Moore]

On Fri, Feb 27, 2026 at 5:04 AM Alice Ryhl <aliceryhl@google.com> 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@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?

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@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.

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