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?
-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
>
>
