On Tue, 2022-07-26 at 16:45 +1000, NeilBrown wrote:
> From: Jeff Layton <jlayton@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> 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@xxxxxxxxxx>
