Re: [PATCH v5 8/9] mm/page_owner: clamp skip_buddy_pages() PFN advance at MAX_ORDER_NR_PAGES boundary

[Vlastimil Babka (SUSE)]

On 7/1/26 08:10, Ye Liu wrote:
> The lockless buddy_order_unsafe() read can return a garbage order
> value if the page is concurrently allocated between the PageBuddy
> check and the private read.  If this bogus order is <= MAX_PAGE_ORDER,
> skip_buddy_pages() would arbitrarily advance the PFN, potentially
> jumping past a MAX_ORDER_NR_PAGES boundary whose pfn_valid() check
> would have caught an offline memory section.
> 
> In read_page_owner(), which relies solely on boundary-aligned
> pfn_valid() to guard pfn_to_page(), skipping the boundary could
> cause pfn_to_page() to access an unmapped mem_section.
> 
> Clamp the advance so it never crosses the next MAX_ORDER_NR_PAGES
> boundary.  This is safe for all three callers: the pageblock-iterating
> ones already handle boundary transitions in their outer loops, and
> for read_page_owner() the worst case is one extra PageBuddy check per
> 1024 pages for a huge buddy block straddling the boundary.

I don't see how a huge buddy block can straddle the boundary, as the largest
buddy block order is MAX_ORDER?

> Signed-off-by: Ye Liu <ye.liu@xxxxxxxxx>

Other than that, LGTM

Reviewed-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>

> ---
>  mm/page_owner.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 46a933f9c229..2e3880053a34 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -428,6 +428,12 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old)
>   * to skip less than the full buddy block, but that is acceptable for page owner
>   * iteration purposes.
>   *
> + * The lockless read of buddy_order_unsafe() can also return a garbage order if
> + * the page is concurrently allocated and PageBuddy is cleared between the check
> + * and the read. Clamp the advance at the next MAX_ORDER_NR_PAGES boundary so
> + * that a bogus order cannot carry @pfn into an unvalidated memory section,
> + * which would break callers that rely on boundary-aligned pfn_valid() checks.
> + *
>   * Return: true if the page was skipped (caller should continue its loop),
>   *         false if the page is not a buddy page and should be processed normally.
>   */
> @@ -439,8 +445,12 @@ static inline bool skip_buddy_pages(unsigned long *pfn, struct page *page)
>  		return false;
>  
>  	order = buddy_order_unsafe(page);
> -	if (order <= MAX_PAGE_ORDER)
> -		*pfn += (1UL << order) - 1;
> +	if (order <= MAX_PAGE_ORDER) {
> +		unsigned long new_pfn = *pfn + (1UL << order);
> +		unsigned long boundary = ALIGN(*pfn + 1, MAX_ORDER_NR_PAGES);
> +
> +		*pfn = min(new_pfn, boundary) - 1;
> +	}
>  
>  	return true;
>  }