From: ebiederm@xmission.com (Eric W. Biederman)
To: Elena Reshetova <elena.reshetova@intel.com>
Cc: linux-kernel@vger.kernel.org, peterz@infradead.org,
        gregkh@linuxfoundation.org, akpm@linux-foundation.org,
        mingo@redhat.com, adobriyan@gmail.com, serge@hallyn.com,
        arozansk@redhat.com, dave@stgolabs.net, keescook@chromium.org,
        Hans Liljestrand <ishkamiel@gmail.com>,
        David Windsor <dwindsor@gmail.com>
References: <1499417992-3238-1-git-send-email-elena.reshetova@intel.com>
        <1499417992-3238-2-git-send-email-elena.reshetova@intel.com>
Date: Sun, 09 Jul 2017 16:59:55 -0500
In-Reply-To: <1499417992-3238-2-git-send-email-elena.reshetova@intel.com>
        (Elena Reshetova's message of "Fri, 7 Jul 2017 11:59:50 +0300")
Message-ID: <87bmottgo4.fsf@xmission.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2716
Lines: 88

Elena Reshetova <elena.reshetova@intel.com> writes:

> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.

In this patch you can see all of the uses of the count.
What accidental refcount overflows are possible?

Eric

> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: David Windsor <dwindsor@gmail.com>
> ---
>  include/linux/ipc_namespace.h | 5 +++--
>  ipc/msgutil.c                 | 2 +-
>  ipc/namespace.c               | 4 ++--
>  3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
> index 65327ee..e81445c 100644
> --- a/include/linux/ipc_namespace.h
> +++ b/include/linux/ipc_namespace.h
> @@ -7,6 +7,7 @@
>  #include <linux/notifier.h>
>  #include <linux/nsproxy.h>
>  #include <linux/ns_common.h>
> +#include <linux/refcount.h>
>  
>  struct user_namespace;
>  
> @@ -19,7 +20,7 @@ struct ipc_ids {
>  };
>  
>  struct ipc_namespace {
> -	atomic_t	count;
> +	refcount_t	count;
>  	struct ipc_ids	ids[3];
>  
>  	int		sem_ctls[4];
> @@ -118,7 +119,7 @@ extern struct ipc_namespace *copy_ipcs(unsigned long flags,
>  static inline struct ipc_namespace *get_ipc_ns(struct ipc_namespace *ns)
>  {
>  	if (ns)
> -		atomic_inc(&ns->count);
> +		refcount_inc(&ns->count);
>  	return ns;
>  }
>  
> diff --git a/ipc/msgutil.c b/ipc/msgutil.c
> index bf74eaa..8459802 100644
> --- a/ipc/msgutil.c
> +++ b/ipc/msgutil.c
> @@ -29,7 +29,7 @@ DEFINE_SPINLOCK(mq_lock);
>   * and not CONFIG_IPC_NS.
>   */
>  struct ipc_namespace init_ipc_ns = {
> -	.count		= ATOMIC_INIT(1),
> +	.count		= REFCOUNT_INIT(1),
>  	.user_ns = &init_user_ns,
>  	.ns.inum = PROC_IPC_INIT_INO,
>  #ifdef CONFIG_IPC_NS
> diff --git a/ipc/namespace.c b/ipc/namespace.c
> index b4d80f9..7af6e6b 100644
> --- a/ipc/namespace.c
> +++ b/ipc/namespace.c
> @@ -50,7 +50,7 @@ static struct ipc_namespace *create_ipc_ns(struct user_namespace *user_ns,
>  		goto fail_free;
>  	ns->ns.ops = &ipcns_operations;
>  
> -	atomic_set(&ns->count, 1);
> +	refcount_set(&ns->count, 1);
>  	ns->user_ns = get_user_ns(user_ns);
>  	ns->ucounts = ucounts;
>  
> @@ -144,7 +144,7 @@ static void free_ipc_ns(struct ipc_namespace *ns)
>   */
>  void put_ipc_ns(struct ipc_namespace *ns)
>  {
> -	if (atomic_dec_and_lock(&ns->count, &mq_lock)) {
> +	if (refcount_dec_and_lock(&ns->count, &mq_lock)) {
>  		mq_clear_sbinfo(ns);
>  		spin_unlock(&mq_lock);
>  		mq_put_mnt(ns);