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 <[email protected]>
---
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);
}
}
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);
--
2.31.1
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 <[email protected]>
> ---
> 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.
> }
> }
>
> 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
[email protected]
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 <[email protected]>
> > ---
> > ?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
> [email protected]
>
>
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 <[email protected]>
> > > ---
> > > 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
> > [email protected]
> >
> >
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]