Skip to contentSkip to conversation

Two fixes for NFS RCU-walk support

PATCH New NeilBrown 1 participant 3 messages Aug 4, 2014

Subsystems (3)
Networking NFS client NFS server
lore
Share & tools
lore

NeilBrown

author
#0 lore raw

cover Two fixes for NFS RCU-walk support

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 · 1 line
Signature
author
#1 lore raw

PATCH 1/2 NFS: fix two problems in lookup_revalidate in RCU-walk

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(-)
fs/nfs/dir.c +4 -4
index e754d205ea54..0295f78f2976 100644
@@ -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;
 	}
author
#2 lore raw

PATCH 2/2 SUNRPC: remove all refcounting of groupinfo from rpcauth_lookupcred

In reply to NeilBrown

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(-)
net/sunrpc/auth.c +1 -7
index 794fc0f4fc4c..df2bd96f129d 100644
@@ -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);