Re: [PATCH 02/13] NFSD: verify the opened dentry after setting a delegation

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2022-07-26 at 16:45 +1000, NeilBrown wrote:
> From: Jeff Layton <jlayton@kernel.org>
> 
> Between opening a file and setting a delegation on it, someone could
> rename or unlink the dentry. If this happens, we do not want to grant a
> delegation on the open.
> 
> On a CLAIM_NULL open, we're opening by filename, and we may (in the
> non-create case) or may not (in the create case) be holding i_rwsem
> when attempting to set a delegation.  The latter case allows a
> race.
> 
> After getting a lease, redo the lookup of the file being opened and
> validate that the resulting dentry matches the one in the open file
> description.
> 
> To properly redo the lookup we need an rqst pointer to pass to
> nfsd_lookup_dentry(), so make sure that is available.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4proc.c  |    1 +
>  fs/nfsd/nfs4state.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  fs/nfsd/xdr4.h      |    1 +
>  3 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 5af9f8d1feb6..d57f91fa9f70 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -547,6 +547,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		open->op_openowner);
>  
>  	open->op_filp = NULL;
> +	open->op_rqstp = rqstp;
>  
>  	/* This check required by spec. */
>  	if (open->op_create && open->op_claim_type != NFS4_OPEN_CLAIM_NULL)
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 279c7a1502c9..8f64af3e38d8 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5288,11 +5288,44 @@ static int nfsd4_check_conflicting_opens(struct nfs4_client *clp,
>  	return 0;
>  }
>  
> +/*
> + * It's possible that between opening the dentry and setting the delegation,
> + * that it has been renamed or unlinked. Redo the lookup to verify that this
> + * hasn't happened.
> + */
> +static int
> +nfsd4_verify_deleg_dentry(struct nfsd4_open *open, struct nfs4_file *fp,
> +			  struct svc_fh *parent)
> +{
> +	struct svc_export *exp;
> +	struct dentry *child;
> +	__be32 err;
> +
> +	/* parent may already be locked, and it may get unlocked by
> +	 * this call, but that is safe.
> +	 */
> +	err = nfsd_lookup_dentry(open->op_rqstp, parent,
> +				 open->op_fname, open->op_fnamelen,
> +				 &exp, &child);
> +

Note that in the middle of this series, you may end up triggering the
printk in fh_lock_nested if you call nfsd_lookup_dentry and the fh is
already locked. I assume that gets resolved by the end of the series
however.

> +	if (err)
> +		return -EAGAIN;
> +
> +	dput(child);
> +	if (child != file_dentry(fp->fi_deleg_file->nf_file))
> +		return -EAGAIN;
> +
> +	return 0;
> +}
> +
>  static struct nfs4_delegation *
> -nfs4_set_delegation(struct nfs4_client *clp,
> -		    struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate)
> +nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> +		    struct svc_fh *parent)
>  {
>  	int status = 0;
> +	struct nfs4_client *clp = stp->st_stid.sc_client;
> +	struct nfs4_file *fp = stp->st_stid.sc_file;
> +	struct nfs4_clnt_odstate *odstate = stp->st_clnt_odstate;
>  	struct nfs4_delegation *dp;
>  	struct nfsd_file *nf;
>  	struct file_lock *fl;
> @@ -5347,6 +5380,13 @@ nfs4_set_delegation(struct nfs4_client *clp,
>  		locks_free_lock(fl);
>  	if (status)
>  		goto out_clnt_odstate;
> +
> +	if (parent) {
> +		status = nfsd4_verify_deleg_dentry(open, fp, parent);
> +		if (status)
> +			goto out_unlock;
> +	}
> +
>  	status = nfsd4_check_conflicting_opens(clp, fp);
>  	if (status)
>  		goto out_unlock;
> @@ -5402,11 +5442,13 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
>   * proper support for them.
>   */
>  static void
> -nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
> +nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> +		     struct svc_fh *currentfh)
>  {
>  	struct nfs4_delegation *dp;
>  	struct nfs4_openowner *oo = openowner(stp->st_stateowner);
>  	struct nfs4_client *clp = stp->st_stid.sc_client;
> +	struct svc_fh *parent = NULL;
>  	int cb_up;
>  	int status = 0;
>  
> @@ -5420,6 +5462,8 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
>  				goto out_no_deleg;
>  			break;
>  		case NFS4_OPEN_CLAIM_NULL:
> +			parent = currentfh;
> +			fallthrough;
>  		case NFS4_OPEN_CLAIM_FH:
>  			/*
>  			 * Let's not give out any delegations till everyone's
> @@ -5434,7 +5478,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
>  		default:
>  			goto out_no_deleg;
>  	}
> -	dp = nfs4_set_delegation(clp, stp->st_stid.sc_file, stp->st_clnt_odstate);
> +	dp = nfs4_set_delegation(open, stp, parent);
>  	if (IS_ERR(dp))
>  		goto out_no_deleg;
>  
> @@ -5566,7 +5610,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
>  	* Attempt to hand out a delegation. No error return, because the
>  	* OPEN succeeds even if we fail.
>  	*/
> -	nfs4_open_delegation(open, stp);
> +	nfs4_open_delegation(open, stp, &resp->cstate.current_fh);
>  nodeleg:
>  	status = nfs_ok;
>  	trace_nfsd_open(&stp->st_stid.sc_stateid);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 7b744011f2d3..189f0600dbe8 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -279,6 +279,7 @@ struct nfsd4_open {
>  	struct nfs4_clnt_odstate *op_odstate; /* used during processing */
>  	struct nfs4_acl *op_acl;
>  	struct xdr_netobj op_label;
> +	struct svc_rqst *op_rqstp;
>  };
>  
>  struct nfsd4_open_confirm {
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>