Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   David Laight <David.Laight@ACULAB.COM>
To:     'Xiaoming Ni' <nixiaoming@huawei.com>,
        "paul@paul-moore.com" <paul@paul-moore.com>,
        "edumazet@google.com" <edumazet@google.com>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "paulmck@kernel.org" <paulmck@kernel.org>,
        "dhowells@redhat.com" <dhowells@redhat.com>,
        "keescook@chromium.org" <keescook@chromium.org>,
        "shakeelb@google.com" <shakeelb@google.com>,
        "jamorris@linux.microsoft.com" <jamorris@linux.microsoft.com>
CC:     "alex.huangjianhui@huawei.com" <alex.huangjianhui@huawei.com>,
        "dylix.dailei@huawei.com" <dylix.dailei@huawei.com>,
        "chenzefeng2@huawei.com" <chenzefeng2@huawei.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH RFC] cred: Add WARN to detect wrong use of get/put_cred
Thread-Topic: [PATCH RFC] cred: Add WARN to detect wrong use of get/put_cred
Thread-Index: AQHWQKQ4FhLI5egRjUmWWYYpyV8csKjVJp0w
Date:   Fri, 12 Jun 2020 16:16:56 +0000
Message-ID: <9a680489a44b44d397f8a3e77a6503e7@AcuMS.aculab.com>
References: <1591957695-118312-1-git-send-email-nixiaoming@huawei.com>
In-Reply-To: <1591957695-118312-1-git-send-email-nixiaoming@huawei.com>
Accept-Language: en-GB, en-US
Content-Language: en-US
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 <nixiaoming@huawei.com>
> ---
>  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)