[PATCH 1/2] NFSv4.0: Fix the wake up of the next waiter in nfs_release_seqid()

[PATCH 1/2] NFSv4.0: Fix the wake up of the next waiter in nfs_release_seqid()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

There is no need to wake up another waiter on the seqid list unless the
seqid being removed is at the head of the list, and so is relinquishing
control of the sequence counter to the next entry.

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

diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index dafd61186557..9a9f60a2291b 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1083,14 +1083,12 @@ void nfs_release_seqid(struct nfs_seqid *seqid)
 		return;
 	sequence = seqid->sequence;
 	spin_lock(&sequence->lock);
-	list_del_init(&seqid->list);
-	if (!list_empty(&sequence->list)) {
-		struct nfs_seqid *next;
-
-		next = list_first_entry(&sequence->list,
-				struct nfs_seqid, list);
+	if (list_is_first(&seqid->list, &sequence->list) &&
+	    !list_is_singular(&sequence->list)) {
+		struct nfs_seqid *next = list_next_entry(seqid, list);
 		rpc_wake_up_queued_task(&sequence->wait, next->task);
 	}
+	list_del_init(&seqid->list);
 	spin_unlock(&sequence->lock);
 }
 
-- 
2.47.0

[PATCH 2/2] NFSv4.0: Fix a use-after-free problem in the asynchronous open()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Yang Erkun reports that when two threads are opening files at the same
time, and are forced to abort before a reply is seen, then the call to
nfs_release_seqid() in nfs4_opendata_free() can result in a
use-after-free of the pointer to the defunct rpc task of the other
thread.
The fix is to ensure that if the RPC call is aborted before the call to
nfs_wait_on_sequence() is complete, then we must call nfs_release_seqid()
in nfs4_open_release() before the rpc_task is freed.

Reported-by: Yang Erkun <yangerkun@huawei.com>
Fixes: 24ac23ab88df ("NFSv4: Convert open() into an asynchronous RPC call")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/nfs4proc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9d40319e063d..405f17e6e0b4 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2603,12 +2603,14 @@ static void nfs4_open_release(void *calldata)
 	struct nfs4_opendata *data = calldata;
 	struct nfs4_state *state = NULL;
 
+	/* In case of error, no cleanup! */
+	if (data->rpc_status != 0 || !data->rpc_done) {
+		nfs_release_seqid(data->o_arg.seqid);
+		goto out_free;
+	}
 	/* If this request hasn't been cancelled, do nothing */
 	if (!data->cancelled)
 		goto out_free;
-	/* In case of error, no cleanup! */
-	if (data->rpc_status != 0 || !data->rpc_done)
-		goto out_free;
 	/* In case we need an open_confirm, no cleanup! */
 	if (data->o_res.rflags & NFS4_OPEN_RESULT_CONFIRM)
 		goto out_free;
-- 
2.47.0

Re: [PATCH 2/2] NFSv4.0: Fix a use-after-free problem in the asynchronous open()

[yangerkun]

LGTM

Reviewed-by: Yang Erkun <yangerkun@huawei.com>

在 2024/11/9 6:13, trondmy@kernel.org 写道:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> Yang Erkun reports that when two threads are opening files at the same
> time, and are forced to abort before a reply is seen, then the call to
> nfs_release_seqid() in nfs4_opendata_free() can result in a
> use-after-free of the pointer to the defunct rpc task of the other
> thread.
> The fix is to ensure that if the RPC call is aborted before the call to
> nfs_wait_on_sequence() is complete, then we must call nfs_release_seqid()
> in nfs4_open_release() before the rpc_task is freed.
> 
> Reported-by: Yang Erkun <yangerkun@huawei.com>
> Fixes: 24ac23ab88df ("NFSv4: Convert open() into an asynchronous RPC call")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>   fs/nfs/nfs4proc.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 9d40319e063d..405f17e6e0b4 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2603,12 +2603,14 @@ static void nfs4_open_release(void *calldata)
>   	struct nfs4_opendata *data = calldata;
>   	struct nfs4_state *state = NULL;
>   
> +	/* In case of error, no cleanup! */
> +	if (data->rpc_status != 0 || !data->rpc_done) {
> +		nfs_release_seqid(data->o_arg.seqid);
> +		goto out_free;
> +	}
>   	/* If this request hasn't been cancelled, do nothing */
>   	if (!data->cancelled)
>   		goto out_free;
> -	/* In case of error, no cleanup! */
> -	if (data->rpc_status != 0 || !data->rpc_done)
> -		goto out_free;
>   	/* In case we need an open_confirm, no cleanup! */
>   	if (data->o_res.rflags & NFS4_OPEN_RESULT_CONFIRM)
>   		goto out_free;

Re: [PATCH 2/2] NFSv4.0: Fix a use-after-free problem in the asynchronous open()

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

Found matching upstream commit: 2fdb05dc0931250574f0cb0ebeb5ed8e20f4a889

WARNING: Author mismatch between patch and found commit:
Backport author: trondmy@kernel.org
Commit author: Trond Myklebust <trond.myklebust@hammerspace.com>


Status in newer kernel trees:
6.12.y | Present (different SHA1: b56ae8e71555)

Note: The patch differs from the upstream commit:
---
Failed to apply patch cleanly, falling back to interdiff...
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-6.12.y       |  Failed     |  N/A       |
| stable/linux-6.6.y        |  Failed     |  N/A       |
| stable/linux-6.1.y        |  Failed     |  N/A       |
| stable/linux-5.15.y       |  Failed     |  N/A       |
| stable/linux-5.10.y       |  Failed     |  N/A       |
| stable/linux-5.4.y        |  Failed     |  N/A       |

Re: [PATCH 1/2] NFSv4.0: Fix the wake up of the next waiter in nfs_release_seqid()

[yangerkun]

LGTM

Reviewed-by: Yang Erkun <yangerkun@huawei.com>

在 2024/11/9 6:13, trondmy@kernel.org 写道:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> There is no need to wake up another waiter on the seqid list unless the
> seqid being removed is at the head of the list, and so is relinquishing
> control of the sequence counter to the next entry.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>   fs/nfs/nfs4state.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index dafd61186557..9a9f60a2291b 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -1083,14 +1083,12 @@ void nfs_release_seqid(struct nfs_seqid *seqid)
>   		return;
>   	sequence = seqid->sequence;
>   	spin_lock(&sequence->lock);
> -	list_del_init(&seqid->list);
> -	if (!list_empty(&sequence->list)) {
> -		struct nfs_seqid *next;
> -
> -		next = list_first_entry(&sequence->list,
> -				struct nfs_seqid, list);
> +	if (list_is_first(&seqid->list, &sequence->list) &&
> +	    !list_is_singular(&sequence->list)) {
> +		struct nfs_seqid *next = list_next_entry(seqid, list);
>   		rpc_wake_up_queued_task(&sequence->wait, next->task);
>   	}
> +	list_del_init(&seqid->list);
>   	spin_unlock(&sequence->lock);
>   }
>   

Re: [PATCH 1/2] NFSv4.0: Fix the wake up of the next waiter in nfs_release_seqid()

[Li Lingfeng]

Hi, I noticed that [PATCH 2] has been applied to the stable, but [PATCH 1]
has not.
Based on 6.6 where [PATCH 2] was merged, I can still reproduce the
original issue, and [PATCH 1] needs to be applied as well to resolve it.
It may be better to push [PATCH 1] to the stable as well.
Thanks.

在 2024/11/9 6:13, trondmy@kernel.org 写道:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> There is no need to wake up another waiter on the seqid list unless the
> seqid being removed is at the head of the list, and so is relinquishing
> control of the sequence counter to the next entry.
>
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>   fs/nfs/nfs4state.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index dafd61186557..9a9f60a2291b 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -1083,14 +1083,12 @@ void nfs_release_seqid(struct nfs_seqid *seqid)
>   		return;
>   	sequence = seqid->sequence;
>   	spin_lock(&sequence->lock);
> -	list_del_init(&seqid->list);
> -	if (!list_empty(&sequence->list)) {
> -		struct nfs_seqid *next;
> -
> -		next = list_first_entry(&sequence->list,
> -				struct nfs_seqid, list);
> +	if (list_is_first(&seqid->list, &sequence->list) &&
> +	    !list_is_singular(&sequence->list)) {
> +		struct nfs_seqid *next = list_next_entry(seqid, list);
>   		rpc_wake_up_queued_task(&sequence->wait, next->task);
>   	}
> +	list_del_init(&seqid->list);
>   	spin_unlock(&sequence->lock);
>   }
>   

Re: [PATCH 1/2] NFSv4.0: Fix the wake up of the next waiter in nfs_release_seqid()

[yangerkun]

You need cc stable.

在 2024/12/25 10:50, Li Lingfeng 写道:
> Hi, I noticed that [PATCH 2] has been applied to the stable, but [PATCH 1]
> has not.
> Based on 6.6 where [PATCH 2] was merged, I can still reproduce the
> original issue, and [PATCH 1] needs to be applied as well to resolve it.
> It may be better to push [PATCH 1] to the stable as well.
> Thanks.
> 
> 在 2024/11/9 6:13, trondmy@kernel.org 写道:
>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>>
>> There is no need to wake up another waiter on the seqid list unless the
>> seqid being removed is at the head of the list, and so is relinquishing
>> control of the sequence counter to the next entry.
>>
>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>> ---
>>   fs/nfs/nfs4state.c | 10 ++++------
>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index dafd61186557..9a9f60a2291b 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -1083,14 +1083,12 @@ void nfs_release_seqid(struct nfs_seqid *seqid)
>>           return;
>>       sequence = seqid->sequence;
>>       spin_lock(&sequence->lock);
>> -    list_del_init(&seqid->list);
>> -    if (!list_empty(&sequence->list)) {
>> -        struct nfs_seqid *next;
>> -
>> -        next = list_first_entry(&sequence->list,
>> -                struct nfs_seqid, list);
>> +    if (list_is_first(&seqid->list, &sequence->list) &&
>> +        !list_is_singular(&sequence->list)) {
>> +        struct nfs_seqid *next = list_next_entry(seqid, list);
>>           rpc_wake_up_queued_task(&sequence->wait, next->task);
>>       }
>> +    list_del_init(&seqid->list);
>>       spin_unlock(&sequence->lock);
>>   }

Re: [PATCH 1/2] NFSv4.0: Fix the wake up of the next waiter in nfs_release_seqid()

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

Found matching upstream commit: c968fd23c68e9929ab6cad4faffc8ea603e98e5d

WARNING: Author mismatch between patch and found commit:
Backport author: trondmy@kernel.org
Commit author: Trond Myklebust <trond.myklebust@hammerspace.com>


Status in newer kernel trees:
6.12.y | Not found

Note: The patch differs from the upstream commit:
---
1:  c968fd23c68e ! 1:  4b6aa0b050af NFSv4.0: Fix the wake up of the next waiter in nfs_release_seqid()
    @@ Commit message
         seqid being removed is at the head of the list, and so is relinquishing
         control of the sequence counter to the next entry.
     
    -    Reviewed-by: Yang Erkun <yangerkun@huawei.com>
         Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
     
      ## fs/nfs/nfs4state.c ##
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-6.12.y       |  Success    |  Success   |
| stable/linux-6.6.y        |  Success    |  Success   |
| stable/linux-6.1.y        |  Success    |  Success   |
| stable/linux-5.15.y       |  Success    |  Success   |
| stable/linux-5.10.y       |  Success    |  Success   |
| stable/linux-5.4.y        |  Success    |  Success   |