[PATCH] NFSv4: Memory not being freed on memory allocation failure

[PATCH] NFSv4: Memory not being freed on memory allocation failure

[Steve Dickson]

nfs4_run_open_task() puts a ref count on the nfs4_opendata
data pointer, then tries to allocate the task pointer.
If that task allocation fails nfs4_run_open_task() returns
leaving the ref count on the data pointer.

That extra ref count stops the data pointer from being
freed in _nfs4_do_open() as expected.

This patch reverse the order of the task allocation and
ref count. The task is allocated before the ref count
is done, allowing the allocation to fail (and return)
without the ref count.

Signed-off-by: Steve Dickson <steved@redhat.com>
---
 fs/nfs/nfs4proc.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 2da6a69..a5a61ec 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1846,6 +1846,13 @@ static int nfs4_run_open_task(struct nfs4_opendata *data, int isrecover)
 	};
 	int status;
 
+	/*
+	 * Allocate the task before kref-ing the data
+	 */
+	task = rpc_run_task(&task_setup_data);
+	if (IS_ERR(task))
+		return PTR_ERR(task);
+
 	nfs4_init_sequence(&o_arg->seq_args, &o_res->seq_res, 1);
 	kref_get(&data->kref);
 	data->rpc_done = 0;
@@ -1856,16 +1863,13 @@ static int nfs4_run_open_task(struct nfs4_opendata *data, int isrecover)
 		nfs4_set_sequence_privileged(&o_arg->seq_args);
 		data->is_recover = 1;
 	}
-	task = rpc_run_task(&task_setup_data);
-        if (IS_ERR(task))
-                return PTR_ERR(task);
-        status = nfs4_wait_for_completion_rpc_task(task);
-        if (status != 0) {
-                data->cancelled = 1;
-                smp_wmb();
-        } else
-                status = data->rpc_status;
-        rpc_put_task(task);
+	status = nfs4_wait_for_completion_rpc_task(task);
+	if (status != 0) {
+		data->cancelled = 1;
+		smp_wmb();
+	} else
+		status = data->rpc_status;
+	rpc_put_task(task);
 
 	return status;
 }
-- 
1.8.3.1

Re: [PATCH] NFSv4: Memory not being freed on memory allocation failure

[Trond Myklebust]

On Mar 26, 2014, at 8:50, Steve Dickson <steved@redhat.com> wrote:

> nfs4_run_open_task() puts a ref count on the nfs4_opendata
> data pointer, then tries to allocate the task pointer.
> If that task allocation fails nfs4_run_open_task() returns
> leaving the ref count on the data pointer.
> 

Hi Steve,

That should not be the case. rpc_run_task() will always call nfs4_open_release() even if it returns an error.

Cheers
  Trond

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

Re: [PATCH] NFSv4: Memory not being freed on memory allocation failure

[Steve Dickson]

On 03/26/2014 01:25 PM, Trond Myklebust wrote:
> 
> On Mar 26, 2014, at 8:50, Steve Dickson <steved@redhat.com> wrote:
> 
>> nfs4_run_open_task() puts a ref count on the nfs4_opendata
>> data pointer, then tries to allocate the task pointer.
>> If that task allocation fails nfs4_run_open_task() returns
>> leaving the ref count on the data pointer.
>>
> 
> Hi Steve,
> 
> That should not be the case. rpc_run_task() will always call nfs4_open_release() even if it returns an error.
How can rpc_run_task() run without a task pointer? and I think 
you need a task allocated pointer to even call nfs4_open_release()

Here is what I'm seeing:

If rpc_alloc_task() fails, rpc_new_task() returns ERR_PTR(-ENOMEM)

In rpc_run_task() we do

task = rpc_new_task(task_setup_data);
if (IS_ERR(task))
   goto out;
:
:
:
out:    
    return task;

In nfs4_run_open_task() we do

kref_get(&data->kref);
:
:
:
task = rpc_run_task(&task_setup_data)
If (IS_ERR(task))
    return PTR_ERR(task);

What am I missing?

steved.

Re: [PATCH] NFSv4: Memory not being freed on memory allocation failure

[Trond Myklebust]

On Mar 26, 2014, at 10:48, Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 03/26/2014 01:25 PM, Trond Myklebust wrote:
>> 
>> On Mar 26, 2014, at 8:50, Steve Dickson <steved@redhat.com> wrote:
>> 
>>> nfs4_run_open_task() puts a ref count on the nfs4_opendata
>>> data pointer, then tries to allocate the task pointer.
>>> If that task allocation fails nfs4_run_open_task() returns
>>> leaving the ref count on the data pointer.
>>> 
>> 
>> Hi Steve,
>> 
>> That should not be the case. rpc_run_task() will always call nfs4_open_release() even if it returns an error.
> How can rpc_run_task() run without a task pointer? and I think 
> you need a task allocated pointer to even call nfs4_open_release()

No. nfs4_open_release() is called without an rpc task. This is how it is designed to work.

> 
> Here is what I'm seeing:
> 
> If rpc_alloc_task() fails, rpc_new_task() returns ERR_PTR(-ENOMEM)

Yes, but before returning ERR_PTR(-ENOMEM), it calls

rpc_release_calldata(setup_data->callback_ops, setup_data->callback_data);

which again calls rpc_ops->rpc_release(calldata)

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

Re: [PATCH] NFSv4: Memory not being freed on memory allocation failure

[Steve Dickson]

On 03/26/2014 02:39 PM, Trond Myklebust wrote:
>> Here is what I'm seeing:
>>
>> If rpc_alloc_task() fails, rpc_new_task() returns ERR_PTR(-ENOMEM)
> 
> Yes, but before returning ERR_PTR(-ENOMEM), it calls
> 
> rpc_release_calldata(setup_data->callback_ops, setup_data->callback_data);
> 
> which again calls rpc_ops->rpc_release(calldata)
Ah... I did miss that call... sorry or the noise... 

steved.


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