[1/1] NFS prevent double free in async nfs4_exchange_id
diff mbox

Message ID 20170310213509.38215-1-kolga@netapp.com
State New
Headers show

Commit Message

Olga Kornievskaia March 10, 2017, 9:35 p.m. UTC
Since rpc_task is async, the release function should be called which
will free the impl_id, scope, and owner.

Trond pointed at 2 more problems:
-- use of client pointer after free in the nfs4_exchangeid_release() function
-- cl_count mismatch if rpc_run_task() isn't run

Fixes: 8d89bd70bc9 ("NFS setup async exchange_id")
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/nfs4proc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Olga Kornievskaia March 10, 2017, 9:56 p.m. UTC | #1
On Fri, Mar 10, 2017 at 4:35 PM, Olga Kornievskaia <kolga@netapp.com> wrote:
> Since rpc_task is async, the release function should be called which
> will free the impl_id, scope, and owner.
>
> Trond pointed at 2 more problems:
> -- use of client pointer after free in the nfs4_exchangeid_release() function
> -- cl_count mismatch if rpc_run_task() isn't run
>
> Fixes: 8d89bd70bc9 ("NFS setup async exchange_id")
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/nfs4proc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 59be0f7..3a79d3a 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -7426,11 +7426,11 @@ static void nfs4_exchange_id_release(void *data)
>         struct nfs41_exchange_id_data *cdata =
>                                         (struct nfs41_exchange_id_data *)data;
>
> -       nfs_put_client(cdata->args.client);
>         if (cdata->xprt) {
>                 xprt_put(cdata->xprt);
>                 rpc_clnt_xprt_switch_put(cdata->args.client->cl_rpcclient);
>         }
> +       nfs_put_client(cdata->args.client);
>         kfree(cdata->res.impl_id);
>         kfree(cdata->res.server_scope);
>         kfree(cdata->res.server_owner);
> @@ -7537,10 +7537,8 @@ static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred,
>         task_setup_data.callback_data = calldata;
>
>         task = rpc_run_task(&task_setup_data);
> -       if (IS_ERR(task)) {
> -       status = PTR_ERR(task);
> -               goto out_impl_id;
> -       }
> +       if (IS_ERR(task))
> +               return PTR_ERR(task);
>
>         if (!xprt) {
>                 status = rpc_wait_for_completion_task(task);
> @@ -7558,6 +7556,8 @@ static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred,
>                         clp->cl_implid->date.seconds,
>                         clp->cl_implid->date.nseconds);
>         dprintk("NFS reply exchange_id: %d\n", status);
> +       if (status)
> +               nfs_put_client(clp);
>         return status;
>
>  out_impl_id:

Urgh. scratch this one, it's causing problems. Will try again.


> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Trond Myklebust March 11, 2017, 3:49 p.m. UTC | #2
On Fri, 2017-03-10 at 16:56 -0500, Olga Kornievskaia wrote:
> On Fri, Mar 10, 2017 at 4:35 PM, Olga Kornievskaia <kolga@netapp.com>

> wrote:

> > Since rpc_task is async, the release function should be called

> > which

> > will free the impl_id, scope, and owner.

> > 

> > Trond pointed at 2 more problems:

> > -- use of client pointer after free in the

> > nfs4_exchangeid_release() function

> > -- cl_count mismatch if rpc_run_task() isn't run

> > 

> > Fixes: 8d89bd70bc9 ("NFS setup async exchange_id")

> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>

> > ---

> >  fs/nfs/nfs4proc.c | 10 +++++-----

> >  1 file changed, 5 insertions(+), 5 deletions(-)

> > 

> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c

> > index 59be0f7..3a79d3a 100644

> > --- a/fs/nfs/nfs4proc.c

> > +++ b/fs/nfs/nfs4proc.c

> > @@ -7426,11 +7426,11 @@ static void nfs4_exchange_id_release(void

> > *data)

> >         struct nfs41_exchange_id_data *cdata =

> >                                         (struct

> > nfs41_exchange_id_data *)data;

> > 

> > -       nfs_put_client(cdata->args.client);

> >         if (cdata->xprt) {

> >                 xprt_put(cdata->xprt);

> >                 rpc_clnt_xprt_switch_put(cdata->args.client-

> > >cl_rpcclient);

> >         }

> > +       nfs_put_client(cdata->args.client);

> >         kfree(cdata->res.impl_id);

> >         kfree(cdata->res.server_scope);

> >         kfree(cdata->res.server_owner);

> > @@ -7537,10 +7537,8 @@ static int _nfs4_proc_exchange_id(struct

> > nfs_client *clp, struct rpc_cred *cred,

> >         task_setup_data.callback_data = calldata;

> > 

> >         task = rpc_run_task(&task_setup_data);

> > -       if (IS_ERR(task)) {

> > -       status = PTR_ERR(task);

> > -               goto out_impl_id;

> > -       }

> > +       if (IS_ERR(task))

> > +               return PTR_ERR(task);

> > 

> >         if (!xprt) {

> >                 status = rpc_wait_for_completion_task(task);

> > @@ -7558,6 +7556,8 @@ static int _nfs4_proc_exchange_id(struct

> > nfs_client *clp, struct rpc_cred *cred,

> >                         clp->cl_implid->date.seconds,

> >                         clp->cl_implid->date.nseconds);

> >         dprintk("NFS reply exchange_id: %d\n", status);

> > +       if (status)

> > +               nfs_put_client(clp);


This needs to be in the "out_calldata" error path only. It isn't needed
once we've called rpc_run_task(). Otherwise the patch looks good.

> >         return status;

> > 

> >  out_impl_id:

> 

> Urgh. scratch this one, it's causing problems. Will try again.

> 

> 

> > --

> > 1.8.3.1

> > 

> > --

> > To unsubscribe from this list: send the line "unsubscribe linux-

> > nfs" in

> > the body of a message to majordomo@vger.kernel.org

> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

> 

> 

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com

Patch
diff mbox

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 59be0f7..3a79d3a 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7426,11 +7426,11 @@  static void nfs4_exchange_id_release(void *data)
 	struct nfs41_exchange_id_data *cdata =
 					(struct nfs41_exchange_id_data *)data;
 
-	nfs_put_client(cdata->args.client);
 	if (cdata->xprt) {
 		xprt_put(cdata->xprt);
 		rpc_clnt_xprt_switch_put(cdata->args.client->cl_rpcclient);
 	}
+	nfs_put_client(cdata->args.client);
 	kfree(cdata->res.impl_id);
 	kfree(cdata->res.server_scope);
 	kfree(cdata->res.server_owner);
@@ -7537,10 +7537,8 @@  static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred,
 	task_setup_data.callback_data = calldata;
 
 	task = rpc_run_task(&task_setup_data);
-	if (IS_ERR(task)) {
-	status = PTR_ERR(task);
-		goto out_impl_id;
-	}
+	if (IS_ERR(task))
+		return PTR_ERR(task);
 
 	if (!xprt) {
 		status = rpc_wait_for_completion_task(task);
@@ -7558,6 +7556,8 @@  static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred,
 			clp->cl_implid->date.seconds,
 			clp->cl_implid->date.nseconds);
 	dprintk("NFS reply exchange_id: %d\n", status);
+	if (status)
+		nfs_put_client(clp);
 	return status;
 
 out_impl_id: