diff mbox series

[nfsd-fixes] nfsd: fix nfsd4_deleg_getattr_conflict in presence of third party lease

Message ID 172488638886.4433.12153470259536099118@noble.neil.brown.name (mailing list archive)
State New
Headers show
Series [nfsd-fixes] nfsd: fix nfsd4_deleg_getattr_conflict in presence of third party lease | expand

Commit Message

NeilBrown Aug. 28, 2024, 11:06 p.m. UTC
It is not safe to dereference fl->c.flc_owner without first confirming
fl->fl_lmops is the expected manager.  nfsd4_deleg_getattr_conflict()
tests fl_lmops but largely ignores the result and assumes that flc_owner
is an nfs4_delegation anyway.  This is wrong.

With this patch we restore the "!= &nfsd_lease_mng_ops" case to behave
as it did before the changed mentioned below.  This the same as the
current code, but without any reference to a possible delegation.

Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4state.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Chuck Lever Aug. 29, 2024, 2:27 p.m. UTC | #1
On Thu, 29 Aug 2024 09:06:28 +1000, NeilBrown wrote:
> It is not safe to dereference fl->c.flc_owner without first confirming
> fl->fl_lmops is the expected manager.  nfsd4_deleg_getattr_conflict()
> tests fl_lmops but largely ignores the result and assumes that flc_owner
> is an nfs4_delegation anyway.  This is wrong.
> 
> With this patch we restore the "!= &nfsd_lease_mng_ops" case to behave
> as it did before the changed mentioned below.  This the same as the
> current code, but without any reference to a possible delegation.
> 
> [...]

Applied to nfsd-fixes for v6.11-rc, thanks!

[1/1] nfsd: fix nfsd4_deleg_getattr_conflict in presence of third party lease
      commit: 1271e30308ff96cc702bc7ef764fa6546c0541fb
Jeff Layton Aug. 30, 2024, 2:46 p.m. UTC | #2
On Thu, 2024-08-29 at 09:06 +1000, NeilBrown wrote:
> It is not safe to dereference fl->c.flc_owner without first confirming
> fl->fl_lmops is the expected manager.  nfsd4_deleg_getattr_conflict()
> tests fl_lmops but largely ignores the result and assumes that flc_owner
> is an nfs4_delegation anyway.  This is wrong.
> 
> With this patch we restore the "!= &nfsd_lease_mng_ops" case to behave
> as it did before the changed mentioned below.  This the same as the
> current code, but without any reference to a possible delegation.
> 
> Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 07f2496850c4..f6a67ef27435 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -8859,7 +8859,15 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
>  			 */
>  			if (type == F_RDLCK)
>  				break;
> -			goto break_lease;
> +
> +			nfsd_stats_wdeleg_getattr_inc(nn);
> +			spin_unlock(&ctx->flc_lock);
> +
> +			status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
> +			if (status != nfserr_jukebox ||
> +			    !nfsd_wait_for_delegreturn(rqstp, inode))
> +					return status;
> +			return 0;
>  		}
>  		if (type == F_WRLCK) {
>  			struct nfs4_delegation *dp = fl->c.flc_owner;
> @@ -8868,7 +8876,6 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
>  				spin_unlock(&ctx->flc_lock);
>  				return 0;
>  			}
> -break_lease:
>  			nfsd_stats_wdeleg_getattr_inc(nn);
>  			dp = fl->c.flc_owner;
>  			refcount_inc(&dp->dl_stid.sc_count);

I meant to send this the other day and got sidetracked!

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

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 07f2496850c4..f6a67ef27435 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -8859,7 +8859,15 @@  nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
 			 */
 			if (type == F_RDLCK)
 				break;
-			goto break_lease;
+
+			nfsd_stats_wdeleg_getattr_inc(nn);
+			spin_unlock(&ctx->flc_lock);
+
+			status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
+			if (status != nfserr_jukebox ||
+			    !nfsd_wait_for_delegreturn(rqstp, inode))
+					return status;
+			return 0;
 		}
 		if (type == F_WRLCK) {
 			struct nfs4_delegation *dp = fl->c.flc_owner;
@@ -8868,7 +8876,6 @@  nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
 				spin_unlock(&ctx->flc_lock);
 				return 0;
 			}
-break_lease:
 			nfsd_stats_wdeleg_getattr_inc(nn);
 			dp = fl->c.flc_owner;
 			refcount_inc(&dp->dl_stid.sc_count);