linux-kernel - Re: [PATCH 1/2] nfsd: hold reference to delegation when updating it for cb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zsn2V3cmrNfc7HXA@tissot.1015granger.net>
Date: Sat, 24 Aug 2024 11:03:51 -0400
From: Chuck Lever <chuck.lever@...cle.com>
To: Jeff Layton <jlayton@...nel.org>
Cc: Neil Brown <neilb@...e.de>, Olga Kornievskaia <kolga@...app.com>,
        Dai Ngo <dai.ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] nfsd: hold reference to delegation when updating it
 for cb_getattr

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@...nel.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