| Message ID | ca2f5e31d321b19fdb033f10ce6aec79c337a648.1709311699.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfs: fix UAF in direct writes | expand |
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 --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.
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(-)