> On Jul 14, 2022, at 11:28 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> 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.
>
> Take the i_rwsem before setting the delegation. If we're granted a
> lease, redo the lookup of the file being opened and validate that the
> resulting dentry matches the one in the open file description. We only
> need to do this for the CLAIM_NULL open case however.
>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/nfsd/nfs4state.c | 55 ++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 50 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 347794028c98..e5c7f6690d2d 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1172,6 +1172,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
>
> void
> nfs4_put_stid(struct nfs4_stid *s)
> + __releases(&clp->cl_lock)
> {
> struct nfs4_file *fp = s->sc_file;
> struct nfs4_client *clp = s->sc_client;
This hunk causes a bunch of new sparse warnings:
/home/cel/src/linux/klimt/include/linux/list.h:137:19: warning: context imbalance in 'put_clnt_odstate' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:1174:1: warning: context imbalance in 'nfs4_put_stid' - different lock contexts for basic block
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:1230:13: warning: context imbalance in 'destroy_unhashed_deleg' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:1528:17: warning: context imbalance in 'release_lock_stateid' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:1624:17: warning: context imbalance in 'release_last_closed_stateid' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:2212:22: warning: context imbalance in '__destroy_client' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:4481:17: warning: context imbalance in 'nfsd4_find_and_lock_existing_open' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:4557:25: warning: context imbalance in 'init_open_stateid' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:4606:17: warning: context imbalance in 'move_to_close_lru' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:4752:13: warning: context imbalance in 'nfsd4_cb_recall_release' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:5003:17: warning: context imbalance in 'nfs4_check_deleg' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:5392:9: warning: context imbalance in 'nfs4_set_delegation' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:5467:9: warning: context imbalance in 'nfs4_open_delegation' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:5619:17: warning: context imbalance in 'nfsd4_process_open2' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:5638:17: warning: context imbalance in 'nfsd4_cleanup_open_state' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:5934:27: warning: context imbalance in 'nfs4_laundromat' - different lock contexts for basic block
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6160:17: warning: context imbalance in 'nfsd4_lookup_stateid' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6374:25: warning: context imbalance in 'nfs4_preprocess_stateid_op' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6422:9: warning: context imbalance in 'nfsd4_free_lock_stateid' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6459:28: warning: context imbalance in 'nfsd4_free_stateid' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6528:17: warning: context imbalance in 'nfs4_preprocess_seqid_op' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6545:17: warning: context imbalance in 'nfs4_preprocess_confirmed_seqid_op' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6588:9: warning: context imbalance in 'nfsd4_open_confirm' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6657:9: warning: context imbalance in 'nfsd4_open_downgrade' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6730:9: warning: context imbalance in 'nfsd4_close' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6762:9: warning: context imbalance in 'nfsd4_delegreturn' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:7034:17: warning: context imbalance in 'init_lock_stateid' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:7063:17: warning: context imbalance in 'find_or_create_lock_stateid' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:7362:17: warning: context imbalance in 'nfsd4_lock' - unexpected unlock
/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:7535:9: warning: context imbalance in 'nfsd4_locku' - unexpected unlock
Let's repair the "/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:1185:9:
warning: context imbalance in 'nfs4_put_stid' - unexpected unlock" message
in a separate patch.
Otherwise, this seems reasonable and the surgery is not invasive.
Do you have a sense of the overhead of this new check?
> @@ -5259,13 +5260,41 @@ 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 validate that this
> + * hasn't happened.
> + */
> +static int
> +nfsd4_vet_deleg_parent(struct nfsd4_open *open, struct nfs4_file *fp, struct dentry *parent)
> +{
> + struct dentry *child;
> +
> + /* Only need to do this if this is an open-by-name */
> + if (!parent)
> + return 0;
> +
> + child = lookup_one_len(open->op_fname, parent, open->op_fnamelen);
> + if (IS_ERR(child))
> + return PTR_ERR(child);
> + dput(child);
> +
> + /* Uh-oh, there has been a rename or unlink of the file. No deleg! */
> + if (child != file_dentry(fp->fi_deleg_file->nf_file))
> + return -EAGAIN;
> +
> + return 0;
> +}
> +
> static struct nfs4_delegation *
> -nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
> +nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> + struct svc_fh *parent_fh)
> {
> 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 dentry *parent = parent_fh ? parent_fh->fh_dentry : NULL;
> struct nfs4_delegation *dp;
> struct nfsd_file *nf;
> struct file_lock *fl;
> @@ -5315,11 +5344,23 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
> if (!fl)
> goto out_clnt_odstate;
>
> + if (parent)
> + inode_lock_shared_nested(d_inode(parent), I_MUTEX_PARENT);
> status = vfs_setlease(fp->fi_deleg_file->nf_file, fl->fl_type, &fl, NULL);
> if (fl)
> locks_free_lock(fl);
> - if (status)
> + if (status) {
> + if (parent)
> + inode_unlock_shared(d_inode(parent));
> goto out_clnt_odstate;
> + }
> +
> + status = nfsd4_vet_deleg_parent(open, fp, parent);
> + if (parent)
> + inode_unlock_shared(d_inode(parent));
> + if (status)
> + goto out_unlock;
> +
> status = nfsd4_check_conflicting_opens(clp, fp);
> if (status)
> goto out_unlock;
> @@ -5375,11 +5416,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 *current_fh)
> {
> 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_fh = NULL;
> int cb_up;
> int status = 0;
>
> @@ -5393,6 +5436,8 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
> goto out_no_deleg;
> break;
> case NFS4_OPEN_CLAIM_NULL:
> + parent_fh = current_fh;
> + fallthrough;
> case NFS4_OPEN_CLAIM_FH:
> /*
> * Let's not give out any delegations till everyone's
> @@ -5407,7 +5452,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
> default:
> goto out_no_deleg;
> }
> - dp = nfs4_set_delegation(open, stp);
> + dp = nfs4_set_delegation(open, stp, parent_fh);
> if (IS_ERR(dp))
> goto out_no_deleg;
>
> @@ -5539,7 +5584,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);
> --
> 2.36.1
>
--
Chuck Lever
