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 <[email protected]>
---
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
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
> [email protected]
>
On Mar 26, 2014, at 8:50, Steve Dickson <[email protected]> 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
[email protected]
On 03/26/2014 01:25 PM, Trond Myklebust wrote:
>
> On Mar 26, 2014, at 8:50, Steve Dickson <[email protected]> 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.
On Mar 26, 2014, at 10:48, Steve Dickson <[email protected]> wrote:
>
>
> On 03/26/2014 01:25 PM, Trond Myklebust wrote:
>>
>> On Mar 26, 2014, at 8:50, Steve Dickson <[email protected]> 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
[email protected]