Re: [PATCH v3 1/7] mm/page_owner: extract page_owner_next_eligible() from read loop

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 4369 bytes --]

> mm/page_owner: extract page_owner_next_eligible() from read loop

TL;DR: The nid filter's page->flags read now happens outside the page_ext RCU
window, so it may fault on an unmapped vmemmap during memory hot-remove; later
patches fix it, so it is only a bisect window.

> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index fbbda7ba914b..92a92cdf74ec 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c

[ ... ]

> @@ -798,13 +794,45 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>  			goto ext_put_continue;
>
> -		/*
> -		 * Access to page_ext->handle isn't synchronous so we should
> -		 * be careful to access it.
> +		/*
> +		 * Take a snapshot of the page_owner record inside the RCU
> +		 * window, then validate it, so that print_page_owner()
> +		 * reads the checked handle from the same snapshot.
>  		 */
> -		handle = READ_ONCE(page_owner->handle);
> -		if (!handle)
> +		scan->pfn = pfn;
> +		scan->po_snap = *page_owner;
> +
> +		if (!scan->po_snap.handle)
>  			goto ext_put_continue;
>
> +		page_ext_put(page_ext);
> +		return true;
> +
> +ext_put_continue:
> +		page_ext_put(page_ext);
> +		cond_resched();
> +	}
> +
> +	return false;
> +}

[ ... ]

> +	/* Find an allocated page */
> +	while (page_owner_next_eligible(&scan)) {
> +		struct page *page = pfn_to_page(scan.pfn);
> +
>  		if (state->nid_filter_enabled) {
>  			int nid;
>  			memdesc_flags_t page_flags = READ_ONCE(page->flags);

Can this READ_ONCE(page->flags) fault on an unmapped vmemmap?

In the parent commit the nid check ran between page_ext_get() and
page_ext_put(), so it was inside rcu_read_lock():

    page_ext = page_ext_get(page);   /* rcu_read_lock() */
    ...
    if (state->nid_filter_enabled) {
        memdesc_flags_t page_flags = READ_ONCE(page->flags);
        ...
            goto ext_put_continue;
    }
    ...
    ext_put_continue:
        page_ext_put(page_ext);      /* rcu_read_unlock() */

Now page_owner_next_eligible() calls page_ext_put() before it returns true,
and read_page_owner() reads page->flags with no RCU section, no
get_online_mems(), and no page reference held.

The RCU section mattered for memory hot-remove:

    offline_pages()
      memory_notify(MEM_OFFLINE)
        page_ext_callback()
          offline_page_ext()
            synchronize_rcu()

try_remove_memory() requires the blocks to be offline before
arch_remove_memory() -> ... -> section_deactivate() ->
depopulate_section_memmap() unmaps the vmemmap.  While a page_ext_get()
reader was active, that section's memmap could not be torn down.

After this patch, an offline plus remove can complete between page_ext_put()
and the READ_ONCE(page->flags), because synchronize_rcu() no longer waits
for this reader.  This needs CONFIG_PAGE_OWNER with page_owner=on,
CONFIG_MEMORY_HOTREMOVE, a nid filter set through page_owner_write(), and a
concurrent hot-remove of movable memory.

The commit message says "No change to the output is intended", but it does
not mention that the nid filter's struct page access moves out of the page_ext
RCU window.

For context, the parent already read struct page without a pin in
skip_buddy_pages() (PageBuddy() before page_ext_get()) and in
print_page_owner() (page->flags, pageblock flags and page->memcg_data after
page_ext_put()), so this widens an existing race rather than adding a new
class of problem.  It does turn one protected access into an unprotected
one, though.

This is fixed later in the series, so it is only a bisect window issue:

- 915f86c7862f ("mm/page_owner: protect page access with mem-hotplug read
  lock") re-resolves the page in read_page_owner() with pfn_to_online_page()
  under get_online_mems() before the nid check.

- 2cf991a93cbd ("mm/page_owner: record the NUMA node in the scan cursor")
  moves the nid computation back into page_owner_next_eligible(), "to avoid
  dereferencing a possibly-unmapped page".

Could the nid be read in page_owner_next_eligible() before page_ext_put(),
for example by storing it in the cursor?  Alternatively, could the
mem-hotplug protection patch be reordered ahead of this one?

[ ... ]


---
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