Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] selftests/proc: getdents test faults in dir_emit_dots() before entry lookup --
commit fc0ab3f791b6ab04b43d13e015f97882092d54fc Author: Christian Brauner <[email protected]> selftests/proc: check that faulting sysctl accesses don't hold up removal This commit adds a proc selftest using userfaultfd and a tun device in a new network namespace to verify that page faults during read(), write(), and getdents64() on /proc/sys do not block sysctl table unregistration. > diff --git a/tools/testing/selftests/proc/proc-remove-slow-sysctl.c > b/tools/testing/selftests/proc/proc-remove-slow-sysctl.c > new file mode 100644 > index 0000000000000..e1b1fd19a07ab > --- /dev/null > +++ b/tools/testing/selftests/proc/proc-remove-slow-sysctl.c > @@ -0,0 +1,213 @@ [ ... ] > +TEST_F(proc_sys_slow, getdents) > +{ > + struct worker w = { > + .op = SYSCTL_GETDENTS, > + .len = BUF_PAGES * self->psz, > + }; > + > + w.fd = open(CONF_PATH, O_RDONLY | O_DIRECTORY | O_CLOEXEC); > + ASSERT_GE(w.fd, 0); > + stall_and_remove(_metadata, self, &w); > + > + /* What was looked up before the removal still arrives, then nothing. */ > + EXPECT_GT(w.ret, 0); [Severity: Medium] Does TEST_F(proc_sys_slow, getdents) in tools/testing/selftests/proc/proc-remove-slow-sysctl.c actually fault on a looked-up sysctl entry, or does it fault inside dir_emit_dots() before any entry is looked up? In FIXTURE_SETUP(proc_sys_slow), only the first page of self->buf is registered with userfaultfd. When TEST_F(proc_sys_slow, getdents) opens CONF_PATH, w.fd starts at file position 0 and is passed directly to stall_and_remove() -> work_thread() -> getdents64(). In fs/proc/proc_sysctl.c:proc_sys_readdir(), dir_emit_dots() runs before sysctl_dir_next(): fs/proc/proc_sysctl.c:proc_sys_readdir() { ... if (!dir_emit_dots(file, ctx)) return 0; if (cursor && cursor->pos == ctx->pos) { err = sysctl_dir_next(ctl_dir, &d, cursor->name, cursor->len); } else { loff_t pos = 2; err = sysctl_dir_next(ctl_dir, &d, NULL, 0); ... } Because ctx->pos is 0, dir_emit_dots() writes "." at the start of self->buf and triggers the single userfaultfd page fault before sysctl_dir_next() is called and while d.head is still NULL. While work_thread() is sleeping in dir_emit_dots(), stall_and_remove() closes self->tun (which unregisters CONF_PATH and marks ctl_dir->header as unregistering) and resolves the fault on the first page with UFFDIO_ZEROPAGE. When work_thread() resumes, dir_emit_dots() finishes emitting "." and "..", sysctl_dir_next() immediately returns -ENOENT, and getdents64() returns the 48 bytes from the dot entries without ever entering the entry-emission loop. Should TEST_F(proc_sys_slow, getdents) advance w.fd past "." and ".." first (for example with lseek(w.fd, 2, SEEK_SET)) before calling stall_and_remove() so the fault occurs in dir_emit() after sysctl_dir_next() and proc_sys_fill_dirent() have looked up an entry? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=36
