[PATCH 1/3] NFSv4: Fix a slot leak in nfs40_sequence_done

[PATCH 1/3] NFSv4: Fix a slot leak in nfs40_sequence_done

[Trond Myklebust]

The check for whether or not we sent an RPC call in nfs40_sequence_done
is insufficient to decide whether or not we are holding a session slot,
and thus should not be used to decide when to free that slot.

This patch replaces the RPC_WAS_SENT() test with the correct test for
whether or not slot == NULL.

Cc: Chuck Lever <chuck.lever@oracle.com>
Cc: stable@vger.kernel.org # 3.12+
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/nfs4proc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ae00c3ed733f..493e9cce1f11 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -539,7 +539,7 @@ static int nfs40_sequence_done(struct rpc_task *task,
 	struct nfs4_slot *slot = res->sr_slot;
 	struct nfs4_slot_table *tbl;
 
-	if (!RPC_WAS_SENT(task))
+	if (slot == NULL)
 		goto out;
 
 	tbl = slot->table;
-- 
1.8.5.3

[PATCH 2/3] NFSv4.1: Clean up nfs41_sequence_done

[Trond Myklebust]

Move the test for res->sr_slot == NULL out of the nfs41_sequence_free_slot
helper and into the main function for efficiency.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/nfs4proc.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 493e9cce1f11..42da6af77587 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -559,15 +559,10 @@ static void nfs41_sequence_free_slot(struct nfs4_sequence_res *res)
 {
 	struct nfs4_session *session;
 	struct nfs4_slot_table *tbl;
+	struct nfs4_slot *slot = res->sr_slot;
 	bool send_new_highest_used_slotid = false;
 
-	if (!res->sr_slot) {
-		/* just wake up the next guy waiting since
-		 * we may have not consumed a slot after all */
-		dprintk("%s: No slot\n", __func__);
-		return;
-	}
-	tbl = res->sr_slot->table;
+	tbl = slot->table;
 	session = tbl->session;
 
 	spin_lock(&tbl->slot_tbl_lock);
@@ -577,11 +572,11 @@ static void nfs41_sequence_free_slot(struct nfs4_sequence_res *res)
 	if (tbl->highest_used_slotid > tbl->target_highest_slotid)
 		send_new_highest_used_slotid = true;
 
-	if (nfs41_wake_and_assign_slot(tbl, res->sr_slot)) {
+	if (nfs41_wake_and_assign_slot(tbl, slot)) {
 		send_new_highest_used_slotid = false;
 		goto out_unlock;
 	}
-	nfs4_free_slot(tbl, res->sr_slot);
+	nfs4_free_slot(tbl, slot);
 
 	if (tbl->highest_used_slotid != NFS4_NO_SLOT)
 		send_new_highest_used_slotid = false;
@@ -595,16 +590,17 @@ out_unlock:
 int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *res)
 {
 	struct nfs4_session *session;
-	struct nfs4_slot *slot;
+	struct nfs4_slot *slot = res->sr_slot;
 	struct nfs_client *clp;
 	bool interrupted = false;
 	int ret = 1;
 
+	if (slot == NULL)
+		goto out_noaction;
 	/* don't increment the sequence number if the task wasn't sent */
 	if (!RPC_WAS_SENT(task))
 		goto out;
 
-	slot = res->sr_slot;
 	session = slot->table->session;
 
 	if (slot->interrupted) {
@@ -679,6 +675,7 @@ out:
 	/* The session may be reset by one of the error handlers. */
 	dprintk("%s: Error %d free the slot \n", __func__, res->sr_status);
 	nfs41_sequence_free_slot(res);
+out_noaction:
 	return ret;
 retry_nowait:
 	if (rpc_restart_call_prepare(task)) {
-- 
1.8.5.3

[PATCH 3/3] NFSv4.1: Cleanup

[Trond Myklebust]

It is now completely safe to call nfs41_sequence_free_slot with a NULL
slot.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/nfs4filelayout.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
index 20a56fa271bd..12c8132ad408 100644
--- a/fs/nfs/nfs4filelayout.c
+++ b/fs/nfs/nfs4filelayout.c
@@ -336,8 +336,7 @@ static void filelayout_read_call_done(struct rpc_task *task, void *data)
 
 	if (test_bit(NFS_IOHDR_REDO, &rdata->header->flags) &&
 	    task->tk_status == 0) {
-		if (rdata->res.seq_res.sr_slot != NULL)
-			nfs41_sequence_done(task, &rdata->res.seq_res);
+		nfs41_sequence_done(task, &rdata->res.seq_res);
 		return;
 	}
 
@@ -446,8 +445,7 @@ static void filelayout_write_call_done(struct rpc_task *task, void *data)
 
 	if (test_bit(NFS_IOHDR_REDO, &wdata->header->flags) &&
 	    task->tk_status == 0) {
-		if (wdata->res.seq_res.sr_slot != NULL)
-			nfs41_sequence_done(task, &wdata->res.seq_res);
+		nfs41_sequence_done(task, &wdata->res.seq_res);
 		return;
 	}
 
-- 
1.8.5.3

Re: [PATCH 1/3] NFSv4: Fix a slot leak in nfs40_sequence_done

[Chuck Lever]

On Jan 29, 2014, at 9:36 AM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:

> The check for whether or not we sent an RPC call in nfs40_sequence_done
> is insufficient to decide whether or not we are holding a session slot,
> and thus should not be used to decide when to free that slot.
> 
> This patch replaces the RPC_WAS_SENT() test with the correct test for
> whether or not slot == NULL.
> 
> Cc: Chuck Lever <chuck.lever@oracle.com>
> Cc: stable@vger.kernel.org # 3.12+
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
> fs/nfs/nfs4proc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index ae00c3ed733f..493e9cce1f11 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -539,7 +539,7 @@ static int nfs40_sequence_done(struct rpc_task *task,
> 	struct nfs4_slot *slot = res->sr_slot;
> 	struct nfs4_slot_table *tbl;
> 
> -	if (!RPC_WAS_SENT(task))
> +	if (slot == NULL)
> 		goto out;

When CONFIG_NFS_V4_1 is enabled, nfs4_sequence_done() already does the slot == NULL test, though the other nfs40_sequence_done() call sites do not.  This patch should clean that up?

> 
> 	tbl = slot->table;
> -- 
> 1.8.5.3
> 

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 1/3] NFSv4: Fix a slot leak in nfs40_sequence_done

[Trond Myklebust]

On Jan 30, 2014, at 11:49, Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Jan 29, 2014, at 9:36 AM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:
> 
>> The check for whether or not we sent an RPC call in nfs40_sequence_done
>> is insufficient to decide whether or not we are holding a session slot,
>> and thus should not be used to decide when to free that slot.
>> 
>> This patch replaces the RPC_WAS_SENT() test with the correct test for
>> whether or not slot == NULL.
>> 
>> Cc: Chuck Lever <chuck.lever@oracle.com>
>> Cc: stable@vger.kernel.org # 3.12+
>> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>> ---
>> fs/nfs/nfs4proc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index ae00c3ed733f..493e9cce1f11 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -539,7 +539,7 @@ static int nfs40_sequence_done(struct rpc_task *task,
>> 	struct nfs4_slot *slot = res->sr_slot;
>> 	struct nfs4_slot_table *tbl;
>> 
>> -	if (!RPC_WAS_SENT(task))
>> +	if (slot == NULL)
>> 		goto out;
> 
> When CONFIG_NFS_V4_1 is enabled, nfs4_sequence_done() already does the slot == NULL test, though the other nfs40_sequence_done() call sites do not.  This patch should clean that up?

Unfortunately we can’t touch nfs4_sequence_done: it wants to test for whether or not a NFSv4.1 session exists, which requires it to look at slot->table->session. :-(
--
Trond Myklebust
Linux NFS client maintainer

Re: [PATCH 1/3] NFSv4: Fix a slot leak in nfs40_sequence_done

[Chuck Lever]

On Jan 30, 2014, at 8:55 AM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:

> 
> On Jan 30, 2014, at 11:49, Chuck Lever <chuck.lever@oracle.com> wrote:
> 
>> 
>> On Jan 29, 2014, at 9:36 AM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:
>> 
>>> The check for whether or not we sent an RPC call in nfs40_sequence_done
>>> is insufficient to decide whether or not we are holding a session slot,
>>> and thus should not be used to decide when to free that slot.
>>> 
>>> This patch replaces the RPC_WAS_SENT() test with the correct test for
>>> whether or not slot == NULL.
>>> 
>>> Cc: Chuck Lever <chuck.lever@oracle.com>
>>> Cc: stable@vger.kernel.org # 3.12+
>>> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>>> ---
>>> fs/nfs/nfs4proc.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index ae00c3ed733f..493e9cce1f11 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -539,7 +539,7 @@ static int nfs40_sequence_done(struct rpc_task *task,
>>> 	struct nfs4_slot *slot = res->sr_slot;
>>> 	struct nfs4_slot_table *tbl;
>>> 
>>> -	if (!RPC_WAS_SENT(task))
>>> +	if (slot == NULL)
>>> 		goto out;
>> 
>> When CONFIG_NFS_V4_1 is enabled, nfs4_sequence_done() already does the slot == NULL test, though the other nfs40_sequence_done() call sites do not.  This patch should clean that up?
> 
> Unfortunately we can’t touch nfs4_sequence_done: it wants to test for whether or not a NFSv4.1 session exists, which requires it to look at slot->table->session. :-(

Move the slot == NULL check to all nfs40_sequence_done() call sites?  <shrug>

If not, can we get a code comment that explains why that check is done twice in the common path?

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 1/3] NFSv4: Fix a slot leak in nfs40_sequence_done

[Trond Myklebust]

On Jan 30, 2014, at 12:02, Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Jan 30, 2014, at 8:55 AM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:
> 
>> 
>> On Jan 30, 2014, at 11:49, Chuck Lever <chuck.lever@oracle.com> wrote:
>> 
>>> 
>>> On Jan 29, 2014, at 9:36 AM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:
>>> 
>>>> The check for whether or not we sent an RPC call in nfs40_sequence_done
>>>> is insufficient to decide whether or not we are holding a session slot,
>>>> and thus should not be used to decide when to free that slot.
>>>> 
>>>> This patch replaces the RPC_WAS_SENT() test with the correct test for
>>>> whether or not slot == NULL.
>>>> 
>>>> Cc: Chuck Lever <chuck.lever@oracle.com>
>>>> Cc: stable@vger.kernel.org # 3.12+
>>>> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>>>> ---
>>>> fs/nfs/nfs4proc.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>> index ae00c3ed733f..493e9cce1f11 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -539,7 +539,7 @@ static int nfs40_sequence_done(struct rpc_task *task,
>>>> 	struct nfs4_slot *slot = res->sr_slot;
>>>> 	struct nfs4_slot_table *tbl;
>>>> 
>>>> -	if (!RPC_WAS_SENT(task))
>>>> +	if (slot == NULL)
>>>> 		goto out;
>>> 
>>> When CONFIG_NFS_V4_1 is enabled, nfs4_sequence_done() already does the slot == NULL test, though the other nfs40_sequence_done() call sites do not.  This patch should clean that up?
>> 
>> Unfortunately we can’t touch nfs4_sequence_done: it wants to test for whether or not a NFSv4.1 session exists, which requires it to look at slot->table->session. :-(
> 
> Move the slot == NULL check to all nfs40_sequence_done() call sites?  <shrug>

No...

--
Trond Myklebust
Linux NFS client maintainer

Re: [PATCH 1/3] NFSv4: Fix a slot leak in nfs40_sequence_done

[Trond Myklebust]

On Jan 30, 2014, at 12:03, Trond Myklebust <trond.myklebust@primarydata.com> wrote:

> 
> On Jan 30, 2014, at 12:02, Chuck Lever <chuck.lever@oracle.com> wrote:
> 
>> 
>> On Jan 30, 2014, at 8:55 AM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:
>> 
>>> 
>>> On Jan 30, 2014, at 11:49, Chuck Lever <chuck.lever@oracle.com> wrote:
>>> 
>>>> 
>>>> On Jan 29, 2014, at 9:36 AM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:
>>>> 
>>>>> The check for whether or not we sent an RPC call in nfs40_sequence_done
>>>>> is insufficient to decide whether or not we are holding a session slot,
>>>>> and thus should not be used to decide when to free that slot.
>>>>> 
>>>>> This patch replaces the RPC_WAS_SENT() test with the correct test for
>>>>> whether or not slot == NULL.
>>>>> 
>>>>> Cc: Chuck Lever <chuck.lever@oracle.com>
>>>>> Cc: stable@vger.kernel.org # 3.12+
>>>>> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>>>>> ---
>>>>> fs/nfs/nfs4proc.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>>> index ae00c3ed733f..493e9cce1f11 100644
>>>>> --- a/fs/nfs/nfs4proc.c
>>>>> +++ b/fs/nfs/nfs4proc.c
>>>>> @@ -539,7 +539,7 @@ static int nfs40_sequence_done(struct rpc_task *task,
>>>>> 	struct nfs4_slot *slot = res->sr_slot;
>>>>> 	struct nfs4_slot_table *tbl;
>>>>> 
>>>>> -	if (!RPC_WAS_SENT(task))
>>>>> +	if (slot == NULL)
>>>>> 		goto out;
>>>> 
>>>> When CONFIG_NFS_V4_1 is enabled, nfs4_sequence_done() already does the slot == NULL test, though the other nfs40_sequence_done() call sites do not.  This patch should clean that up?
>>> 
>>> Unfortunately we can’t touch nfs4_sequence_done: it wants to test for whether or not a NFSv4.1 session exists, which requires it to look at slot->table->session. :-(
>> 
>> Move the slot == NULL check to all nfs40_sequence_done() call sites?  <shrug>
> 
> No...

We could move the entire function minus the slot == NULL test into a helper that could be called by nfs4_sequence_done, but I’m not convinced that particular micro-optimisation is really worth it.

--
Trond Myklebust
Linux NFS client maintainer