[PATCH v2 0/4] kernfs: three standalone fixes

[PATCH v2 0/4] kernfs: three standalone fixes

[Shakeel Butt]

Three independent kernfs fixes, plus selftest coverage for the paths
they touch.  The fixes do not depend on each other.

Patch 1 adds tests for decoding a file handle and for a rename that
keeps the same parent.  Patches 2-4:

 - kernfs_rename_ns() takes kernfs_rename_lock only when the parent
   changes, so two same-parent renames inside one
   kernfs_path_from_node() walk can build a path that never existed.
   sysfs_warn_dup() is the only caller that can see it.

 - __kernfs_fh_to_dentry() creates inodes with no lock, so a decode
   racing rmdir() can hash an inode after the removal's ilookup()
   pass.  The inode keeps i_nlink 1, so no IN_DELETE_SELF is sent.

 - kernfs_create_link() reads the target's uid and gid unlocked, so a
   chown in between gives the link an owner the target never had.

Patch 1 applies on top of the patch [1] ("selftests: cover kernfs
dentry revalidation") with vfs-7.4.kernfs branch in vfs tree as base.

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

Changes since v1:
http://lore.kernel.org/20260903040253.670020-1-shakeel.butt@linux.dev
- Added cover letter
- Used approppriate tags
- Updated commit messages to be more concise
- Replaced data_race() with READ_ONCE() (TJ)

Shakeel Butt (4):
  selftests: cover kernfs file handles and same-parent rename
  kernfs: take kernfs_rename_lock for same-parent renames too
  kernfs: don't lose IN_DELETE_SELF when decoding a file handle
  kernfs: fix up the unlocked attribute reads on the creation paths

 fs/kernfs/dir.c                               |  40 ++-
 fs/kernfs/kernfs-internal.h                   |   9 +-
 fs/kernfs/mount.c                             |  32 +-
 fs/kernfs/symlink.c                           |  17 +-
 tools/testing/selftests/filesystems/config    |   1 +
 .../selftests/filesystems/kernfs_test.c       | 296 +++++++++++++++++-
 6 files changed, 362 insertions(+), 33 deletions(-)


base-commit: 47fc64fb3b433abb9f2242a85ba808ac6f87df22
-- 
2.53.0-Meta

[PATCH v2 1/4] selftests: cover kernfs file handles and same-parent rename

[Shakeel Butt]

The kernfs tests only reach kernfs through lookup and readdir.  Two
paths are not covered: file handles, which find a node without a lookup
through its parent, and rename of a node that keeps its parent.

Add three tests:

 - decode a file handle, live and after the node is gone;
 - decode while the node is being removed;
 - look up a name while an interface is renamed, which renames its
   /sys/class/net entry with the parent unchanged.

Two small fixes while here.  ns_tag_isolates_class_net now counts only
symlinks, because bonding adds a bonding_masters attribute that is not a
device.  A failing mkdtemp() now skips instead of aborting.

Assisted-by: LLM
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 tools/testing/selftests/filesystems/config    |   1 +
 .../selftests/filesystems/kernfs_test.c       | 296 +++++++++++++++++-
 2 files changed, 295 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/filesystems/config b/tools/testing/selftests/filesystems/config
index 7231710d5ce7..9f45bc493a30 100644
--- a/tools/testing/selftests/filesystems/config
+++ b/tools/testing/selftests/filesystems/config
@@ -1,5 +1,6 @@
 CONFIG_CGROUPS=y
 CONFIG_CGROUP_PIDS=y
+CONFIG_FHANDLE=y
 CONFIG_NAMESPACES=y
 CONFIG_NET=y
 CONFIG_NET_NS=y
diff --git a/tools/testing/selftests/filesystems/kernfs_test.c b/tools/testing/selftests/filesystems/kernfs_test.c
index 2178428b9665..6e74da91ebca 100644
--- a/tools/testing/selftests/filesystems/kernfs_test.c
+++ b/tools/testing/selftests/filesystems/kernfs_test.c
@@ -610,6 +610,189 @@ TEST_F(kernfs_cgroup, lookup_vs_create_remove_stress)
 	}
 }
 
+struct kernfs_handle {
+	struct file_handle h;
+	unsigned char buf[MAX_HANDLE_SZ];
+};
+
+static int kernfs_encode(const char *path, struct kernfs_handle *fh)
+{
+	int mount_id;
+
+	memset(fh, 0, sizeof(*fh));
+	fh->h.handle_bytes = sizeof(fh->buf);
+	return name_to_handle_at(AT_FDCWD, path, &fh->h, &mount_id, 0);
+}
+
+/*
+ * Skip only where file handles do not work at all.  ENOENT must still
+ * fail: mkdir leaves a negative dentry cached, so the name resolves only
+ * after ->d_revalidate() drops it.  The encode tests revalidation too.
+ */
+static bool fh_unsupported(int err)
+{
+	return err == EOPNOTSUPP || err == EPERM || err == ENOSYS;
+}
+
+/*
+ * Decoding a file needs CAP_DAC_READ_SEARCH in the initial user
+ * namespace.  Probe once so the tests skip instead of fail.
+ */
+static bool fh_can_decode(int mfd, struct kernfs_handle *fh)
+{
+	int fd = open_by_handle_at(mfd, &fh->h, O_PATH);
+
+	if (fd < 0)
+		return errno != EPERM;
+	close(fd);
+	return true;
+}
+
+/*
+ * A file handle reaches a node without a lookup through its parent.  A
+ * live node must decode.  A removed one must not, because
+ * kernfs_find_and_get_node_by_id() refuses inactive nodes.
+ *
+ * Use O_PATH: opening a removed node fails with ENODEV, which would hide
+ * what is being tested.
+ */
+TEST_F(kernfs_cgroup, exportfs_decode_and_stale)
+{
+	char victim[PATH_MAX], procs[PATH_MAX];
+	struct kernfs_handle fh;
+	struct stat st;
+	int mfd, fd;
+
+	snprintf(victim, sizeof(victim), "%s/fh", self->scratch);
+	snprintf(procs, sizeof(procs), "%s/cgroup.procs", victim);
+	ASSERT_EQ(mkdir(victim, 0755), 0);
+
+	/* Any fd on the filesystem identifies it to open_by_handle_at(). */
+	mfd = open(self->scratch, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
+	ASSERT_GE(mfd, 0);
+
+	if (kernfs_encode(procs, &fh)) {
+		int err = errno;
+
+		close(mfd);
+		rmdir(victim);
+		ASSERT_TRUE(fh_unsupported(err))
+			TH_LOG("name_to_handle_at: %s", strerror(err));
+		SKIP(return, "name_to_handle_at: %s", strerror(err));
+	}
+
+	if (!fh_can_decode(mfd, &fh)) {
+		close(mfd);
+		rmdir(victim);
+		SKIP(return, "open_by_handle_at: no CAP_DAC_READ_SEARCH");
+	}
+
+	fd = open_by_handle_at(mfd, &fh.h, O_PATH);
+	ASSERT_GE(fd, 0);
+	EXPECT_EQ(fstat(fd, &st), 0);
+	EXPECT_EQ(st.st_nlink, 1);
+	EXPECT_EQ(close(fd), 0);
+
+	ASSERT_EQ(rmdir(victim), 0);
+
+	fd = open_by_handle_at(mfd, &fh.h, O_PATH);
+	EXPECT_LT(fd, 0);
+	if (fd >= 0)
+		close(fd);
+	else
+		EXPECT_EQ(errno, ESTALE);
+
+	EXPECT_EQ(close(mfd), 0);
+}
+
+#define FH_STRESS_SECS	2
+#define FH_DECODE_CAP	10000
+
+/*
+ * Decode file handles while the node is being removed.  A decode must
+ * answer with a usable handle or ESTALE, never garbage and never a hang.
+ *
+ * The link count is checked too.  An inode that reaches the inode hash
+ * after the removal cleared link counts keeps the 1 it was born with, so
+ * it never gets an IN_DELETE_SELF.  This has not been seen to fire: it
+ * needs the decode to stall between the lookup by id and the hash insert,
+ * and nothing there blocks.  It is kept because it is cheap and only
+ * looks once the directory is gone, so it cannot fail falsely.
+ */
+TEST_F(kernfs_cgroup, exportfs_decode_vs_rmdir_stress)
+{
+	int mfd, bad = 0, rounds = 0;
+	struct timespec end;
+
+	mfd = open(self->scratch, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
+	ASSERT_GE(mfd, 0);
+
+	clock_gettime(CLOCK_MONOTONIC, &end);
+	end.tv_sec += FH_STRESS_SECS;
+
+	while (!stress_deadline(&end)) {
+		char victim[PATH_MAX], procs[PATH_MAX];
+		int last = -1, fd, i;
+		struct kernfs_handle fh;
+		struct stat st;
+		pid_t pid;
+
+		snprintf(victim, sizeof(victim), "%s/fh%d", self->scratch,
+			 rounds++);
+		snprintf(procs, sizeof(procs), "%s/cgroup.procs", victim);
+		if (mkdir(victim, 0755))
+			break;
+		if (kernfs_encode(procs, &fh)) {
+			int err = errno;
+
+			rmdir(victim);
+			ASSERT_TRUE(fh_unsupported(err))
+				TH_LOG("name_to_handle_at: %s", strerror(err));
+			SKIP(goto out, "name_to_handle_at: %s", strerror(err));
+		}
+		if (rounds == 1 && !fh_can_decode(mfd, &fh)) {
+			rmdir(victim);
+			SKIP(goto out,
+			     "open_by_handle_at: no CAP_DAC_READ_SEARCH");
+		}
+
+		pid = fork();
+		ASSERT_GE(pid, 0);
+		if (pid == 0) {
+			rmdir_retry(victim);
+			_exit(0);
+		}
+
+		/*
+		 * Decode until the removal deactivates the node.  Keep the
+		 * last one that worked: it ran closest to the removal.
+		 */
+		for (i = 0; i < FH_DECODE_CAP; i++) {
+			fd = open_by_handle_at(mfd, &fh.h, O_PATH);
+			if (fd < 0)
+				break;
+			if (last >= 0)
+				close(last);
+			last = fd;
+		}
+		ASSERT_EQ(waitpid(pid, NULL, 0), pid);
+
+		if (last >= 0) {
+			if (access(victim, F_OK) && errno == ENOENT &&
+			    !fstat(last, &st) && st.st_nlink != 0)
+				bad++;
+			close(last);
+		}
+		rmdir(victim);
+	}
+
+	EXPECT_EQ(bad, 0)
+		TH_LOG("%d of %d rounds decoded a removed node whose inode kept its link count",
+		       bad, rounds);
+out:
+	close(mfd);
+}
+
 /*
  * sysfs is namespace tagged (KERNFS_NS) and supports rename; cgroup2 does
  * neither.  Run in a private netns with its own sysfs so the host is
@@ -635,7 +818,8 @@ FIXTURE_SETUP(kernfs_netns)
 	ASSERT_EQ(mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL), 0);
 
 	strcpy(self->mnt, "/tmp/kernfs_selftest_sysfs.XXXXXX");
-	ASSERT_NE(mkdtemp(self->mnt), NULL);
+	if (!mkdtemp(self->mnt))
+		SKIP(return, "mkdtemp: %s", strerror(errno));
 
 	if (mount("none", self->mnt, "sysfs", 0, NULL)) {
 		rmdir(self->mnt);
@@ -662,6 +846,9 @@ FIXTURE_TEARDOWN(kernfs_netns)
  * depends on the modules the host has.  Check the set instead --
  * if_nametoindex() resolves in the current netns, so every name sysfs shows
  * must resolve there, and the counts must agree.
+ *
+ * Count only symlinks.  Not every entry is a device: bonding adds a
+ * bonding_masters attribute to /sys/class/net in every namespace.
  */
 TEST_F(kernfs_netns, ns_tag_isolates_class_net)
 {
@@ -674,7 +861,7 @@ TEST_F(kernfs_netns, ns_tag_isolates_class_net)
 	d = opendir(self->net);
 	ASSERT_NE(d, NULL);
 	while ((de = readdir(d))) {
-		if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, ".."))
+		if (de->d_type != DT_LNK)
 			continue;
 		EXPECT_NE(if_nametoindex(de->d_name), 0u)
 			TH_LOG("%s is not in this netns", de->d_name);
@@ -728,4 +915,109 @@ TEST_F(kernfs_netns, rename_is_revalidated)
 	EXPECT_EQ(stat(new_path, &st), 0);
 }
 
+static int netdev_rename(const char *from, const char *to)
+{
+	struct ifreq ifr = {};
+	int sk, ret;
+
+	sk = socket(AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+	if (sk < 0)
+		return -1;
+	strncpy(ifr.ifr_name, from, IFNAMSIZ - 1);
+	strncpy(ifr.ifr_newname, to, IFNAMSIZ - 1);
+	ret = ioctl(sk, SIOCSIFNAME, &ifr);
+	close(sk);
+	return ret;
+}
+
+/*
+ * Bounded by a count, not by time: every rename is logged and not rate
+ * limited, so a timed loop would flood the kernel log.
+ */
+#define RENAME_FLIPS		200
+#define RENAME_READERS		4
+
+/*
+ * Rename an interface while other tasks look up the names it moves
+ * between.  This renames its /sys/class/net entry through
+ * kernfs_rename_ns() with the parent unchanged.
+ *
+ * The renamer checks what is certain: SIOCSIFNAME returns once the rename
+ * is done and nothing else renames here, so the new name must resolve and
+ * the old must not.  The readers cannot check that, because the name can
+ * move between their two lstat() calls.  They only check that a lookup
+ * returns success or ENOENT, and keep the lock busy while renames run.
+ *
+ * lstat() not stat(): /sys/class/net/<dev> is a symlink and is renamed
+ * before the directory it points at, so the two are not atomic.
+ */
+TEST_F(kernfs_netns, rename_vs_lookup_stress)
+{
+	char old_path[PATH_MAX], new_path[PATH_MAX];
+	pid_t pids[RENAME_READERS];
+	int i, status, n = 0, bad = 0;
+	struct stat st;
+	int done[2];
+
+	snprintf(old_path, sizeof(old_path), "%s/lo", self->net);
+	snprintf(new_path, sizeof(new_path), "%s/%s", self->net, TEST_IFNAME);
+
+	if (netdev_rename("lo", TEST_IFNAME))
+		SKIP(return, "SIOCSIFNAME: %s", strerror(errno));
+	if (netdev_rename(TEST_IFNAME, "lo"))
+		SKIP(return, "SIOCSIFNAME back: %s", strerror(errno));
+
+	/* Readers run until the renamer closes the write end. */
+	ASSERT_EQ(pipe2(done, O_NONBLOCK | O_CLOEXEC), 0);
+
+	for (i = 0; i < RENAME_READERS; i++) {
+		pid_t pid = fork();
+
+		ASSERT_GE(pid, 0);
+		if (pid == 0) {
+			struct stat rst;
+			char c;
+
+			close(done[1]);
+			while (read(done[0], &c, 1) < 0 && errno == EAGAIN) {
+				if (lstat(old_path, &rst) && errno != ENOENT)
+					_exit(20);
+				if (lstat(new_path, &rst) && errno != ENOENT)
+					_exit(21);
+			}
+			_exit(0);
+		}
+		pids[n++] = pid;
+	}
+	close(done[0]);
+
+	for (i = 0; i < RENAME_FLIPS; i++) {
+		if (netdev_rename("lo", TEST_IFNAME))
+			break;
+		if (lstat(new_path, &st) || !lstat(old_path, &st)) {
+			bad++;
+			break;
+		}
+		if (netdev_rename(TEST_IFNAME, "lo"))
+			break;
+		if (lstat(old_path, &st) || !lstat(new_path, &st)) {
+			bad++;
+			break;
+		}
+	}
+	close(done[1]);
+
+	for (i = 0; i < n; i++) {
+		ASSERT_EQ(waitpid(pids[i], &status, 0), pids[i]);
+		ASSERT_TRUE(WIFEXITED(status));
+		EXPECT_EQ(WEXITSTATUS(status), 0);
+	}
+
+	EXPECT_EQ(bad, 0)
+		TH_LOG("a completed rename left the wrong name resolving");
+
+	/* Leave the interface as the fixture found it. */
+	netdev_rename(TEST_IFNAME, "lo");
+}
+
 TEST_HARNESS_MAIN
-- 
2.53.0-Meta

[PATCH v2 2/4] kernfs: take kernfs_rename_lock for same-parent renames too

[Shakeel Butt]

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 <tj@kernel.org>
Assisted-by: LLM
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 v2 3/4] kernfs: don't lose IN_DELETE_SELF when decoding a file handle

[Shakeel Butt]

__kernfs_remove() clears i_nlink on a node's inodes and finds them with
ilookup().  A decode that has pinned the node but not yet hashed its
inode 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 fixes it later: kernfs_refresh_inode() never touches i_nlink for
a file and skips it for a directory being removed.  The inode keeps the
1 it was born with, and dentry_unlink_inode() sends IN_DELETE_SELF only
at 0, so a watcher never learns the node went away.

The other callers of kernfs_get_inode() are safe: those in fs/kernfs
hold kernfs_rwsem, and cgroup_may_write() is covered by cgroup_mutex,
which cgroup_destroy_locked() holds across kernfs_remove().
__kernfs_fh_to_dentry() has held nothing since exportfs support was
added.

Take kernfs_rwsem for reading, as ->get_parent already does, and cover
the lookup as well as kernfs_get_inode().  __kernfs_remove() deactivates
the whole subtree under the write lock, so under the read lock either
the lookup refuses the node, or the inode is hashed before the ilookup()
pass runs.  The same holds for ->fh_to_parent, since a node cannot be
active while an ancestor is being removed.

Reproduced with a 300ms delay between the lookup and kernfs_get_inode(),
decoding a handle for a file in a cgroup directory while another task
rmdir()s it: st_nlink is 1 without this patch and 0 with it.

->get_parent still has a window of its own.  It takes the same lock but
has no active check, so reconnect_path() can build an inode for an
ancestor that is already gone.  That needs the active test rather than a
lock, and changes what ->get_parent returns, 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
Acked-by: Tejun Heo <tj@kernel.org>
Assisted-by: LLM
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

[PATCH v2 4/4] 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
need different fixes.

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.
Read both under the rwsem.

kernfs_new_node() reads the parent's mode and ia_gid for S_ISGID
inheritance.  Either value is fine there: the node does not exist yet,
so nothing orders a racing chmod or chown against the creation.  Taking
the rwsem would only pick between two answers that are both right.  Mark
the reads with READ_ONCE() instead.

The pointer that leads to them is already fine: __kernfs_iattrs()
publishes kernfs_node::iattr with try_cmpxchg(), and both sides read it
with READ_ONCE(), like the rest of fs/kernfs.

The Fixes tag is for the symlink half.  kernfs_create_link() has read
the pair unlocked since it started copying the target's owner; only the
name of the lock its writer takes has changed.  The READ_ONCE() markings
are not a fix.

Fixes: 488dee96bb62 ("kernfs: allow creating kernfs objects with arbitrary uid/gid")
Acked-by: Tejun Heo <tj@kernel.org>
Assisted-by: LLM
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 fs/kernfs/dir.c     | 12 +++++++++---
 fs/kernfs/symlink.c | 17 ++++++++++++++---
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 214c97130a8a..07abf59f0264 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -736,13 +736,19 @@ struct kernfs_node *kernfs_new_node(struct kernfs_node *parent,
 {
 	struct kernfs_node *kn;
 
-	if (parent->mode & S_ISGID) {
+	/*
+	 * The mode and the gid below are read unlocked on purpose: they feed
+	 * a node that does not exist yet, so nothing orders a racing chmod or
+	 * chown against this creation.
+	 */
+	if (READ_ONCE(parent->mode) & S_ISGID) {
 		/* this code block imitates inode_init_owner() for
 		 * kernfs
 		 */
+		struct kernfs_iattrs *attrs = READ_ONCE(parent->iattr);
 
-		if (parent->iattr)
-			gid = parent->iattr->ia_gid;
+		if (attrs)
+			gid = READ_ONCE(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