| Message ID | 20170310213509.38215-1-kolga@netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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:
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(-)