diff mbox series

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

Message ID e82e8f89d5449c947c7e81e2bfe9c7c4a980c0d8.1731103952.git.trond.myklebust@hammerspace.com (mailing list archive)
State New
Headers show
Series [1/2] NFSv4.0: Fix the wake up of the next waiter in nfs_release_seqid() | expand

Commit Message

Trond Myklebust Nov. 8, 2024, 10:13 p.m. UTC 
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(-)

Comments

yangerkun Nov. 9, 2024, 2:25 a.m. UTC | #1 
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;
diff mbox series

Patch

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;