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

[Jeff Layton <jlayton@poochiereds.net>]

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>