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

[Reshetova, Elena]

> On Fri, 2017-03-17 at 09:02 -0400, Jeff Layton wrote:
> > On Fri, 2017-03-17 at 12:50 +, 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 
> > > > Signed-off-by: Hans Liljestrand 
> > > > Signed-off-by: Kees Cook 
> > > > Signed-off-by: David Windsor 
> > > > ---
> > > >  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 
> > > >  #include 
> > > >
> > > > -#include 
> > > > +#include 
> > > >  #include 
> > > >  #include 
> > > >  #include 
> > > > @@ -68,7 +68,7 @@ struct rpc_cred {
> > > >  #endif
> > > >     unsigned long   cr_expire;  /* when
> > > > to gc
> > > > */
> > > >     unsigned long   cr_flags;   /* various
> > > > flags */
> > > > -   atomic_tcr_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.
> 
> The main issue is that this "refcount_t" implementation appears to be
> assuming that there is one and only one model for refcounts (the one
> where a value of "0" means "free me immediately").
> 
> The kernel has a plethora of object caching implementations where this
> is simply not the case; the dcache is a prime example, and this cache
> is another. In both these implementation, the atomic_t variable is
> being used more as a semaphore-style lock that prevents freeing of the
> object while it is in active use as opposed to being freeable, but
> cached. This is why these automated conversions are a nuisance and a
> source of bugs.

Ok, in this particular patch I agree that we missed that object is being reused 
(and yes there are many parts in kernel where similar thing happens as we 
learned from this exercise). 
Note that refcount_t implementation is fine with you "correctly" reusing your 
object:
i.e. when counter reaches zero, you take the object away from active use, but 
it might still stay in cache. 
BUT when you get a new object from cache you should initialize refcounter 
properly: set it to one vs. just do a "inc" on it. 
Problem really comes from this "increment me from zero". 

And the goal with these conversions is to take a look broadly on the kernel 
source and determine (with the feedback from maintainers who know code best, 
like your feedback now) what can be converted already now. 
Maintainers know their code and their usage of counters, so if it doesn't make 
sense to do it in a particular place (because of errors or other reasons), then 
it doesn't. 
But more we cover with new refcount_t, less chances we have with ever hitting 
refcounter bugs anywhere in the future. 

Best Regards,
Elena.

> 
> --
> Trond Myklebust
> Linux NFS client maintainer, PrimaryData
> trond.mykleb...@primarydata.com

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

[Trond Myklebust]

On Fri, 2017-03-17 at 09:02 -0400, Jeff Layton wrote:
> On Fri, 2017-03-17 at 12:50 +, 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 
> > > Signed-off-by: Hans Liljestrand 
> > > Signed-off-by: Kees Cook 
> > > Signed-off-by: David Windsor 
> > > ---
> > >  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 
> > >  #include 
> > >  
> > > -#include 
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > > @@ -68,7 +68,7 @@ struct rpc_cred {
> > >  #endif
> > >   unsigned long   cr_expire;  /* when
> > > to gc
> > > */
> > >   unsigned long   cr_flags;   /* various
> > > flags */
> > > - atomic_tcr_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.

The main issue is that this "refcount_t" implementation appears to be
assuming that there is one and only one model for refcounts (the one
where a value of "0" means "free me immediately").

The kernel has a plethora of object caching implementations where this
is simply not the case; the dcache is a prime example, and this cache
is another. In both these implementation, the atomic_t variable is
being used more as a semaphore-style lock that prevents freeing of the
object while it is in active use as opposed to being freeable, but
cached. This is why these automated conversions are a nuisance and a
source of bugs.

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.mykleb...@primarydata.com

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

[Jeff Layton]

On Fri, 2017-03-17 at 12:50 +, 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 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> > ---
> >  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 
> >  #include 
> >  
> > -#include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -68,7 +68,7 @@ struct rpc_cred {
> >  #endif
> >     unsigned long   cr_expire;  /* when to gc
> > */
> >     unsigned long   cr_flags;   /* various
> > flags */
> > -   atomic_tcr_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 

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

[Trond Myklebust]

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 
> Signed-off-by: Hans Liljestrand 
> Signed-off-by: Kees Cook 
> Signed-off-by: David Windsor 
> ---
>  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 
>  #include 
>  
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -68,7 +68,7 @@ struct rpc_cred {
>  #endif
>   unsigned long   cr_expire;  /* when to gc
> */
>   unsigned long   cr_flags;   /* various
> flags */
> - atomic_tcr_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.

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.mykleb...@primarydata.com

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

[Elena Reshetova]

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 
Signed-off-by: Hans Liljestrand 
Signed-off-by: Kees Cook 
Signed-off-by: David Windsor 
---
 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 
 #include 
 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -68,7 +68,7 @@ struct rpc_cred {
 #endif
unsigned long   cr_expire;  /* when to gc */
unsigned long   cr_flags;   /* various flags */
-   atomic_tcr_count;   /* ref count */
+   refcount_t  cr_count;   /* ref count */
 
kuid_t  cr_uid;
 
@@ -209,7 +209,7 @@ static inline
 struct rpc_cred *  get_rpccred(struct rpc_cred *cred)
 {
if (cred != NULL)
-   atomic_inc(>cr_count);
+   refcount_inc(>cr_count);
return cred;
 }
 
@@ -226,7 +226,7 @@ struct rpc_cred *   get_rpccred(struct rpc_cred *cred)
 static inline struct rpc_cred *
 get_rpccred_rcu(struct rpc_cred *cred)
 {
-   if (atomic_inc_not_zero(>cr_count))
+   if (refcount_inc_not_zero(>cr_count))
return cred;
return NULL;
 }
diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index 2bff63a..b6439b9 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -310,7 +310,7 @@ rpcauth_unhash_cred(struct rpc_cred *cred)
 
cache_lock = >cr_auth->au_credcache->lock;
spin_lock(cache_lock);
-   ret = atomic_read(>cr_count) == 0;
+   ret = refcount_read(>cr_count) == 0;
if (ret)
rpcauth_unhash_cred_locked(cred);
spin_unlock(cache_lock);
@@ -470,12 +470,12 @@ rpcauth_prune_expired(struct list_head *free, int 
nr_to_scan)
list_del_init(>cr_lru);
number_cred_unused--;
freed++;
-   if (atomic_read(>cr_count) != 0)
+   if (refcount_read(>cr_count) != 0)
continue;
 
cache_lock = >cr_auth->au_credcache->lock;
spin_lock(cache_lock);
-   if (atomic_read(>cr_count) == 0) {
+   if (refcount_read(>cr_count) == 0) {
get_rpccred(cred);
list_add_tail(>cr_lru, free);
rpcauth_unhash_cred_locked(cred);
@@ -642,7 +642,7 @@ rpcauth_init_cred(struct rpc_cred *cred, const struct 
auth_cred *acred,
 {
INIT_HLIST_NODE(>cr_hash);
INIT_LIST_HEAD(>cr_lru);
-   atomic_set(>cr_count, 1);
+   refcount_set(>cr_count, 1);
cred->cr_auth = auth;
cred->cr_ops = ops;
cred->cr_expire = jiffies;
@@ -715,12 +715,12 @@ put_rpccred(struct rpc_cred *cred)
return;
/* Fast path for unhashed credentials */
if (test_bit(RPCAUTH_CRED_HASHED, >cr_flags) == 0) {
-   if (atomic_dec_and_test(>cr_count))
+   if (refcount_dec_and_test(>cr_count))
cred->cr_ops->crdestroy(cred);
return;
}
 
-   if (!atomic_dec_and_lock(>cr_count, _credcache_lock))
+   if (!refcount_dec_and_lock(>cr_count, _credcache_lock))
return;
if (!list_empty(>cr_lru)) {
number_cred_unused--;
-- 
2.7.4