diff mbox series

[1/2] nfs: properly protect nfs_direct_req fields

Message ID ca2f5e31d321b19fdb033f10ce6aec79c337a648.1709311699.git.josef@toxicpanda.com (mailing list archive)
State New, archived
Headers show
Series nfs: fix UAF in direct writes | expand

Commit Message

Josef Bacik March 1, 2024, 4:49 p.m. UTC
We protect accesses to the nfs_direct_req fields with the dreq->lock
ever where except nfs_direct_commit_complete.  This isn't a huge deal,
but it does lead to confusion, and we could potentially end up setting
NFS_ODIRECT_RESCHED_WRITES in one thread where we've had an error in
another.  Clean this up to properly protect ->error and ->flags in the
commit completion path.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/nfs/direct.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Jeff Layton March 4, 2024, 1:14 p.m. UTC | #1
On Fri, 2024-03-01 at 11:49 -0500, Josef Bacik wrote:
> We protect accesses to the nfs_direct_req fields with the dreq->lock
> ever where except nfs_direct_commit_complete.  This isn't a huge deal,
> but it does lead to confusion, and we could potentially end up setting
> NFS_ODIRECT_RESCHED_WRITES in one thread where we've had an error in
> another.  Clean this up to properly protect ->error and ->flags in the
> commit completion path.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/nfs/direct.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
> index c03926a1cc73..befcc167e25f 100644
> --- a/fs/nfs/direct.c
> +++ b/fs/nfs/direct.c
> @@ -606,6 +606,7 @@ static void nfs_direct_commit_complete(struct nfs_commit_data *data)
>  
>  	trace_nfs_direct_commit_complete(dreq);
>  
> +	spin_lock(&dreq->lock);
>  	if (status < 0) {
>  		/* Errors in commit are fatal */
>  		dreq->error = status;
> @@ -613,6 +614,7 @@ static void nfs_direct_commit_complete(struct nfs_commit_data *data)
>  	} else {
>  		status = dreq->error;
>  	}
> +	spin_unlock(&dreq->lock);
>  
>  	nfs_init_cinfo_from_dreq(&cinfo, dreq);
>  
> @@ -625,7 +627,10 @@ static void nfs_direct_commit_complete(struct nfs_commit_data *data)
>  			spin_unlock(&dreq->lock);
>  			nfs_release_request(req);
>  		} else if (!nfs_write_match_verf(verf, req)) {
> -			dreq->flags = NFS_ODIRECT_RESCHED_WRITES;
> +			spin_lock(&dreq->lock);
> +			if (dreq->flags == 0)
> +				dreq->flags = NFS_ODIRECT_RESCHED_WRITES;
> +			spin_unlock(&dreq->lock);
>  			/*
>  			 * Despite the reboot, the write was successful,
>  			 * so reset wb_nio.

Seems reasonable, even if just for consistency's sake:

Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff mbox series

Patch

diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index c03926a1cc73..befcc167e25f 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -606,6 +606,7 @@  static void nfs_direct_commit_complete(struct nfs_commit_data *data)
 
 	trace_nfs_direct_commit_complete(dreq);
 
+	spin_lock(&dreq->lock);
 	if (status < 0) {
 		/* Errors in commit are fatal */
 		dreq->error = status;
@@ -613,6 +614,7 @@  static void nfs_direct_commit_complete(struct nfs_commit_data *data)
 	} else {
 		status = dreq->error;
 	}
+	spin_unlock(&dreq->lock);
 
 	nfs_init_cinfo_from_dreq(&cinfo, dreq);
 
@@ -625,7 +627,10 @@  static void nfs_direct_commit_complete(struct nfs_commit_data *data)
 			spin_unlock(&dreq->lock);
 			nfs_release_request(req);
 		} else if (!nfs_write_match_verf(verf, req)) {
-			dreq->flags = NFS_ODIRECT_RESCHED_WRITES;
+			spin_lock(&dreq->lock);
+			if (dreq->flags == 0)
+				dreq->flags = NFS_ODIRECT_RESCHED_WRITES;
+			spin_unlock(&dreq->lock);
 			/*
 			 * Despite the reboot, the write was successful,
 			 * so reset wb_nio.