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

[Josef Bacik]

Hey Christian,

This is a summary of my Claude review of v2.  v2 went out before my mail
on the RFC [1] did, so the first part is what's left of that one with
the new patch numbers and the second part is what's new in v2.  What
says "run" was run in a VM on top of v2, the rest is from reading the
code.

3 (selftests Makefile), 4 (pde_seq_next()), 5 (sysctl readdir ending
early) and the allocinfo deadlock from that mail are gone in v2.  These
still apply:

- 1 is bugfixes 32/51 ("proc: resume readdir after the last name it
  returned") now and the patch is unchanged.  Run again, readdir(2)
  still returns 1 without a dirent at the end of /proc/fs, bus, driver,
  irq, tty, net and self/net, for all of the 100000 calls.

- 2 is cleanup 5/6 ("proc: drop the _buf suffix from the new methods")
  now.  Run again, the module with .proc_read = seq_read builds without
  a warning with W=1, loads without a warning, and every read() fails
  with -EFAULT.  There's still nothing in porting.rst.

- 6 is bugfixes 47/51, 48/51 and 50/51 now.  47/51 has no Fixes: or
  stable tag, 48/51 is tagged for stable and git apply takes it on top
  of 46/51 with two hunks offset by 6 lines.  50/51 has the same Fixes:
  7e0e953bb0cf.

- All of the smaller stuff.  The cursor is allocated in one place since
  core 03/23 and it's still GFP_KERNEL, copy_to_user_fromio() loses its
  last caller in drivers 24/26, the sentence about ntfs_set_label() is
  in fs 07/30, and snd_info_text_entry_write() still drops what was
  written before it grows the buffer.

New in v2:

1) bugfixes 01/51 ("alloc_tag: don't remove /proc/allocinfo under the
   codetag module lock")

shutdown_mem_profiling_async() is defined outside of #ifdef
CONFIG_MODULES and its three callers are inside of it.  Run with
CONFIG_MODULES=n, CONFIG_MEM_ALLOC_PROFILING=y and CONFIG_WERROR=y,
v7.4-rc1 builds mm/alloc_tag.o and with 01/51 it's

	mm/alloc_tag.c:620:13: error: 'shutdown_mem_profiling_async' defined but not used [-Werror=unused-function]

This one is tagged for stable.

2) drivers 07/26 ("md: let procfs own the wait queue of /proc/mdstat")

md_exit() used to remove /proc/mdstat first, which waited for the
readers.  It now does that after destroy_workqueue(md_misc_wq), so that
nothing can reach proc_poll_notify() on a removed entry.  But a reader
can be in here while md_exit() runs:

	mddev = list_entry(v, struct mddev, all_mddevs);
	if (!mddev_get(mddev))
		return 0;

	spin_unlock(&all_mddevs_lock);
	...
	spin_lock(&all_mddevs_lock);
	...
	mddev_put_locked(mddev);

md_exit() clears the array, but its mddev_put_locked() isn't the last
one anymore, so nothing gets queued and destroy_workqueue() has nothing
to wait for.  The reader's put is the last one and ends up in

	queue_work(md_misc_wq, &mddev->del_work);

on the destroyed workqueue.  It takes an rmmod with an inactive array
left over and somebody reading /proc/mdstat, and this wasn't run.

The two orders each fix one side.  If proc_poll_notify() was safe to
call on an entry that's been removed the removal could stay where it
was.

3) core 21/23 ("proc: let lockdep check the pin")

The class is keyed on the proc_ops, and every ALSA text entry uses
snd_info_text_entry_ops, so they're all one class.  Run with
PROVE_LOCKING and snd-dummy built in, cat /proc/asound/pcm and then
unbind snd_dummy.0:

	WARNING: possible circular locking dependency detected
	init/1 is trying to acquire lock:
	 (pde->in_use){++++}-{0:0}, at: __remove_proc_subtree
	but task is already holding lock:
	 (register_mutex#3){+.+.}-{4:4}, at: snd_pcm_dev_disconnect

	-> #1 (register_mutex#3){+.+.}-{4:4}:
	       snd_pcm_proc_read
	       snd_info_seq_show
	       seq_read_iter

	-> #0 (pde->in_use){++++}-{0:0}:
	       proc_entry_rundown
	       __remove_proc_subtree
	       snd_info_disconnect_entry
	       snd_pcm_dev_disconnect

snd_pcm_proc_read() is the only proc callback that takes register_mutex
and it only serves /proc/asound/pcm.  What snd_pcm_dev_disconnect()
removes under the mutex is pcmNp and pcmNc, and nothing below those
takes it.  So it's a false positive, and it turns lockdep off for
whoever unplugs a sound card.  The removal under register_mutex is
already there at your base, only the annotation is new.

proc_create_single_data() gets a key per registration site, but there's
no way for a proc_create_data() user that multiplexes callbacks behind
one proc_ops to say so.  sysfs has the per attribute key and
ignore_lockdep for this.

4) core 20/23 ("selftests/proc: check that a mapped entry can be
   removed")

The core cover letter says this one "fails without the mmap patch and
passes with it".  Run against a virtio-rng device, it passes on core
18/23, which has no proc_vm_ops in System.map, and it passes on the
whole series.  The test checks that the removal doesn't wait, that the
entry is gone and that pread() fails with -EIO, and it doesn't touch the
mapping after the removal, so none of the new wrappers run on a removed
entry.  These kernels had neither KASAN nor lockdep, so if what fails
for you is a splat then that's the difference, but the test itself
doesn't notice.

5) core 18/23 ("proc: let procfs copy ioctl arguments in and out")

No Signed-off-by in the mail.  It's the only one of the 172, and the RFC
version had it.

Smaller stuff:

- The 0/7 mail still says "the xt_recent deadlock fixed in (1) below".
  The part 1 cover letter has it right, it goes through netfilter now.

- cleanup 03/06 says mmap() and write() are all that changes for kcore,
  kmsg and the kpage files.  proc_reg_poll() answered for every regular
  file, and kcore_fops and the three kpage ones have no ->poll(), so
  EPOLL_CTL_ADD on them goes from working to -EPERM.  I doubt anybody
  cares, it's only the changelog.

One more old one for part 1, and this is the lockdep patch doing its
job.  devm_rtc_unregister_device() holds rtc->ops_lock over
rtc_proc_del_device(), and rtc_proc_show() -> rtc_read_time() takes
ops_lock while it's pinned.  Run with PROVE_LOCKING and rtc-test as
rtc0, cat /proc/driver/rtc and then unbind rtc-test.0:

	WARNING: possible circular locking dependency detected
	init/1 is trying to acquire lock:
	 (pde->in_use){++++}-{0:0}, at: remove_proc_entry
	but task is already holding lock:
	 (&rtc->ops_lock){+.+.}-{4:4}, at: devm_rtc_unregister_device

	-> #1 (&rtc->ops_lock){+.+.}-{4:4}:
	       rtc_read_time
	       rtc_proc_show
	       seq_read_iter

That one is the same entry on both sides, so it's real.
rtc_read_time() uses mutex_lock_interruptible(), so a signal to the
reader gets both of them out of it.

The end state still checks out.  Every proc_ops initializer matches the
final struct, nothing copies to or from userspace while pinned, and it
didn't find anything wrong with the new seq_file, open node, poll or
vm_ops code itself.

[1] https://lore.kernel.org/r/464ef986eab97abf226791c7e19ad111.josef@xxxxxxxxxxxxxx

Thanks,
Josef