[PATCH 0/2] nfsd: CB_GETATTR fixes

[PATCH 0/2] nfsd: CB_GETATTR fixes

[Jeff Layton]

Fixes for a couple of CB_GETATTR bugs I found while working on the
delstid set. Mostly this just ensures that we hold references to the
delegation while working with it.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Jeff Layton (2):
      nfsd: hold reference to delegation when updating it for cb_getattr
      nfsd: fix potential UAF in nfsd4_cb_getattr_release

 fs/nfsd/nfs4state.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
---
base-commit: a204501e1743d695ca2930ed25a2be9f8ced96d3
change-id: 20240823-nfsd-fixes-61f0c785d125

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH 1/2] nfsd: hold reference to delegation when updating it for cb_getattr

[Jeff Layton]

Once we've dropped the flc_lock, there is nothing that ensures that the
delegation that was found will still be around later. Take a reference
to it while holding the lock and then drop it when we've finished with
the delegation.

Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index dafff707e23a..19d39872be32 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -8837,7 +8837,6 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
 	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
 	struct file_lock_context *ctx;
 	struct file_lease *fl;
-	struct nfs4_delegation *dp;
 	struct iattr attrs;
 	struct nfs4_cb_fattr *ncf;
 
@@ -8862,7 +8861,8 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
 			goto break_lease;
 		}
 		if (type == F_WRLCK) {
-			dp = fl->c.flc_owner;
+			struct nfs4_delegation *dp = fl->c.flc_owner;
+
 			if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
 				spin_unlock(&ctx->flc_lock);
 				return 0;
@@ -8870,6 +8870,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
 break_lease:
 			nfsd_stats_wdeleg_getattr_inc(nn);
 			dp = fl->c.flc_owner;
+			refcount_inc(&dp->dl_stid.sc_count);
 			ncf = &dp->dl_cb_fattr;
 			nfs4_cb_getattr(&dp->dl_cb_fattr);
 			spin_unlock(&ctx->flc_lock);
@@ -8879,8 +8880,10 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
 				/* Recall delegation only if client didn't respond */
 				status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
 				if (status != nfserr_jukebox ||
-						!nfsd_wait_for_delegreturn(rqstp, inode))
+						!nfsd_wait_for_delegreturn(rqstp, inode)) {
+					nfs4_put_stid(&dp->dl_stid);
 					return status;
+				}
 			}
 			if (!ncf->ncf_file_modified &&
 					(ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
@@ -8900,6 +8903,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
 				*size = ncf->ncf_cur_fsize;
 				*modified = true;
 			}
+			nfs4_put_stid(&dp->dl_stid);
 			return 0;
 		}
 		break;

-- 
2.46.0

Re: [PATCH 1/2] nfsd: hold reference to delegation when updating it for cb_getattr

[Chuck Lever]

On Fri, Aug 23, 2024 at 06:27:38PM -0400, Jeff Layton wrote:
> Once we've dropped the flc_lock, there is nothing that ensures that the
> delegation that was found will still be around later. Take a reference
> to it while holding the lock and then drop it when we've finished with
> the delegation.
> 
> Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/nfs4state.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index dafff707e23a..19d39872be32 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -8837,7 +8837,6 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
>  	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>  	struct file_lock_context *ctx;
>  	struct file_lease *fl;
> -	struct nfs4_delegation *dp;
>  	struct iattr attrs;
>  	struct nfs4_cb_fattr *ncf;
>  
> @@ -8862,7 +8861,8 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
>  			goto break_lease;
>  		}
>  		if (type == F_WRLCK) {
> -			dp = fl->c.flc_owner;
> +			struct nfs4_delegation *dp = fl->c.flc_owner;

Setting @dp here seems redundant; just below, after the break_lease
label it is set again to the same value. May I change this line to:

			struct nfs4_delegation *dp;

> +
>  			if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
>  				spin_unlock(&ctx->flc_lock);
>  				return 0;
> @@ -8870,6 +8870,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
>  break_lease:
>  			nfsd_stats_wdeleg_getattr_inc(nn);
>  			dp = fl->c.flc_owner;
> +			refcount_inc(&dp->dl_stid.sc_count);
>  			ncf = &dp->dl_cb_fattr;
>  			nfs4_cb_getattr(&dp->dl_cb_fattr);
>  			spin_unlock(&ctx->flc_lock);
> @@ -8879,8 +8880,10 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
>  				/* Recall delegation only if client didn't respond */
>  				status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
>  				if (status != nfserr_jukebox ||
> -						!nfsd_wait_for_delegreturn(rqstp, inode))
> +						!nfsd_wait_for_delegreturn(rqstp, inode)) {
> +					nfs4_put_stid(&dp->dl_stid);
>  					return status;
> +				}
>  			}
>  			if (!ncf->ncf_file_modified &&
>  					(ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
> @@ -8900,6 +8903,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
>  				*size = ncf->ncf_cur_fsize;
>  				*modified = true;
>  			}
> +			nfs4_put_stid(&dp->dl_stid);
>  			return 0;
>  		}
>  		break;
> 
> -- 
> 2.46.0
> 

-- 
Chuck Lever

Re: [PATCH 1/2] nfsd: hold reference to delegation when updating it for cb_getattr

[Jeff Layton]

On Sat, 2024-08-24 at 11:03 -0400, Chuck Lever wrote:
> On Fri, Aug 23, 2024 at 06:27:38PM -0400, Jeff Layton wrote:
> > Once we've dropped the flc_lock, there is nothing that ensures that the
> > delegation that was found will still be around later. Take a reference
> > to it while holding the lock and then drop it when we've finished with
> > the delegation.
> > 
> > Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/nfs4state.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index dafff707e23a..19d39872be32 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -8837,7 +8837,6 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
> >  	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> >  	struct file_lock_context *ctx;
> >  	struct file_lease *fl;
> > -	struct nfs4_delegation *dp;
> >  	struct iattr attrs;
> >  	struct nfs4_cb_fattr *ncf;
> >  
> > @@ -8862,7 +8861,8 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
> >  			goto break_lease;
> >  		}
> >  		if (type == F_WRLCK) {
> > -			dp = fl->c.flc_owner;
> > +			struct nfs4_delegation *dp = fl->c.flc_owner;
> 
> Setting @dp here seems redundant; just below, after the break_lease
> label it is set again to the same value. May I change this line to:
> 
> 			struct nfs4_delegation *dp;
> 

I don't think you can just remove that one since it's dereferenced just
after that. The problem is the goto break_lease case needs to have that
assigned too so you also can't just remove the later one.

The way the code flows here is weird, unfortunately, but I don't see an
easy way to improve it right offhand. Maybe assign "dp" just before the
"goto break_lease" ?


> > +
> >  			if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
> >  				spin_unlock(&ctx->flc_lock);
> >  				return 0;
> > @@ -8870,6 +8870,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
> >  break_lease:
> >  			nfsd_stats_wdeleg_getattr_inc(nn);
> >  			dp = fl->c.flc_owner;
> > +			refcount_inc(&dp->dl_stid.sc_count);
> >  			ncf = &dp->dl_cb_fattr;
> >  			nfs4_cb_getattr(&dp->dl_cb_fattr);
> >  			spin_unlock(&ctx->flc_lock);
> > @@ -8879,8 +8880,10 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
> >  				/* Recall delegation only if client didn't respond */
> >  				status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
> >  				if (status != nfserr_jukebox ||
> > -						!nfsd_wait_for_delegreturn(rqstp, inode))
> > +						!nfsd_wait_for_delegreturn(rqstp, inode)) {
> > +					nfs4_put_stid(&dp->dl_stid);
> >  					return status;
> > +				}
> >  			}
> >  			if (!ncf->ncf_file_modified &&
> >  					(ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
> > @@ -8900,6 +8903,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
> >  				*size = ncf->ncf_cur_fsize;
> >  				*modified = true;
> >  			}
> > +			nfs4_put_stid(&dp->dl_stid);
> >  			return 0;
> >  		}
> >  		break;
> > 
> > -- 
> > 2.46.0
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>

[PATCH 2/2] nfsd: fix potential UAF in nfsd4_cb_getattr_release

[Jeff Layton]

Once we drop the delegation reference, the fields embedded in it are no
longer safe to access. Do that last.

Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 19d39872be32..02d43f95146e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3078,9 +3078,9 @@ nfsd4_cb_getattr_release(struct nfsd4_callback *cb)
 	struct nfs4_delegation *dp =
 			container_of(ncf, struct nfs4_delegation, dl_cb_fattr);
 
-	nfs4_put_stid(&dp->dl_stid);
 	clear_bit(CB_GETATTR_BUSY, &ncf->ncf_cb_flags);
 	wake_up_bit(&ncf->ncf_cb_flags, CB_GETATTR_BUSY);
+	nfs4_put_stid(&dp->dl_stid);
 }
 
 static const struct nfsd4_callback_ops nfsd4_cb_recall_any_ops = {

-- 
2.46.0

Re: [PATCH 0/2] nfsd: CB_GETATTR fixes

[Chuck Lever]

On Fri, Aug 23, 2024 at 06:27:37PM -0400, Jeff Layton wrote:
> Fixes for a couple of CB_GETATTR bugs I found while working on the
> delstid set. Mostly this just ensures that we hold references to the
> delegation while working with it.
> 
> 

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

[1/2] nfsd: hold reference to delegation when updating it for cb_getattr
      commit: 8fceb5f6636bbbf803fe29fff59f138206559964
[2/2] nfsd: fix potential UAF in nfsd4_cb_getattr_release
      commit: 8bc97f9b84c8852fcc56be2382f5115c518de785

-- 
Chuck Lever

Re: [PATCH 0/2] nfsd: CB_GETATTR fixes

[NeilBrown]

On Sun, 25 Aug 2024, Chuck Lever wrote:
> On Fri, Aug 23, 2024 at 06:27:37PM -0400, Jeff Layton wrote:
> > Fixes for a couple of CB_GETATTR bugs I found while working on the
> > delstid set. Mostly this just ensures that we hold references to the
> > delegation while working with it.
> > 
> > 
> 
> Applied to nfsd-fixes for v6.11-rc, thanks!
> 
> [1/2] nfsd: hold reference to delegation when updating it for cb_getattr
>       commit: 8fceb5f6636bbbf803fe29fff59f138206559964
> [2/2] nfsd: fix potential UAF in nfsd4_cb_getattr_release
>       commit: 8bc97f9b84c8852fcc56be2382f5115c518de785
> 
> -- 
> Chuck Lever
> 

Maybe the following can tidy up that code.  I can split this into
a few separate patches if you like.
Thoughts?

Note that the patch is easier to review if you apply it then use "git
diff -b".

NeilBrown


From: NeilBrown <neilb@suse.de>
Subject: [PATCH] nfsd: untangle code in nfsd4_deleg_getattr_conflict()

The code in nfsd4_deleg_getattr_conflict() is convoluted and buggy.

With this patch we:
 - properly handle non-nfsd leases.  We must not assume flc_owner is a
    delegation unless fl_lmops == &nfsd_lease_mng_ops
 - move the main code out of the for loop
 - have a single exit which calls nfs4_put_stid()
   (and other exits which don't need to call that)

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

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2c4b9a22b2bb..7672fa7a70f3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -8837,6 +8837,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
 	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
 	struct inode *inode = d_inode(dentry);
 	struct file_lock_context *ctx;
+	struct nfs4_delegation *dp = NULL;
 	struct nfs4_cb_fattr *ncf;
 	struct file_lease *fl;
 	struct iattr attrs;
@@ -8845,77 +8846,76 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
 	ctx = locks_inode_context(inode);
 	if (!ctx)
 		return 0;
+
+#define NON_NFSD_LEASE ((void*)1)
+
 	spin_lock(&ctx->flc_lock);
 	for_each_file_lock(fl, &ctx->flc_lease) {
-		unsigned char type = fl->c.flc_type;
-
 		if (fl->c.flc_flags == FL_LAYOUT)
 			continue;
-		if (fl->fl_lmops != &nfsd_lease_mng_ops) {
-			/*
-			 * non-nfs lease, if it's a lease with F_RDLCK then
-			 * we are done; there isn't any write delegation
-			 * on this inode
-			 */
-			if (type == F_RDLCK)
-				break;
-			goto break_lease;
-		}
-		if (type == F_WRLCK) {
-			struct nfs4_delegation *dp = fl->c.flc_owner;
-
-			if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
-				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);
-			ncf = &dp->dl_cb_fattr;
-			nfs4_cb_getattr(&dp->dl_cb_fattr);
-			spin_unlock(&ctx->flc_lock);
-			wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
-					TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
-			if (ncf->ncf_cb_status) {
-				/* Recall delegation only if client didn't respond */
-				status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
-				if (status != nfserr_jukebox ||
-						!nfsd_wait_for_delegreturn(rqstp, inode)) {
-					nfs4_put_stid(&dp->dl_stid);
-					return status;
-				}
-			}
-			if (!ncf->ncf_file_modified &&
-					(ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
-					ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
-				ncf->ncf_file_modified = true;
-			if (ncf->ncf_file_modified) {
-				int err;
-
-				/*
-				 * Per section 10.4.3 of RFC 8881, the server would
-				 * not update the file's metadata with the client's
-				 * modified size
-				 */
-				attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
-				attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG;
-				inode_lock(inode);
-				err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
-				inode_unlock(inode);
-				if (err) {
-					nfs4_put_stid(&dp->dl_stid);
-					return nfserrno(err);
-				}
-				ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
-				*size = ncf->ncf_cur_fsize;
-				*modified = true;
-			}
-			nfs4_put_stid(&dp->dl_stid);
-			return 0;
+		if (fl->c.flc_type == F_WRLCK) {
+			if (fl->fl_lmops == &nfsd_lease_mng_ops)
+				dp = fl->c.flc_owner;
+			else
+				dp = NON_NFSD_LEASE;
 		}
 		break;
 	}
+	if (dp == NULL || dp == NON_NFSD_LEASE ||
+	    dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
+		spin_unlock(&ctx->flc_lock);
+		if (dp == NON_NFSD_LEASE) {
+			status = nfserrno(nfsd_open_break_lease(inode,
+								NFSD_MAY_READ));
+			if (status != nfserr_jukebox ||
+			    !nfsd_wait_for_delegreturn(rqstp, inode))
+				return status;
+		}
+		return 0;
+	}
+
+	nfsd_stats_wdeleg_getattr_inc(nn);
+	refcount_inc(&dp->dl_stid.sc_count);
+	ncf = &dp->dl_cb_fattr;
+	nfs4_cb_getattr(&dp->dl_cb_fattr);
 	spin_unlock(&ctx->flc_lock);
-	return 0;
+
+	wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
+			    TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
+	if (ncf->ncf_cb_status) {
+		/* Recall delegation only if client didn't respond */
+		status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
+		if (status != nfserr_jukebox ||
+		    !nfsd_wait_for_delegreturn(rqstp, inode))
+			goto out_status;
+	}
+	if (!ncf->ncf_file_modified &&
+	    (ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
+	     ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
+		ncf->ncf_file_modified = true;
+	if (ncf->ncf_file_modified) {
+		int err;
+
+		/*
+		 * Per section 10.4.3 of RFC 8881, the server would
+		 * not update the file's metadata with the client's
+		 * modified size
+		 */
+		attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
+		attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG;
+		inode_lock(inode);
+		err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
+		inode_unlock(inode);
+		if (err) {
+			status = nfserrno(err);
+			goto out_status;
+		}
+		ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
+		*size = ncf->ncf_cur_fsize;
+		*modified = true;
+	}
+	status = 0;
+out_status:
+	nfs4_put_stid(&dp->dl_stid);
+	return status;
 }
-- 
2.44.0

Re: [PATCH 0/2] nfsd: CB_GETATTR fixes

[Jeff Layton]

On Mon, 2024-08-26 at 09:22 +1000, NeilBrown wrote:
> On Sun, 25 Aug 2024, Chuck Lever wrote:
> > On Fri, Aug 23, 2024 at 06:27:37PM -0400, Jeff Layton wrote:
> > > Fixes for a couple of CB_GETATTR bugs I found while working on the
> > > delstid set. Mostly this just ensures that we hold references to the
> > > delegation while working with it.
> > > 
> > > 
> > 
> > Applied to nfsd-fixes for v6.11-rc, thanks!
> > 
> > [1/2] nfsd: hold reference to delegation when updating it for cb_getattr
> >       commit: 8fceb5f6636bbbf803fe29fff59f138206559964
> > [2/2] nfsd: fix potential UAF in nfsd4_cb_getattr_release
> >       commit: 8bc97f9b84c8852fcc56be2382f5115c518de785
> > 
> > -- 
> > Chuck Lever
> > 
> 
> Maybe the following can tidy up that code.  I can split this into
> a few separate patches if you like.
> Thoughts?
> 
> Note that the patch is easier to review if you apply it then use "git
> diff -b".
> 
> NeilBrown
> 
> 
> From: NeilBrown <neilb@suse.de>
> Subject: [PATCH] nfsd: untangle code in nfsd4_deleg_getattr_conflict()
> 
> The code in nfsd4_deleg_getattr_conflict() is convoluted and buggy.
> 
> With this patch we:
>  - properly handle non-nfsd leases.  We must not assume flc_owner is a
>     delegation unless fl_lmops == &nfsd_lease_mng_ops

AFAICT, non-nfsd leases are already properly handled (though I do agree
that the "flow" of this code is awkward). What case do you see that's
wrong?

>  - move the main code out of the for loop
>  - have a single exit which calls nfs4_put_stid()
>    (and other exits which don't need to call that)
> 
> Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 130 ++++++++++++++++++++++----------------------
>  1 file changed, 65 insertions(+), 65 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2c4b9a22b2bb..7672fa7a70f3 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -8837,6 +8837,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
>  	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>  	struct inode *inode = d_inode(dentry);
>  	struct file_lock_context *ctx;
> +	struct nfs4_delegation *dp = NULL;
>  	struct nfs4_cb_fattr *ncf;
>  	struct file_lease *fl;
>  	struct iattr attrs;
> @@ -8845,77 +8846,76 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
>  	ctx = locks_inode_context(inode);
>  	if (!ctx)
>  		return 0;
> +
> +#define NON_NFSD_LEASE ((void*)1)
> +
>  	spin_lock(&ctx->flc_lock);
>  	for_each_file_lock(fl, &ctx->flc_lease) {
> -		unsigned char type = fl->c.flc_type;
> -
>  		if (fl->c.flc_flags == FL_LAYOUT)
>  			continue;
> -		if (fl->fl_lmops != &nfsd_lease_mng_ops) {
> -			/*
> -			 * non-nfs lease, if it's a lease with F_RDLCK then
> -			 * we are done; there isn't any write delegation
> -			 * on this inode
> -			 */
> -			if (type == F_RDLCK)
> -				break;
> -			goto break_lease;
> -		}
> -		if (type == F_WRLCK) {
> -			struct nfs4_delegation *dp = fl->c.flc_owner;
> -
> -			if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
> -				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);
> -			ncf = &dp->dl_cb_fattr;
> -			nfs4_cb_getattr(&dp->dl_cb_fattr);
> -			spin_unlock(&ctx->flc_lock);
> -			wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
> -					TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
> -			if (ncf->ncf_cb_status) {
> -				/* Recall delegation only if client didn't respond */
> -				status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
> -				if (status != nfserr_jukebox ||
> -						!nfsd_wait_for_delegreturn(rqstp, inode)) {
> -					nfs4_put_stid(&dp->dl_stid);
> -					return status;
> -				}
> -			}
> -			if (!ncf->ncf_file_modified &&
> -					(ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
> -					ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
> -				ncf->ncf_file_modified = true;
> -			if (ncf->ncf_file_modified) {
> -				int err;
> -
> -				/*
> -				 * Per section 10.4.3 of RFC 8881, the server would
> -				 * not update the file's metadata with the client's
> -				 * modified size
> -				 */
> -				attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
> -				attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG;
> -				inode_lock(inode);
> -				err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
> -				inode_unlock(inode);
> -				if (err) {
> -					nfs4_put_stid(&dp->dl_stid);
> -					return nfserrno(err);
> -				}
> -				ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
> -				*size = ncf->ncf_cur_fsize;
> -				*modified = true;
> -			}
> -			nfs4_put_stid(&dp->dl_stid);
> -			return 0;
> +		if (fl->c.flc_type == F_WRLCK) {
> +			if (fl->fl_lmops == &nfsd_lease_mng_ops)
> +				dp = fl->c.flc_owner;
> +			else
> +				dp = NON_NFSD_LEASE;
>  		}
>  		break;
>  	}
> +	if (dp == NULL || dp == NON_NFSD_LEASE ||
> +	    dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
> +		spin_unlock(&ctx->flc_lock);
> +		if (dp == NON_NFSD_LEASE) {
> +			status = nfserrno(nfsd_open_break_lease(inode,
> +								NFSD_MAY_READ));
> +			if (status != nfserr_jukebox ||
> +			    !nfsd_wait_for_delegreturn(rqstp, inode))
> +				return status;
> +		}
> +		return 0;
> +	}
> +
> +	nfsd_stats_wdeleg_getattr_inc(nn);
> +	refcount_inc(&dp->dl_stid.sc_count);
> +	ncf = &dp->dl_cb_fattr;
> +	nfs4_cb_getattr(&dp->dl_cb_fattr);
>  	spin_unlock(&ctx->flc_lock);
> -	return 0;
> +
> +	wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
> +			    TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
> +	if (ncf->ncf_cb_status) {
> +		/* Recall delegation only if client didn't respond */
> +		status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
> +		if (status != nfserr_jukebox ||
> +		    !nfsd_wait_for_delegreturn(rqstp, inode))
> +			goto out_status;
> +	}
> +	if (!ncf->ncf_file_modified &&
> +	    (ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
> +	     ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
> +		ncf->ncf_file_modified = true;
> +	if (ncf->ncf_file_modified) {
> +		int err;
> +
> +		/*
> +		 * Per section 10.4.3 of RFC 8881, the server would
> +		 * not update the file's metadata with the client's
> +		 * modified size
> +		 */
> +		attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
> +		attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG;
> +		inode_lock(inode);
> +		err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
> +		inode_unlock(inode);
> +		if (err) {
> +			status = nfserrno(err);
> +			goto out_status;
> +		}
> +		ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
> +		*size = ncf->ncf_cur_fsize;
> +		*modified = true;
> +	}
> +	status = 0;
> +out_status:
> +	nfs4_put_stid(&dp->dl_stid);
> +	return status;
>  }

Patch looks like a nice cleanup, but I don't think the Fixes tag is
appropriate here.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 0/2] nfsd: CB_GETATTR fixes

[Jeff Layton]

On Mon, 2024-08-26 at 10:37 -0400, Jeff Layton wrote:
> On Mon, 2024-08-26 at 09:22 +1000, NeilBrown wrote:
> > On Sun, 25 Aug 2024, Chuck Lever wrote:
> > > On Fri, Aug 23, 2024 at 06:27:37PM -0400, Jeff Layton wrote:
> > > > Fixes for a couple of CB_GETATTR bugs I found while working on the
> > > > delstid set. Mostly this just ensures that we hold references to the
> > > > delegation while working with it.
> > > > 
> > > > 
> > > 
> > > Applied to nfsd-fixes for v6.11-rc, thanks!
> > > 
> > > [1/2] nfsd: hold reference to delegation when updating it for cb_getattr
> > >       commit: 8fceb5f6636bbbf803fe29fff59f138206559964
> > > [2/2] nfsd: fix potential UAF in nfsd4_cb_getattr_release
> > >       commit: 8bc97f9b84c8852fcc56be2382f5115c518de785
> > > 
> > > -- 
> > > Chuck Lever
> > > 
> > 
> > Maybe the following can tidy up that code.  I can split this into
> > a few separate patches if you like.
> > Thoughts?
> > 
> > Note that the patch is easier to review if you apply it then use "git
> > diff -b".
> > 
> > NeilBrown
> > 
> > 
> > From: NeilBrown <neilb@suse.de>
> > Subject: [PATCH] nfsd: untangle code in nfsd4_deleg_getattr_conflict()
> > 
> > The code in nfsd4_deleg_getattr_conflict() is convoluted and buggy.
> > 
> > With this patch we:
> >  - properly handle non-nfsd leases.  We must not assume flc_owner is a
> >     delegation unless fl_lmops == &nfsd_lease_mng_ops
> 
> AFAICT, non-nfsd leases are already properly handled (though I do agree
> that the "flow" of this code is awkward). What case do you see that's
> wrong?
> 

Doh! Nevermind -- I see it now. It looks like the break_lease tag is
just in the wrong place. We should definitely fix that.

In any case, your patch looks reasonable to me, but I couldn't get it
to apply. Care to send a real PATCH instead? It's fine if you want to
drop my patch and just replace it with yours.

> >  - move the main code out of the for loop
> >  - have a single exit which calls nfs4_put_stid()
> >    (and other exits which don't need to call that)
> > 
> > Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >  fs/nfsd/nfs4state.c | 130 ++++++++++++++++++++++----------------------
> >  1 file changed, 65 insertions(+), 65 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 2c4b9a22b2bb..7672fa7a70f3 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -8837,6 +8837,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
> >  	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> >  	struct inode *inode = d_inode(dentry);
> >  	struct file_lock_context *ctx;
> > +	struct nfs4_delegation *dp = NULL;
> >  	struct nfs4_cb_fattr *ncf;
> >  	struct file_lease *fl;
> >  	struct iattr attrs;
> > @@ -8845,77 +8846,76 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
> >  	ctx = locks_inode_context(inode);
> >  	if (!ctx)
> >  		return 0;
> > +
> > +#define NON_NFSD_LEASE ((void*)1)
> > +
> >  	spin_lock(&ctx->flc_lock);
> >  	for_each_file_lock(fl, &ctx->flc_lease) {
> > -		unsigned char type = fl->c.flc_type;
> > -
> >  		if (fl->c.flc_flags == FL_LAYOUT)
> >  			continue;
> > -		if (fl->fl_lmops != &nfsd_lease_mng_ops) {
> > -			/*
> > -			 * non-nfs lease, if it's a lease with F_RDLCK then
> > -			 * we are done; there isn't any write delegation
> > -			 * on this inode
> > -			 */
> > -			if (type == F_RDLCK)
> > -				break;
> > -			goto break_lease;
> > -		}
> > -		if (type == F_WRLCK) {
> > -			struct nfs4_delegation *dp = fl->c.flc_owner;
> > -
> > -			if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
> > -				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);
> > -			ncf = &dp->dl_cb_fattr;
> > -			nfs4_cb_getattr(&dp->dl_cb_fattr);
> > -			spin_unlock(&ctx->flc_lock);
> > -			wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
> > -					TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
> > -			if (ncf->ncf_cb_status) {
> > -				/* Recall delegation only if client didn't respond */
> > -				status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
> > -				if (status != nfserr_jukebox ||
> > -						!nfsd_wait_for_delegreturn(rqstp, inode)) {
> > -					nfs4_put_stid(&dp->dl_stid);
> > -					return status;
> > -				}
> > -			}
> > -			if (!ncf->ncf_file_modified &&
> > -					(ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
> > -					ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
> > -				ncf->ncf_file_modified = true;
> > -			if (ncf->ncf_file_modified) {
> > -				int err;
> > -
> > -				/*
> > -				 * Per section 10.4.3 of RFC 8881, the server would
> > -				 * not update the file's metadata with the client's
> > -				 * modified size
> > -				 */
> > -				attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
> > -				attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG;
> > -				inode_lock(inode);
> > -				err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
> > -				inode_unlock(inode);
> > -				if (err) {
> > -					nfs4_put_stid(&dp->dl_stid);
> > -					return nfserrno(err);
> > -				}
> > -				ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
> > -				*size = ncf->ncf_cur_fsize;
> > -				*modified = true;
> > -			}
> > -			nfs4_put_stid(&dp->dl_stid);
> > -			return 0;
> > +		if (fl->c.flc_type == F_WRLCK) {
> > +			if (fl->fl_lmops == &nfsd_lease_mng_ops)
> > +				dp = fl->c.flc_owner;
> > +			else
> > +				dp = NON_NFSD_LEASE;
> >  		}
> >  		break;
> >  	}
> > +	if (dp == NULL || dp == NON_NFSD_LEASE ||
> > +	    dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
> > +		spin_unlock(&ctx->flc_lock);
> > +		if (dp == NON_NFSD_LEASE) {
> > +			status = nfserrno(nfsd_open_break_lease(inode,
> > +								NFSD_MAY_READ));
> > +			if (status != nfserr_jukebox ||
> > +			    !nfsd_wait_for_delegreturn(rqstp, inode))
> > +				return status;
> > +		}
> > +		return 0;
> > +	}
> > +
> > +	nfsd_stats_wdeleg_getattr_inc(nn);
> > +	refcount_inc(&dp->dl_stid.sc_count);
> > +	ncf = &dp->dl_cb_fattr;
> > +	nfs4_cb_getattr(&dp->dl_cb_fattr);
> >  	spin_unlock(&ctx->flc_lock);
> > -	return 0;
> > +
> > +	wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
> > +			    TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
> > +	if (ncf->ncf_cb_status) {
> > +		/* Recall delegation only if client didn't respond */
> > +		status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
> > +		if (status != nfserr_jukebox ||
> > +		    !nfsd_wait_for_delegreturn(rqstp, inode))
> > +			goto out_status;
> > +	}
> > +	if (!ncf->ncf_file_modified &&
> > +	    (ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
> > +	     ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
> > +		ncf->ncf_file_modified = true;
> > +	if (ncf->ncf_file_modified) {
> > +		int err;
> > +
> > +		/*
> > +		 * Per section 10.4.3 of RFC 8881, the server would
> > +		 * not update the file's metadata with the client's
> > +		 * modified size
> > +		 */
> > +		attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
> > +		attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG;
> > +		inode_lock(inode);
> > +		err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
> > +		inode_unlock(inode);
> > +		if (err) {
> > +			status = nfserrno(err);
> > +			goto out_status;
> > +		}
> > +		ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
> > +		*size = ncf->ncf_cur_fsize;
> > +		*modified = true;
> > +	}
> > +	status = 0;
> > +out_status:
> > +	nfs4_put_stid(&dp->dl_stid);
> > +	return status;
> >  }
> 
> Patch looks like a nice cleanup, but I don't think the Fixes tag is
> appropriate here.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 0/2] nfsd: CB_GETATTR fixes

[Chuck Lever]

On Mon, Aug 26, 2024 at 10:47:46AM -0400, Jeff Layton wrote:
> On Mon, 2024-08-26 at 10:37 -0400, Jeff Layton wrote:
> > On Mon, 2024-08-26 at 09:22 +1000, NeilBrown wrote:
> > > On Sun, 25 Aug 2024, Chuck Lever wrote:
> > > > On Fri, Aug 23, 2024 at 06:27:37PM -0400, Jeff Layton wrote:
> > > > > Fixes for a couple of CB_GETATTR bugs I found while working on the
> > > > > delstid set. Mostly this just ensures that we hold references to the
> > > > > delegation while working with it.
> > > > > 
> > > > > 
> > > > 
> > > > Applied to nfsd-fixes for v6.11-rc, thanks!
> > > > 
> > > > [1/2] nfsd: hold reference to delegation when updating it for cb_getattr
> > > >       commit: 8fceb5f6636bbbf803fe29fff59f138206559964
> > > > [2/2] nfsd: fix potential UAF in nfsd4_cb_getattr_release
> > > >       commit: 8bc97f9b84c8852fcc56be2382f5115c518de785
> > > > 
> > > > -- 
> > > > Chuck Lever
> > > > 
> > > 
> > > Maybe the following can tidy up that code.  I can split this into
> > > a few separate patches if you like.
> > > Thoughts?
> > > 
> > > Note that the patch is easier to review if you apply it then use "git
> > > diff -b".
> > > 
> > > NeilBrown
> > > 
> > > 
> > > From: NeilBrown <neilb@suse.de>
> > > Subject: [PATCH] nfsd: untangle code in nfsd4_deleg_getattr_conflict()
> > > 
> > > The code in nfsd4_deleg_getattr_conflict() is convoluted and buggy.
> > > 
> > > With this patch we:
> > >  - properly handle non-nfsd leases.  We must not assume flc_owner is a
> > >     delegation unless fl_lmops == &nfsd_lease_mng_ops
> > 
> > AFAICT, non-nfsd leases are already properly handled (though I do agree
> > that the "flow" of this code is awkward). What case do you see that's
> > wrong?
> > 
> 
> Doh! Nevermind -- I see it now. It looks like the break_lease tag is
> just in the wrong place. We should definitely fix that.
> 
> In any case, your patch looks reasonable to me, but I couldn't get it
> to apply.

I applied Jeff's weekend CB_GETATTR patches to nfsd-fixes. If
there's an additional bug fix carried in Neil's clean-up, I would
like that to apply to that branch, as a small surgical fix, so it
can go into v6.11-rc.

Seems like these CB_GETATTR fixes need to be applicable to LTS
kernels, so let's keep them narrow.


> Care to send a real PATCH instead?  It's fine if you want to
> drop my patch and just replace it with yours.

Neil, I'd prefer:

1) specific fixes to apply to the nfsd-fixes branch
2) larger clean-ups to apply to the nfsd-next branch

Untangling nfsd4_deleg_getattr_conflict() is a sensible thing to do,
IMO, but I'd bet that Linus would consider that development rather
than an urgent bug fix.

-- 
Chuck Lever