On Tue, 2021-11-02 at 16:11 -0400, Scott Mayhew wrote:
> 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@xxxxxxxxxx>
> > > ---
> > > 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?
>
Can you please send a v2? Thanks!
> -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@xxxxxxxxxxxxxxx
> >
> >
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
