Re: [PATCH RFC 0/7] proc: move procfs to what kernfs does

[Josef Bacik <josef@toxicpanda.com>]

Hey Christian,

This is a summary of my Claude review of everything.  It didn't find
anything wrong with the conversions in parts 3 to 6, the findings are in
part 1 and on the procfs side.  1-3 are reproduced in a VM, 4-6 are from
reading the code.

1) bugfixes 19/38 ("proc: resume readdir after the last name it
   returned")

proc_readdir_de() now returns 1 instead of 0 when there's nothing left
to emit:

	if (!de) {
		read_unlock(&proc_subdir_lock);
		return 1;
	}

proc_root_readdir() needs that to go on to the pids, but proc_readdir()
and proc_tgid_net_readdir() are also the ->iterate_shared() of every
other directory, and iterate_dir() hands the value to its caller.
getdents doesn't look at it, old_readdir() and the compat one do:

	error = iterate_dir(fd_file(f), &buf.ctx);
	if (buf.result)
		error = buf.result;

	return error;

So at the end of the directory readdir(2) returns 1 without having
written a dirent, and keeps doing that.  This is syscall 89 through
int $0x80 in a loop on x86_64 with IA32_EMULATION, giving up after
100000 calls:

	             base, 18/38         19/38, whole series
	/proc        106 entries, 0      106 entries, 0
	/proc/fs       4 entries, 0        4 entries, 1 from then on
	/proc/net     31 entries, 0       31 entries, 1 from then on
	/proc/sys     10 entries, 0       10 entries, 0

/proc/bus, driver, irq, tty and self/net do the same as /proc/fs.
glibc doesn't use that syscall, but this patch is tagged for stable.

proc_readdir() has to keep returning the 1 for proc_root_readdir(), so
I think proc_dir_operations and proc_net_operations want a wrapper that
turns it into 0.  The comment above proc_readdir_de() that says the VFS
doesn't care about the return value should go as well.

2) cleanup 5/5 ("proc: drop the _buf suffix from the new methods")

4/5 renames the old ->proc_read() so that "A module that sets one of
them doesn't build anymore".  5/5 hands the name to the new method, and
the old and the new prototype only differ in __user:

	ssize_t (*proc_read)(struct file *, char __user *, size_t, loff_t *);
	ssize_t (*proc_read)(struct file *, char *, size_t, loff_t *);

Only sparse sees that.  The compiler does reject an old ->proc_write()
or ->proc_ioctl() (gcc 13, clang 18), but this

	.proc_open	= foo_open,	/* single_open() */
	.proc_read	= seq_read,
	.proc_lseek	= seq_lseek,
	.proc_release	= single_release,

builds without a warning with W=1.  pde_set_flags() only refuses the
release helpers for seq_file entries, which this isn't one of, so it
registers too.  With a module that has exactly that loaded on top of the
series there's no warning, open() works and every read() fails with
-EFAULT.

The tree had 84 of those at your base.  None are left, but whatever is
in flight in other trees, gets backported or lives out of tree ends up
there without anybody noticing.  Either the read method keeps a name of
its own or pde_set_flags() refuses seq_read, and either way this
probably wants an entry in porting.rst.

3) bugfixes 38/38 ("selftests/proc: check that a faulting readlink()
   doesn't hold up removal")

This adds proc-remove-slow-reader to the Makefile and to .gitignore
along with proc-remove-slow-readlink, but proc-remove-slow-reader.c
only shows up in core 08/16.  From here up to core 07/16 make says

	make: *** No rule to make target '[...]/proc-remove-slow-reader'.  Stop.

in tools/testing/selftests/proc, so also with part 1 applied on its
own.  The two lines want to move to core 08/16.

4) core 04/16 ("proc: let procfs own the seq_file of seq_file entries")

pde_seq_start() returns right after it unpinned, pde_seq_next() goes on:

	v = pde->seq_ops->next(m, v, pos);
	if (v == ERR_PTR(-EIO))
		pde_seq_stop_pinned(m, v);
	/* seq_file would blame these seq_operations, blame the entry's */
	if (*pos == old) {
		pr_info_ratelimited("buggy .next function %ps did not update position index\n",
				    pde->seq_ops->next);
		++*pos;
	}
	return v;

core 16/16 documents that next() may return ERR_PTR(-EIO).  A next()
that does and leaves *pos alone is called buggy for it, and
pde->seq_ops is read after the unpin, when the module it points into
can be gone.  Nothing in the tree returns -EIO from next() today.  A
return v after pde_seq_stop_pinned() takes care of both.

5) bugfixes 35/38 ("proc: sysctl: don't hold the tables while readdir
   copies to userspace")

	while (err > 0) {
		if (!d.skip) {
			err = proc_sys_fill_dirent(file, &d);
			if (err)
				break;
	...
	sysctl_dir_done(&d);
	return 0;

sysctl_dir_next() only pins the memory, so the entry can be unregistered
before proc_sys_make_inode() looks at it and returns -ENOENT.  The loop
breaks without moving ctx->pos and returns 0.  If that was the first
entry of the getdents64() call userspace sees the end of the directory
and loses everything behind it.

The old code bailed out the same way, so it's not a regression, but
it's the "terminated early" case you set out to fix and these are the
lines that get rewritten.  Handling -ENOENT like d.skip would carry on
after that name.  -ENOMEM probably wants to be returned instead of
turning into EOF.

6) stable tags, bugfixes 34/38 and 37/38

34/38 has no Fixes: or stable tag and says "No functional changes", but
35/38 is tagged for stable and depends on it.  Without 34/38
proc_sys_make_inode() reads table->mode and calls ->set_ownership()
after it dropped sysctl_lock, and the caller 35/38 adds doesn't keep the
table registered.  35/38 applies cleanly without 34/38, so nothing will
complain if only the tagged one gets picked up.

37/38 has Fixes: 7e0e953bb0cf, which is 4.0.  "the inode holds a
reference to the entry" only covers RCU walk since 47458802f660
("procfs: move dropping pde and pid from ->evict_inode() to
->free_inode()"), which went into 6.8.  Before that the inode drops the
entry in ->evict_inode() without a grace period, and the pin is what
keeps ->data around for a walker that got it from ->get_link().  Which
stable trees have that commit isn't something I checked.

Smaller stuff:

- The cursors in bugfixes 19/38 and 31/38 hang off an open file and
  anybody can have as many as they have fds, so GFP_KERNEL_ACCOUNT.

- copy_to_user_fromio() has no callers left after drivers 22/23.

- fs 07/27 says ntfs_set_label() refused a label that's too long with
  -ENAMETOOLONG or -EFBIG.  It doesn't always, cpu_str.len is a u8 and
  ntfs_nls_to_utf16() is allowed (PATH_MAX - 2) / 2 here, so 300
  characters come out as 44 and pass the check.  That's an old ntfs3
  bug, it's only the sentence in the changelog.

Two more old ones that you might want in part 1, the series doesn't
change either of them:

- allocinfo can deadlock against a module load.
  codetag_alloc_module_section() calls reserve_module_tags() with
  mod_lock held for write, and its three error paths call
  shutdown_mem_profiling(true) -> remove_proc_entry("allocinfo"), which
  waits for the readers.  A reader that's pinned and in
  allocinfo_start() -> codetag_lock_module_list() sits in
  down_read(mod_lock).  There's no copy to userspace involved, so
  moving the copies out doesn't help this one.

- snd_info_text_entry_write() allocates the larger buffer and frees the
  old one without copying what's in it, so a text entry written in
  pieces loses the start once it crosses a page.  It's been like that
  since ffb73b08e794.

For what it's worth the end state checks out.  Every proc_ops
initializer in the tree matches the final struct, the only entries that
still read straight into userspace are kcore, kmsg, kpagecount,
kpageflags, kpagecgroup and profile, all built in and never removed, and
nothing in the tree trips the new checks in pde_set_flags().

This part is me. I have a selftest that my claude wrote to validate #1.
If you want I can send it along, but we should probably have a test for
that.

Thanks,
Josef