Re: [PATCH] mm: page_alloc: fix cma pageblock was stolen in rmqueue fallback

[Vlastimil Babka]

On 9/11/23 20:11, Johannes Weiner wrote:
> On Mon, Sep 11, 2023 at 06:13:59PM +0200, Vlastimil Babka wrote:
>> On 9/11/23 17:57, Johannes Weiner wrote:
>> > On Tue, Sep 05, 2023 at 10:09:22AM +0100, Mel Gorman wrote:
>> >> mm: page_alloc: Free pages to correct buddy list after PCP lock contention
>> >> 
>> >> Commit 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")
>> >> returns pages to the buddy list on PCP lock contention. However, for
>> >> migratetypes that are not MIGRATE_PCPTYPES, the migratetype may have
>> >> been clobbered already for pages that are not being isolated. In
>> >> practice, this means that CMA pages may be returned to the wrong
>> >> buddy list. While this might be harmless in some cases as it is
>> >> MIGRATE_MOVABLE, the pageblock could be reassigned in rmqueue_fallback
>> >> and prevent a future CMA allocation. Lookup the PCP migratetype
>> >> against unconditionally if the PCP lock is contended.
>> >> 
>> >> [lecopzer.chen@xxxxxxxxxxxx: CMA-specific fix]
>> >> Fixes: 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")
>> >> Reported-by: Joe Liu <joe.liu@xxxxxxxxxxxx>
>> >> Signed-off-by: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
>> >> ---
>> >>  mm/page_alloc.c | 8 +++++++-
>> >>  1 file changed, 7 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> >> index 452459836b71..4053c377fee8 100644
>> >> --- a/mm/page_alloc.c
>> >> +++ b/mm/page_alloc.c
>> >> @@ -2428,7 +2428,13 @@ void free_unref_page(struct page *page, unsigned int order)
>> >>  		free_unref_page_commit(zone, pcp, page, migratetype, order);
>> >>  		pcp_spin_unlock(pcp);
>> >>  	} else {
>> >> -		free_one_page(zone, page, pfn, order, migratetype, FPI_NONE);
>> >> +		/*
>> >> +		 * The page migratetype may have been clobbered for types
>> >> +		 * (type >= MIGRATE_PCPTYPES && !is_migrate_isolate) so
>> >> +		 * must be rechecked.
>> >> +		 */
>> >> +		free_one_page(zone, page, pfn, order,
>> >> +			      get_pcppage_migratetype(page), FPI_NONE);
>> >>  	}
>> >>  	pcp_trylock_finish(UP_flags);
>> >>  }
>> >> 
>> > 
>> > I had sent a (similar) fix for this here:
>> > 
>> > https://lore.kernel.org/lkml/20230821183733.106619-4-hannes@xxxxxxxxxxx/
>> > 
>> > The context wasn't CMA, but HIGHATOMIC pages going to the movable
>> > freelist. But the class of bug is the same: the migratetype tweaking
>> > really only applies to the pcplist, not the buddy slowpath; I added a
>> > local pcpmigratetype to make it more clear, and hopefully prevent bugs
>> > of this nature down the line.
>> 
>> Seems to be the cleanest solution to me, indeed.
>> 
>> > I'm just preparing v2 of the above series. Do you want me to break
>> > this change out and send it separately?
>> 
>> Works for me, if you combine the it with the information about what commit
>> that fixes, the CMA implications reported, and Cc stable.
> 
> How about this? Based on v6.6-rc1.
> 
> ---
> 
> From 84e4490095ed3d1f2991e7f0e58e2968e56cc7c0 Mon Sep 17 00:00:00 2001
> From: Johannes Weiner <hannes@xxxxxxxxxxx>
> Date: Fri, 28 Jul 2023 14:29:41 -0400
> Subject: [PATCH] mm: page_alloc: fix CMA and HIGHATOMIC landing on the wrong
>  buddy list
> 
> Commit 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a
> spinlock") bypasses the pcplist on lock contention and returns the
> page directly to the buddy list of the page's migratetype.
> 
> For pages that don't have their own pcplist, such as CMA and
> HIGHATOMIC, the migratetype is temporarily updated such that the page
> can hitch a ride on the MOVABLE pcplist. Their true type is later
> reassessed when flushing in free_pcppages_bulk(). However, when lock
> contention is detected after the type was already overriden, the
> bypass will then put the page on the wrong buddy list.
> 
> Once on the MOVABLE buddy list, the page becomes eligible for
> fallbacks and even stealing. In the case of HIGHATOMIC, otherwise
> ineligible allocations can dip into the highatomic reserves. In the
> case of CMA, the page can be lost from the CMA region permanently.
> 
> Use a separate pcpmigratetype variable for the pcplist override. Use
> the original migratetype when going directly to the buddy. This fixes
> the bug and should make the intentions more obvious in the code.
> 
> Originally sent here to address the HIGHATOMIC case:
> https://lore.kernel.org/lkml/20230821183733.106619-4-hannes@xxxxxxxxxxx/
> 
> Changelog updated in response to the CMA-specific bug report.
> 
> [mgorman@xxxxxxxxxxxxxxxxxxx: updated changelog]
> Reported-by: Joe Liu <joe.liu@xxxxxxxxxxxx>
> Fixes: 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>

Reviewed-by: Vlastimil Babka <vbabka@xxxxxxx>

> ---
>  mm/page_alloc.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0c5be12f9336..95546f376302 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2400,7 +2400,7 @@ void free_unref_page(struct page *page, unsigned int order)
>  	struct per_cpu_pages *pcp;
>  	struct zone *zone;
>  	unsigned long pfn = page_to_pfn(page);
> -	int migratetype;
> +	int migratetype, pcpmigratetype;
>  
>  	if (!free_unref_page_prepare(page, pfn, order))
>  		return;
> @@ -2408,24 +2408,24 @@ void free_unref_page(struct page *page, unsigned int order)
>  	/*
>  	 * We only track unmovable, reclaimable and movable on pcp lists.
>  	 * Place ISOLATE pages on the isolated list because they are being
> -	 * offlined but treat HIGHATOMIC as movable pages so we can get those
> -	 * areas back if necessary. Otherwise, we may have to free
> +	 * offlined but treat HIGHATOMIC and CMA as movable pages so we can
> +	 * get those areas back if necessary. Otherwise, we may have to free
>  	 * excessively into the page allocator
>  	 */
> -	migratetype = get_pcppage_migratetype(page);
> +	migratetype = pcpmigratetype = get_pcppage_migratetype(page);
>  	if (unlikely(migratetype >= MIGRATE_PCPTYPES)) {
>  		if (unlikely(is_migrate_isolate(migratetype))) {
>  			free_one_page(page_zone(page), page, pfn, order, migratetype, FPI_NONE);
>  			return;
>  		}
> -		migratetype = MIGRATE_MOVABLE;
> +		pcpmigratetype = MIGRATE_MOVABLE;
>  	}
>  
>  	zone = page_zone(page);
>  	pcp_trylock_prepare(UP_flags);
>  	pcp = pcp_spin_trylock(zone->per_cpu_pageset);
>  	if (pcp) {
> -		free_unref_page_commit(zone, pcp, page, migratetype, order);
> +		free_unref_page_commit(zone, pcp, page, pcpmigratetype, order);
>  		pcp_spin_unlock(pcp);
>  	} else {
>  		free_one_page(zone, page, pfn, order, migratetype, FPI_NONE);