Re: [PATCH 2/6] mm/page_owner: hold off memory offline while a page is read

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm/page_owner: read_page_owner() mixes guard(pfn_walk) with goto cleanup
--

commit b2d3acd01e2d83565fff990fcda39fb4ee08ffa7
Author: Gregory Price (Meta) <gourry@gourry.net>

mm/page_owner: hold off memory offline while a page is read

This commit prevents page faults during concurrent memory offlining or removal
when reading /sys/kernel/debug/page_owner by checking PFNs with
pfn_to_online_page() inside a pfn_walk read section and moving kmalloc() and
copy_to_user() outside that section.

> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index cfc31c92d7657..d2c0b831fcd3b 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c

[ ... ]

> @@ -766,7 +760,12 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>  			continue;
>  		}
>  
> -		page = pfn_to_page(pfn);
> +		/* Offline waits for us before it poisons the memmap */
> +		guard(pfn_walk)();
> +		page = pfn_to_online_page(pfn);
> +		if (!page)
> +			continue;
> +
>  		if (skip_buddy_pages(&pfn, page))
>  			continue;
>  
> @@ -825,14 +824,18 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>  
>  		page_owner_tmp = *page_owner;
>  		page_ext_put(page_ext);
> -		return print_page_owner(buf, count, pfn, page,
> +		ret = print_page_owner(kbuf, count, pfn, page,
>  				&page_owner_tmp, handle, state);
> +		break;
>  ext_put_continue:
>  		page_ext_put(page_ext);
>  		cond_resched();
>  	}

[Severity: Medium]
In read_page_owner(), guard(pfn_walk)() is declared inside the for-loop body,
so its scope extends to the closing brace of the loop after cond_resched().

When a PFN is skipped via goto ext_put_continue (for example on an unallocated
page, tail page, missing handle, or filtered node ID):

mm/page_owner.c:read_page_owner() {
    for (; pfn < max_pfn; pfn++) {
        ...
        guard(pfn_walk)();
        ...
        if (!test_bit(PAGE_EXT_OWNER, &page_ext->flags))
            goto ext_put_continue;
        ...
ext_put_continue:
        page_ext_put(page_ext);
        cond_resched();
    }
}

First, include/linux/cleanup.h notes that goto-based cleanup and scope-based
cleanup helpers should not be mixed in the same function.

Second, because the guard(pfn_walk) scope only ends at the closing brace of
the loop, cond_resched() is called while the pfn_walk SRCU read lock is still
held, even though all accesses to struct page and page_ext for that PFN have
already completed. If the task is scheduled out in cond_resched(), concurrent
memory offlining in offline_pages() -> remove_pfn_range_from_zone() ->
pfn_walk_synchronize() has to wait for the reader to be scheduled back in
while holding the memory hotplug locks.

Should the pfn_walk guard and page_ext cleanup be scoped so that the pfn_walk
read section ends before cond_resched(), and without mixing guard() with
goto-based cleanup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027042204.3741307-1-gourry@gourry.net?part=2