[PATCH v2 0/2] Let cgroup use RCU for kernfs_node::name lookup.

[PATCH v2 0/2] Let cgroup use RCU for kernfs_node::name lookup.

[Sebastian Andrzej Siewior]

This is a follow up to Hillf/syzbot report. The thread started at
  https://lore.kernel.org/all/20241102001224.2789-1-hdanton@sina.com/

v1…v2 https://lore.kernel.org/all/20241108222406.n5azgO98@linutronix.de
  - Use RCU lookup only for ::name and only for cgroup.

Sebastian

[PATCH v2 1/2] kernfs: Make it possible to use RCU for kernfs_node::name lookup.

[Sebastian Andrzej Siewior]

Instead of using kernfs_rename_lock for lookups of ::name allow to use
RCU protection for lookup. Rely on kn's kernfs_root::kernfs_rwsem for
update synchronisation.

KERNFS_ROOT_SAME_PARENT is added to signal that the parent never
changes. kernfs_rename_ns() checks that flag and if it is seen, it
ensures that the parent is the same and then does not acquire
kernfs_rename_lock during parent/ name assignment and updates only the
name attribute. Without the flag, the update is performed as always.

kernfs_name_rcu() is a copy of kernfs_name() which is using RCU
protection while accessing the kernfs_node::name. Both functions
validate the KERNFS_ROOT_SAME_PARENT flag. The same is true for
kernfs_path_from_node() and kernfs_path_from_node_rcu().

Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 fs/kernfs/dir.c        | 151 ++++++++++++++++++++++++++++++-----------
 include/linux/kernfs.h |  23 ++++++-
 2 files changed, 133 insertions(+), 41 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 458519e416fe7..41c87ee76aa70 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -51,14 +51,6 @@ static bool kernfs_lockdep(struct kernfs_node *kn)
 #endif
 }
 
-static int kernfs_name_locked(struct kernfs_node *kn, char *buf, size_t buflen)
-{
-	if (!kn)
-		return strscpy(buf, "(null)", buflen);
-
-	return strscpy(buf, kn->parent ? kn->name : "/", buflen);
-}
-
 /* kernfs_node_depth - compute depth from @from to @to */
 static size_t kernfs_depth(struct kernfs_node *from, struct kernfs_node *to)
 {
@@ -168,10 +160,13 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
 
 	/* Calculate how many bytes we need for the rest */
 	for (i = depth_to - 1; i >= 0; i--) {
+		const char *name;
+
 		for (kn = kn_to, j = 0; j < i; j++)
 			kn = kn->parent;
 
-		len += scnprintf(buf + len, buflen - len, "/%s", kn->name);
+		name = rcu_dereference_check(kn->name_rcu, lockdep_is_held(&kernfs_rename_lock));
+		len += scnprintf(buf + len, buflen - len, "/%s", name);
 	}
 
 	return len;
@@ -184,7 +179,8 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
  * @buflen: size of @buf
  *
  * Copies the name of @kn into @buf of @buflen bytes.  The behavior is
- * similar to strscpy().
+ * similar to strscpy(). The root node must not be initialized with
+ * KERNFS_ROOT_SAME_PARENT.
  *
  * Fills buffer with "(null)" if @kn is %NULL.
  *
@@ -195,13 +191,47 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
  */
 int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
 {
-	unsigned long flags;
-	int ret;
+	struct kernfs_root *root;
 
-	read_lock_irqsave(&kernfs_rename_lock, flags);
-	ret = kernfs_name_locked(kn, buf, buflen);
-	read_unlock_irqrestore(&kernfs_rename_lock, flags);
-	return ret;
+	guard(read_lock_irqsave)(&kernfs_rename_lock);
+	if (kn) {
+		root = kernfs_root(kn);
+		if (WARN_ON_ONCE(root->flags & KERNFS_ROOT_SAME_PARENT))
+			kn = NULL;
+	}
+
+	if (!kn)
+		return strscpy(buf, "(null)", buflen);
+
+	return strscpy(buf, kn->parent ? kn->name : "/", buflen);
+}
+
+/**
+ * kernfs_name_rcu - obtain the name of a given node
+ * @kn: kernfs_node of interest
+ * @buf: buffer to copy @kn's name into
+ * @buflen: size of @buf
+ *
+ * Same as kernfs_name except it uses RCU for name lookup. The root node must
+ * be with KERNFS_ROOT_SAME_PARENT.
+ *
+ * This function can be called from any context.
+ */
+
+int kernfs_name_rcu(struct kernfs_node *kn, char *buf, size_t buflen)
+{
+	struct kernfs_root *root;
+
+	if (kn) {
+		root = kernfs_root(kn);
+		if (WARN_ON_ONCE(!(root->flags & KERNFS_ROOT_SAME_PARENT)))
+			kn = NULL;
+	}
+	if (!kn)
+		return strscpy(buf, "(null)", buflen);
+
+	guard(rcu)();
+	return strscpy(buf, kn->parent ? rcu_dereference(kn->name_rcu) : "/", buflen);
 }
 
 /**
@@ -214,7 +244,8 @@ int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
  * Builds @to's path relative to @from in @buf. @from and @to must
  * be on the same kernfs-root. If @from is not parent of @to, then a relative
  * path (which includes '..'s) as needed to reach from @from to @to is
- * returned.
+ * returned. The root node must not be initialized with
+ * KERNFS_ROOT_SAME_PARENT.
  *
  * Return: the length of the constructed path.  If the path would have been
  * greater than @buflen, @buf contains the truncated path with the trailing
@@ -223,16 +254,42 @@ int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
 int kernfs_path_from_node(struct kernfs_node *to, struct kernfs_node *from,
 			  char *buf, size_t buflen)
 {
-	unsigned long flags;
-	int ret;
+	struct kernfs_root *root;
 
-	read_lock_irqsave(&kernfs_rename_lock, flags);
-	ret = kernfs_path_from_node_locked(to, from, buf, buflen);
-	read_unlock_irqrestore(&kernfs_rename_lock, flags);
-	return ret;
+	guard(read_lock_irqsave)(&kernfs_rename_lock);
+	if (to) {
+		root = kernfs_root(to);
+		if (WARN_ON_ONCE(root->flags & KERNFS_ROOT_SAME_PARENT))
+			to = NULL;
+	}
+	return kernfs_path_from_node_locked(to, from, buf, buflen);
 }
 EXPORT_SYMBOL_GPL(kernfs_path_from_node);
 
+/**
+ * kernfs_path_from_node_rcu - build path of node @to relative to @from.
+ * @from: parent kernfs_node relative to which we need to build the path
+ * @to: kernfs_node of interest
+ * @buf: buffer to copy @to's path into
+ * @buflen: size of @buf
+ *
+ * Same as kernfs_path_from_node. Uses RCU for the name lookup. The root node
+ * must be initialized with KERNFS_ROOT_SAME_PARENT.
+ */
+int kernfs_path_from_node_rcu(struct kernfs_node *to, struct kernfs_node *from,
+			      char *buf, size_t buflen)
+{
+	struct kernfs_root *root;
+
+	if (to) {
+		root = kernfs_root(to);
+		if (WARN_ON_ONCE(!(root->flags & KERNFS_ROOT_SAME_PARENT)))
+			to = NULL;
+	}
+	guard(rcu)();
+	return kernfs_path_from_node_locked(to, from, buf, buflen);
+}
+
 /**
  * pr_cont_kernfs_name - pr_cont name of a kernfs_node
  * @kn: kernfs_node of interest
@@ -1717,7 +1774,8 @@ 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 = NULL;
+	bool rcu_name = false;
+	const char *kn_name;
 	int error;
 
 	/* can't move or rename root */
@@ -1732,9 +1790,18 @@ int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
 	    (new_parent->flags & KERNFS_EMPTY_DIR))
 		goto out;
 
+	if (root->flags & KERNFS_ROOT_SAME_PARENT) {
+		error = -EINVAL;
+		if (WARN_ON_ONCE(kn->parent != new_parent))
+			goto out;
+		rcu_name = true;
+	}
+
 	error = 0;
+	kn_name = rcu_dereference_check(kn->name_rcu,
+					lockdep_is_held(&root->kernfs_rwsem));
 	if ((kn->parent == new_parent) && (kn->ns == new_ns) &&
-	    (strcmp(kn->name, new_name) == 0))
+	    (strcmp(kn_name, new_name) == 0))
 		goto out;	/* nothing to rename */
 
 	error = -EEXIST;
@@ -1742,7 +1809,7 @@ int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
 		goto out;
 
 	/* rename kernfs_node */
-	if (strcmp(kn->name, new_name) != 0) {
+	if (strcmp(kn_name, new_name) != 0) {
 		error = -ENOMEM;
 		new_name = kstrdup_const(new_name, GFP_KERNEL);
 		if (!new_name)
@@ -1755,27 +1822,33 @@ int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
 	 * Move to the appropriate place in the appropriate directories rbtree.
 	 */
 	kernfs_unlink_sibling(kn);
-	kernfs_get(new_parent);
 
 	/* rename_lock protects ->parent and ->name accessors */
-	write_lock_irq(&kernfs_rename_lock);
+	if (!rcu_name) {
+		write_lock_irq(&kernfs_rename_lock);
 
-	old_parent = kn->parent;
-	kn->parent = new_parent;
+		kernfs_get(new_parent);
+		old_parent = kn->parent;
+		kn->parent = new_parent;
 
-	kn->ns = new_ns;
-	if (new_name) {
-		old_name = kn->name;
-		kn->name = new_name;
+		kn->ns = new_ns;
+		if (new_name)
+			kn->name = new_name;
+
+		write_unlock_irq(&kernfs_rename_lock);
+		kernfs_put(old_parent);
+	} else {
+		/* name assignment is RCU protected, parent is the same */
+		kn->ns = new_ns;
+		if (new_name)
+			rcu_assign_pointer(kn->name_rcu, new_name);
 	}
 
-	write_unlock_irq(&kernfs_rename_lock);
-
-	kn->hash = kernfs_name_hash(kn->name, kn->ns);
+	kn->hash = kernfs_name_hash(new_name ?: kn_name, kn->ns);
 	kernfs_link_sibling(kn);
 
-	kernfs_put(old_parent);
-	kfree_const(old_name);
+	if (new_name && !is_kernel_rodata((unsigned long)kn_name))
+		kfree_rcu_mightsleep(kn_name);
 
 	error = 0;
  out:
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 87c79d076d6d7..b52393f1045c6 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -147,6 +147,11 @@ enum kernfs_root_flag {
 	 * Support user xattrs to be written to nodes rooted at this root.
 	 */
 	KERNFS_ROOT_SUPPORT_USER_XATTR		= 0x0008,
+
+	/*
+	 * Renames must not change the parent node.
+	 */
+	KERNFS_ROOT_SAME_PARENT			= 0x0010,
 };
 
 /* type-specific structures for kernfs_node union members */
@@ -200,7 +205,10 @@ struct kernfs_node {
 	 * parent directly.
 	 */
 	struct kernfs_node	*parent;
-	const char		*name;
+	union {
+		const char		__rcu *name_rcu;
+		const char		*name;
+	};
 
 	struct rb_node		rb;
 
@@ -395,8 +403,11 @@ static inline bool kernfs_ns_enabled(struct kernfs_node *kn)
 }
 
 int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen);
-int kernfs_path_from_node(struct kernfs_node *root_kn, struct kernfs_node *kn,
+int kernfs_name_rcu(struct kernfs_node *kn, char *buf, size_t buflen);
+int kernfs_path_from_node(struct kernfs_node *kn_to, struct kernfs_node *kn_from,
 			  char *buf, size_t buflen);
+int kernfs_path_from_node_rcu(struct kernfs_node *kn_to, struct kernfs_node *kn_from,
+			      char *buf, size_t buflen);
 void pr_cont_kernfs_name(struct kernfs_node *kn);
 void pr_cont_kernfs_path(struct kernfs_node *kn);
 struct kernfs_node *kernfs_get_parent(struct kernfs_node *kn);
@@ -475,11 +486,19 @@ static inline bool kernfs_ns_enabled(struct kernfs_node *kn)
 static inline int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
 { return -ENOSYS; }
 
+static inline int kernfs_name_rcu(struct kernfs_node *kn, char *buf, size_t buflen)
+{ return -ENOSYS; }
+
 static inline int kernfs_path_from_node(struct kernfs_node *root_kn,
 					struct kernfs_node *kn,
 					char *buf, size_t buflen)
 { return -ENOSYS; }
 
+static inline int kernfs_path_from_node_rcu(struct kernfs_node *root_kn,
+					    struct kernfs_node *kn,
+					    char *buf, size_t buflen)
+{ return -ENOSYS; }
+
 static inline void pr_cont_kernfs_name(struct kernfs_node *kn) { }
 static inline void pr_cont_kernfs_path(struct kernfs_node *kn) { }
 
-- 
2.45.2

[PATCH v2 2/2] cgroup, kernfs: Move cgroup to the RCU interface for name lookups

[Sebastian Andrzej Siewior]

cgroup only renames nodes and keeps the same parent node. It can be
switched RCU for name lookups to avoid acquiring a lock.
For the switch the flag KERNFS_ROOT_SAME_PARENT is added while creating
the root node and lookups are switched to the _rcu() interface.

The pr_cont_kernfs_.*() is only used by cgroup and is renamed as part of
the switch.
kernfs_path() has one user in tree, sysfs.
kernfs_name() has no callers, could be removed.
kernfs_path_from_node() has no in-tree module callers, module export is
removed.

Fixes: 2b5067a8143e3 ("mm: mmap_lock: add tracepoints around lock acquisition")
Reported-by: syzbot+6ea37e2e6ffccf41a7e6@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/lkml/67251dc6.050a0220.529b6.015e.GAE@google.com/
Reported-by: Hillf Danton <hdanton@sina.com>
Closes: https://lore.kernel.org/20241102001224.2789-1-hdanton@sina.com
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 fs/kernfs/dir.c        | 21 +++++++++++----------
 include/linux/cgroup.h |  8 ++++----
 include/linux/kernfs.h | 26 ++++++++++++++++++++++----
 kernel/cgroup/cgroup.c |  9 +++++----
 4 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 41c87ee76aa70..93bdaad8b9886 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -264,7 +264,6 @@ int kernfs_path_from_node(struct kernfs_node *to, struct kernfs_node *from,
 	}
 	return kernfs_path_from_node_locked(to, from, buf, buflen);
 }
-EXPORT_SYMBOL_GPL(kernfs_path_from_node);
 
 /**
  * kernfs_path_from_node_rcu - build path of node @to relative to @from.
@@ -291,38 +290,40 @@ int kernfs_path_from_node_rcu(struct kernfs_node *to, struct kernfs_node *from,
 }
 
 /**
- * pr_cont_kernfs_name - pr_cont name of a kernfs_node
+ * pr_cont_kernfs_name_rcu - pr_cont name of a kernfs_node
  * @kn: kernfs_node of interest
  *
- * This function can be called from any context.
+ * This function can be called from any context. The root node must be with
+ * KERNFS_ROOT_SAME_PARENT.
  */
-void pr_cont_kernfs_name(struct kernfs_node *kn)
+void pr_cont_kernfs_name_rcu(struct kernfs_node *kn)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&kernfs_pr_cont_lock, flags);
 
-	kernfs_name(kn, kernfs_pr_cont_buf, sizeof(kernfs_pr_cont_buf));
+	kernfs_name_rcu(kn, kernfs_pr_cont_buf, sizeof(kernfs_pr_cont_buf));
 	pr_cont("%s", kernfs_pr_cont_buf);
 
 	spin_unlock_irqrestore(&kernfs_pr_cont_lock, flags);
 }
 
 /**
- * pr_cont_kernfs_path - pr_cont path of a kernfs_node
+ * pr_cont_kernfs_path_rcu - pr_cont path of a kernfs_node
  * @kn: kernfs_node of interest
  *
- * This function can be called from any context.
+ * This function can be called from any context. The root node must be with
+ * KERNFS_ROOT_SAME_PARENT.
  */
-void pr_cont_kernfs_path(struct kernfs_node *kn)
+void pr_cont_kernfs_path_rcu(struct kernfs_node *kn)
 {
 	unsigned long flags;
 	int sz;
 
 	spin_lock_irqsave(&kernfs_pr_cont_lock, flags);
 
-	sz = kernfs_path_from_node(kn, NULL, kernfs_pr_cont_buf,
-				   sizeof(kernfs_pr_cont_buf));
+	sz = kernfs_path_from_node_rcu(kn, NULL, kernfs_pr_cont_buf,
+				       sizeof(kernfs_pr_cont_buf));
 	if (sz < 0) {
 		if (sz == -E2BIG)
 			pr_cont("(name too long)");
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index f8ef47f8a634d..555a299e583ef 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -591,22 +591,22 @@ static inline struct cgroup_subsys_state *seq_css(struct seq_file *seq)
 
 static inline int cgroup_name(struct cgroup *cgrp, char *buf, size_t buflen)
 {
-	return kernfs_name(cgrp->kn, buf, buflen);
+	return kernfs_name_rcu(cgrp->kn, buf, buflen);
 }
 
 static inline int cgroup_path(struct cgroup *cgrp, char *buf, size_t buflen)
 {
-	return kernfs_path(cgrp->kn, buf, buflen);
+	return kernfs_path_rcu(cgrp->kn, buf, buflen);
 }
 
 static inline void pr_cont_cgroup_name(struct cgroup *cgrp)
 {
-	pr_cont_kernfs_name(cgrp->kn);
+	pr_cont_kernfs_name_rcu(cgrp->kn);
 }
 
 static inline void pr_cont_cgroup_path(struct cgroup *cgrp)
 {
-	pr_cont_kernfs_path(cgrp->kn);
+	pr_cont_kernfs_path_rcu(cgrp->kn);
 }
 
 bool cgroup_psi_enabled(void);
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index b52393f1045c6..a1907f3c944d0 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -408,8 +408,8 @@ int kernfs_path_from_node(struct kernfs_node *kn_to, struct kernfs_node *kn_from
 			  char *buf, size_t buflen);
 int kernfs_path_from_node_rcu(struct kernfs_node *kn_to, struct kernfs_node *kn_from,
 			      char *buf, size_t buflen);
-void pr_cont_kernfs_name(struct kernfs_node *kn);
-void pr_cont_kernfs_path(struct kernfs_node *kn);
+void pr_cont_kernfs_name_rcu(struct kernfs_node *kn);
+void pr_cont_kernfs_path_rcu(struct kernfs_node *kn);
 struct kernfs_node *kernfs_get_parent(struct kernfs_node *kn);
 struct kernfs_node *kernfs_find_and_get_ns(struct kernfs_node *parent,
 					   const char *name, const void *ns);
@@ -499,8 +499,8 @@ static inline int kernfs_path_from_node_rcu(struct kernfs_node *root_kn,
 					    char *buf, size_t buflen)
 { return -ENOSYS; }
 
-static inline void pr_cont_kernfs_name(struct kernfs_node *kn) { }
-static inline void pr_cont_kernfs_path(struct kernfs_node *kn) { }
+static inline void pr_cont_kernfs_name_rcu(struct kernfs_node *kn) { }
+static inline void pr_cont_kernfs_path_rcu(struct kernfs_node *kn) { }
 
 static inline struct kernfs_node *kernfs_get_parent(struct kernfs_node *kn)
 { return NULL; }
@@ -617,6 +617,24 @@ static inline int kernfs_path(struct kernfs_node *kn, char *buf, size_t buflen)
 	return kernfs_path_from_node(kn, NULL, buf, buflen);
 }
 
+/**
+ * kernfs_path_rcu - build full path of a given node
+ * @kn: kernfs_node of interest
+ * @buf: buffer to copy @kn's name into
+ * @buflen: size of @buf
+ *
+ * If @kn is NULL result will be "(null)". The root node must be with
+ * KERNFS_ROOT_SAME_PARENT.
+ *
+ * Returns the length of the full path.  If the full length is equal to or
+ * greater than @buflen, @buf contains the truncated path with the trailing
+ * '\0'.  On error, -errno is returned.
+ */
+static inline int kernfs_path_rcu(struct kernfs_node *kn, char *buf, size_t buflen)
+{
+	return kernfs_path_from_node_rcu(kn, NULL, buf, buflen);
+}
+
 static inline struct kernfs_node *
 kernfs_find_and_get(struct kernfs_node *kn, const char *name)
 {
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 044c7ba1cc482..6f8d555529525 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -1906,7 +1906,7 @@ int cgroup_show_path(struct seq_file *sf, struct kernfs_node *kf_node,
 
 	spin_lock_irq(&css_set_lock);
 	ns_cgroup = current_cgns_cgroup_from_root(kf_cgroot);
-	len = kernfs_path_from_node(kf_node, ns_cgroup->kn, buf, PATH_MAX);
+	len = kernfs_path_from_node_rcu(kf_node, ns_cgroup->kn, buf, PATH_MAX);
 	spin_unlock_irq(&css_set_lock);
 
 	if (len == -E2BIG)
@@ -2118,7 +2118,8 @@ int cgroup_setup_root(struct cgroup_root *root, u16 ss_mask)
 	root->kf_root = kernfs_create_root(kf_sops,
 					   KERNFS_ROOT_CREATE_DEACTIVATED |
 					   KERNFS_ROOT_SUPPORT_EXPORTOP |
-					   KERNFS_ROOT_SUPPORT_USER_XATTR,
+					   KERNFS_ROOT_SUPPORT_USER_XATTR |
+					   KERNFS_ROOT_SAME_PARENT,
 					   root_cgrp);
 	if (IS_ERR(root->kf_root)) {
 		ret = PTR_ERR(root->kf_root);
@@ -2387,7 +2388,7 @@ int cgroup_path_ns_locked(struct cgroup *cgrp, char *buf, size_t buflen,
 {
 	struct cgroup *root = cset_cgroup_from_root(ns->root_cset, cgrp->root);
 
-	return kernfs_path_from_node(cgrp->kn, root->kn, buf, buflen);
+	return kernfs_path_from_node_rcu(cgrp->kn, root->kn, buf, buflen);
 }
 
 int cgroup_path_ns(struct cgroup *cgrp, char *buf, size_t buflen,
@@ -6275,7 +6276,7 @@ void cgroup_path_from_kernfs_id(u64 id, char *buf, size_t buflen)
 	kn = kernfs_find_and_get_node_by_id(cgrp_dfl_root.kf_root, id);
 	if (!kn)
 		return;
-	kernfs_path(kn, buf, buflen);
+	kernfs_path_rcu(kn, buf, buflen);
 	kernfs_put(kn);
 }
 
-- 
2.45.2

Re: [PATCH v2 1/2] kernfs: Make it possible to use RCU for kernfs_node::name lookup.

[Tejun Heo]

Hello,

On Tue, Nov 12, 2024 at 04:52:38PM +0100, Sebastian Andrzej Siewior wrote:
...
> KERNFS_ROOT_SAME_PARENT is added to signal that the parent never

Maybe KERNFS_ROOT_INVARIANT_PARENT captures it better?

...
> @@ -195,13 +191,47 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
>   */
>  int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
>  {
> +	struct kernfs_root *root;
>  
> +	guard(read_lock_irqsave)(&kernfs_rename_lock);
> +	if (kn) {
> +		root = kernfs_root(kn);
> +		if (WARN_ON_ONCE(root->flags & KERNFS_ROOT_SAME_PARENT))
> +			kn = NULL;

Hmm... does kn need to be set to NULL here?

> +	}
> +
> +	if (!kn)
> +		return strscpy(buf, "(null)", buflen);
> +
> +	return strscpy(buf, kn->parent ? kn->name : "/", buflen);
...
> +int kernfs_name_rcu(struct kernfs_node *kn, char *buf, size_t buflen)
> +{
> +	struct kernfs_root *root;
> +
> +	if (kn) {
> +		root = kernfs_root(kn);
> +		if (WARN_ON_ONCE(!(root->flags & KERNFS_ROOT_SAME_PARENT)))
> +			kn = NULL;

Ah, I suppose it's to keep things symmetric. That's fine.

> +	}
> +	if (!kn)
> +		return strscpy(buf, "(null)", buflen);
> +
> +	guard(rcu)();

Also, why are guards in different locations? Even when !SAME_PARENT, kn's
can't jump across roots, so guard there can also be in the same location as
this one?

...
> @@ -200,7 +205,10 @@ struct kernfs_node {
>  	 * parent directly.
>  	 */
>  	struct kernfs_node	*parent;
> -	const char		*name;
> +	union {
> +		const char		__rcu *name_rcu;
> +		const char		*name;
> +	};

Wouldn't it be simpler if ->name is always __rcu and !SAME_PARENT just
requires further protection on the read side?

Thanks.

-- 
tejun

Re: [PATCH v2 2/2] cgroup, kernfs: Move cgroup to the RCU interface for name lookups

[Tejun Heo]

Hello,

On Tue, Nov 12, 2024 at 04:52:39PM +0100, Sebastian Andrzej Siewior wrote:
...
>  /**
> - * pr_cont_kernfs_name - pr_cont name of a kernfs_node
> + * pr_cont_kernfs_name_rcu - pr_cont name of a kernfs_node
>   * @kn: kernfs_node of interest
>   *
> - * This function can be called from any context.
> + * This function can be called from any context. The root node must be with
> + * KERNFS_ROOT_SAME_PARENT.
>   */
> -void pr_cont_kernfs_name(struct kernfs_node *kn)
> +void pr_cont_kernfs_name_rcu(struct kernfs_node *kn)

Having to split the interface all the way up isn't great. While there are
also downsides, I wonder whether a better approach here is just making the
backend function (kernfs_path_from_node()) automatically use RCU locking if
the flag is set rather than propagating the difference by splitting the
interface. The distinction doesn't mean anything to most users after all.

Thanks.

-- 
tejun

Re: [PATCH v2 1/2] kernfs: Make it possible to use RCU for kernfs_node::name lookup.

[Sebastian Andrzej Siewior]

On 2024-11-12 08:52:11 [-1000], Tejun Heo wrote:
> Hello,
Hi,

> On Tue, Nov 12, 2024 at 04:52:38PM +0100, Sebastian Andrzej Siewior wrote:
> ...
> > KERNFS_ROOT_SAME_PARENT is added to signal that the parent never
> 
> Maybe KERNFS_ROOT_INVARIANT_PARENT captures it better?

Sure.

> ...
> > @@ -195,13 +191,47 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
> >   */
> >  int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
> >  {
> > +	struct kernfs_root *root;
> >  
> > +	guard(read_lock_irqsave)(&kernfs_rename_lock);
> > +	if (kn) {
> > +		root = kernfs_root(kn);
> > +		if (WARN_ON_ONCE(root->flags & KERNFS_ROOT_SAME_PARENT))
> > +			kn = NULL;
> 
> Hmm... does kn need to be set to NULL here?

actually no, because read_lock() implies RCU protection.

> > +	}
> > +
> > +	if (!kn)
> > +		return strscpy(buf, "(null)", buflen);
> > +
> > +	return strscpy(buf, kn->parent ? kn->name : "/", buflen);
> ...
> > +int kernfs_name_rcu(struct kernfs_node *kn, char *buf, size_t buflen)
> > +{
> > +	struct kernfs_root *root;
> > +
> > +	if (kn) {
> > +		root = kernfs_root(kn);
> > +		if (WARN_ON_ONCE(!(root->flags & KERNFS_ROOT_SAME_PARENT)))
> > +			kn = NULL;
> 
> Ah, I suppose it's to keep things symmetric. That's fine.
> 
> > +	}
> > +	if (!kn)
> > +		return strscpy(buf, "(null)", buflen);
> > +
> > +	guard(rcu)();
> 
> Also, why are guards in different locations? Even when !SAME_PARENT, kn's
> can't jump across roots, so guard there can also be in the same location as
> this one?

I tried to limit the scope but it can be symmetrical.

> ...
> > @@ -200,7 +205,10 @@ struct kernfs_node {
> >  	 * parent directly.
> >  	 */
> >  	struct kernfs_node	*parent;
> > -	const char		*name;
> > +	union {
> > +		const char		__rcu *name_rcu;
> > +		const char		*name;
> > +	};
> 
> Wouldn't it be simpler if ->name is always __rcu and !SAME_PARENT just
> requires further protection on the read side?

Let me try that again.

> Thanks.
> 

Sebastian

Re: [PATCH v2 2/2] cgroup, kernfs: Move cgroup to the RCU interface for name lookups

[Sebastian Andrzej Siewior]

On 2024-11-12 08:59:16 [-1000], Tejun Heo wrote:
> Hello,

Hi,

> On Tue, Nov 12, 2024 at 04:52:39PM +0100, Sebastian Andrzej Siewior wrote:
> ...
> >  /**
> > - * pr_cont_kernfs_name - pr_cont name of a kernfs_node
> > + * pr_cont_kernfs_name_rcu - pr_cont name of a kernfs_node
> >   * @kn: kernfs_node of interest
> >   *
> > - * This function can be called from any context.
> > + * This function can be called from any context. The root node must be with
> > + * KERNFS_ROOT_SAME_PARENT.
> >   */
> > -void pr_cont_kernfs_name(struct kernfs_node *kn)
> > +void pr_cont_kernfs_name_rcu(struct kernfs_node *kn)
> 
> Having to split the interface all the way up isn't great. While there are
> also downsides, I wonder whether a better approach here is just making the
> backend function (kernfs_path_from_node()) automatically use RCU locking if
> the flag is set rather than propagating the difference by splitting the
> interface. The distinction doesn't mean anything to most users after all.

Indeed.

> Thanks.
> 

Sebastian

Re: [PATCH v2 2/2] cgroup, kernfs: Move cgroup to the RCU interface for name lookups

[Sebastian Andrzej Siewior]

On 2024-11-13 08:43:32 [+0100], To Tejun Heo wrote:
> On 2024-11-12 08:59:16 [-1000], Tejun Heo wrote:
> > Hello,
> 
> Hi,
> 
> > On Tue, Nov 12, 2024 at 04:52:39PM +0100, Sebastian Andrzej Siewior wrote:
> > ...
> > >  /**
> > > - * pr_cont_kernfs_name - pr_cont name of a kernfs_node
> > > + * pr_cont_kernfs_name_rcu - pr_cont name of a kernfs_node
> > >   * @kn: kernfs_node of interest
> > >   *
> > > - * This function can be called from any context.
> > > + * This function can be called from any context. The root node must be with
> > > + * KERNFS_ROOT_SAME_PARENT.
> > >   */
> > > -void pr_cont_kernfs_name(struct kernfs_node *kn)
> > > +void pr_cont_kernfs_name_rcu(struct kernfs_node *kn)
> > 
> > Having to split the interface all the way up isn't great. While there are
> > also downsides, I wonder whether a better approach here is just making the
> > backend function (kernfs_path_from_node()) automatically use RCU locking if
> > the flag is set rather than propagating the difference by splitting the
> > interface. The distinction doesn't mean anything to most users after all.
> 
> Indeed.

Now I see what the problems are. If we merge both into one, then I get
this:
| int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
| {
|         struct kernfs_root *root;
|         bool rcu_lookup;
|
|         if (!kn)
|                 return strscpy(buf, "(null)", buflen);
|
|         root = kernfs_root(kn);

This is the tricky part. For KERNFS_ROOT_INVARIANT_PARENT I don't worry
that the parent goes away and I need it to get a reference to the
kernfs_root node. For the !KERNFS_ROOT_INVARIANT_PARENT I need the lock
for kernfs_root() so I put the guard/ lock at the top.

I think that is why you suggested the two functions (or this is what I
understood). Looking at the remaining bits:

|         rcu_lookup = root->flags & KERNFS_ROOT_INVARIANT_PARENT;
|         if (rcu_lookup) {
|                 guard(rcu)();
|                 return strscpy(buf, kn->parent ? rcu_dereference(kn->name) : "/", buflen);
|         }
|         guard(read_lock_irqsave)(&kernfs_rename_lock);
|         return strscpy(buf, kn->parent ? rcu_dereference(kn->name) : "/", buflen);
| }

This could collapse into the RCU version because read_lock_irqsave()
implies RCU protection. And since ->name is always RCU assigned/
deallocated I don't really need the lock here, RCU would be enough.
Except for the parent. The kn->parent does not matter here (it should be
always be != NULL if assigned), the problematic part is kernfs_root()
which checks the parent for the root node.

To make this simple I could avoid kernfs_root lookup and just have:
| int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
| {       
|         if (!kn)
|                 return strscpy(buf, "(null)", buflen);
|         
|         guard(rcu)();
|         return strscpy(buf, kn->parent ? rcu_dereference(kn->name) : "/", buflen);
| }                             

That is the easy part. kernfs_path_from_node() is different as it
requires the parent pointer. In order to distinguish the RCU from the
non-RCU version I need kernfs_root for the flag and depending on it, the
lock so the parent does not go away.

Would it work to add the pointer to kernfs_root into kernfs_node? This
would shrink kernfs_elem_dir by a pointer but the union would remain the
same size due to kernfs_elem_attr so the struct would grow.

> > Thanks.

Sebastian

Re: [PATCH v2 2/2] cgroup, kernfs: Move cgroup to the RCU interface for name lookups

[Sebastian Andrzej Siewior]

- Zefan Li

On 2024-11-13 13:07:08 [+0100], To Tejun Heo wrote:
> On 2024-11-13 08:43:32 [+0100], To Tejun Heo wrote:
> > On 2024-11-12 08:59:16 [-1000], Tejun Heo wrote:
> > > Hello,
> > 
> > Hi,
> > 
> > > On Tue, Nov 12, 2024 at 04:52:39PM +0100, Sebastian Andrzej Siewior wrote:
> > > ...
> > > >  /**
> > > > - * pr_cont_kernfs_name - pr_cont name of a kernfs_node
> > > > + * pr_cont_kernfs_name_rcu - pr_cont name of a kernfs_node
> > > >   * @kn: kernfs_node of interest
> > > >   *
> > > > - * This function can be called from any context.
> > > > + * This function can be called from any context. The root node must be with
> > > > + * KERNFS_ROOT_SAME_PARENT.
> > > >   */
> > > > -void pr_cont_kernfs_name(struct kernfs_node *kn)
> > > > +void pr_cont_kernfs_name_rcu(struct kernfs_node *kn)
> > > 
> > > Having to split the interface all the way up isn't great. While there are
> > > also downsides, I wonder whether a better approach here is just making the
> > > backend function (kernfs_path_from_node()) automatically use RCU locking if
> > > the flag is set rather than propagating the difference by splitting the
> > > interface. The distinction doesn't mean anything to most users after all.
> > 
> > Indeed.
> 
> Now I see what the problems are. If we merge both into one, then I get
> this:
> | int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
> | {
> |         struct kernfs_root *root;
> |         bool rcu_lookup;
> |
> |         if (!kn)
> |                 return strscpy(buf, "(null)", buflen);
> |
> |         root = kernfs_root(kn);
> 
> This is the tricky part. For KERNFS_ROOT_INVARIANT_PARENT I don't worry
> that the parent goes away and I need it to get a reference to the
> kernfs_root node. For the !KERNFS_ROOT_INVARIANT_PARENT I need the lock
> for kernfs_root() so I put the guard/ lock at the top.
> 
> I think that is why you suggested the two functions (or this is what I
> understood). Looking at the remaining bits:
> 
> |         rcu_lookup = root->flags & KERNFS_ROOT_INVARIANT_PARENT;
> |         if (rcu_lookup) {
> |                 guard(rcu)();
> |                 return strscpy(buf, kn->parent ? rcu_dereference(kn->name) : "/", buflen);
> |         }
> |         guard(read_lock_irqsave)(&kernfs_rename_lock);
> |         return strscpy(buf, kn->parent ? rcu_dereference(kn->name) : "/", buflen);
> | }
> 
> This could collapse into the RCU version because read_lock_irqsave()
> implies RCU protection. And since ->name is always RCU assigned/
> deallocated I don't really need the lock here, RCU would be enough.
> Except for the parent. The kn->parent does not matter here (it should be
> always be != NULL if assigned), the problematic part is kernfs_root()
> which checks the parent for the root node.
> 
> To make this simple I could avoid kernfs_root lookup and just have:
> | int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
> | {       
> |         if (!kn)
> |                 return strscpy(buf, "(null)", buflen);
> |         
> |         guard(rcu)();
> |         return strscpy(buf, kn->parent ? rcu_dereference(kn->name) : "/", buflen);
> | }                             
> 
> That is the easy part. kernfs_path_from_node() is different as it
> requires the parent pointer. In order to distinguish the RCU from the
> non-RCU version I need kernfs_root for the flag and depending on it, the
> lock so the parent does not go away.
> 
> Would it work to add the pointer to kernfs_root into kernfs_node? This
> would shrink kernfs_elem_dir by a pointer but the union would remain the
> same size due to kernfs_elem_attr so the struct would grow.

The kernfs_node is released via RCU. That means if the RCU read section
starts before kernfs_root() then we should always get a stable pointer,
pointing to the same kernfs_root node since it is always the same one.
Even if the `parent' pointer is replaced. Wouldn't we need __rcu
annotation then for the `parent' pointer then?

> > > Thanks.
> 
Sebastian

Re: [PATCH v2 2/2] cgroup, kernfs: Move cgroup to the RCU interface for name lookups

[Tejun Heo]

Hello,

On Wed, Nov 13, 2024 at 02:23:33PM +0100, Sebastian Andrzej Siewior wrote:
...
> > That is the easy part. kernfs_path_from_node() is different as it
> > requires the parent pointer. In order to distinguish the RCU from the
> > non-RCU version I need kernfs_root for the flag and depending on it, the
> > lock so the parent does not go away.
> > 
> > Would it work to add the pointer to kernfs_root into kernfs_node? This
> > would shrink kernfs_elem_dir by a pointer but the union would remain the
> > same size due to kernfs_elem_attr so the struct would grow.
> 
> The kernfs_node is released via RCU. That means if the RCU read section
> starts before kernfs_root() then we should always get a stable pointer,
> pointing to the same kernfs_root node since it is always the same one.
> Even if the `parent' pointer is replaced. Wouldn't we need __rcu
> annotation then for the `parent' pointer then?

Yeah, I think this is the better direction. Just make both the parent and
name RCU protected, drop the rename rwlock and use RCU deref for both
->parent and ->name so that the code path doesn't have to diverge.

Thanks.

-- 
tejun

Re: [PATCH v2 1/2] kernfs: Make it possible to use RCU for kernfs_node::name lookup.

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 748 bytes --]

On Tue, Nov 12, 2024 at 04:52:38PM GMT, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> --- a/include/linux/kernfs.h
> +++ b/include/linux/kernfs.h
> @@ -147,6 +147,11 @@ enum kernfs_root_flag {
>  	 * Support user xattrs to be written to nodes rooted at this root.
>  	 */
>  	KERNFS_ROOT_SUPPORT_USER_XATTR		= 0x0008,
> +
> +	/*
> +	 * Renames must not change the parent node.
> +	 */
> +	KERNFS_ROOT_SAME_PARENT			= 0x0010,

FTR, cgroup v2 doesn't even define renames and the full rename
(different parent) is only used by resctrl filesystem AFAICS.
I'm only mentioning it in the case you wanted to replace the flag with
two different rename methods in kernfs_syscall_ops.

Thanks for fixing this locking situation in general,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] kernfs: Make it possible to use RCU for kernfs_node::name lookup.

[Sebastian Andrzej Siewior]

On 2024-11-14 14:48:16 [+0100], Michal Koutný wrote:
> On Tue, Nov 12, 2024 at 04:52:38PM GMT, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> > --- a/include/linux/kernfs.h
> > +++ b/include/linux/kernfs.h
> > @@ -147,6 +147,11 @@ enum kernfs_root_flag {
> >  	 * Support user xattrs to be written to nodes rooted at this root.
> >  	 */
> >  	KERNFS_ROOT_SUPPORT_USER_XATTR		= 0x0008,
> > +
> > +	/*
> > +	 * Renames must not change the parent node.
> > +	 */
> > +	KERNFS_ROOT_SAME_PARENT			= 0x0010,
> 
> FTR, cgroup v2 doesn't even define renames and the full rename
> (different parent) is only used by resctrl filesystem AFAICS.
> I'm only mentioning it in the case you wanted to replace the flag with
> two different rename methods in kernfs_syscall_ops.

Thanks. I think I have enough clues for a v3 now. I wanted to do
something today but didn't manage it yet.

> Thanks for fixing this locking situation in general,
> Michal

Sebastian