Thanks to the Feng Wu and the kbuild test robot I see
that some uses for rcu_dereference are wrong.
This two patches resolve the issue and also fix an
embarrassing but harmless bug.
---
NeilBrown (2):
NFS: fix two problems in lookup_revalidate in RCU-walk
SUNRPC: remove all refcounting of groupinfo from rpcauth_lookupcred
fs/nfs/dir.c | 8 ++++----
net/sunrpc/auth.c | 8 +-------
2 files changed, 5 insertions(+), 11 deletions(-)
--
Signature
current_cred() can only be changed by 'current', and
cred->group_info is never changed. If a new group_info is
needed, a new 'cred' is created.
Consequently it is always safe to access
current_cred()->group_info
without taking any further references.
So drop the refcounting and the incorrect rcu_dereference().
Signed-off-by: NeilBrown <[email protected]>
---
net/sunrpc/auth.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index 794fc0f4fc4c..df2bd96f129d 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -595,14 +595,8 @@ rpcauth_lookupcred(struct rpc_auth *auth, int flags)
memset(&acred, 0, sizeof(acred));
acred.uid = cred->fsuid;
acred.gid = cred->fsgid;
- if (flags & RPCAUTH_LOOKUP_RCU)
- acred.group_info = rcu_dereference(cred->group_info);
- else
- acred.group_info = get_group_info(((struct cred *)cred)->group_info);
-
+ acred.group_info = cred->group_info;
ret = auth->au_ops->lookup_cred(auth, &acred, flags);
- if (!(flags & RPCAUTH_LOOKUP_RCU))
- put_group_info(acred.group_info);
return ret;
}
EXPORT_SYMBOL_GPL(rpcauth_lookupcred);
1/ rcu_dereference isn't correct: that field isn't
RCU protected. It could potentially change at any time
so ACCESS_ONCE might be justified.
changes to ->d_parent are protected by ->d_seq. However
that isn't always checked after ->d_revalidate is called,
so it is safest to keep the double-check that ->d_parent
hasn't changed at the end of these functions.
2/ in nfs4_lookup_revalidate, "->d_parent" was forgotten.
So 'parent' was not the parent of 'dentry'.
This fails safe is the context is that dentry->d_inode is
NULL, and the result of parent->d_inode being NULL is
that ECHILD is returned, which is always safe.
Reported-by: kbuild test robot <[email protected]>
Signed-off-by: NeilBrown <[email protected]>
---
fs/nfs/dir.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index e754d205ea54..0295f78f2976 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1102,7 +1102,7 @@ static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
int error;
if (flags & LOOKUP_RCU) {
- parent = rcu_dereference(dentry->d_parent);
+ parent = ACCESS_ONCE(dentry->d_parent);
dir = ACCESS_ONCE(parent->d_inode);
if (!dir)
return -ECHILD;
@@ -1184,7 +1184,7 @@ out_set_verifier:
nfs_advise_use_readdirplus(dir);
out_valid_noent:
if (flags & LOOKUP_RCU) {
- if (parent != rcu_dereference(dentry->d_parent))
+ if (parent != ACCESS_ONCE(dentry->d_parent))
return -ECHILD;
} else
dput(parent);
@@ -1585,7 +1585,7 @@ static int nfs4_lookup_revalidate(struct dentry *dentry, unsigned int flags)
struct inode *dir;
if (flags & LOOKUP_RCU) {
- parent = rcu_dereference(dentry);
+ parent = ACCESS_ONCE(dentry->d_parent);
dir = ACCESS_ONCE(parent->d_inode);
if (!dir)
return -ECHILD;
@@ -1599,7 +1599,7 @@ static int nfs4_lookup_revalidate(struct dentry *dentry, unsigned int flags)
ret = -ECHILD;
if (!(flags & LOOKUP_RCU))
dput(parent);
- else if (parent != rcu_dereference(dentry))
+ else if (parent != ACCESS_ONCE(dentry->d_parent))
return -ECHILD;
goto out;
}