From: Xiaoming Ni
> Sent: 12 June 2020 11:28
> Cred release and usage check code flow:
> 1. put_cred()
> if (atomic_dec_and_test(&(cred)->usage))
> __put_cred(cred);
>
> 2. __put_cred()
> BUG_ON(atomic_read(&cred->usage) != 0);
> call_rcu(&cred->rcu, put_cred_rcu);
>
> 3. put_cred_rcu()
> if (atomic_read(&cred->usage) != 0)
> panic("CRED: put_cred_rcu() sees %p with usage %d\n",
> cred, atomic_read(&cred->usage));
> kmem_cache_free(cred_jar, cred);
>
> If panic is triggered on put_cred_rcu(), there are two possibilities
> 1. Call get_cred() after __put_cred(), usage > 0
> 2. Call put_cred() after __put_cred(), usage < 0
> Since put_cred_rcu is an asynchronous behavior, it is no longer the first
> scene when panic, there is no information about the murderer in the panic
> call stack...
>
> So, add WARN() in get_cred()/put_cred(), and pray to catch the murderer
> at the first scene.
>
> Signed-off-by: Xiaoming Ni <[email protected]>
> ---
> include/linux/cred.h | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/cred.h b/include/linux/cred.h
> index 18639c0..c00d5a1 100644
> --- a/include/linux/cred.h
> +++ b/include/linux/cred.h
> @@ -224,11 +224,16 @@ static inline bool cap_ambient_invariant_ok(const
> struct cred *cred)
> *
> * Get a reference on the specified set of new credentials. The caller must
> * release the reference.
> + *
> + * Initialize usage to 1 during cred resource allocation,
> + * so when calling get_cred, usage cannot be 0.
> */
> static inline struct cred *get_new_cred(struct cred *cred)
> {
> - atomic_inc(&cred->usage);
> - return cred;
> + if (atomic_inc_not_zero(&cred->usage))
> + return cred;
> + WARN(1, "get_new_cred after __put_cred");
> + return NULL;
> }
>
> /**
> @@ -280,11 +285,14 @@ static inline const struct cred *get_cred_rcu(const
> struct cred *cred)
> static inline void put_cred(const struct cred *_cred)
> {
> struct cred *cred = (struct cred *) _cred;
> + int usage;
>
> if (cred) {
> validate_creds(cred);
> - if (atomic_dec_and_test(&(cred)->usage))
> + usage = atomic_dec_return(&(cred)->usage);
> + if (usage == 0)
> __put_cred(cred);
> + WARN(usage < 0, "put_cred after __put_cred");
> }
> }
You really don't want to add WARN() to static inline functions.
It will bloat horribly.
It might be possible to the message into a called function.
One thing I've thought about for reference counts is for the
code that allocates and frees the item to add a big number
and code that only borrows a reference just adds 1.
If the counter is large enough you can separately detect
double frees and missing frees for the two different types
of allocation.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
