mm: page_alloc: fix cma pageblock was stolen in rmqueue fallback

PATCH In review Lecopzer Chen 4 participants 9 messages Aug 30, 2023 – Sep 11, 2023

Subsystems (2)

Memory management Page allocator

lore

Share & tools

lore

Lecopzer Chen

author

2023-08-30 11:14 UTC #0 lore raw

PATCH 1/1 mm: page_alloc: fix cma pageblock was stolen in rmqueue fallback

commit 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock") fallback freeing page to free_one_page() if pcp trylock failed. This make MIGRATE_CMA be able to fallback and be stolen whole pageblock by MIGRATE_UNMOVABLE in the page allocation.

PCP free is fine because free_pcppages_bulk() will always get migratetype again before freeing the page, thus this only happen when someone tried to put CMA page in to other MIGRATE_TYPE's freelist.

Fixes 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")

Reported-by Joe Liu <[email protected]>

Signed-off-by Lecopzer Chen <[email protected]>

Cc Mark-pk Tsai <[email protected]>

Cc Joe Liu <[email protected]>

---
 mm/page_alloc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

mm/page_alloc.c +8 -0

index 452459836b71..0ea88c031838 100644
@@ -2428,6 +2428,14 @@ void free_unref_page(struct page *page, unsigned int order)
 		free_unref_page_commit(zone, pcp, page, migratetype, order);
 		pcp_spin_unlock(pcp);
 	} else {
+#ifdef CONFIG_CMA
+		/*
+		 * CMA must be back to its freelist. Otherwise CMA pageblock may
+		 * be stolen by fallback flow while getting free page.
+		 */
+		if (get_pcppage_migratetype(page) == MIGRATE_CMA)
+			migratetype = MIGRATE_CMA;
+#endif
 		free_one_page(zone, page, pfn, order, migratetype, FPI_NONE);
 	}
 	pcp_trylock_finish(UP_flags);

Signature · 1 line

2.18.0

Mel Gorman

2023-09-05 09:09 UTC #1 lore raw

Quoted 9 lines · Lecopzer Chen

commit 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock") fallback freeing page to free_one_page() if pcp trylock failed. This make MIGRATE_CMA be able to fallback and be stolen whole pageblock by MIGRATE_UNMOVABLE in the page allocation.
PCP free is fine because free_pcppages_bulk() will always get migratetype again before freeing the page, thus this only happen when someone tried to put CMA page in to other MIGRATE_TYPE's freelist.

Quoted 5 lines

Fixes 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")
Reported-by Joe Liu <[email protected]>
Signed-off-by Lecopzer Chen <[email protected]>
Cc Mark-pk Tsai <[email protected]>
Cc Joe Liu <[email protected]>

Sorry for the long delay and thanks Lecopzer for the patch.

This changelog is difficult to parse but the fix may also me too specific and could be more robust against types other than CMA. It is true that a failed PCP acquire may return a !is_migrate_isolate page to the wrong list but it's more straight-forward to unconditionally lookup the PCP migratetype of the spinlock is not acquired.

How about this? It unconditionally looks up the PCP migratetype after spinlock contention. It's build tested only

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

[[email protected]: CMA-specific fix]

Fixes 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")

Reported-by Joe Liu <[email protected]>

Signed-off-by Mel Gorman <[email protected]>

---
 mm/page_alloc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

mm/page_alloc.c +7 -1

index 452459836b71..4053c377fee8 100644
@@ -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);
 }

Lecopzer Chen

author

2023-09-05 09:23 UTC #2 lore raw

Hi Mel,

Of course, because we only test for CMA and only make sure that CMA part is wrong.
You patch is definitly better.
Please go ahead with your patch, thank you.

Quoted 67 lines · Lecopzer Chen

Quoted 9 lines
commit 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock") fallback freeing page to free_one_page() if pcp trylock failed. This make MIGRATE_CMA be able to fallback and be stolen whole pageblock by MIGRATE_UNMOVABLE in the page allocation.
PCP free is fine because free_pcppages_bulk() will always get migratetype again before freeing the page, thus this only happen when someone tried to put CMA page in to other MIGRATE_TYPE's freelist.
Quoted 5 lines
Fixes 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")
Reported-by Joe Liu <[email protected]>
Signed-off-by Lecopzer Chen <[email protected]>
Cc Mark-pk Tsai <[email protected]>
Cc Joe Liu <[email protected]>
Sorry for the long delay and thanks Lecopzer for the patch.
This changelog is difficult to parse but the fix may also me too specific and could be more robust against types other than CMA. It is true that a failed PCP acquire may return a !is_migrate_isolate page to the wrong list but it's more straight-forward to unconditionally lookup the PCP migratetype of the spinlock is not acquired.
How about this? It unconditionally looks up the PCP migratetype after spinlock contention. It's build tested only
--8<--
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.
[[email protected]: CMA-specific fix]
Fixes 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")
Reported-by Joe Liu <[email protected]>
Signed-off-by Mel Gorman <[email protected]>
---
 mm/page_alloc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
mm/page_alloc.c +7 -1
index 452459836b71..4053c377fee8 100644
@@ -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);
 }

Vlastimil Babka

Acked-by

2023-09-05 09:37 UTC #3 lore raw

In reply to Mel Gorman

Quoted 40 lines · Mel Gorman

Quoted 9 lines · Lecopzer Chen
commit 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock") fallback freeing page to free_one_page() if pcp trylock failed. This make MIGRATE_CMA be able to fallback and be stolen whole pageblock by MIGRATE_UNMOVABLE in the page allocation.
PCP free is fine because free_pcppages_bulk() will always get migratetype again before freeing the page, thus this only happen when someone tried to put CMA page in to other MIGRATE_TYPE's freelist.
Quoted 5 lines
Fixes 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")
Reported-by Joe Liu <[email protected]>
Signed-off-by Lecopzer Chen <[email protected]>
Cc Mark-pk Tsai <[email protected]>
Cc Joe Liu <[email protected]>
Sorry for the long delay and thanks Lecopzer for the patch.
This changelog is difficult to parse but the fix may also me too specific and could be more robust against types other than CMA. It is true that a failed PCP acquire may return a !is_migrate_isolate page to the wrong list but it's more straight-forward to unconditionally lookup the PCP migratetype of the spinlock is not acquired.
How about this? It unconditionally looks up the PCP migratetype after spinlock contention. It's build tested only
--8<--
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.

Quoted 2 lines

[[email protected]: CMA-specific fix]
Fixes 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")

I think we should Cc: stable for the sake of 6.1 LTS?

Quoted 2 lines

Reported-by Joe Liu <[email protected]>
Signed-off-by Mel Gorman <[email protected]>

Acked-by Vlastimil Babka <[email protected]>

Quoted 23 lines

---
 mm/page_alloc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

mm/page_alloc.c +7 -1

index 452459836b71..4053c377fee8 100644
@@ -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);
 }

Mel Gorman

2023-09-05 12:29 UTC #4 lore raw

Quoted 41 lines · Vlastimil Babka

Quoted 40 lines · Mel Gorman
Quoted 9 lines · Lecopzer Chen
commit 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock") fallback freeing page to free_one_page() if pcp trylock failed. This make MIGRATE_CMA be able to fallback and be stolen whole pageblock by MIGRATE_UNMOVABLE in the page allocation.
PCP free is fine because free_pcppages_bulk() will always get migratetype again before freeing the page, thus this only happen when someone tried to put CMA page in to other MIGRATE_TYPE's freelist.
Quoted 5 lines
Fixes 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")
Reported-by Joe Liu <[email protected]>
Signed-off-by Lecopzer Chen <[email protected]>
Cc Mark-pk Tsai <[email protected]>
Cc Joe Liu <[email protected]>
Sorry for the long delay and thanks Lecopzer for the patch.
This changelog is difficult to parse but the fix may also me too specific and could be more robust against types other than CMA. It is true that a failed PCP acquire may return a !is_migrate_isolate page to the wrong list but it's more straight-forward to unconditionally lookup the PCP migratetype of the spinlock is not acquired.
How about this? It unconditionally looks up the PCP migratetype after spinlock contention. It's build tested only
--8<--
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.

Quoted 5 lines

Quoted 2 lines
[[email protected]: CMA-specific fix]
Fixes 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")
I think we should Cc: stable for the sake of 6.1 LTS?

Yep.

Quoted 5 lines

Quoted 2 lines
Reported-by Joe Liu <[email protected]>
Signed-off-by Mel Gorman <[email protected]>
Acked-by Vlastimil Babka <[email protected]>

Thanks.

Signature · 2 lines

Mel Gorman
SUSE Labs

Johannes Weiner

reviewer

2023-09-11 15:57 UTC #5 lore raw

In reply to Mel Gorman

Quoted 30 lines · Mel Gorman

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.
[[email protected]: CMA-specific fix]
Fixes 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")
Reported-by Joe Liu <[email protected]>
Signed-off-by Mel Gorman <[email protected]>
---
 mm/page_alloc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
mm/page_alloc.c +0 -1
index 452459836b71..4053c377fee8 100644
@@ -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);

mm/page_alloc.c void free_unref_page(struct page *page, unsigned int order) · line 2440

Quoted 10 lines

mm/page_alloc.c +7 -0

+		/*
+		 * 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/[email protected]/

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.

I'm just preparing v2 of the above series. Do you want me to break this change out and send it separately?

Vlastimil Babka

2023-09-11 16:14 UTC #6 lore raw

Quoted 44 lines · Johannes Weiner

Quoted 30 lines · Mel Gorman
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.
[[email protected]: CMA-specific fix]
Fixes 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")
Reported-by Joe Liu <[email protected]>
Signed-off-by Mel Gorman <[email protected]>
---
 mm/page_alloc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
mm/page_alloc.c +0 -1
index 452459836b71..4053c377fee8 100644
@@ -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);
mm/page_alloc.c void free_unref_page(struct page *page, unsigned int order) · line 2440
Quoted 10 lines
mm/page_alloc.c +7 -0
+		/*
+		 * 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:

Quoted 7 lines

https://lore.kernel.org/lkml/[email protected]/
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.

Quoted 2 lines

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.

Johannes Weiner

reviewer

2023-09-11 18:11 UTC #7 lore raw

Quoted 53 lines · Vlastimil Babka

Quoted 44 lines · Johannes Weiner
Quoted 30 lines · Mel Gorman
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.
[[email protected]: CMA-specific fix]
Fixes 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")
Reported-by Joe Liu <[email protected]>
Signed-off-by Mel Gorman <[email protected]>
---
 mm/page_alloc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
mm/page_alloc.c +0 -1
index 452459836b71..4053c377fee8 100644
@@ -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);
mm/page_alloc.c void free_unref_page(struct page *page, unsigned int order) · line 2440
Quoted 10 lines
mm/page_alloc.c +7 -0
+		/*
+		 * 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:
Quoted 7 lines
https://lore.kernel.org/lkml/[email protected]/
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.

Quoted 7 lines

Seems to be the cleanest solution to me, indeed.
Quoted 2 lines
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 <[email protected]>
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/[email protected]/

Changelog updated in response to the CMA-specific bug report.

[[email protected]: updated changelog]

Reported-by Joe Liu <[email protected]>

Fixes 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")

Cc [email protected]

Signed-off-by Johannes Weiner <[email protected]>

---
 mm/page_alloc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

mm/page_alloc.c +6 -6

index 0c5be12f9336..95546f376302 100644
@@ -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);

Signature · 1 line

2.42.0

Vlastimil Babka

Reviewed-by

2023-09-11 19:59 UTC #8 lore raw

Quoted 95 lines · Johannes Weiner

Quoted 53 lines · Vlastimil Babka
Quoted 44 lines · Johannes Weiner
Quoted 30 lines · Mel Gorman
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.
[[email protected]: CMA-specific fix]
Fixes 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")
Reported-by Joe Liu <[email protected]>
Signed-off-by Mel Gorman <[email protected]>
---
 mm/page_alloc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
mm/page_alloc.c +0 -1
index 452459836b71..4053c377fee8 100644
@@ -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);
mm/page_alloc.c void free_unref_page(struct page *page, unsigned int order) · line 2440
Quoted 10 lines
mm/page_alloc.c +7 -0
+		/*
+		 * 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:
Quoted 7 lines
https://lore.kernel.org/lkml/[email protected]/
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.
Quoted 7 lines
Seems to be the cleanest solution to me, indeed.
Quoted 2 lines
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 <[email protected]>
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/[email protected]/

Quoted 7 lines

Changelog updated in response to the CMA-specific bug report.
[[email protected]: updated changelog]
Reported-by Joe Liu <[email protected]>
Fixes 4b23a68f9536 ("mm/page_alloc: protect PCP lists with a spinlock")
Cc [email protected]
Signed-off-by Johannes Weiner <[email protected]>

Reviewed-by Vlastimil Babka <[email protected]>

Quoted 47 lines

---
 mm/page_alloc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

mm/page_alloc.c +6 -6

index 0c5be12f9336..95546f376302 100644
@@ -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);