[PATCH 1/3] kernfs: take kernfs_rename_lock for same-parent renames too

[Shakeel Butt]

kernfs_rename_ns() only takes kernfs_rename_lock when the rename moves
the node to a new parent.  A rename that keeps the same parent, like
renaming a network interface, changes kernfs_node::name with only
kernfs_rwsem held.  So the lock protects ->__parent but not ->name, and
a reader that wants a stable name has to take kernfs_rwsem, the same
lock every path lookup needs.

That also makes for a small but real bug.  kernfs_path_from_node()
takes kernfs_rename_lock for reading, and kernfs_path_from_node_locked()
then reads the name of each ancestor.  It reads each one once, so a
single same-parent rename only moves the answer from the old path to the
new one, but two of them landing inside one walk build a path that never
existed:

  CPU0                                   CPU1
  kernfs_path_from_node() on /a/b/c
    reads the name of a, gets "a"
                                         renames a to a2
                                         renames b to b2
    reads the name of b, gets "b2"
    returns "/a/b2/c"

This hits roots without KERNFS_ROOT_INVARIANT_PARENT: sysfs, where the
bad path can reach sysfs_warn_dup() and pr_cont_kernfs_path(), and
resctrl, which renames a mon group inside its mon_groups directory.
cgroup sets the flag, so it skips the lock and reads names under RCU
alone; that case needs something else and is not addressed here.

So take the lock in both cases, and let kernfs_rcu_name() accept it the
way kernfs_parent() already does for ->__parent.  Same-parent renames
are rare, the lock is per filesystem, and the locked section is at most
three stores.  It also gives a future rename sequence counter one place
to sit that covers every rename.

Fixes: 741c10b096bc ("kernfs: Use RCU to access kernfs_node::name.")
Signed-off-by: Shakeel Butt <shakeel.butt@xxxxxxxxx>
---
 fs/kernfs/dir.c             | 28 +++++++++++++++-------------
 fs/kernfs/kernfs-internal.h |  9 ++++++++-
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index cd7a8ff8b6b2..214c97130a8a 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -1808,6 +1808,7 @@ int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
 	struct kernfs_node *old_parent;
 	struct kernfs_root *root;
 	const char *old_name;
+	bool reparent;
 	int error;
 
 	/* can't move or rename root */
@@ -1857,25 +1858,26 @@ int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
 	 */
 	kernfs_unlink_sibling(kn);
 
-	/* rename_lock protects ->parent accessors */
-	if (old_parent != new_parent) {
+	reparent = old_parent != new_parent;
+	if (reparent)
 		kernfs_get(new_parent);
-		write_lock_irq(&root->kernfs_rename_lock);
 
+	/*
+	 * kernfs_rename_lock protects ->__parent, ->ns and ->name, so take it
+	 * even when the parent does not change.
+	 */
+	write_lock_irq(&root->kernfs_rename_lock);
+
+	if (reparent)
 		rcu_assign_pointer(kn->__parent, new_parent);
+	WRITE_ONCE(kn->ns, new_ns);
+	if (new_name)
+		rcu_assign_pointer(kn->name, new_name);
 
-		WRITE_ONCE(kn->ns, new_ns);
-		if (new_name)
-			rcu_assign_pointer(kn->name, new_name);
+	write_unlock_irq(&root->kernfs_rename_lock);
 
-		write_unlock_irq(&root->kernfs_rename_lock);
+	if (reparent)
 		kernfs_put(old_parent);
-	} else {
-		/* name assignment is RCU protected, parent is the same */
-		WRITE_ONCE(kn->ns, new_ns);
-		if (new_name)
-			rcu_assign_pointer(kn->name, new_name);
-	}
 
 	kn->hash = kernfs_name_hash(new_name ?: old_name, kn->ns);
 	kernfs_link_sibling(kn);
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index 20a0cf42ba8d..1609c1519698 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -117,7 +117,14 @@ static inline bool kernfs_rename_is_locked(const struct kernfs_node *kn)
 
 static inline const char *kernfs_rcu_name(const struct kernfs_node *kn)
 {
-	return rcu_dereference_check(kn->name, kernfs_root_is_locked(kn));
+	/*
+	 * Like kernfs_node::__parent below, the name is only replaced under
+	 * both kernfs_root::kernfs_rwsem and kernfs_root::kernfs_rename_lock,
+	 * so either one keeps it, and the string it points at, stable.
+	 */
+	return rcu_dereference_check(kn->name,
+				     kernfs_root_is_locked(kn) ||
+				     kernfs_rename_is_locked(kn));
 }
 
 static inline struct kernfs_node *kernfs_parent(const struct kernfs_node *kn)
-- 
2.53.0-Meta