Re: [PATCH v3 2/7] mm/page_owner: protect page access with mem-hotplug read lock

[bot+bpf-ci]

> 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