Re: [PATCH 01/23] net, sunrpc: convert rpc_cred.cr_count from atomic_t to refcount_t

[Jeff Layton <jlayton@poochiereds.net>]

On Fri, 2017-03-17 at 12:50 +0000, Trond Myklebust wrote:
> On Fri, 2017-03-17 at 14:10 +0200, Elena Reshetova wrote:
> > 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.
> > 
> > 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/sunrpc/auth.h |  8 ++++----
> >  net/sunrpc/auth.c           | 12 ++++++------
> >  2 files changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/linux/sunrpc/auth.h
> > b/include/linux/sunrpc/auth.h
> > index b1bc62b..bd36e0b 100644
> > --- a/include/linux/sunrpc/auth.h
> > +++ b/include/linux/sunrpc/auth.h
> > @@ -15,7 +15,7 @@
> >  #include <linux/sunrpc/msg_prot.h>
> >  #include <linux/sunrpc/xdr.h>
> >  
> > -#include <linux/atomic.h>
> > +#include <linux/refcount.h>
> >  #include <linux/rcupdate.h>
> >  #include <linux/uidgid.h>
> >  #include <linux/utsname.h>
> > @@ -68,7 +68,7 @@ struct rpc_cred {
> >  #endif
> >  	unsigned long		cr_expire;	/* when to gc
> > */
> >  	unsigned long		cr_flags;	/* various
> > flags */
> > -	atomic_t		cr_count;	/* ref count */
> > +	refcount_t		cr_count;	/* ref count */
> > 
> 
> NACK. That's going to be hitting WARN_ONCE(!refcount_inc_not_zero(r),
> "refcount_t: increment on 0; use-after-free.\n") like there's no
> tomorrow...
> 
> Please stop with these automated conversions. They are going to cause a
> lot more bugs than they fix.
> 

Agreed. These patchsets are touching places where we've already banged
out most of the refcounting bugs. I'm against doing large scale
conversions like this without a damned good reason.

I think it may be best to do this sort of thing in a more piecemeal
fashion. Pick a subsystem or two and do the conversions there to prove
that they're better than what we have. If the subsystem already has
problems with its refcounting, then so much the better. Point to bugs
that this new infrastructure helped find.

Encourage people to adopt your new infrastructure as new refcounted
objects are introduced into the kernel. You might even consider a LWN
article about this.

Eventually we'll get around to changing existing code to use it, once
there is a sufficient advantage to doing so. Most likely when we're
reworking the code for other reasons, or when we're chasing some horrid
refcounting bug and think that this might help find it.
-- 
Jeff Layton <jlayton@poochiereds.net>