NFSv4: Memory not being freed on memory allocation failure

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.

---
 fs/nfs/nfs4proc.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

index 2da6a69..a5a61ec 100644
@@ -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

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
[email protected]

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.

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

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
[email protected]

Ah... I did miss that call... sorry or the noise...

steved.