Here's a first stab at a patchset to close a potential race when setting
a delegation on a file. Between the point where we open the file and
where we set the delegation, another task or client could unlink or
rename the dentry. If that occurs, we shouldn't hand out a delegation
in the open response, but we don't prevent that today.
The basic idea here is to re-do the lookup after setting the delegation.
If the resulting dentry is not the one we have in the open, then we can
reject handing out a delegation.
Only lightly tested, so this is an RFC for now.
Jeff Layton (3):
nfsd: drop fh argument from alloc_init_deleg
nfsd: rework arguments to nfs4_set_delegation
nfsd: vet the opened dentry after setting a delegation
fs/nfsd/nfs4state.c | 65 ++++++++++++++++++++++++++++++++++++++-------
1 file changed, 55 insertions(+), 10 deletions(-)
--
2.36.1
We'll need the nfs4_open to vet the filename. Change nfs4_set_delegation
to take the same arguments are nfs4_open_delegation.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/nfs4state.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4f81c0bbd27b..347794028c98 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5260,10 +5260,12 @@ static int nfsd4_check_conflicting_opens(struct nfs4_client *clp,
}
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)
{
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;
@@ -5405,7 +5407,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);
if (IS_ERR(dp))
goto out_no_deleg;
--
2.36.1
Currently, we pass the fh of the opened file down through several
functions so that alloc_init_deleg can pass it to delegation_blocked.
The filehandle of the open file is available in the nfs4_file struct
however, so there's no need to pass it in a separate argument.
Drop the argument from alloc_init_deleg, and follow suit in several of
the callers.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/nfs4state.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 994bd11bafe0..4f81c0bbd27b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1131,7 +1131,6 @@ static void block_delegations(struct knfsd_fh *fh)
static struct nfs4_delegation *
alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
- struct svc_fh *current_fh,
struct nfs4_clnt_odstate *odstate)
{
struct nfs4_delegation *dp;
@@ -1141,7 +1140,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
n = atomic_long_inc_return(&num_delegations);
if (n < 0 || n > max_delegations)
goto out_dec;
- if (delegation_blocked(¤t_fh->fh_handle))
+ if (delegation_blocked(&fp->fi_fhandle))
goto out_dec;
dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab, nfs4_free_deleg));
if (dp == NULL)
@@ -5261,7 +5260,7 @@ static int nfsd4_check_conflicting_opens(struct nfs4_client *clp,
}
static struct nfs4_delegation *
-nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
+nfs4_set_delegation(struct nfs4_client *clp,
struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate)
{
int status = 0;
@@ -5306,7 +5305,7 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
return ERR_PTR(status);
status = -ENOMEM;
- dp = alloc_init_deleg(clp, fp, fh, odstate);
+ dp = alloc_init_deleg(clp, fp, odstate);
if (!dp)
goto out_delegees;
@@ -5374,8 +5373,7 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
* proper support for them.
*/
static void
-nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open,
- struct nfs4_ol_stateid *stp)
+nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
{
struct nfs4_delegation *dp;
struct nfs4_openowner *oo = openowner(stp->st_stateowner);
@@ -5407,7 +5405,7 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open,
default:
goto out_no_deleg;
}
- dp = nfs4_set_delegation(clp, fh, stp->st_stid.sc_file, stp->st_clnt_odstate);
+ dp = nfs4_set_delegation(clp, stp->st_stid.sc_file, stp->st_clnt_odstate);
if (IS_ERR(dp))
goto out_no_deleg;
@@ -5539,7 +5537,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(current_fh, open, stp);
+ nfs4_open_delegation(open, stp);
nodeleg:
status = nfs_ok;
trace_nfsd_open(&stp->st_stid.sc_stateid);
--
2.36.1
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 <[email protected]>
---
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;
@@ -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
> On Jul 14, 2022, at 11:28 AM, Jeff Layton <[email protected]> wrote:
>
> We'll need the nfs4_open to vet the filename. Change nfs4_set_delegation
> to take the same arguments are nfs4_open_delegation.
^are^as
Nit: Considering that in the next patch you change the synopsis of
nfs4_open_delegation again but not nfs4_set_delegation, this
description causes a little whiplash.
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 4f81c0bbd27b..347794028c98 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5260,10 +5260,12 @@ static int nfsd4_check_conflicting_opens(struct nfs4_client *clp,
> }
>
> 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)
> {
> 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;
> @@ -5405,7 +5407,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);
> if (IS_ERR(dp))
> goto out_no_deleg;
>
> --
> 2.36.1
>
--
Chuck Lever
> On Jul 14, 2022, at 11:28 AM, Jeff Layton <[email protected]> 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 <[email protected]>
> ---
> 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
On Thu, 2022-07-14 at 16:53 +0000, Chuck Lever III wrote:
>
> > On Jul 14, 2022, at 11:28 AM, Jeff Layton <[email protected]> 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 <[email protected]>
> > ---
> > 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.
>
Yeah, I saw that too after I sent this. That hunk doesn't belong in
here. I'll drop it from my local copy.
> Otherwise, this seems reasonable and the surgery is not invasive.
> Do you have a sense of the overhead of this new check?
>
>
Not really...
It's an extra (shared) rwsem acquisition, and a lookup (though that
should be in cache in most cases). Neither of those sound too awful on
their own, but the open codepath can be sensitive. It's probably heavily
workload dependent as well (as is commonly the case). I think we'd need
to take it in and do some testing to see if it harms anything.
> > @@ -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
>
>
>
--
Jeff Layton <[email protected]>
On Thu, 2022-07-14 at 16:47 +0000, Chuck Lever III wrote:
>
> > On Jul 14, 2022, at 11:28 AM, Jeff Layton <[email protected]> wrote:
> >
> > We'll need the nfs4_open to vet the filename. Change nfs4_set_delegation
> > to take the same arguments are nfs4_open_delegation.
>
> ^are^as
>
> Nit: Considering that in the next patch you change the synopsis of
> nfs4_open_delegation again but not nfs4_set_delegation, this
> description causes a little whiplash.
>
>
Yeah, I should have squashed a couple of those together. I _did_ say it
was an RFC. I can resend a cleaned-up version later if you want to take
this in.
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > fs/nfsd/nfs4state.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 4f81c0bbd27b..347794028c98 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5260,10 +5260,12 @@ static int nfsd4_check_conflicting_opens(struct nfs4_client *clp,
> > }
> >
> > 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)
> > {
> > 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;
> > @@ -5405,7 +5407,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);
> > if (IS_ERR(dp))
> > goto out_no_deleg;
> >
> > --
> > 2.36.1
> >
>
> --
> Chuck Lever
>
>
>
--
Jeff Layton <[email protected]>
> On Jul 14, 2022, at 1:12 PM, Jeff Layton <[email protected]> wrote:
>
> On Thu, 2022-07-14 at 16:47 +0000, Chuck Lever III wrote:
>>
>>> On Jul 14, 2022, at 11:28 AM, Jeff Layton <[email protected]> wrote:
>>>
>>> We'll need the nfs4_open to vet the filename. Change nfs4_set_delegation
>>> to take the same arguments are nfs4_open_delegation.
>>
>> ^are^as
>>
>> Nit: Considering that in the next patch you change the synopsis of
>> nfs4_open_delegation again but not nfs4_set_delegation, this
>> description causes a little whiplash.
>>
>>
>
> Yeah, I should have squashed a couple of those together. I _did_ say it
> was an RFC. I can resend a cleaned-up version later if you want to take
> this in.
I'm interested in Neil's thoughts about this approach, but I'm
willing to run with it unless test results show a regression.
>>> Signed-off-by: Jeff Layton <[email protected]>
>>> ---
>>> fs/nfsd/nfs4state.c | 8 +++++---
>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 4f81c0bbd27b..347794028c98 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -5260,10 +5260,12 @@ static int nfsd4_check_conflicting_opens(struct nfs4_client *clp,
>>> }
>>>
>>> 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)
>>> {
>>> 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;
>>> @@ -5405,7 +5407,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);
>>> if (IS_ERR(dp))
>>> goto out_no_deleg;
>>>
>>> --
>>> 2.36.1
>>>
>>
>> --
>> Chuck Lever
>>
>>
>>
>
> --
> Jeff Layton <[email protected]>
--
Chuck Lever
> On Jul 14, 2022, at 1:11 PM, Jeff Layton <[email protected]> wrote:
>
> On Thu, 2022-07-14 at 16:53 +0000, Chuck Lever III wrote:
>>
>>> On Jul 14, 2022, at 11:28 AM, Jeff Layton <[email protected]> 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 <[email protected]>
>>> ---
>>> 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.
>>
>
>
> Yeah, I saw that too after I sent this. That hunk doesn't belong in
> here. I'll drop it from my local copy.
Well, I'm interested in trying to get rid of the existing sparse
warnings too, since those tend to block our ability to see new
warnings that arise.
If you have suggestions or proposals, please send them!
--
Chuck Lever
On Thu, 2022-07-14 at 17:16 +0000, Chuck Lever III wrote:
>
> > On Jul 14, 2022, at 1:11 PM, Jeff Layton <[email protected]> wrote:
> >
> > On Thu, 2022-07-14 at 16:53 +0000, Chuck Lever III wrote:
> > >
> > > > On Jul 14, 2022, at 11:28 AM, Jeff Layton <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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.
> > >
> >
> >
> > Yeah, I saw that too after I sent this. That hunk doesn't belong in
> > here. I'll drop it from my local copy.
>
> Well, I'm interested in trying to get rid of the existing sparse
> warnings too, since those tend to block our ability to see new
> warnings that arise.
>
> If you have suggestions or proposals, please send them!
>
We should definitely annotate these functions that have unbalanced
locking like this one. That's the usual way to silence these sorts of
warnings. I'm hoping Neil's patches will make it easier to address.
--
Jeff Layton <[email protected]>
On Thu, 2022-07-14 at 17:14 +0000, Chuck Lever III wrote:
>
>
> > On Jul 14, 2022, at 1:12 PM, Jeff Layton <[email protected]> wrote:
> >
> > On Thu, 2022-07-14 at 16:47 +0000, Chuck Lever III wrote:
> > >
> > > > On Jul 14, 2022, at 11:28 AM, Jeff Layton <[email protected]> wrote:
> > > >
> > > > We'll need the nfs4_open to vet the filename. Change nfs4_set_delegation
> > > > to take the same arguments are nfs4_open_delegation.
> > >
> > > ^are^as
> > >
> > > Nit: Considering that in the next patch you change the synopsis of
> > > nfs4_open_delegation again but not nfs4_set_delegation, this
> > > description causes a little whiplash.
> > >
> > >
> >
> > Yeah, I should have squashed a couple of those together. I _did_ say it
> > was an RFC. I can resend a cleaned-up version later if you want to take
> > this in.
>
> I'm interested in Neil's thoughts about this approach, but I'm
> willing to run with it unless test results show a regression.
>
>
...and there is a regression. Partial lockdep pop here:
Jul 14 14:46:54 quad3 kernel: ============================================
Jul 14 14:46:54 quad3 kernel: WARNING: possible recursive locking detected
Jul 14 14:46:54 quad3 kernel: 5.19.0-rc5+ #316 Tainted: G OE
Jul 14 14:46:54 quad3 kernel: --------------------------------------------
Jul 14 14:46:54 quad3 kernel: nfsd/1148 is trying to acquire lock:
Jul 14 14:46:54 quad3 kernel: ffff88812a3a7388 (&inode->i_sb->s_type->i_mutex_dir_key/1){++++}-{3:3}, at: nfsd4_process_open2+0x1890/0x2710 [nfsd]
Jul 14 14:46:54 quad3 kernel:
but task is already holding lock:
Jul 14 14:46:54 quad3 kernel: ffff88812a3a7388 (&inode->i_sb->s_type->i_mutex_dir_key/1){++++}-{3:3}, at: nfsd_lookup_dentry+0x16f/0x6a0 [nfsd]
Jul 14 14:46:54 quad3 kernel:
other info that might help us debug this:
Jul 14 14:46:54 quad3 kernel: Possible unsafe locking scenario:
Jul 14 14:46:54 quad3 kernel: CPU0
Jul 14 14:46:54 quad3 kernel: ----
Jul 14 14:46:54 quad3 kernel: lock(&inode->i_sb->s_type->i_mutex_dir_key/1);
Jul 14 14:46:54 quad3 kernel: lock(&inode->i_sb->s_type->i_mutex_dir_key/1);
Jul 14 14:46:54 quad3 kernel:
*** DEADLOCK ***
Jul 14 14:46:54 quad3 kernel: May be due to missing lock nesting notation
Jul 14 14:46:54 quad3 kernel: 1 lock held by nfsd/1148:
Jul 14 14:46:54 quad3 kernel: #0: ffff88812a3a7388 (&inode->i_sb->s_type->i_mutex_dir_key/1){++++}-{3:3}, at: nfsd_lookup_dentry+0x16f/0x6a0 [nfsd]
Jul 14 14:46:54 quad3 kernel:
The core problem is the unclear locking in nfsd_lookup_dentry. Sometimes
that returns with the i_rwsem held, but there's no clear indication of
whether that's the case when the function returns. I guess fh_unlock
just takes care of that (which is a little scary, tbqh).
Now that I've taken a stab at it, I don't see how we can fix this w/o
taking Neil's locking cleanups series first. I think I'll pull that in
and try to redo this series on top of it.
Cheers,
--
Jeff Layton <[email protected]>
On Thu, Jul 14, 2022 at 11:28:16AM -0400, Jeff Layton wrote:
> Here's a first stab at a patchset to close a potential race when setting
> a delegation on a file. Between the point where we open the file and
> where we set the delegation, another task or client could unlink or
> rename the dentry. If that occurs, we shouldn't hand out a delegation
> in the open response, but we don't prevent that today.
>
> The basic idea here is to re-do the lookup after setting the delegation.
> If the resulting dentry is not the one we have in the open, then we can
> reject handing out a delegation.
I have this distinct memory of actually doing that before.
But looking through the git history all I find is 4335723e8e9f "nfsd4:
fix delegation-unlink/rename race", from 2014, which claims to fix a
similar-sounding race in a different way.
How are you reproducing this?
--b.
>
> Only lightly tested, so this is an RFC for now.
>
> Jeff Layton (3): nfsd: drop fh argument from alloc_init_deleg nfsd:
> rework arguments to nfs4_set_delegation nfsd: vet the opened dentry
> after setting a delegation
>
> fs/nfsd/nfs4state.c | 65
> ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 55
> insertions(+), 10 deletions(-)
>
> -- 2.36.1
On Mon, 2022-07-18 at 17:18 -0400, J. Bruce Fields wrote:
> On Thu, Jul 14, 2022 at 11:28:16AM -0400, Jeff Layton wrote:
> > Here's a first stab at a patchset to close a potential race when setting
> > a delegation on a file. Between the point where we open the file and
> > where we set the delegation, another task or client could unlink or
> > rename the dentry. If that occurs, we shouldn't hand out a delegation
> > in the open response, but we don't prevent that today.
> >
> > The basic idea here is to re-do the lookup after setting the delegation.
> > If the resulting dentry is not the one we have in the open, then we can
> > reject handing out a delegation.
>
> I have this distinct memory of actually doing that before.
>
> But looking through the git history all I find is 4335723e8e9f "nfsd4:
> fix delegation-unlink/rename race", from 2014, which claims to fix a
> similar-sounding race in a different way.
>
> How are you reproducing this?
>
> --b.
>
I'm not at all, so far. This race is entirely theoretical. Probably we
could reproduce it by introducing some artificial delays or something.
> >
> > Only lightly tested, so this is an RFC for now.
> >
> > Jeff Layton (3): nfsd: drop fh argument from alloc_init_deleg nfsd:
> > rework arguments to nfs4_set_delegation nfsd: vet the opened dentry
> > after setting a delegation
> >
> > fs/nfsd/nfs4state.c | 65
> > ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 55
> > insertions(+), 10 deletions(-)
> >
> > -- 2.36.1
--
Jeff Layton <[email protected]>