* [PATCH v2 1/4] selftests: cover kernfs file handles and same-parent rename
2026-09-05 19:16 [PATCH v2 0/4] kernfs: three standalone fixes Shakeel Butt
@ 2026-09-05 19:16 ` Shakeel Butt
2026-09-05 19:16 ` [PATCH v2 2/4] kernfs: take kernfs_rename_lock for same-parent renames too Shakeel Butt
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Shakeel Butt @ 2026-09-05 19:16 UTC (permalink / raw)
To: Greg Kroah-Hartman, Tejun Heo, Christian Brauner
Cc: Meta kernel team, linux-kselftest, driver-core, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v2 2/4] kernfs: take kernfs_rename_lock for same-parent renames too
2026-09-05 19:16 [PATCH v2 0/4] kernfs: three standalone fixes Shakeel Butt
2026-09-05 19:16 ` [PATCH v2 1/4] selftests: cover kernfs file handles and same-parent rename Shakeel Butt
@ 2026-09-05 19:16 ` Shakeel Butt
2026-09-05 19:16 ` [PATCH v2 3/4] kernfs: don't lose IN_DELETE_SELF when decoding a file handle Shakeel Butt
2026-09-05 19:16 ` [PATCH v2 4/4] kernfs: fix up the unlocked attribute reads on the creation paths Shakeel Butt
3 siblings, 0 replies; 5+ messages in thread
From: Shakeel Butt @ 2026-09-05 19:16 UTC (permalink / raw)
To: Greg Kroah-Hartman, Tejun Heo, Christian Brauner
Cc: Meta kernel team, linux-kselftest, driver-core, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread