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

[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@linux.dev>
---
 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

[PATCH 2/3] kernfs: don't lose IN_DELETE_SELF when decoding a file handle

[Shakeel Butt]

__kernfs_remove() clears i_nlink on the inode each superblock holds for
the node, and finds those inodes with ilookup().  A lookup that has
pinned the node but has not put its new inode in the inode hash yet is
invisible to that pass:

  CPU0                                    CPU1
  open_by_handle_at()
    kernfs_find_and_get_node_by_id()
      pins the node, still active
                                          rmdir()
                                            marks the subtree removing
                                            ilookup() finds no inode
    kernfs_get_inode()
      hashes an inode with i_nlink 1

Nothing corrects it afterwards, because kernfs_refresh_inode() skips
set_nlink() for a directory being removed and never touches i_nlink for
a file.  The inode keeps the i_nlink of 1 that inode_init_always() gave
it, and dentry_unlink_inode() only sends IN_DELETE_SELF when i_nlink is
0, so a watcher is never told the node went away.

kernfs_rwsem keeps the other callers of kernfs_get_inode() in fs/kernfs
out of that window, and the one outside it, cgroup_may_write(), is kept
out by cgroup_mutex, which cgroup_destroy_locked() holds across its
kernfs_remove().  __kernfs_fh_to_dentry() has neither; it has created
inodes without a lock ever since exportfs support was added.

Take kernfs_rwsem for reading, as ->get_parent already does.  It has to
cover the lookup by id as well, not just kernfs_get_inode():
__kernfs_remove() marks and deactivates the whole subtree under the write
lock before kernfs_drain() first drops it, so under the read lock an
active node is one that is not going away, and the lookup returns NULL
for the rest.  The other outcome, that CPU0 takes the lock first, leaves
the inode in the hash before the ilookup() pass runs, so the pass finds
it.

The same argument covers ->fh_to_parent, since a node cannot be active
while an ancestor is being removed.

Reproduced with a 300ms delay in __kernfs_fh_to_dentry(), between the
lookup by id and kernfs_get_inode(): one task decodes a handle for a
file in a cgroup directory while another rmdir()s that directory.  The
result has st_nlink 1 without this patch and 0 with it.

->get_parent keeps a window of its own that this does not close.  It
already holds the same lock, but it has no active check, so
reconnect_path() can still build an inode for an ancestor whose removal
has finished, on the very open_by_handle_at() that got here.  Closing
that wants the active test rather than another lock, and it changes what
->get_parent returns for a node that is already gone, so it is left to
the series that reworks these paths.

Fixes: eea5d2bb34ba ("kernfs: Send IN_DELETE_SELF and IN_IGNORED")
Cc: stable@vger.kernel.org
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 fs/kernfs/mount.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
index f183a96778b9..c15ba6357162 100644
--- a/fs/kernfs/mount.c
+++ b/fs/kernfs/mount.c
@@ -124,22 +124,32 @@ static struct dentry *__kernfs_fh_to_dentry(struct super_block *sb,
 		return NULL;
 	}
 
-	kn = kernfs_find_and_get_node_by_id(info->root, id);
-	if (!kn)
-		return ERR_PTR(-ESTALE);
+	/*
+	 * Hold kernfs_rwsem across the lookup as well as kernfs_get_inode().
+	 * __kernfs_remove() deactivates the subtree and clears i_nlink on its
+	 * inodes under the write lock, so under the read lock either
+	 * kernfs_find_and_get_node_by_id() refuses the node, or the inode is
+	 * in the inode hash before the ilookup() pass goes looking for it.
+	 */
+	scoped_guard(rwsem_read, &info->root->kernfs_rwsem) {
+		kn = kernfs_find_and_get_node_by_id(info->root, id);
+		if (!kn)
+			return ERR_PTR(-ESTALE);
 
-	if (get_parent) {
-		struct kernfs_node *parent;
+		if (get_parent) {
+			struct kernfs_node *parent;
 
-		parent = kernfs_get_parent(kn);
+			parent = kernfs_get_parent(kn);
+			kernfs_put(kn);
+			kn = parent;
+			if (!kn)
+				return ERR_PTR(-ESTALE);
+		}
+
+		inode = kernfs_get_inode(sb, kn);
 		kernfs_put(kn);
-		kn = parent;
-		if (!kn)
-			return ERR_PTR(-ESTALE);
 	}
 
-	inode = kernfs_get_inode(sb, kn);
-	kernfs_put(kn);
 	return d_obtain_alias(inode);
 }
 
-- 
2.53.0-Meta

Re: [PATCH 2/3] kernfs: don't lose IN_DELETE_SELF when decoding a file handle

[Tejun Heo]

On Wed, Sep 02, 2026 at 09:02:52PM -0700, Shakeel Butt wrote:
> __kernfs_remove() clears i_nlink on the inode each superblock holds for
> the node, and finds those inodes with ilookup().  A lookup that has
> pinned the node but has not put its new inode in the inode hash yet is
> invisible to that pass:
> 
>   CPU0                                    CPU1
>   open_by_handle_at()
>     kernfs_find_and_get_node_by_id()
>       pins the node, still active
>                                           rmdir()
>                                             marks the subtree removing
>                                             ilookup() finds no inode
>     kernfs_get_inode()
>       hashes an inode with i_nlink 1
...
> Fixes: eea5d2bb34ba ("kernfs: Send IN_DELETE_SELF and IN_IGNORED")
> Cc: stable@vger.kernel.org
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>

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

Thanks.

-- 
tejun

[PATCH 3/3] kernfs: fix up the unlocked attribute reads on the creation paths

[Shakeel Butt]

Two creation paths read a live node's attributes without holding
kernfs_iattr_rwsem, which kernfs_iop_setattr() takes for writing.  They
do not want the same fix.

kernfs_create_link() copies the target's ia_uid and then its ia_gid
into the new link.  A chown of the target between the two reads leaves
the link with the old uid and the new gid, an owner the target never
had, when copying the target's owner is the whole point.  Read both
fields under the rwsem.

kernfs_new_node() reads the parent's ia_gid for S_ISGID inheritance,
and that one does not care which value it gets: the node does not exist
yet, so nothing orders a racing chown against the creation either way.
Taking the rwsem there would only serialize creation under a set-gid
parent against a chown of that parent, to pick between two answers that
are both right.  Mark the field read data_race() instead.

The pointer that leads to it is a different matter: __kernfs_iattrs()
publishes kernfs_node::iattr with try_cmpxchg(), so there is no
unmarked write for that read to pair with, and both sides read it with
READ_ONCE() like the rest of fs/kernfs does.

The Fixes tag is for the symlink half.  kernfs_create_link() has read
the pair without a lock since it started copying the target's owner at
all; only the name of the lock its writer takes has changed since.  The
data_race() is a marking rather than a fix.

Fixes: 488dee96bb62 ("kernfs: allow creating kernfs objects with arbitrary uid/gid")
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 fs/kernfs/dir.c     | 16 +++++++++++++---
 fs/kernfs/symlink.c | 17 ++++++++++++++---
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 214c97130a8a..3997c02f0165 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -740,9 +740,19 @@ struct kernfs_node *kernfs_new_node(struct kernfs_node *parent,
 		/* this code block imitates inode_init_owner() for
 		 * kernfs
 		 */
-
-		if (parent->iattr)
-			gid = parent->iattr->ia_gid;
+		struct kernfs_iattrs *attrs = READ_ONCE(parent->iattr);
+
+		if (attrs) {
+			/*
+			 * Unlocked on purpose: the gid is inherited onto a
+			 * node that does not exist yet, so nothing orders a
+			 * racing chown against this creation, and either
+			 * value is correct.  The pointer above needs no such
+			 * marking, __kernfs_iattrs() publishes it with
+			 * try_cmpxchg().
+			 */
+			gid = data_race(attrs->ia_gid);
+		}
 
 		if (flags & KERNFS_DIR)
 			mode |= S_ISGID;
diff --git a/fs/kernfs/symlink.c b/fs/kernfs/symlink.c
index 90e2b3221b83..3e53105d3abf 100644
--- a/fs/kernfs/symlink.c
+++ b/fs/kernfs/symlink.c
@@ -31,9 +31,20 @@ struct kernfs_node *kernfs_create_link(struct kernfs_node *parent,
 	kuid_t uid = GLOBAL_ROOT_UID;
 	kgid_t gid = GLOBAL_ROOT_GID;
 
-	if (target->iattr) {
-		uid = target->iattr->ia_uid;
-		gid = target->iattr->ia_gid;
+	/*
+	 * A symlink takes its owner from its target, so both fields have to
+	 * come from the same moment: read them under kernfs_iattr_rwsem, or
+	 * a chown of the target racing this could leave the link with the
+	 * old uid and the new gid.  The section ends before kernfs_add_one()
+	 * takes kernfs_rwsem.
+	 */
+	scoped_guard(rwsem_read, &kernfs_root(target)->kernfs_iattr_rwsem) {
+		struct kernfs_iattrs *attrs = READ_ONCE(target->iattr);
+
+		if (attrs) {
+			uid = attrs->ia_uid;
+			gid = attrs->ia_gid;
+		}
 	}
 
 	kn = kernfs_new_node(parent, name, S_IFLNK|0777, uid, gid, KERNFS_LINK);
-- 
2.53.0-Meta

Re: [PATCH 3/3] kernfs: fix up the unlocked attribute reads on the creation paths

[Tejun Heo]

On Wed, Sep 02, 2026 at 09:02:53PM -0700, Shakeel Butt wrote:
> Two creation paths read a live node's attributes without holding
> kernfs_iattr_rwsem, which kernfs_iop_setattr() takes for writing.  They
> do not want the same fix.
> 
> kernfs_create_link() copies the target's ia_uid and then its ia_gid
> into the new link.  A chown of the target between the two reads leaves
> the link with the old uid and the new gid, an owner the target never
> had, when copying the target's owner is the whole point.  Read both
> fields under the rwsem.
> 
> kernfs_new_node() reads the parent's ia_gid for S_ISGID inheritance,
> and that one does not care which value it gets: the node does not exist
> yet, so nothing orders a racing chown against the creation either way.
> Taking the rwsem there would only serialize creation under a set-gid
> parent against a chown of that parent, to pick between two answers that
> are both right.  Mark the field read data_race() instead.
> 
> The pointer that leads to it is a different matter: __kernfs_iattrs()
> publishes kernfs_node::iattr with try_cmpxchg(), so there is no
> unmarked write for that read to pair with, and both sides read it with
> READ_ONCE() like the rest of fs/kernfs does.
> 
> The Fixes tag is for the symlink half.  kernfs_create_link() has read
> the pair without a lock since it started copying the target's owner at
> all; only the name of the lock its writer takes has changed since.  The
> data_race() is a marking rather than a fix.
> 
> Fixes: 488dee96bb62 ("kernfs: allow creating kernfs objects with arbitrary uid/gid")
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>

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

> +		if (attrs) {
> +			/*
> +			 * Unlocked on purpose: the gid is inherited onto a
> +			 * node that does not exist yet, so nothing orders a
> +			 * racing chown against this creation, and either
> +			 * value is correct.  The pointer above needs no such
> +			 * marking, __kernfs_iattrs() publishes it with
> +			 * try_cmpxchg().
> +			 */
> +			gid = data_race(attrs->ia_gid);

Nit: READ_ONCE() is likely the better fit as it's an intentional lockless
read whose value is used.

Thanks.

-- 
tejun

Re: [PATCH 3/3] kernfs: fix up the unlocked attribute reads on the creation paths

[Shakeel Butt]

On Thu, Sep 03, 2026 at 10:26:07AM -1000, Tejun Heo wrote:
> On Wed, Sep 02, 2026 at 09:02:53PM -0700, Shakeel Butt wrote:
> > Two creation paths read a live node's attributes without holding
> > kernfs_iattr_rwsem, which kernfs_iop_setattr() takes for writing.  They
> > do not want the same fix.
> > 
> > kernfs_create_link() copies the target's ia_uid and then its ia_gid
> > into the new link.  A chown of the target between the two reads leaves
> > the link with the old uid and the new gid, an owner the target never
> > had, when copying the target's owner is the whole point.  Read both
> > fields under the rwsem.
> > 
> > kernfs_new_node() reads the parent's ia_gid for S_ISGID inheritance,
> > and that one does not care which value it gets: the node does not exist
> > yet, so nothing orders a racing chown against the creation either way.
> > Taking the rwsem there would only serialize creation under a set-gid
> > parent against a chown of that parent, to pick between two answers that
> > are both right.  Mark the field read data_race() instead.
> > 
> > The pointer that leads to it is a different matter: __kernfs_iattrs()
> > publishes kernfs_node::iattr with try_cmpxchg(), so there is no
> > unmarked write for that read to pair with, and both sides read it with
> > READ_ONCE() like the rest of fs/kernfs does.
> > 
> > The Fixes tag is for the symlink half.  kernfs_create_link() has read
> > the pair without a lock since it started copying the target's owner at
> > all; only the name of the lock its writer takes has changed since.  The
> > data_race() is a marking rather than a fix.
> > 
> > Fixes: 488dee96bb62 ("kernfs: allow creating kernfs objects with arbitrary uid/gid")
> > Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
> > +		if (attrs) {
> > +			/*
> > +			 * Unlocked on purpose: the gid is inherited onto a
> > +			 * node that does not exist yet, so nothing orders a
> > +			 * racing chown against this creation, and either
> > +			 * value is correct.  The pointer above needs no such
> > +			 * marking, __kernfs_iattrs() publishes it with
> > +			 * try_cmpxchg().
> > +			 */
> > +			gid = data_race(attrs->ia_gid);
> 
> Nit: READ_ONCE() is likely the better fit as it's an intentional lockless
> read whose value is used.
> 

Thanks TJ for the review. I will fix this in next version.

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

[Greg Kroah-Hartman]

On Wed, Sep 02, 2026 at 09:02:51PM -0700, Shakeel Butt wrote:
> 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@linux.dev>
> ---
>  fs/kernfs/dir.c             | 28 +++++++++++++++-------------
>  fs/kernfs/kernfs-internal.h |  9 ++++++++-
>  2 files changed, 23 insertions(+), 14 deletions(-)

How was this found and tested?  Did you forget an Assisted-by: tag?

thanks,

greg k-h

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

[Shakeel Butt]

On Thu, Sep 03, 2026 at 06:31:58AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 02, 2026 at 09:02:51PM -0700, Shakeel Butt wrote:
> > 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@linux.dev>
> > ---
> >  fs/kernfs/dir.c             | 28 +++++++++++++++-------------
> >  fs/kernfs/kernfs-internal.h |  9 ++++++++-
> >  2 files changed, 23 insertions(+), 14 deletions(-)
> 
> How was this found and tested?  Did you forget an Assisted-by: tag?

I am working on a series to improve kernfs_rwsem and going through
review-prompt with AI to review my series and these were existing 
issues AI found. I have created reproducers with AI for these and
tested that these patches those.

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

[Greg Kroah-Hartman]

On Wed, Sep 02, 2026 at 10:37:21PM -0700, Shakeel Butt wrote:
> On Thu, Sep 03, 2026 at 06:31:58AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Sep 02, 2026 at 09:02:51PM -0700, Shakeel Butt wrote:
> > > 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@linux.dev>
> > > ---
> > >  fs/kernfs/dir.c             | 28 +++++++++++++++-------------
> > >  fs/kernfs/kernfs-internal.h |  9 ++++++++-
> > >  2 files changed, 23 insertions(+), 14 deletions(-)
> > 
> > How was this found and tested?  Did you forget an Assisted-by: tag?
> 
> I am working on a series to improve kernfs_rwsem and going through
> review-prompt with AI to review my series and these were existing 
> issues AI found. I have created reproducers with AI for these and
> tested that these patches those.

Then please read our documentation for how to properly document this
usage of a LLM tool.

If you have reproducers, please add them to the kernfs tests as well as
patches part of this series when you resend them.

thanks,

gre gk-h

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

[Shakeel Butt]

On Thu, Sep 03, 2026 at 07:41:53AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 02, 2026 at 10:37:21PM -0700, Shakeel Butt wrote:
> > On Thu, Sep 03, 2026 at 06:31:58AM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Sep 02, 2026 at 09:02:51PM -0700, Shakeel Butt wrote:
> > > > 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@linux.dev>
> > > > ---
> > > >  fs/kernfs/dir.c             | 28 +++++++++++++++-------------
> > > >  fs/kernfs/kernfs-internal.h |  9 ++++++++-
> > > >  2 files changed, 23 insertions(+), 14 deletions(-)
> > > 
> > > How was this found and tested?  Did you forget an Assisted-by: tag?
> > 
> > I am working on a series to improve kernfs_rwsem and going through
> > review-prompt with AI to review my series and these were existing 
> > issues AI found. I have created reproducers with AI for these and
> > tested that these patches those.
> 
> Then please read our documentation for how to properly document this
> usage of a LLM tool.

Sure

> 
> If you have reproducers, please add them to the kernfs tests as well as
> patches part of this series when you resend them.
>

The reproducers are like stress tests and are targeting race conditions.
In one case delay was added to fully expose the race. I am not sure
selftests is the right place for this kind of tests. I can just publish
the reproducer on the list to have them on record if that is what you
are looking for.

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

[Shakeel Butt]

On Wed, Sep 02, 2026 at 11:15:16PM -0700, Shakeel Butt wrote:
> On Thu, Sep 03, 2026 at 07:41:53AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Sep 02, 2026 at 10:37:21PM -0700, Shakeel Butt wrote:
> > > On Thu, Sep 03, 2026 at 06:31:58AM +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Sep 02, 2026 at 09:02:51PM -0700, Shakeel Butt wrote:
> > > > > 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@linux.dev>
> > > > > ---
> > > > >  fs/kernfs/dir.c             | 28 +++++++++++++++-------------
> > > > >  fs/kernfs/kernfs-internal.h |  9 ++++++++-
> > > > >  2 files changed, 23 insertions(+), 14 deletions(-)
> > > > 
> > > > How was this found and tested?  Did you forget an Assisted-by: tag?
> > > 
> > > I am working on a series to improve kernfs_rwsem and going through
> > > review-prompt with AI to review my series and these were existing 
> > > issues AI found. I have created reproducers with AI for these and
> > > tested that these patches those.
> > 
> > Then please read our documentation for how to properly document this
> > usage of a LLM tool.
> 
> Sure
> 
> > 
> > If you have reproducers, please add them to the kernfs tests as well as
> > patches part of this series when you resend them.
> >
> 
> The reproducers are like stress tests and are targeting race conditions.
> In one case delay was added to fully expose the race. I am not sure
> selftests is the right place for this kind of tests. I can just publish
> the reproducer on the list to have them on record if that is what you
> are looking for.

Greg, let me know what would you prefer. I can add selftests which execise the
paths these bugs are on but to trigger the bug, more stress would be needed and
still will not trigger the bug always.

Also I have inflight kernfs selftest patch [1] as well. I can combine that to
this series. 

[1] https://lore.kernel.org/all/20260902014050.499002-1-shakeel.butt@linux.dev/

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

[Tejun Heo]

Hello,

On Wed, Sep 02, 2026 at 09:02:51PM -0700, Shakeel Butt wrote:
...
>   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.

Well, I'm not sure this is a real problem. Do we even have places where
multiple nodes along the hierarchy can be renamed? And the only thing we do
with the formatted paths is printing them out somewhere.

> 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@linux.dev>

That said, it theoretically is a bug, so, why not?

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

Thanks.

-- 
tejun