RE: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t

[Reshetova, Elena]

> Elena Reshetova <elena.reshetova@xxxxxxxxx> 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?

Even if one can guarantee and prove that in the current implementation
there are no overflows possible, we can't say that for
sure for any future implementation. Bugs might always happen
unfortunately, but if we convert the refcounter to a safer
type we can be sure that overflows are not possible. 

Does it make sense to you?

Best Regards,
Elena.

> 
> Eric
> 
> > Signed-off-by: Elena Reshetova <elena.reshetova@xxxxxxxxx>
> > Signed-off-by: Hans Liljestrand <ishkamiel@xxxxxxxxx>
> > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > Signed-off-by: David Windsor <dwindsor@xxxxxxxxx>
> > ---
> >  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);