kernfs_rename_ns() takes kernfs_rename_lock only when the rename moves
the node to a new parent. A rename that keeps the parent, like renaming
a network interface, changes kernfs_node::name under kernfs_rwsem alone.
So the lock covers ->__parent but not ->name, and a reader that wants a
stable name has to take kernfs_rwsem, the lock every lookup needs.
It is also a real bug. kernfs_path_from_node() holds the lock for
reading and reads each ancestor's name once. One rename only moves the
answer from the old path to the new one, but two renames 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"
Only sysfs can hit this: sysfs_warn_dup() is the one caller on a root
without KERNFS_ROOT_INVARIANT_PARENT. The rest are cgroup, which sets
the flag, so it skips the lock and reads names under RCU alone. That
case needs something else and is left alone here.
So take the lock for both kinds of rename, and let kernfs_rcu_name()
accept it, like kernfs_parent() already does for ->__parent. Renames
are rare, the lock is per filesystem, and the locked section is at most
three stores. It also gives a future rename counter one place to sit.
Fixes: 741c10b096bc ("kernfs: Use RCU to access kernfs_node::name.")
Acked-by: Tejun Heo <[email protected]>
Assisted-by: LLM
Signed-off-by: Shakeel Butt <[email protected]>
---
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
