[RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session

[RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session

[David Windsor]

In furtherance of the KSPP effort to add overflow protection to kernel
reference counters, a new type (refcount_t) and API have been created.
Part of the refcount_t API is refcount_inc(), which will not increment a
refcount_t variable if its value is 0 (as this would indicate a possible
use-after-free condition). 

In auditing the kernel for refcounting corner cases, we've come across the
case of struct nfsd4_session.  

Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session

[David Windsor]

<snip>

> Signed-off-by: David Windsor <dwindsor@gmail.com>
> ---
>  fs/nfsd/nfs4state.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index a0dee8a..b0f3010 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -196,7 +196,7 @@ static void nfsd4_put_session_locked(struct nfsd4_session *ses)
>
>         lockdep_assert_held(&nn->client_lock);
>
> -       if (atomic_dec_and_test(&ses->se_ref) && is_session_dead(ses))
> +       if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_des(ses))

This should read:
if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_dead(ses))

>                 free_session(ses);
>         put_client_renew_locked(clp);
>  }
> @@ -1645,7 +1645,7 @@ static void init_session(struct svc_rqst *rqstp, struct nfsd4_session *new, stru
>         new->se_flags = cses->flags;
>         new->se_cb_prog = cses->callback_prog;
>         new->se_cb_sec = cses->cb_sec;
> -       atomic_set(&new->se_ref, 0);
> +       atomic_set(&new->se_ref, 1);
>         idx = hash_sessionid(&new->se_sessionid);
>         list_add(&new->se_hash, &nn->sessionid_hashtbl[idx]);
>         spin_lock(&clp->cl_lock);
> @@ -1792,7 +1792,7 @@ free_client(struct nfs4_client *clp)
>                 ses = list_entry(clp->cl_sessions.next, struct nfsd4_session,
>                                 se_perclnt);
>                 list_del(&ses->se_perclnt);
> -               WARN_ON_ONCE(atomic_read(&ses->se_ref));
> +               WARN_ON_ONCE((atomic_read(&ses->se_ref) > 1));
>                 free_session(ses);
>         }
>         rpc_destroy_wait_queue(&clp->cl_cb_waitq);
> --
> 2.7.4
>

Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session

[Jeff Layton]

On Sat, 2017-02-11 at 01:42 -0500, David Windsor wrote:
> <snip>
> 
> > Signed-off-by: David Windsor <dwindsor@gmail.com>
> > ---
> >  fs/nfsd/nfs4state.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index a0dee8a..b0f3010 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -196,7 +196,7 @@ static void nfsd4_put_session_locked(struct nfsd4_session *ses)
> > 
> >         lockdep_assert_held(&nn->client_lock);
> > 
> > -       if (atomic_dec_and_test(&ses->se_ref) && is_session_dead(ses))
> > +       if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_des(ses))
> 
> This should read:
> if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_dead(ses))
> 
> >                 free_session(ses);
> >         put_client_renew_locked(clp);
> >  }
> > @@ -1645,7 +1645,7 @@ static void init_session(struct svc_rqst *rqstp, struct nfsd4_session *new, stru
> >         new->se_flags = cses->flags;
> >         new->se_cb_prog = cses->callback_prog;
> >         new->se_cb_sec = cses->cb_sec;
> > -       atomic_set(&new->se_ref, 0);
> > +       atomic_set(&new->se_ref, 1);
> >         idx = hash_sessionid(&new->se_sessionid);
> >         list_add(&new->se_hash, &nn->sessionid_hashtbl[idx]);
> >         spin_lock(&clp->cl_lock);
> > @@ -1792,7 +1792,7 @@ free_client(struct nfs4_client *clp)
> >                 ses = list_entry(clp->cl_sessions.next, struct nfsd4_session,
> >                                 se_perclnt);
> >                 list_del(&ses->se_perclnt);
> > -               WARN_ON_ONCE(atomic_read(&ses->se_ref));
> > +               WARN_ON_ONCE((atomic_read(&ses->se_ref) > 1));
> >                 free_session(ses);
> >         }
> >         rpc_destroy_wait_queue(&clp->cl_cb_waitq);
> > --
> > 2.7.4
> > 

The basic idea here is that nfsv4 sessions have a "resting state" of 0.
We want to keep them around, but if they go "dead" then we we'll tear
them down if they aren't actively in use at the time. So, we still free
the thing when the refcount goes to zero, but we have an extra condition
before we free it on the put -- that the session is also "dead" (meaning
that the client asked us to destroy it).

Your patch doesn't look like it'll break anything, but I personally find
 it harder to follow that way. The freeable reference state will be 1
instead of the normal 0.

--
Jeff Layton <jlayton@poochiereds.net>

Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session

[David Windsor]

On Sat, Feb 11, 2017 at 7:31 AM, Jeff Layton <jlayton@poochiereds.net> wrote:
> On Sat, 2017-02-11 at 01:42 -0500, David Windsor wrote:
>> <snip>
>>
>> > Signed-off-by: David Windsor <dwindsor@gmail.com>
>> > ---
>> >  fs/nfsd/nfs4state.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> > index a0dee8a..b0f3010 100644
>> > --- a/fs/nfsd/nfs4state.c
>> > +++ b/fs/nfsd/nfs4state.c
>> > @@ -196,7 +196,7 @@ static void nfsd4_put_session_locked(struct nfsd4_session *ses)
>> >
>> >         lockdep_assert_held(&nn->client_lock);
>> >
>> > -       if (atomic_dec_and_test(&ses->se_ref) && is_session_dead(ses))
>> > +       if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_des(ses))
>>
>> This should read:
>> if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_dead(ses))
>>
>> >                 free_session(ses);
>> >         put_client_renew_locked(clp);
>> >  }
>> > @@ -1645,7 +1645,7 @@ static void init_session(struct svc_rqst *rqstp, struct nfsd4_session *new, stru
>> >         new->se_flags = cses->flags;
>> >         new->se_cb_prog = cses->callback_prog;
>> >         new->se_cb_sec = cses->cb_sec;
>> > -       atomic_set(&new->se_ref, 0);
>> > +       atomic_set(&new->se_ref, 1);
>> >         idx = hash_sessionid(&new->se_sessionid);
>> >         list_add(&new->se_hash, &nn->sessionid_hashtbl[idx]);
>> >         spin_lock(&clp->cl_lock);
>> > @@ -1792,7 +1792,7 @@ free_client(struct nfs4_client *clp)
>> >                 ses = list_entry(clp->cl_sessions.next, struct nfsd4_session,
>> >                                 se_perclnt);
>> >                 list_del(&ses->se_perclnt);
>> > -               WARN_ON_ONCE(atomic_read(&ses->se_ref));
>> > +               WARN_ON_ONCE((atomic_read(&ses->se_ref) > 1));
>> >                 free_session(ses);
>> >         }
>> >         rpc_destroy_wait_queue(&clp->cl_cb_waitq);
>> > --
>> > 2.7.4
>> >
>
> The basic idea here is that nfsv4 sessions have a "resting state" of 0.
> We want to keep them around, but if they go "dead" then we we'll tear
> them down if they aren't actively in use at the time. So, we still free
> the thing when the refcount goes to zero, but we have an extra condition
> before we free it on the put -- that the session is also "dead" (meaning
> that the client asked us to destroy it).
>
> Your patch doesn't look like it'll break anything, but I personally find
>  it harder to follow that way. The freeable reference state will be 1
> instead of the normal 0.
>

I'm not sure there's another way to accomplish what we need
(initializing struct nfsd4_session objects with refcount=1) without
also modifying the freeable reference state.  After migrating to the
refcount_t API, if we leave init_session() as is, the first call to
nfsd4_get_session_locked() will fail:

static __be32 nfsd4_get_session_locked(struct nfsd4_session *ses)
{
         __be32 status;

         if (is_session_dead(ses))
                 return nfserr_badsession;
         status = get_client_locked(ses->se_client);
         if (status)
                 return status;
         refcount_inc(&ses->se_ref);    /* This fails and WARNS when
ses->se_ref is 0. */
         return nfs_ok;
}


The refcount_t API patches aren't yet merged, so this discussion is a
bit limited in that respect, but refcount_inc() WARNS when called with
a refcount_t object whose value is 0, as this may represent a
use-after-free attempt.

Given this, I'm unsure how it's possible to achieve initialization of
struct nfsd4_session objects with refcount=1 while still maintaining
these objects' "rest state" at refcount=0.

> --
> Jeff Layton <jlayton@poochiereds.net>

Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session

[Jeff Layton]

On Sat, 2017-02-11 at 09:01 -0500, David Windsor wrote:
> On Sat, Feb 11, 2017 at 7:31 AM, Jeff Layton <jlayton@poochiereds.net> wrote:
> > On Sat, 2017-02-11 at 01:42 -0500, David Windsor wrote:
> > > <snip>
> > > 
> > > > Signed-off-by: David Windsor <dwindsor@gmail.com>
> > > > ---
> > > >  fs/nfsd/nfs4state.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > index a0dee8a..b0f3010 100644
> > > > --- a/fs/nfsd/nfs4state.c
> > > > +++ b/fs/nfsd/nfs4state.c
> > > > @@ -196,7 +196,7 @@ static void nfsd4_put_session_locked(struct nfsd4_session *ses)
> > > > 
> > > >         lockdep_assert_held(&nn->client_lock);
> > > > 
> > > > -       if (atomic_dec_and_test(&ses->se_ref) && is_session_dead(ses))
> > > > +       if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_des(ses))
> > > 
> > > This should read:
> > > if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_dead(ses))
> > > 
> > > >                 free_session(ses);
> > > >         put_client_renew_locked(clp);
> > > >  }
> > > > @@ -1645,7 +1645,7 @@ static void init_session(struct svc_rqst *rqstp, struct nfsd4_session *new, stru
> > > >         new->se_flags = cses->flags;
> > > >         new->se_cb_prog = cses->callback_prog;
> > > >         new->se_cb_sec = cses->cb_sec;
> > > > -       atomic_set(&new->se_ref, 0);
> > > > +       atomic_set(&new->se_ref, 1);
> > > >         idx = hash_sessionid(&new->se_sessionid);
> > > >         list_add(&new->se_hash, &nn->sessionid_hashtbl[idx]);
> > > >         spin_lock(&clp->cl_lock);
> > > > @@ -1792,7 +1792,7 @@ free_client(struct nfs4_client *clp)
> > > >                 ses = list_entry(clp->cl_sessions.next, struct nfsd4_session,
> > > >                                 se_perclnt);
> > > >                 list_del(&ses->se_perclnt);
> > > > -               WARN_ON_ONCE(atomic_read(&ses->se_ref));
> > > > +               WARN_ON_ONCE((atomic_read(&ses->se_ref) > 1));
> > > >                 free_session(ses);
> > > >         }
> > > >         rpc_destroy_wait_queue(&clp->cl_cb_waitq);
> > > > --
> > > > 2.7.4
> > > > 
> > 
> > The basic idea here is that nfsv4 sessions have a "resting state" of 0.
> > We want to keep them around, but if they go "dead" then we we'll tear
> > them down if they aren't actively in use at the time. So, we still free
> > the thing when the refcount goes to zero, but we have an extra condition
> > before we free it on the put -- that the session is also "dead" (meaning
> > that the client asked us to destroy it).
> > 
> > Your patch doesn't look like it'll break anything, but I personally find
> >  it harder to follow that way. The freeable reference state will be 1
> > instead of the normal 0.
> > 
> 
> I'm not sure there's another way to accomplish what we need
> (initializing struct nfsd4_session objects with refcount=1) without
> also modifying the freeable reference state.  After migrating to the
> refcount_t API, if we leave init_session() as is, the first call to
> nfsd4_get_session_locked() will fail:
> 
> static __be32 nfsd4_get_session_locked(struct nfsd4_session *ses)
> {
>          __be32 status;
> 
>          if (is_session_dead(ses))
>                  return nfserr_badsession;
>          status = get_client_locked(ses->se_client);
>          if (status)
>                  return status;
>          refcount_inc(&ses->se_ref);    /* This fails and WARNS when
> ses->se_ref is 0. */
>          return nfs_ok;
> }
> 
> 
> The refcount_t API patches aren't yet merged, so this discussion is a
> bit limited in that respect, but refcount_inc() WARNS when called with
> a refcount_t object whose value is 0, as this may represent a
> use-after-free attempt.
> 
> Given this, I'm unsure how it's possible to achieve initialization of
> struct nfsd4_session objects with refcount=1 while still maintaining
> these objects' "rest state" at refcount=0.
> 

One idea might be to take an extra reference on the thing when creating
it, and then drop that reference when the thing is marked DEAD. The
extra reference would be superfluous, but it might make it look a little
more natural.

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session

[Christoph Hellwig]

On Sat, Feb 11, 2017 at 09:01:15AM -0500, David Windsor wrote:
> I'm not sure there's another way to accomplish what we need
> (initializing struct nfsd4_session objects with refcount=1) without
> also modifying the freeable reference state.  After migrating to the
> refcount_t API, if we leave init_session() as is, the first call to
> nfsd4_get_session_locked() will fail:

Which is a pretty clear indicator that this code should simply not
migrate to the recount_t API.  Why was it even considered if the
conversion is obviously broken?

Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session

[David Windsor]

On Mon, Feb 13, 2017 at 5:38 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Sat, Feb 11, 2017 at 09:01:15AM -0500, David Windsor wrote:
>> I'm not sure there's another way to accomplish what we need
>> (initializing struct nfsd4_session objects with refcount=1) without
>> also modifying the freeable reference state.  After migrating to the
>> refcount_t API, if we leave init_session() as is, the first call to
>> nfsd4_get_session_locked() will fail:
>
> Which is a pretty clear indicator that this code should simply not
> migrate to the recount_t API.  Why was it even considered if the
> conversion is obviously broken?

I'm not sure this is a sound argument for not converting to
refcount_t.  In other locations in which refcounting schemes are
"unnatural," i.e. freeing refcounted objects when their refcount is -1
(rather than 0), conversion to refcount_t is accomplished by
performing a logical +1 to the overall refcounting scheme.  We're
auditing all refcounting corner cases, such as these, to see if
similar solutions can be found.

Thanks,
David

Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session

[Christoph Hellwig]

On Mon, Feb 13, 2017 at 06:42:56AM -0500, David Windsor wrote:
> I'm not sure this is a sound argument for not converting to
> refcount_t.

It's an argument again the way how this patch was sent.  Please take care
of all the trivial conversions first, and then do anything non-trivial
on a case by case basis.

Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session

[David Windsor]

On Mon, Feb 13, 2017 at 7:12 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Feb 13, 2017 at 06:42:56AM -0500, David Windsor wrote:
>> I'm not sure this is a sound argument for not converting to
>> refcount_t.
>
> It's an argument again the way how this patch was sent.  Please take care
> of all the trivial conversions first, and then do anything non-trivial
> on a case by case basis.

This is actually what we're currently doing.

The vast majority of atomic_t to refcount_t conversions are trivial,
as they most always follow the pattern:

1.  Initialize the atomic_t refcounter to 1.
2.  Perform shared object get/put operations.
3.  Free the shared object when its refcount becomes 0.

We've identified a handful of instances in which steps 1 and 3 are
different from above (i.e. initializing refcounts to 0, freeing
objects when refcount is non-zero, etc.).  The above patch addresses
one of these instances in NFS.

Thanks,
David

Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session

[Bruce Fields]

On Sat, Feb 11, 2017 at 07:31:42AM -0500, Jeff Layton wrote:
> The basic idea here is that nfsv4 sessions have a "resting state" of 0.
> We want to keep them around, but if they go "dead" then we we'll tear
> them down if they aren't actively in use at the time. So, we still free
> the thing when the refcount goes to zero, but we have an extra condition
> before we free it on the put -- that the session is also "dead" (meaning
> that the client asked us to destroy it).
> 
> Your patch doesn't look like it'll break anything, but I personally find
>  it harder to follow that way. The freeable reference state will be 1
> instead of the normal 0.

Alas, I don't have any examples in mind, but doesn't this pattern happen
all over?

You have objects that live in some data structure.  They're freed only
when they're removed from the data structure.  You want removal to fail
whenever they're in use.

So it's natural to use an atomic counter to track the number of external
users and some other lock to serialize lookup and destruction.

--b.

Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session

[David Windsor]

On Sat, Feb 11, 2017 at 8:15 PM, Bruce Fields <bfields@fieldses.org> wrote:
> On Sat, Feb 11, 2017 at 07:31:42AM -0500, Jeff Layton wrote:
>> The basic idea here is that nfsv4 sessions have a "resting state" of 0.
>> We want to keep them around, but if they go "dead" then we we'll tear
>> them down if they aren't actively in use at the time. So, we still free
>> the thing when the refcount goes to zero, but we have an extra condition
>> before we free it on the put -- that the session is also "dead" (meaning
>> that the client asked us to destroy it).
>>
>> Your patch doesn't look like it'll break anything, but I personally find
>>  it harder to follow that way. The freeable reference state will be 1
>> instead of the normal 0.
>
> Alas, I don't have any examples in mind, but doesn't this pattern happen
> all over?
>

The majority of refcounted objects are allocated with refcount=1: the
very fact that they're being allocated means that they already have a
user.

The issue with struct nfsd4_session is that, like struct nfs4_client,
references to it are only taken when the server is actively working on
it.  Its default "resting state" is with refcount=0.

I would like to make its default resting state with refcount=1.  In
other cases similar to this, we've gotten around it by doing a
semantic +1 to the object's overall refcounting scheme.

Jeff suggested taking an additional reference in init_session(), and
dropping it in is_session_dead(), after determining, in fact, that the
object is DEAD.

> You have objects that live in some data structure.  They're freed only
> when they're removed from the data structure.  You want removal to fail
> whenever they're in use.
>

When they're in use, these objects' refcount should be > 0.

> So it's natural to use an atomic counter to track the number of external
> users and some other lock to serialize lookup and destruction.
>

When considering refcounted objects, the most "natural" interpretation
of refcount=0 means that the object no longer has any users and can be
freed.  Increments on objects with refcount=0 shouldn't be allowed, as
this may indicate a use-after-free condition.

This discussion is difficult because the refcount_t API hasn't yet
been introduced.  The purpose of that API is to eliminate
use-after-free bugs.

> --b.

Re: [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session

[Hans Liljestrand]

On Sat, Feb 11, 2017 at 01:42:53AM -0500, David Windsor wrote:
><snip>
>
>> Signed-off-by: David Windsor <dwindsor@gmail.com>
>> ---
>>  fs/nfsd/nfs4state.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index a0dee8a..b0f3010 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -196,7 +196,7 @@ static void nfsd4_put_session_locked(struct nfsd4_session *ses)
>>
>>         lockdep_assert_held(&nn->client_lock);
>>
>> -       if (atomic_dec_and_test(&ses->se_ref) && is_session_dead(ses))
>> +       if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_des(ses))
>
>This should read:
>if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_dead(ses))
>
>>                 free_session(ses);

Hi, 

I'm not sure if I have this correctly; But both before and after the patch 
free_session gets called when se_ref count was 1, shouldn't this have changed 
with the +1 scheme?

Also, since the !atomic_add_unless doesn't actually decrement when at 1, 
doesn't this leave the se_ref as 1 when it's destroyed? The function seems to 
always be locked, so perhaps this doesn't matter, but still seems a bit risky.

Thanks,
-hans

>>         put_client_renew_locked(clp);
>>  }
>> @@ -1645,7 +1645,7 @@ static void init_session(struct svc_rqst *rqstp, struct nfsd4_session *new, stru
>>         new->se_flags = cses->flags;
>>         new->se_cb_prog = cses->callback_prog;
>>         new->se_cb_sec = cses->cb_sec;
>> -       atomic_set(&new->se_ref, 0);
>> +       atomic_set(&new->se_ref, 1);
>>         idx = hash_sessionid(&new->se_sessionid);
>>         list_add(&new->se_hash, &nn->sessionid_hashtbl[idx]);
>>         spin_lock(&clp->cl_lock);
>> @@ -1792,7 +1792,7 @@ free_client(struct nfs4_client *clp)
>>                 ses = list_entry(clp->cl_sessions.next, struct nfsd4_session,
>>                                 se_perclnt);
>>                 list_del(&ses->se_perclnt);
>> -               WARN_ON_ONCE(atomic_read(&ses->se_ref));
>> +               WARN_ON_ONCE((atomic_read(&ses->se_ref) > 1));
>>                 free_session(ses);
>>         }
>>         rpc_destroy_wait_queue(&clp->cl_cb_waitq);
>> --
>> 2.7.4
>>

Re: [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session

[David Windsor]

On Mon, Feb 13, 2017 at 5:54 AM, Hans Liljestrand <ishkamiel@gmail.com> wrote:
> On Sat, Feb 11, 2017 at 01:42:53AM -0500, David Windsor wrote:
>>
>> <snip>
>>
>>> Signed-off-by: David Windsor <dwindsor@gmail.com>
>>> ---
>>>  fs/nfsd/nfs4state.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index a0dee8a..b0f3010 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -196,7 +196,7 @@ static void nfsd4_put_session_locked(struct
>>> nfsd4_session *ses)
>>>
>>>         lockdep_assert_held(&nn->client_lock);
>>>
>>> -       if (atomic_dec_and_test(&ses->se_ref) && is_session_dead(ses))
>>> +       if (!atomic_add_unless(&ses->se_ref, -1, 1) &&
>>> is_session_des(ses))
>>
>>
>> This should read:
>> if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_dead(ses))
>>
>>>                 free_session(ses);
>
>
> Hi,
> I'm not sure if I have this correctly; But both before and after the patch
> free_session gets called when se_ref count was 1, shouldn't this have
> changed with the +1 scheme?
>
> Also, since the !atomic_add_unless doesn't actually decrement when at 1,
> doesn't this leave the se_ref as 1 when it's destroyed? The function seems
> to always be locked, so perhaps this doesn't matter, but still seems a bit
> risky.
>

Yes; I forgot the additional call to atomic_dec_and_test() before
free_session().  Thanks!

I'll resubmit this after seeing how the rest of this discussion goes.
We may end up abandoning this refcounting case.

> Thanks,
> -hans
>
>
>>>         put_client_renew_locked(clp);
>>>  }
>>> @@ -1645,7 +1645,7 @@ static void init_session(struct svc_rqst *rqstp,
>>> struct nfsd4_session *new, stru
>>>         new->se_flags = cses->flags;
>>>         new->se_cb_prog = cses->callback_prog;
>>>         new->se_cb_sec = cses->cb_sec;
>>> -       atomic_set(&new->se_ref, 0);
>>> +       atomic_set(&new->se_ref, 1);
>>>         idx = hash_sessionid(&new->se_sessionid);
>>>         list_add(&new->se_hash, &nn->sessionid_hashtbl[idx]);
>>>         spin_lock(&clp->cl_lock);
>>> @@ -1792,7 +1792,7 @@ free_client(struct nfs4_client *clp)
>>>                 ses = list_entry(clp->cl_sessions.next, struct
>>> nfsd4_session,
>>>                                 se_perclnt);
>>>                 list_del(&ses->se_perclnt);
>>> -               WARN_ON_ONCE(atomic_read(&ses->se_ref));
>>> +               WARN_ON_ONCE((atomic_read(&ses->se_ref) > 1));
>>>                 free_session(ses);
>>>         }
>>>         rpc_destroy_wait_queue(&clp->cl_cb_waitq);
>>> --
>>> 2.7.4
>>>
>

Re: [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session

[Bruce Fields]

On Mon, Feb 13, 2017 at 06:46:19AM -0500, David Windsor wrote:
> On Mon, Feb 13, 2017 at 5:54 AM, Hans Liljestrand <ishkamiel@gmail.com> wrote:
> > On Sat, Feb 11, 2017 at 01:42:53AM -0500, David Windsor wrote:
> >>
> >> <snip>
> >>
> >>> Signed-off-by: David Windsor <dwindsor@gmail.com>
> >>> ---
> >>>  fs/nfsd/nfs4state.c | 6 +++---
> >>>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >>> index a0dee8a..b0f3010 100644
> >>> --- a/fs/nfsd/nfs4state.c
> >>> +++ b/fs/nfsd/nfs4state.c
> >>> @@ -196,7 +196,7 @@ static void nfsd4_put_session_locked(struct
> >>> nfsd4_session *ses)
> >>>
> >>>         lockdep_assert_held(&nn->client_lock);
> >>>
> >>> -       if (atomic_dec_and_test(&ses->se_ref) && is_session_dead(ses))
> >>> +       if (!atomic_add_unless(&ses->se_ref, -1, 1) &&
> >>> is_session_des(ses))
> >>
> >>
> >> This should read:
> >> if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_dead(ses))
> >>
> >>>                 free_session(ses);
> >
> >
> > Hi,
> > I'm not sure if I have this correctly; But both before and after the patch
> > free_session gets called when se_ref count was 1, shouldn't this have
> > changed with the +1 scheme?
> >
> > Also, since the !atomic_add_unless doesn't actually decrement when at 1,
> > doesn't this leave the se_ref as 1 when it's destroyed? The function seems
> > to always be locked, so perhaps this doesn't matter, but still seems a bit
> > risky.
> >
> 
> Yes; I forgot the additional call to atomic_dec_and_test() before
> free_session().  Thanks!
> 
> I'll resubmit this after seeing how the rest of this discussion goes.
> We may end up abandoning this refcounting case.

I could live with it.

My knee jerk reaction is like Jeff's--it just seems more natural to me
for reference count 0 to mean "not in use, OK to free" in cases like
this--but maybe I just need to get used to the idea.

It'd be interesting to see what the final result looks like after
conversion to refcount_t.

--b.