Hi folks,
As I was writing a new async operation I was looking at other async
functions for guidance (i.e.., nfs4_do_close()) and I noticed what
looks to me like a memory leak. Am I missing something?
I don=E2=80=99t see that =E2=80=9Ccalldata=E2=80=9D is freed if rpc_run_tas=
k() fails.
Thoughts?
Here=E2=80=99s what I have to fix it:
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index d293f06..408cb5b 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3317,8 +3317,10 @@ int nfs4_do_close(struct nfs4_state *state,
gfp_t gfp_mask, int wait)
msg.rpc_resp =3D &calldata->res;
task_setup_data.callback_data =3D calldata;
task =3D rpc_run_task(&task_setup_data);
- if (IS_ERR(task))
+ if (IS_ERR(task)) {
+ kfree(calldata);
return PTR_ERR(task);
+ }
status =3D 0;
if (wait)
status =3D rpc_wait_for_completion_task(task);
@@ -6023,6 +6025,7 @@ static struct rpc_task *nfs4_do_unlck(struct
file_lock *fl,
.workqueue =3D nfsiod_workqueue,
.flags =3D RPC_TASK_ASYNC,
};
+ struct rpc_task *task;
nfs4_state_protect(NFS_SERVER(lsp->ls_state->inode)->nfs_client,
NFS_SP4_MACH_CRED_CLEANUP, &task_setup_data.rpc_client, &msg);
@@ -6042,7 +6045,10 @@ static struct rpc_task *nfs4_do_unlck(struct
file_lock *fl,
msg.rpc_argp =3D &data->arg;
msg.rpc_resp =3D &data->res;
task_setup_data.callback_data =3D data;
- return rpc_run_task(&task_setup_data);
+ task =3D rpc_run_task(&task_setup_data);
+ if (IS_ERR(task))
+ kfree(data);
+ return task;
}
static int nfs4_proc_unlck(struct nfs4_state *state, int cmd, struct
file_lock *request)
@@ -6311,8 +6317,10 @@ static int _nfs4_do_setlk(struct nfs4_state
*state, int cmd, struct file_lock *f
} else
data->arg.new_lock =3D 1;
task =3D rpc_run_task(&task_setup_data);
- if (IS_ERR(task))
+ if (IS_ERR(task)) {
+ kfree(data);
return PTR_ERR(task);
+ }
ret =3D nfs4_wait_for_completion_rpc_task(task);
if (ret =3D=3D 0) {
ret =3D data->rpc_status;
Hi Olga,
On 02/10/2017 04:11 PM, Olga Kornievskaia wrote:
> Hi folks,
>
> As I was writing a new async operation I was looking at other async
> functions for guidance (i.e.., nfs4_do_close()) and I noticed what
> looks to me like a memory leak. Am I missing something?
>
> I don’t see that “calldata” is freed if rpc_run_task() fails.
I think the current code is okay. From what I can see, the only way rpc_run_task() can fail is if rpc_new_task() can't allocate the new task. If this happens, then rpc_new_task() calls rpc_release_calldata() to free the calldata.
Am I missing something?
Anna
>
> Thoughts?
>
> Here’s what I have to fix it:
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index d293f06..408cb5b 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -3317,8 +3317,10 @@ int nfs4_do_close(struct nfs4_state *state,
> gfp_t gfp_mask, int wait)
> msg.rpc_resp = &calldata->res;
> task_setup_data.callback_data = calldata;
> task = rpc_run_task(&task_setup_data);
> - if (IS_ERR(task))
> + if (IS_ERR(task)) {
> + kfree(calldata);
> return PTR_ERR(task);
Isn't this handled by
> + }
> status = 0;
> if (wait)
> status = rpc_wait_for_completion_task(task);
> @@ -6023,6 +6025,7 @@ static struct rpc_task *nfs4_do_unlck(struct
> file_lock *fl,
> .workqueue = nfsiod_workqueue,
> .flags = RPC_TASK_ASYNC,
> };
> + struct rpc_task *task;
>
>
>
> nfs4_state_protect(NFS_SERVER(lsp->ls_state->inode)->nfs_client,
> NFS_SP4_MACH_CRED_CLEANUP, &task_setup_data.rpc_client, &msg);
> @@ -6042,7 +6045,10 @@ static struct rpc_task *nfs4_do_unlck(struct
> file_lock *fl,
> msg.rpc_argp = &data->arg;
> msg.rpc_resp = &data->res;
> task_setup_data.callback_data = data;
> - return rpc_run_task(&task_setup_data);
> + task = rpc_run_task(&task_setup_data);
> + if (IS_ERR(task))
> + kfree(data);
> + return task;
> }
>
>
>
> static int nfs4_proc_unlck(struct nfs4_state *state, int cmd, struct
> file_lock *request)
> @@ -6311,8 +6317,10 @@ static int _nfs4_do_setlk(struct nfs4_state
> *state, int cmd, struct file_lock *f
> } else
> data->arg.new_lock = 1;
> task = rpc_run_task(&task_setup_data);
> - if (IS_ERR(task))
> + if (IS_ERR(task)) {
> + kfree(data);
> return PTR_ERR(task);
> + }
> ret = nfs4_wait_for_completion_rpc_task(task);
> if (ret == 0) {
> ret = data->rpc_status;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Fri, Feb 10, 2017 at 4:20 PM, Anna Schumaker
<[email protected]> wrote:
> Hi Olga,
>
> On 02/10/2017 04:11 PM, Olga Kornievskaia wrote:
>> Hi folks,
>>
>> As I was writing a new async operation I was looking at other async
>> functions for guidance (i.e.., nfs4_do_close()) and I noticed what
>> looks to me like a memory leak. Am I missing something?
>>
>> I don=E2=80=99t see that =E2=80=9Ccalldata=E2=80=9D is freed if rpc_run_=
task() fails.
>
> I think the current code is okay. From what I can see, the only way rpc_=
run_task() can fail is if rpc_new_task() can't allocate the new task. If t=
his happens, then rpc_new_task() calls rpc_release_calldata() to free the c=
alldata.
>
> Am I missing something?
That's what I was missing :) Ok that makes sense. So release of the
calldata should be in the rpc_release function. Thank you!
>
> Anna
>
>>
>> Thoughts?
>>
>> Here=E2=80=99s what I have to fix it:
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index d293f06..408cb5b 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -3317,8 +3317,10 @@ int nfs4_do_close(struct nfs4_state *state,
>> gfp_t gfp_mask, int wait)
>> msg.rpc_resp =3D &calldata->res;
>> task_setup_data.callback_data =3D calldata;
>> task =3D rpc_run_task(&task_setup_data);
>> - if (IS_ERR(task))
>> + if (IS_ERR(task)) {
>> + kfree(calldata);
>> return PTR_ERR(task);
>
> Isn't this handled by
>
>> + }
>> status =3D 0;
>> if (wait)
>> status =3D rpc_wait_for_completion_task(task);
>> @@ -6023,6 +6025,7 @@ static struct rpc_task *nfs4_do_unlck(struct
>> file_lock *fl,
>> .workqueue =3D nfsiod_workqueue,
>> .flags =3D RPC_TASK_ASYNC,
>> };
>> + struct rpc_task *task;
>>
>>
>>
>> nfs4_state_protect(NFS_SERVER(lsp->ls_state->inode)->nfs_client,
>> NFS_SP4_MACH_CRED_CLEANUP, &task_setup_data.rpc_client, &msg);
>> @@ -6042,7 +6045,10 @@ static struct rpc_task *nfs4_do_unlck(struct
>> file_lock *fl,
>> msg.rpc_argp =3D &data->arg;
>> msg.rpc_resp =3D &data->res;
>> task_setup_data.callback_data =3D data;
>> - return rpc_run_task(&task_setup_data);
>> + task =3D rpc_run_task(&task_setup_data);
>> + if (IS_ERR(task))
>> + kfree(data);
>> + return task;
>> }
>>
>>
>>
>> static int nfs4_proc_unlck(struct nfs4_state *state, int cmd, struct
>> file_lock *request)
>> @@ -6311,8 +6317,10 @@ static int _nfs4_do_setlk(struct nfs4_state
>> *state, int cmd, struct file_lock *f
>> } else
>> data->arg.new_lock =3D 1;
>> task =3D rpc_run_task(&task_setup_data);
>> - if (IS_ERR(task))
>> + if (IS_ERR(task)) {
>> + kfree(data);
>> return PTR_ERR(task);
>> + }
>> ret =3D nfs4_wait_for_completion_rpc_task(task);
>> if (ret =3D=3D 0) {
>> ret =3D data->rpc_status;
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>