> mm/page_owner: protect page access with mem-hotplug read lock
TL;DR: read_page_owner() holds the mem-hotplug read lock across
copy_to_user(); a page fault there may deadlock against mmap_lock (lock-order
inversion with memremap_pages callers) and userspace can stall hot-remove
indefinitely.
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
[ ... ]
> @@ -831,7 +843,17 @@ read_page_owner(struct file *file, char __user *buf,
> size_t count, loff_t *ppos)
>
> /* Find an allocated page */
> while (page_owner_next_eligible(&scan)) {
> - struct page *page = pfn_to_page(scan.pfn);
> + struct page *page;
> + ssize_t ret;
> +
> + /*
> + * The scan is lockless. Re-resolve the page under a mem-hotplug
> + * read-side lock so that we never access a hot-removed section.
> + */
> + get_online_mems();
> + page = pfn_to_online_page(scan.pfn);
> + if (!page)
> + goto skip_continue;
[ ... ]
> @@ -851,10 +873,13 @@ read_page_owner(struct file *file, char __user *buf,
> size_t count, loff_t *ppos)
> /* Record the next PFN to read in the file offset */
> *ppos = scan.pfn + 1;
>
> - return print_page_owner(buf, count, scan.pfn, page,
> + ret = print_page_owner(buf, count, scan.pfn, page,
> &scan.po_snap, scan.po_snap.handle, state);
> + put_online_mems();
> + return ret;
^^^^
Can this deadlock against mmap_lock? read_page_owner() now holds the
mem_hotplug_lock reader across the whole print_page_owner() call, and that
includes the copy_to_user() of the formatted record:
print_page_owner()
...
if (copy_to_user(buf, kbuf, ret))
ret = -EFAULT;
Once kbuf is formatted, nothing in that copy touches struct page, so the
reader isn't needed there. A user page fault on buf taken inside this
region takes current->mm->mmap_lock for read (on x86 a fault from
kernel-mode uaccess always does, see the 'goto lock_mmap' path in
arch/x86/mm/fault.c for !FAULT_FLAG_USER), and __might_fault() records that
dependency for lockdep. That creates the lock order:
mem_hotplug_lock(R) -> mmap_lock(R)
The opposite order already exists in the tree. Code that hotplugs device
memory while holding mmap_read_lock goes through memremap_pages() ->
pagemap_range() -> mem_hotplug_begin(), which does cpus_read_lock() and then
percpu_down_write(&mem_hotplug_lock):
nouveau_svmm_bind()
mmap_read_lock(mm)
nouveau_dmem_migrate_vma()
nouveau_dmem_migrate_chunk()
nouveau_dmem_migrate_copy_one()
nouveau_dmem_page_alloc_locked()
nouveau_dmem_chunk_alloc()
memremap_pages()
lib/test_hmm.c:dmirror_migrate_to_device()
mmap_read_lock(mm)
dmirror_migrate_alloc_and_copy()
dmirror_devmem_alloc_page()
dmirror_allocate_chunk()
memremap_pages()
With three threads sharing one mm, could this sequence occur?
T1 read(page_owner):
get_online_mems(), then copy_to_user() faults on buf
T2 SVM bind ioctl:
holds mmap_read_lock(mm), blocks in percpu_down_write(&mem_hotplug_lock)
waiting for T1
T3 mmap()/munmap():
queues on mmap_write_lock(mm) behind T2
T1 fault:
mmap_read_lock(mm) blocks behind T3
None of these waits has a timeout. While T2 waits it also holds
cpus_read_lock(), so CPU hotplug writers and every later get_online_mems()
caller stall too.
Even without that cycle, the reader can now be held for as long as
userspace wants. A buf backed by userfaultfd or by a slow or remote file
mapping could stall memory hot-remove for an unbounded time. Does that
match the commit message claim:
"Taking the lock per-page bounds the writer's wait to a single page's
check-and-deref, keeping hot-remove stalls brief while all accesses stay
protected."
For comparison, ptdump_walk_pgd() already takes get_online_mems() followed
by mmap_write_lock(current->mm). This new instance sits on a much more
commonly used debug path, and here the lock isn't needed around
copy_to_user() at all.
Would it work to format the record under the reader (or snapshot the
page-derived fields: pageblock migratetype, page->flags, memcg info), call
put_online_mems(), and only then call copy_to_user()? The kmalloc() in
print_page_owner() could also move outside the locked region.
The last commit in the series (5b123f2cd336 "selftests/bpf: add tests for
the page_owner bpf_iter target") still calls print_page_owner() under the
reader, so the series does not address this later.
>
> skip_continue:
> + put_online_mems();
> scan.pfn++;
> cond_resched();
> }
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39593164040
