Re: [PATCH 1/1] nfs4: take a reference on the nfs_client when running FREE_STATEID

[Scott Mayhew <smayhew@redhat.com>]

On Tue, 02 Nov 2021, Trond Myklebust wrote:

> Hi Scott,
> 
> Thanks. This mostly looks good, but I do have 1 comment below.
> 
> On Mon, 2021-11-01 at 16:06 -0400, Scott Mayhew wrote:
> > During umount, the session slot tables are freed.  If there are
> > outstanding FREE_STATEID tasks, a use-after-free and slab corruption
> > can
> > occur when rpc_exit_task calls rpc_call_done -> nfs41_sequence_done -
> > >
> > nfs4_sequence_process/nfs41_sequence_free_slot.
> > 
> > Prevent that from happening by taking a reference on the nfs_client
> > in
> > nfs41_free_stateid and putting it in nfs41_free_stateid_release.
> > 
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> >  fs/nfs/nfs4proc.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index e1214bb6b7ee..76e6786b797e 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -10145,18 +10145,24 @@ static void
> > nfs41_free_stateid_prepare(struct rpc_task *task, void *calldata)
> >  static void nfs41_free_stateid_done(struct rpc_task *task, void
> > *calldata)
> >  {
> >         struct nfs_free_stateid_data *data = calldata;
> > +       struct nfs_client *clp = data->server->nfs_client;
> >  
> >         nfs41_sequence_done(task, &data->res.seq_res);
> >  
> >         switch (task->tk_status) {
> >         case -NFS4ERR_DELAY:
> >                 if (nfs4_async_handle_error(task, data->server, NULL,
> > NULL) == -EAGAIN)
> > -                       rpc_restart_call_prepare(task);
> > +                       if (refcount_read(&clp->cl_count) > 1)
> > +                               rpc_restart_call_prepare(task);
> 
> Do we really need to make the rpc restart call conditional here? Most
> servers prefer that you finish freeing state before calling
> DESTROY_CLIENTID.

Good point.  No, it's not necessary.  Do you want me to send a v2, or
can you apply the patch without this hunk?

-Scott
> 
> >         }
> >  }
> >  
> >  static void nfs41_free_stateid_release(void *calldata)
> >  {
> > +       struct nfs_free_stateid_data *data = calldata;
> > +       struct nfs_client *clp = data->server->nfs_client;
> > +
> > +       nfs_put_client(clp);
> >         kfree(calldata);
> >  }
> >  
> > @@ -10193,6 +10199,10 @@ static int nfs41_free_stateid(struct
> > nfs_server *server,
> >         };
> >         struct nfs_free_stateid_data *data;
> >         struct rpc_task *task;
> > +       struct nfs_client *clp = server->nfs_client;
> > +
> > +       if (!refcount_inc_not_zero(&clp->cl_count))
> > +               return -EIO;
> >  
> >         nfs4_state_protect(server->nfs_client,
> > NFS_SP4_MACH_CRED_STATEID,
> >                 &task_setup.rpc_client, &msg);
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 
>