* [PATCH 1/3] kernfs: don't repeat or skip an entry when readdir resumes
2026-09-10 0:36 [PATCH 0/3] kernfs: don't hold kernfs_rwsem across dir_emit() Shakeel Butt
@ 2026-09-10 0:36 ` Shakeel Butt
2026-09-10 0:36 ` [PATCH 2/3] kernfs: don't hold kernfs_rwsem across dir_emit() Shakeel Butt
2026-09-10 0:36 ` [PATCH 3/3] selftests: cover readdir resuming at a removed entry Shakeel Butt
2 siblings, 0 replies; 4+ messages in thread
From: Shakeel Butt @ 2026-09-10 0:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, Tejun Heo, Christian Brauner
Cc: Sebastian Andrzej Siewior, Meta kernel team, linux-fsdevel,
driver-core, linux-kernel
readdir keeps its place as a name hash in ctx->pos and pins the entry in
file->private_data. If that entry is gone when the listing comes back,
kernfs_dir_pos() searches the rbtree for the hash and keeps whatever
node the descent stopped on. That is the entry before or after the
missing one, depending on the shape of the tree.
Landing before it repeats an entry the previous getdents(2) call already
reported. Landing after it, kernfs_dir_next_pos() calls rb_next() and
steps over an unreported entry.
With children A(10), B(20), C(30):
report A, ctx->pos = 10
A removed
kernfs_dir_next_pos(10, A)
A is gone, the search for 10 stops at B
rb_next(B) -> C, so B is never reported
Only the repeat happens today, between two getdents(2) calls. The skip
needs the pinned entry to go away inside one call, which the next patch
allows when it drops kernfs_rwsem around dir_emit().
Before the commit 4e4d6d860b93 the descent kept a node only on the way
left, which is a search for the first entry at or after the hash. That
commit moved the assignment to the top of the loop, where it runs on
right turns too. Restore that search, and step forward only when the
pinned entry is still there rather than when the hash matches, since two
entries in one directory can share a hash.
While here, kernfs_dir_next_pos() called the hash @ino. It is never an
inode number, so name it @hash.
Fixes: 4e4d6d860b93 ("sysfs: Add s_hash to sysfs_dirent and order directory entries by hash")
Assisted-by: LLM
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
fs/kernfs/dir.c | 43 ++++++++++++++++++++++++++++---------------
1 file changed, 28 insertions(+), 15 deletions(-)
diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 07abf59f0264..24a927c85123 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -1903,9 +1903,16 @@ static int kernfs_dir_fop_release(struct inode *inode, struct file *filp)
return 0;
}
+/*
+ * Find where a listing left off. @resumed says whether @pos is still that
+ * entry; if not, the first entry at or after @hash is returned instead.
+ */
static struct kernfs_node *kernfs_dir_pos(const struct ns_common *ns,
- struct kernfs_node *parent, loff_t hash, struct kernfs_node *pos)
+ struct kernfs_node *parent, loff_t hash, struct kernfs_node *pos,
+ bool *resumed)
{
+ if (resumed)
+ *resumed = false;
if (pos) {
int valid = kernfs_active(pos) &&
rcu_access_pointer(pos->__parent) == parent &&
@@ -1913,23 +1920,26 @@ static struct kernfs_node *kernfs_dir_pos(const struct ns_common *ns,
kernfs_put(pos);
if (!valid)
pos = NULL;
+ else if (resumed)
+ *resumed = true;
}
if (!pos && (hash > 1) && (hash < INT_MAX)) {
struct rb_node *node = parent->dir.children.rb_node;
- u64 ns_id = kernfs_ns_id(ns);
+
+ /*
+ * Keep a node only on the way left, so the search ends on the
+ * first entry at or after @hash. The empty name sorts before
+ * every entry sharing the hash, so it lands on the first.
+ */
while (node) {
- pos = rb_to_kn(node);
+ struct kernfs_node *kn = rb_to_kn(node);
- if (hash < pos->hash)
+ if (kernfs_name_compare(hash, "", ns, kn) < 0) {
+ pos = kn;
node = node->rb_left;
- else if (hash > pos->hash)
+ } else {
node = node->rb_right;
- else if (ns_id < kernfs_ns_id(pos->ns))
- node = node->rb_left;
- else if (ns_id > kernfs_ns_id(pos->ns))
- node = node->rb_right;
- else
- break;
+ }
}
}
/* Skip over entries which are dying/dead or in the wrong namespace */
@@ -1945,10 +1955,13 @@ static struct kernfs_node *kernfs_dir_pos(const struct ns_common *ns,
}
static struct kernfs_node *kernfs_dir_next_pos(const struct ns_common *ns,
- struct kernfs_node *parent, ino_t ino, struct kernfs_node *pos)
+ struct kernfs_node *parent, loff_t hash, struct kernfs_node *pos)
{
- pos = kernfs_dir_pos(ns, parent, ino, pos);
- if (pos) {
+ bool resumed;
+
+ pos = kernfs_dir_pos(ns, parent, hash, pos, &resumed);
+ /* Step over @pos only if it survived; two entries can share a hash. */
+ if (pos && resumed) {
do {
struct rb_node *node = rb_next(&pos->rb);
if (!node)
@@ -1978,7 +1991,7 @@ static int kernfs_fop_readdir(struct file *file, struct dir_context *ctx)
if (kernfs_ns_enabled(parent))
ns = kernfs_info(dentry->d_sb)->ns;
- for (pos = kernfs_dir_pos(ns, parent, ctx->pos, pos);
+ for (pos = kernfs_dir_pos(ns, parent, ctx->pos, pos, NULL);
pos;
pos = kernfs_dir_next_pos(ns, parent, ctx->pos, pos)) {
const char *name = kernfs_rcu_name(pos);
--
2.53.0-Meta
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 2/3] kernfs: don't hold kernfs_rwsem across dir_emit()
2026-09-10 0:36 [PATCH 0/3] kernfs: don't hold kernfs_rwsem across dir_emit() Shakeel Butt
2026-09-10 0:36 ` [PATCH 1/3] kernfs: don't repeat or skip an entry when readdir resumes Shakeel Butt
@ 2026-09-10 0:36 ` Shakeel Butt
2026-09-10 0:36 ` [PATCH 3/3] selftests: cover readdir resuming at a removed entry Shakeel Butt
2 siblings, 0 replies; 4+ messages in thread
From: Shakeel Butt @ 2026-09-10 0:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, Tejun Heo, Christian Brauner
Cc: Sebastian Andrzej Siewior, Meta kernel team, linux-fsdevel,
driver-core, linux-kernel
kernfs_fop_readdir() holds kernfs_rwsem for reading across the whole
listing, dir_emit() included. dir_emit() copies to userspace, so it can
fault into reclaim while holding the lock that every create, remove and
rename in the hierarchy needs. sysfs and cgroupfs have one per machine.
Under memory pressure the monitoring daemons fault on their own
getdents(2) buffer with it held:
below: page allocation stall for 120 secs: order:0,
mode:0x140dca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO|__GFP_COMP)
nodemask=(null),cpuset=hostcritical.slice,mems_allowed=0
Call Trace:
<TASK>
dump_stack_lvl+0x5d/0x80
__alloc_frozen_pages_noprof+0x5f4d/0x6300
? memcg_list_lru_alloc+0x73/0x320
? ima_file_check+0xd0/0x7d0
vma_alloc_folio_noprof+0x145/0x560
handle_mm_fault+0x17c9/0x2720
? find_vma+0x27/0x30
do_user_addr_fault+0x39f/0x6e0
exc_page_fault+0x8f/0x110
asm_exc_page_fault+0x22/0x30
RIP: 0010:filldir64+0xd7/0x1a0
[Code:/RSP:/RAX:..R15: register block elided]
kernfs_fop_readdir+0x2de/0x420
iterate_dir+0x8c/0x1f0
__se_sys_getdents64+0x61/0xe0
? copy_page_from_iter+0x860/0x860
do_syscall_64+0x6a/0x250
entry_SYSCALL_64_after_hwframe+0x4b/0x53
</TASK>
Commit 9aab10a0249e ("kernfs: Don't re-lock kernfs_root::kernfs_rwsem in
kernfs_fop_readdir().") took the lock drop out because dir_emit() was
handed kernfs_node::name, which a rename can free. So copy the name
under the lock and emit the copy, and pass it to the resume so that a
resume within one call keys on (hash, ns_id, name) and not on the hash
alone.
A listing is no longer atomic within one getdents(2) call, which for
most sysfs and cgroup directories is all of it. POSIX leaves that
unspecified for an entry added or removed since opendir(3).
Fixes: 9aab10a0249e ("kernfs: Don't re-lock kernfs_root::kernfs_rwsem in kernfs_fop_readdir().")
Assisted-by: LLM
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
fs/kernfs/dir.c | 52 +++++++++++++++++++++++++++++++++++--------------
1 file changed, 37 insertions(+), 15 deletions(-)
diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 24a927c85123..f7cd2be67e1a 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -1905,11 +1905,11 @@ static int kernfs_dir_fop_release(struct inode *inode, struct file *filp)
/*
* Find where a listing left off. @resumed says whether @pos is still that
- * entry; if not, the first entry at or after @hash is returned instead.
+ * entry; if not, the search falls back to @hash, keyed by @name if given.
*/
static struct kernfs_node *kernfs_dir_pos(const struct ns_common *ns,
struct kernfs_node *parent, loff_t hash, struct kernfs_node *pos,
- bool *resumed)
+ const char *name, bool *resumed)
{
if (resumed)
*resumed = false;
@@ -1928,13 +1928,13 @@ static struct kernfs_node *kernfs_dir_pos(const struct ns_common *ns,
/*
* Keep a node only on the way left, so the search ends on the
- * first entry at or after @hash. The empty name sorts before
- * every entry sharing the hash, so it lands on the first.
+ * first entry after the key. An empty @name sorts before all
+ * entries sharing the hash, so it lands on the first of them.
*/
while (node) {
struct kernfs_node *kn = rb_to_kn(node);
- if (kernfs_name_compare(hash, "", ns, kn) < 0) {
+ if (kernfs_name_compare(hash, name ?: "", ns, kn) < 0) {
pos = kn;
node = node->rb_left;
} else {
@@ -1955,12 +1955,13 @@ static struct kernfs_node *kernfs_dir_pos(const struct ns_common *ns,
}
static struct kernfs_node *kernfs_dir_next_pos(const struct ns_common *ns,
- struct kernfs_node *parent, loff_t hash, struct kernfs_node *pos)
+ struct kernfs_node *parent, loff_t hash, struct kernfs_node *pos,
+ const char *name)
{
bool resumed;
- pos = kernfs_dir_pos(ns, parent, hash, pos, &resumed);
- /* Step over @pos only if it survived; two entries can share a hash. */
+ pos = kernfs_dir_pos(ns, parent, hash, pos, name, &resumed);
+ /* Step over @pos only if it survived; @name handles it if not. */
if (pos && resumed) {
do {
struct rb_node *node = rb_next(&pos->rb);
@@ -1979,34 +1980,55 @@ static int kernfs_fop_readdir(struct file *file, struct dir_context *ctx)
struct dentry *dentry = file->f_path.dentry;
struct kernfs_node *parent = kernfs_dentry_node(dentry);
struct kernfs_node *pos = file->private_data;
+ char *name __free(kfree) = NULL;
struct kernfs_root *root;
const struct ns_common *ns = NULL;
if (!dir_emit_dots(file, ctx))
return 0;
+ /*
+ * One buffer for the call, so each name can be copied out before
+ * dropping kernfs_rwsem. PATH_MAX: kernfs bounds no single name.
+ */
+ name = kmalloc(PATH_MAX, GFP_KERNEL);
+ if (!name)
+ return -ENOMEM;
+
root = kernfs_root(parent);
down_read(&root->kernfs_rwsem);
if (kernfs_ns_enabled(parent))
ns = kernfs_info(dentry->d_sb)->ns;
- for (pos = kernfs_dir_pos(ns, parent, ctx->pos, pos, NULL);
+ for (pos = kernfs_dir_pos(ns, parent, ctx->pos, pos, NULL, NULL);
pos;
- pos = kernfs_dir_next_pos(ns, parent, ctx->pos, pos)) {
- const char *name = kernfs_rcu_name(pos);
+ pos = kernfs_dir_next_pos(ns, parent, ctx->pos, pos, name)) {
unsigned int type = fs_umode_to_dtype(pos->mode);
- int len = strlen(name);
ino_t ino = kernfs_ino(pos);
+ int len;
+
+ len = strscpy(name, kernfs_rcu_name(pos), PATH_MAX);
ctx->pos = pos->hash;
file->private_data = pos;
kernfs_get(pos);
- if (!dir_emit(ctx, name, len, ino, type)) {
- up_read(&root->kernfs_rwsem);
+ /*
+ * getname() caps a path, so only an in-kernel caller can get
+ * here. Skip the entry rather than report a truncated name.
+ */
+ if (WARN_ON_ONCE(len < 0))
+ continue;
+
+ /*
+ * dir_emit() can fault, so run it unlocked. @pos is pinned
+ * above and kernfs_dir_pos() rechecks it on the way back.
+ */
+ up_read(&root->kernfs_rwsem);
+ if (!dir_emit(ctx, name, len, ino, type))
return 0;
- }
+ down_read(&root->kernfs_rwsem);
}
up_read(&root->kernfs_rwsem);
file->private_data = NULL;
--
2.53.0-Meta
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 3/3] selftests: cover readdir resuming at a removed entry
2026-09-10 0:36 [PATCH 0/3] kernfs: don't hold kernfs_rwsem across dir_emit() Shakeel Butt
2026-09-10 0:36 ` [PATCH 1/3] kernfs: don't repeat or skip an entry when readdir resumes Shakeel Butt
2026-09-10 0:36 ` [PATCH 2/3] kernfs: don't hold kernfs_rwsem across dir_emit() Shakeel Butt
@ 2026-09-10 0:36 ` Shakeel Butt
2 siblings, 0 replies; 4+ messages in thread
From: Shakeel Butt @ 2026-09-10 0:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, Tejun Heo, Christian Brauner
Cc: Sebastian Andrzej Siewior, Meta kernel team, linux-fsdevel,
driver-core, linux-kernel
The kernfs tests list only directories that do not change, so nothing
covers the entry a listing stopped on being gone when it comes back.
readdir_resume_at_removed_entry takes a telldir(3) cookie for every
entry, then removes each entry in turn, seeks to its cookie and reads
the rest; nothing reported before it may come back. This is a resume
between two getdents(2) calls, and it fails without "kernfs: don't
repeat or skip an entry when readdir resumes".
readdir_resume_vs_internal_remove is a resume inside one call, which
"kernfs: don't hold kernfs_rwsem across dir_emit()" opens. It churns
cgroup.subtree_control rather than calling rmdir(2), which cannot reach
that window because iterate_dir() holds the listed directory's i_rwsem
for the whole listing. It is a stress test, has not been seen to catch
the ordering bug, and keeps the window busy for lockdep and KASAN. It
fails if the churn died and skips if the listings never overlapped it,
so it cannot pass having listed a static directory.
Assisted-by: LLM
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
.../selftests/filesystems/kernfs_test.c | 200 ++++++++++++++++++
1 file changed, 200 insertions(+)
diff --git a/tools/testing/selftests/filesystems/kernfs_test.c b/tools/testing/selftests/filesystems/kernfs_test.c
index 6e74da91ebca..01cd58e5e41e 100644
--- a/tools/testing/selftests/filesystems/kernfs_test.c
+++ b/tools/testing/selftests/filesystems/kernfs_test.c
@@ -8,6 +8,7 @@
#include <limits.h>
#include <net/if.h>
#include <sched.h>
+#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@@ -17,6 +18,7 @@
#include <sys/mount.h>
#include <sys/socket.h>
#include <sys/stat.h>
+#include <sys/syscall.h>
#include <sys/xattr.h>
#include "kselftest_harness.h"
@@ -472,6 +474,204 @@ TEST_F(kernfs_cgroup, readdir_no_duplicates)
EXPECT_STRNE(names[i], names[j]);
}
+#define RESUME_DIRS 24
+
+/*
+ * Resuming at an entry that has gone must carry on after it, never before.
+ * Take a cookie for every entry, then remove each one, seek to its cookie
+ * and read the rest; nothing already reported may come back.
+ */
+TEST_F(kernfs_cgroup, readdir_resume_at_removed_entry)
+{
+ /* The cgroup's own control files are listed alongside ours. */
+ char names[128][NAME_MAX + 1];
+ long pos[128];
+ char path[PATH_MAX];
+ struct dirent *de;
+ int n = 0, i, j;
+ DIR *d;
+
+ for (i = 0; i < RESUME_DIRS; i++) {
+ snprintf(path, sizeof(path), "%s/e%02d", self->scratch, i);
+ ASSERT_EQ(mkdir(path, 0755), 0);
+ }
+
+ /* Record the cookie before reading each entry, with its name. */
+ d = opendir(self->scratch);
+ ASSERT_NE(d, NULL);
+ while (1) {
+ long here = telldir(d);
+
+ de = readdir(d);
+ if (!de)
+ break;
+ if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, ".."))
+ continue;
+ ASSERT_LT(n, (int)ARRAY_SIZE(pos));
+ pos[n] = here;
+ strncpy(names[n], de->d_name, NAME_MAX);
+ names[n][NAME_MAX] = '\0';
+ n++;
+ }
+ closedir(d);
+ ASSERT_GT(n, 1);
+
+ for (i = 0; i < n; i++) {
+ /* Only the directories we made can be removed and put back. */
+ if (strncmp(names[i], "e", 1))
+ continue;
+
+ snprintf(path, sizeof(path), "%s/%s", self->scratch, names[i]);
+ ASSERT_EQ(rmdir(path), 0);
+
+ /* Reopen so the seek has to reach the kernel. */
+ d = opendir(self->scratch);
+ ASSERT_NE(d, NULL);
+ seekdir(d, pos[i]);
+ while ((de = readdir(d))) {
+ if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, ".."))
+ continue;
+ for (j = 0; j < i; j++)
+ ASSERT_STRNE(de->d_name, names[j])
+ TH_LOG("resuming at %s (gone) went back to %s",
+ names[i], names[j]);
+ }
+ closedir(d);
+
+ ASSERT_EQ(mkdir(path, 0755), 0);
+ }
+
+ for (i = 0; i < RESUME_DIRS; i++) {
+ snprintf(path, sizeof(path), "%s/e%02d", self->scratch, i);
+ EXPECT_EQ(rmdir(path), 0);
+ }
+}
+
+#define CHURN_ROUNDS 400
+#define CHURN_BUFSZ 512 /* small, so a listing takes several calls */
+
+/*
+ * The files appear at the end of the enabling write and go at the start of
+ * the disabling one, so the window where they exist is the short one.
+ */
+#define CHURN_DWELL_ON 2000
+#define CHURN_DWELL_OFF 200
+
+struct kernfs_dirent64 {
+ unsigned long long d_ino;
+ long long d_off;
+ unsigned short d_reclen;
+ unsigned char d_type;
+ char d_name[];
+};
+
+/*
+ * The same resume, but inside one getdents(2) call. rmdir(2) cannot reach
+ * that window because iterate_dir() holds the listed directory's i_rwsem
+ * for the whole listing; cgroup.subtree_control can, having no VFS
+ * operation on the names it adds and removes. The files that are not the
+ * controller's stay throughout, so each must appear exactly once.
+ *
+ * A stress test: it has not been seen to catch the ordering bug, and is
+ * here to keep the unlocked window under load for lockdep and KASAN.
+ */
+TEST_F(kernfs_cgroup, readdir_resume_vs_internal_remove)
+{
+ char buf[CHURN_BUFSZ] __attribute__((aligned(8)));
+ char stable[128][NAME_MAX + 1];
+ int nstable = 0, i, r;
+ int withctl = 0, without = 0;
+ int seen[128], status;
+ pid_t churner;
+ DIR *d;
+
+ /* With the controller off, whatever is left is what must persist. */
+ ASSERT_EQ(write_file(self->scratch_sc, self->disable), 0);
+ d = opendir(self->child);
+ ASSERT_NE(d, NULL);
+ for (;;) {
+ struct dirent *de = readdir(d);
+
+ if (!de)
+ break;
+ if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, ".."))
+ continue;
+ ASSERT_LT(nstable, (int)ARRAY_SIZE(stable));
+ strncpy(stable[nstable], de->d_name, NAME_MAX);
+ stable[nstable][NAME_MAX] = '\0';
+ nstable++;
+ }
+ closedir(d);
+ ASSERT_GT(nstable, 0);
+
+ churner = fork();
+ ASSERT_GE(churner, 0);
+ if (churner == 0) {
+ for (;;) {
+ if (write_file(self->scratch_sc, self->enable))
+ _exit(10);
+ usleep(CHURN_DWELL_ON);
+ if (write_file(self->scratch_sc, self->disable))
+ _exit(11);
+ usleep(CHURN_DWELL_OFF);
+ }
+ }
+
+ for (r = 0; r < CHURN_ROUNDS; r++) {
+ int fd = open(self->child, O_RDONLY | O_DIRECTORY);
+ int extra = 0;
+ int n;
+
+ ASSERT_GE(fd, 0);
+ memset(seen, 0, sizeof(seen));
+
+ while ((n = syscall(SYS_getdents64, fd, buf, sizeof(buf))) > 0) {
+ int off = 0;
+
+ while (off < n) {
+ struct kernfs_dirent64 *de = (void *)(buf + off);
+ bool known = false;
+
+ off += de->d_reclen;
+ for (i = 0; i < nstable; i++)
+ if (!strcmp(de->d_name, stable[i])) {
+ seen[i]++;
+ known = true;
+ }
+ if (!known && strcmp(de->d_name, ".") &&
+ strcmp(de->d_name, ".."))
+ extra++;
+ }
+ }
+ ASSERT_GE(n, 0);
+ EXPECT_EQ(close(fd), 0);
+
+ if (extra)
+ withctl++;
+ else
+ without++;
+
+ for (i = 0; i < nstable; i++)
+ ASSERT_EQ(seen[i], 1)
+ TH_LOG("round %d: %s seen %d times",
+ r, stable[i], seen[i]);
+ }
+
+ /* The churn must have been running, or the listings prove nothing. */
+ EXPECT_EQ(kill(churner, SIGKILL), 0);
+ ASSERT_EQ(waitpid(churner, &status, 0), churner);
+ ASSERT_TRUE(WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL)
+ TH_LOG("churner exited on its own: status %d", status);
+
+ /*
+ * They also have to have overlapped it. How much depends on the
+ * machine, so say the race could not be arranged rather than fail.
+ */
+ if (!withctl || !without)
+ SKIP(return, "listings did not span the churn: %d with, %d without",
+ withctl, without);
+}
+
/*
* A telldir() cookie must resolve back to the same entry after seekdir().
* kernfs encodes the cookie as the node's name hash, so this covers
--
2.53.0-Meta
^ permalink raw reply related [flat|nested] 4+ messages in thread