Hello Greg,
Can you please consider including the following patches in the stable linux-4.14.y branch?
The following patch series attempts to address the issues raised by Stan Hu around NFSv4 lookup revalidation.
The first patch in the series should, in principle suffice to address the exact issue raised by Stan, however when looking at the implementation of nfs4_lookup_revalidate(), it becomes clear that we're not doing enough to revalidate the dentry itself when performing NFSv4.1 opens either.
Trond Myklebust (3): NFS: Fix dentry revalidation on NFSv4 lookup NFS: Refactor nfs_lookup_revalidate() NFSv4: Fix lookup revalidate of regular files
zhangliguang (1): NFS: Remove redundant semicolon
fs/nfs/dir.c | 295 ++++++++++++++++++++++++++++++------------------------ fs/nfs/nfs4proc.c | 15 ++- 2 files changed, 174 insertions(+), 136 deletions(-)
From: Trond Myklebust trond.myklebust@hammerspace.com
commit be189f7e7f03de35887e5a85ddcf39b91b5d7fc1 upstream.
We need to ensure that inode and dentry revalidation occurs correctly on reopen of a file that is already open. Currently, we can end up not revalidating either in the case of NFSv4.0, due to the 'cached open' path. Let's fix that by ensuring that we only do cached open for the special cases of open recovery and delegation return.
Reported-by: Stan Hu stanhu@gmail.com Signed-off-by: Trond Myklebust trond.myklebust@hammerspace.com Cc: stable@vger.kernel.org # 4.14.x Signed-off-by: Qian Lu luqia@amazon.com --- fs/nfs/nfs4proc.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index a225f98c9903..dd0d333974dc 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -1315,12 +1315,20 @@ static bool nfs4_mode_match_open_stateid(struct nfs4_state *state, return false; }
-static int can_open_cached(struct nfs4_state *state, fmode_t mode, int open_mode) +static int can_open_cached(struct nfs4_state *state, fmode_t mode, + int open_mode, enum open_claim_type4 claim) { int ret = 0;
if (open_mode & (O_EXCL|O_TRUNC)) goto out; + switch (claim) { + case NFS4_OPEN_CLAIM_NULL: + case NFS4_OPEN_CLAIM_FH: + goto out; + default: + break; + } switch (mode & (FMODE_READ|FMODE_WRITE)) { case FMODE_READ: ret |= test_bit(NFS_O_RDONLY_STATE, &state->flags) != 0 @@ -1615,7 +1623,7 @@ static struct nfs4_state *nfs4_try_open_cached(struct nfs4_opendata *opendata)
for (;;) { spin_lock(&state->owner->so_lock); - if (can_open_cached(state, fmode, open_mode)) { + if (can_open_cached(state, fmode, open_mode, claim)) { update_open_stateflags(state, fmode); spin_unlock(&state->owner->so_lock); goto out_return_state; @@ -2139,7 +2147,8 @@ static void nfs4_open_prepare(struct rpc_task *task, void *calldata) if (data->state != NULL) { struct nfs_delegation *delegation;
- if (can_open_cached(data->state, data->o_arg.fmode, data->o_arg.open_flags)) + if (can_open_cached(data->state, data->o_arg.fmode, + data->o_arg.open_flags, claim)) goto out_no_action; rcu_read_lock(); delegation = rcu_dereference(NFS_I(data->state->inode)->delegation);
From: Trond Myklebust trond.myklebust@hammerspace.com
commit 5ceb9d7fdaaf6d8ced6cd7861cf1deb9cd93fa47 upstream.
Refactor the code in nfs_lookup_revalidate() as a stepping stone towards optimising and fixing nfs4_lookup_revalidate().
Signed-off-by: Trond Myklebust trond.myklebust@hammerspace.com Cc: stable@vger.kernel.org # 4.14.x Signed-off-by: Qian Lu luqia@amazon.com --- fs/nfs/dir.c | 222 +++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 126 insertions(+), 96 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index bf2c43635062..9befb6752b97 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1059,6 +1059,100 @@ int nfs_neg_need_reval(struct inode *dir, struct dentry *dentry, return !nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU); }
+static int +nfs_lookup_revalidate_done(struct inode *dir, struct dentry *dentry, + struct inode *inode, int error) +{ + switch (error) { + case 1: + dfprintk(LOOKUPCACHE, "NFS: %s(%pd2) is valid\n", + __func__, dentry); + return 1; + case 0: + nfs_mark_for_revalidate(dir); + if (inode && S_ISDIR(inode->i_mode)) { + /* Purge readdir caches. */ + nfs_zap_caches(inode); + /* + * We can't d_drop the root of a disconnected tree: + * its d_hash is on the s_anon list and d_drop() would hide + * it from shrink_dcache_for_unmount(), leading to busy + * inodes on unmount and further oopses. + */ + if (IS_ROOT(dentry)) + return 1; + } + dfprintk(LOOKUPCACHE, "NFS: %s(%pd2) is invalid\n", + __func__, dentry); + return 0; + } + dfprintk(LOOKUPCACHE, "NFS: %s(%pd2) lookup returned error %d\n", + __func__, dentry, error); + return error; +} + +static int +nfs_lookup_revalidate_negative(struct inode *dir, struct dentry *dentry, + unsigned int flags) +{ + int ret = 1; + if (nfs_neg_need_reval(dir, dentry, flags)) { + if (flags & LOOKUP_RCU) + return -ECHILD; + ret = 0; + } + return nfs_lookup_revalidate_done(dir, dentry, NULL, ret); +} + +static int +nfs_lookup_revalidate_delegated(struct inode *dir, struct dentry *dentry, + struct inode *inode) +{ + nfs_set_verifier(dentry, nfs_save_change_attribute(dir)); + return nfs_lookup_revalidate_done(dir, dentry, inode, 1); +} + +static int +nfs_lookup_revalidate_dentry(struct inode *dir, struct dentry *dentry, + struct inode *inode) +{ + struct nfs_fh *fhandle; + struct nfs_fattr *fattr; + struct nfs4_label *label; + int ret; + + ret = -ENOMEM; + fhandle = nfs_alloc_fhandle(); + fattr = nfs_alloc_fattr(); + label = nfs4_label_alloc(NFS_SERVER(inode), GFP_KERNEL); + if (fhandle == NULL || fattr == NULL || IS_ERR(label)) + goto out; + + ret = NFS_PROTO(dir)->lookup(dir, &dentry->d_name, fhandle, fattr, label); + if (ret < 0) { + if (ret == -ESTALE || ret == -ENOENT) + ret = 0; + goto out; + } + ret = 0; + if (nfs_compare_fh(NFS_FH(inode), fhandle)) + goto out; + if (nfs_refresh_inode(inode, fattr) < 0) + goto out; + + nfs_setsecurity(inode, fattr, label); + nfs_set_verifier(dentry, nfs_save_change_attribute(dir)); + + /* set a readdirplus hint that we had a cache miss */ + nfs_force_use_readdirplus(dir); + ret = 1; +out: + nfs_free_fattr(fattr); + nfs_free_fhandle(fhandle); + nfs4_label_free(label); + return nfs_lookup_revalidate_done(dir, dentry, inode, ret); +} + /* * This is called every time the dcache has a lookup hit, * and we should check whether we can really trust that @@ -1070,58 +1164,36 @@ int nfs_neg_need_reval(struct inode *dir, struct dentry *dentry, * If the parent directory is seen to have changed, we throw out the * cached dentry and do a new lookup. */ -static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags) +static int +nfs_do_lookup_revalidate(struct inode *dir, struct dentry *dentry, + unsigned int flags) { - struct inode *dir; struct inode *inode; - struct dentry *parent; - struct nfs_fh *fhandle = NULL; - struct nfs_fattr *fattr = NULL; - struct nfs4_label *label = NULL; int error;
- if (flags & LOOKUP_RCU) { - parent = ACCESS_ONCE(dentry->d_parent); - dir = d_inode_rcu(parent); - if (!dir) - return -ECHILD; - } else { - parent = dget_parent(dentry); - dir = d_inode(parent); - } nfs_inc_stats(dir, NFSIOS_DENTRYREVALIDATE); inode = d_inode(dentry);
- if (!inode) { - if (nfs_neg_need_reval(dir, dentry, flags)) { - if (flags & LOOKUP_RCU) - return -ECHILD; - goto out_bad; - } - goto out_valid; - } + if (!inode) + return nfs_lookup_revalidate_negative(dir, dentry, flags);
if (is_bad_inode(inode)) { - if (flags & LOOKUP_RCU) - return -ECHILD; dfprintk(LOOKUPCACHE, "%s: %pd2 has dud inode\n", __func__, dentry); goto out_bad; }
if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) - goto out_set_verifier; + return nfs_lookup_revalidate_delegated(dir, dentry, inode);
/* Force a full look up iff the parent directory has changed */ if (!nfs_is_exclusive_create(dir, flags) && nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU)) { error = nfs_lookup_verify_inode(inode, flags); if (error) { - if (flags & LOOKUP_RCU) - return -ECHILD; if (error == -ESTALE) - goto out_zap_parent; - goto out_error; + nfs_zap_caches(dir); + goto out_bad; } nfs_advise_use_readdirplus(dir); goto out_valid; @@ -1133,81 +1205,39 @@ static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags) if (NFS_STALE(inode)) goto out_bad;
- error = -ENOMEM; - fhandle = nfs_alloc_fhandle(); - fattr = nfs_alloc_fattr(); - if (fhandle == NULL || fattr == NULL) - goto out_error; - - label = nfs4_label_alloc(NFS_SERVER(inode), GFP_NOWAIT); - if (IS_ERR(label)) - goto out_error; - trace_nfs_lookup_revalidate_enter(dir, dentry, flags); - error = NFS_PROTO(dir)->lookup(dir, &dentry->d_name, fhandle, fattr, label); + error = nfs_lookup_revalidate_dentry(dir, dentry, inode); trace_nfs_lookup_revalidate_exit(dir, dentry, flags, error); - if (error == -ESTALE || error == -ENOENT) - goto out_bad; - if (error) - goto out_error; - if (nfs_compare_fh(NFS_FH(inode), fhandle)) - goto out_bad; - if ((error = nfs_refresh_inode(inode, fattr)) != 0) - goto out_bad; - - nfs_setsecurity(inode, fattr, label); - - nfs_free_fattr(fattr); - nfs_free_fhandle(fhandle); - nfs4_label_free(label); + return error; +out_valid: + return nfs_lookup_revalidate_done(dir, dentry, inode, 1); +out_bad: + if (flags & LOOKUP_RCU) + return -ECHILD; + return nfs_lookup_revalidate_done(dir, dentry, inode, 0); +}
- /* set a readdirplus hint that we had a cache miss */ - nfs_force_use_readdirplus(dir); +static int +nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags) +{ + struct dentry *parent; + struct inode *dir; + int ret;
-out_set_verifier: - nfs_set_verifier(dentry, nfs_save_change_attribute(dir)); - out_valid: if (flags & LOOKUP_RCU) { + parent = ACCESS_ONCE(dentry->d_parent); + dir = d_inode_rcu(parent); + if (!dir) + return -ECHILD; + ret = nfs_do_lookup_revalidate(dir, dentry, flags); if (parent != ACCESS_ONCE(dentry->d_parent)) return -ECHILD; - } else + } else { + parent = dget_parent(dentry); + ret = nfs_do_lookup_revalidate(d_inode(parent), dentry, flags); dput(parent); - dfprintk(LOOKUPCACHE, "NFS: %s(%pd2) is valid\n", - __func__, dentry); - return 1; -out_zap_parent: - nfs_zap_caches(dir); - out_bad: - WARN_ON(flags & LOOKUP_RCU); - nfs_free_fattr(fattr); - nfs_free_fhandle(fhandle); - nfs4_label_free(label); - nfs_mark_for_revalidate(dir); - if (inode && S_ISDIR(inode->i_mode)) { - /* Purge readdir caches. */ - nfs_zap_caches(inode); - /* - * We can't d_drop the root of a disconnected tree: - * its d_hash is on the s_anon list and d_drop() would hide - * it from shrink_dcache_for_unmount(), leading to busy - * inodes on unmount and further oopses. - */ - if (IS_ROOT(dentry)) - goto out_valid; } - dput(parent); - dfprintk(LOOKUPCACHE, "NFS: %s(%pd2) is invalid\n", - __func__, dentry); - return 0; -out_error: - WARN_ON(flags & LOOKUP_RCU); - nfs_free_fattr(fattr); - nfs_free_fhandle(fhandle); - nfs4_label_free(label); - dput(parent); - dfprintk(LOOKUPCACHE, "NFS: %s(%pd2) lookup returned error %d\n", - __func__, dentry, error); - return error; + return ret; }
/*
From: Trond Myklebust trond.myklebust@hammerspace.com
commit c7944ebb9ce9461079659e9e6ec5baaf73724b3b upstream.
If we're revalidating an existing dentry in order to open a file, we need to ensure that we check the directory has not changed before we optimise away the lookup.
Signed-off-by: Trond Myklebust trond.myklebust@hammerspace.com Cc: stable@vger.kernel.org # 4.14.x Signed-off-by: Qian Lu luqia@amazon.com --- fs/nfs/dir.c | 79 ++++++++++++++++++++++++++++++------------------------------ 1 file changed, 39 insertions(+), 40 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 9befb6752b97..85a6fdd76e20 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1218,7 +1218,8 @@ nfs_do_lookup_revalidate(struct inode *dir, struct dentry *dentry, }
static int -nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags) +__nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags, + int (*reval)(struct inode *, struct dentry *, unsigned int)) { struct dentry *parent; struct inode *dir; @@ -1229,17 +1230,22 @@ nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags) dir = d_inode_rcu(parent); if (!dir) return -ECHILD; - ret = nfs_do_lookup_revalidate(dir, dentry, flags); + ret = reval(dir, dentry, flags); if (parent != ACCESS_ONCE(dentry->d_parent)) return -ECHILD; } else { parent = dget_parent(dentry); - ret = nfs_do_lookup_revalidate(d_inode(parent), dentry, flags); + ret = reval(d_inode(parent), dentry, flags); dput(parent); } return ret; }
+static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags) +{ + return __nfs_lookup_revalidate(dentry, flags, nfs_do_lookup_revalidate); +} + /* * A weaker form of d_revalidate for revalidating just the d_inode(dentry) * when we don't really care about the dentry name. This is called when a @@ -1590,62 +1596,55 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry, } EXPORT_SYMBOL_GPL(nfs_atomic_open);
-static int nfs4_lookup_revalidate(struct dentry *dentry, unsigned int flags) +static int +nfs4_do_lookup_revalidate(struct inode *dir, struct dentry *dentry, + unsigned int flags) { struct inode *inode; - int ret = 0;
if (!(flags & LOOKUP_OPEN) || (flags & LOOKUP_DIRECTORY)) - goto no_open; + goto full_reval; if (d_mountpoint(dentry)) - goto no_open; - if (NFS_SB(dentry->d_sb)->caps & NFS_CAP_ATOMIC_OPEN_V1) - goto no_open; + goto full_reval;
inode = d_inode(dentry);
/* We can't create new files in nfs_open_revalidate(), so we * optimize away revalidation of negative dentries. */ - if (inode == NULL) { - struct dentry *parent; - struct inode *dir; - - if (flags & LOOKUP_RCU) { - parent = ACCESS_ONCE(dentry->d_parent); - dir = d_inode_rcu(parent); - if (!dir) - return -ECHILD; - } else { - parent = dget_parent(dentry); - dir = d_inode(parent); - } - if (!nfs_neg_need_reval(dir, dentry, flags)) - ret = 1; - else if (flags & LOOKUP_RCU) - ret = -ECHILD; - if (!(flags & LOOKUP_RCU)) - dput(parent); - else if (parent != ACCESS_ONCE(dentry->d_parent)) - return -ECHILD; - goto out; - } + if (inode == NULL) + goto full_reval; + + if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) + return nfs_lookup_revalidate_delegated(dir, dentry, inode);
/* NFS only supports OPEN on regular files */ if (!S_ISREG(inode->i_mode)) - goto no_open; + goto full_reval; + /* We cannot do exclusive creation on a positive dentry */ - if (flags & LOOKUP_EXCL) - goto no_open; + if (flags & (LOOKUP_EXCL | LOOKUP_REVAL)) + goto reval_dentry; + + /* Check if the directory changed */ + if (!nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU)) + goto reval_dentry;
/* Let f_op->open() actually open (and revalidate) the file */ - ret = 1; + return 1; +reval_dentry: + if (flags & LOOKUP_RCU) + return -ECHILD; + return nfs_lookup_revalidate_dentry(dir, dentry, inode);;
-out: - return ret; +full_reval: + return nfs_do_lookup_revalidate(dir, dentry, flags); +}
-no_open: - return nfs_lookup_revalidate(dentry, flags); +static int nfs4_lookup_revalidate(struct dentry *dentry, unsigned int flags) +{ + return __nfs_lookup_revalidate(dentry, flags, + nfs4_do_lookup_revalidate); }
#endif /* CONFIG_NFSV4 */
From: zhangliguang zhangliguang@linux.alibaba.com
commit 42f72cf368c502c435af4e206e26d651cfb7d9ad upstream.
This removes redundant semicolon for ending code.
Fixes: c7944ebb9ce9 ("NFSv4: Fix lookup revalidate of regular files") Signed-off-by: Liguang Zhang zhangliguang@linux.alibaba.com Signed-off-by: Trond Myklebust trond.myklebust@hammerspace.com Cc: stable@vger.kernel.org # 4.14.x Signed-off-by: Qian Lu luqia@amazon.com --- fs/nfs/dir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 85a6fdd76e20..9065db7c31eb 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1635,7 +1635,7 @@ nfs4_do_lookup_revalidate(struct inode *dir, struct dentry *dentry, reval_dentry: if (flags & LOOKUP_RCU) return -ECHILD; - return nfs_lookup_revalidate_dentry(dir, dentry, inode);; + return nfs_lookup_revalidate_dentry(dir, dentry, inode);
full_reval: return nfs_do_lookup_revalidate(dir, dentry, flags);
On Wed, Jul 31, 2019 at 12:13:27AM -0700, Qian Lu wrote:
From: zhangliguang zhangliguang@linux.alibaba.com
commit 42f72cf368c502c435af4e206e26d651cfb7d9ad upstream.
This removes redundant semicolon for ending code.
Fixes: c7944ebb9ce9 ("NFSv4: Fix lookup revalidate of regular files") Signed-off-by: Liguang Zhang zhangliguang@linux.alibaba.com Signed-off-by: Trond Myklebust trond.myklebust@hammerspace.com Cc: stable@vger.kernel.org # 4.14.x Signed-off-by: Qian Lu luqia@amazon.com
fs/nfs/dir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 85a6fdd76e20..9065db7c31eb 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1635,7 +1635,7 @@ nfs4_do_lookup_revalidate(struct inode *dir, struct dentry *dentry, reval_dentry: if (flags & LOOKUP_RCU) return -ECHILD;
- return nfs_lookup_revalidate_dentry(dir, dentry, inode);;
- return nfs_lookup_revalidate_dentry(dir, dentry, inode);
This is not needed in a stable kernel tree :(
On Wed, Jul 31, 2019 at 12:13:23AM -0700, Qian Lu wrote:
Hello Greg,
Can you please consider including the following patches in the stable linux-4.14.y branch?
The following patch series attempts to address the issues raised by Stan Hu around NFSv4 lookup revalidation.
The first patch in the series should, in principle suffice to address the exact issue raised by Stan, however when looking at the implementation of nfs4_lookup_revalidate(), it becomes clear that we're not doing enough to revalidate the dentry itself when performing NFSv4.1 opens either.
You can not just ignore the 4.19.y kernel tree for these patches, as when you all finally move forward from 4.14.y to 4.19.y, you would have hit these same exact issues :(
I have also queued them up to 4.19.y.
Well, except for the last patch, that's not needed at all.
thanks,
greg k-h
On Wed, Jul 31, 2019 at 11:34:56AM +0200, Greg KH wrote:
On Wed, Jul 31, 2019 at 12:13:23AM -0700, Qian Lu wrote:
Hello Greg,
Can you please consider including the following patches in the stable linux-4.14.y branch?
The following patch series attempts to address the issues raised by Stan Hu around NFSv4 lookup revalidation.
The first patch in the series should, in principle suffice to address the exact issue raised by Stan, however when looking at the implementation of nfs4_lookup_revalidate(), it becomes clear that we're not doing enough to revalidate the dentry itself when performing NFSv4.1 opens either.
You can not just ignore the 4.19.y kernel tree for these patches, as when you all finally move forward from 4.14.y to 4.19.y, you would have hit these same exact issues :(
I have also queued them up to 4.19.y.
Well, except for the last patch, that's not needed at all.
thanks,
greg k-h
Thanks for catching that.
Thanks, Qian
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Qian Lu