syzbot reported two warnings:
- kernfs_node::name was accessed outside of a RCU section so it created
  warning. The kernfs_rwsem was held so it was okay but it wasn't seen.

- While kernfs_rwsem was held invoked lookup_positive_unlocked()->
  kernfs_dop_revalidate() which acquired kernfs_rwsem.

kernfs_rwsem was both acquired as a read lock so it can be acquired
twice. However if a writer acquires the lock after the first reader then
neither the writer nor the second reader can obtain the lock so it
deadlocks.

The reason for the lock is to ensure that kernfs_node::name remain
stable during lookup_positive_unlocked()'s invocation. The function can
not be invoked within a RCU section because it may sleep.

Make a temporary copy of the kernfs_node::name under the lock so
GFP_KERNEL can be used and use this instead.

Reported-by: syzbot+ecccecbc636b455f9084@syzkaller.appspotmail.com
Fixes: 5b2fabf7fe8f ("kernfs: Acquire kernfs_rwsem in kernfs_node_dentry().")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 fs/kernfs/mount.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
index d1f512b7bf867..f1cea282aae32 100644
--- a/fs/kernfs/mount.c
+++ b/fs/kernfs/mount.c
@@ -220,12 +220,19 @@ struct dentry *kernfs_node_dentry(struct kernfs_node *kn,
 		return dentry;
 
 	root = kernfs_root(kn);
-	guard(rwsem_read)(&root->kernfs_rwsem);
-
-	knparent = find_next_ancestor(kn, NULL);
-	if (WARN_ON(!knparent)) {
-		dput(dentry);
+	/*
+	 * As long as kn is valid, its parent can not vanish. This is cgroup's
+	 * kn so it not have its parent replaced. Therefore it is safe to use
+	 * the ancestor node outside of the RCU or locked section.
+	 */
+	if (WARN_ON_ONCE(!(root->flags & KERNFS_ROOT_INVARIANT_PARENT)))
 		return ERR_PTR(-EINVAL);
+	scoped_guard(rcu) {
+		knparent = find_next_ancestor(kn, NULL);
+		if (WARN_ON(!knparent)) {
+			dput(dentry);
+			return ERR_PTR(-EINVAL);
+		}
 	}
 
 	do {
@@ -235,14 +242,22 @@ struct dentry *kernfs_node_dentry(struct kernfs_node *kn,
 
 		if (kn == knparent)
 			return dentry;
-		kntmp = find_next_ancestor(kn, knparent);
-		if (WARN_ON(!kntmp)) {
-			dput(dentry);
-			return ERR_PTR(-EINVAL);
+
+		scoped_guard(rwsem_read, &root->kernfs_rwsem) {
+			kntmp = find_next_ancestor(kn, knparent);
+			if (WARN_ON(!kntmp)) {
+				dput(dentry);
+				return ERR_PTR(-EINVAL);
+			}
+			name = kstrdup(kernfs_rcu_name(kntmp), GFP_KERNEL);
+		}
+		if (!name) {
+			dput(dentry);
+			return ERR_PTR(-ENOMEM);
 		}
-		name = rcu_dereference(kntmp->name);
 		dtmp = lookup_positive_unlocked(name, dentry, strlen(name));
 		dput(dentry);
+		kfree(name);
 		if (IS_ERR(dtmp))
 			return dtmp;
 		knparent = kntmp;
-- 
2.47.2

On Tue, Feb 18, 2025 at 05:39:38PM +0100, Sebastian Andrzej Siewior wrote:

> +	scoped_guard(rcu) {
> +		knparent = find_next_ancestor(kn, NULL);
> +		if (WARN_ON(!knparent)) {
> +			dput(dentry);
> +			return ERR_PTR(-EINVAL);
> +		}

NAK.  dput() is blocking; you *can't* do that under rcu_read_lock().

On 2025-02-20 20:39:24 [+0000], Al Viro wrote:
> On Tue, Feb 18, 2025 at 05:39:38PM +0100, Sebastian Andrzej Siewior wrote:
> 
> > +	scoped_guard(rcu) {
> > +		knparent = find_next_ancestor(kn, NULL);
> > +		if (WARN_ON(!knparent)) {
> > +			dput(dentry);
> > +			return ERR_PTR(-EINVAL);
> > +		}
> 
> NAK.  dput() is blocking; you *can't* do that under rcu_read_lock().

Thank you.
Since it got merged, let me do another one on top.

Sebastian

Al Viro pointed out that dput() might sleep and must not be invoked
within an RCU section.

Keep only find_next_ancestor() winthin the RCU section.
Correct the wording in the comment.

Fixes: 6ef5b6fae3040 ("kernfs: Drop kernfs_rwsem while invoking lookup_positive_unlocked().")
Reported-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 fs/kernfs/mount.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
index f1cea282aae32..5124e196c2bfd 100644
--- a/fs/kernfs/mount.c
+++ b/fs/kernfs/mount.c
@@ -222,17 +222,17 @@ struct dentry *kernfs_node_dentry(struct kernfs_node *kn,
 	root = kernfs_root(kn);
 	/*
 	 * As long as kn is valid, its parent can not vanish. This is cgroup's
-	 * kn so it not have its parent replaced. Therefore it is safe to use
+	 * kn so it can't have its parent replaced. Therefore it is safe to use
 	 * the ancestor node outside of the RCU or locked section.
 	 */
 	if (WARN_ON_ONCE(!(root->flags & KERNFS_ROOT_INVARIANT_PARENT)))
 		return ERR_PTR(-EINVAL);
 	scoped_guard(rcu) {
 		knparent = find_next_ancestor(kn, NULL);
-		if (WARN_ON(!knparent)) {
-			dput(dentry);
-			return ERR_PTR(-EINVAL);
-		}
+	}
+	if (WARN_ON(!knparent)) {
+		dput(dentry);
+		return ERR_PTR(-EINVAL);
 	}
 
 	do {
-- 
2.47.2

On Tue, Feb 18, 2025 at 05:39:38PM +0100, Sebastian Andrzej Siewior wrote:
> syzbot reported two warnings:
> - kernfs_node::name was accessed outside of a RCU section so it created
>   warning. The kernfs_rwsem was held so it was okay but it wasn't seen.
> 
> - While kernfs_rwsem was held invoked lookup_positive_unlocked()->
>   kernfs_dop_revalidate() which acquired kernfs_rwsem.
> 
> kernfs_rwsem was both acquired as a read lock so it can be acquired
> twice. However if a writer acquires the lock after the first reader then
> neither the writer nor the second reader can obtain the lock so it
> deadlocks.
> 
> The reason for the lock is to ensure that kernfs_node::name remain
> stable during lookup_positive_unlocked()'s invocation. The function can
> not be invoked within a RCU section because it may sleep.
> 
> Make a temporary copy of the kernfs_node::name under the lock so
> GFP_KERNEL can be used and use this instead.
> 
> Reported-by: syzbot+ecccecbc636b455f9084@syzkaller.appspotmail.com
> Fixes: 5b2fabf7fe8f ("kernfs: Acquire kernfs_rwsem in kernfs_node_dentry().")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun