diff mbox series

mm/page_isolation: fix isolate_single_pageblock() isolation behavior

Message ID 20220914023913.1855924-1-zi.yan@sent.com (mailing list archive)
State New
Headers show
Series mm/page_isolation: fix isolate_single_pageblock() isolation behavior | expand

Commit Message

Zi Yan Sept. 14, 2022, 2:39 a.m. UTC 
From: Zi Yan <ziy@nvidia.com>

set_migratetype_isolate() does not allow isolating MIGRATE_CMA pageblocks
unless it is used for CMA allocation. isolate_single_pageblock() did not
have the same behavior when it is used together with
set_migratetype_isolate() in start_isolate_page_range(). This allows
alloc_contig_range() with migratetype other than MIGRATE_CMA, like
MIGRATE_MOVABLE (used by alloc_contig_pages()), to isolate first and last
pageblock but fail the rest. The failure leads to changing migratetype
of the first and last pageblock to MIGRATE_MOVABLE from MIGRATE_CMA,
corrupting the CMA region. This can happen during gigantic page
allocations.

Fix it by passing migratetype into isolate_single_pageblock(), so that
set_migratetype_isolate() used by isolate_single_pageblock() will prevent
the isolation happening.

Fixes: b2c9e2fbba32 ("mm: make alloc_contig_range work at pageblock granularity")
Reported-by: Doug Berger <opendmb@gmail.com>
Signed-off-by: Zi Yan <ziy@nvidia.com>
Cc: <stable@vger.kernel.org>
---
 mm/page_isolation.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

Comments

Andrew Morton Sept. 14, 2022, 10:42 p.m. UTC | #1 
On Tue, 13 Sep 2022 22:39:13 -0400 Zi Yan <zi.yan@sent.com> wrote:

> set_migratetype_isolate() does not allow isolating MIGRATE_CMA pageblocks
> unless it is used for CMA allocation. isolate_single_pageblock() did not
> have the same behavior when it is used together with
> set_migratetype_isolate() in start_isolate_page_range(). This allows
> alloc_contig_range() with migratetype other than MIGRATE_CMA, like
> MIGRATE_MOVABLE (used by alloc_contig_pages()), to isolate first and last
> pageblock but fail the rest. The failure leads to changing migratetype
> of the first and last pageblock to MIGRATE_MOVABLE from MIGRATE_CMA,
> corrupting the CMA region. This can happen during gigantic page
> allocations.

How does this bug manifest itself as far as the user is concerned?

> Fix it by passing migratetype into isolate_single_pageblock(), so that
> set_migratetype_isolate() used by isolate_single_pageblock() will prevent
> the isolation happening.
Zi Yan Sept. 14, 2022, 10:47 p.m. UTC | #2 
On 14 Sep 2022, at 18:42, Andrew Morton wrote:

> On Tue, 13 Sep 2022 22:39:13 -0400 Zi Yan <zi.yan@sent.com> wrote:
>
>> set_migratetype_isolate() does not allow isolating MIGRATE_CMA pageblocks
>> unless it is used for CMA allocation. isolate_single_pageblock() did not
>> have the same behavior when it is used together with
>> set_migratetype_isolate() in start_isolate_page_range(). This allows
>> alloc_contig_range() with migratetype other than MIGRATE_CMA, like
>> MIGRATE_MOVABLE (used by alloc_contig_pages()), to isolate first and last
>> pageblock but fail the rest. The failure leads to changing migratetype
>> of the first and last pageblock to MIGRATE_MOVABLE from MIGRATE_CMA,
>> corrupting the CMA region. This can happen during gigantic page
>> allocations.
>
> How does this bug manifest itself as far as the user is concerned?

Like Doug said here: https://lore.kernel.org/linux-mm/a3363a52-883b-dcd1-b77f-f2bb378d6f2d@gmail.com/T/#u,
for gigantic page allocations, the user would notice no difference, since
the allocation on CMA region will fail as well as it did before. But
it might hurt the performance of device drivers that use CMA, since
CMA region size decreases.


>
>> Fix it by passing migratetype into isolate_single_pageblock(), so that
>> set_migratetype_isolate() used by isolate_single_pageblock() will prevent
>> the isolation happening.


--
Best Regards,
Yan, Zi
diff mbox series

Patch

diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index c66d61e0bc72..04141a9bea70 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -288,6 +288,7 @@  __first_valid_page(unsigned long pfn, unsigned long nr_pages)
  * @isolate_before:	isolate the pageblock before the boundary_pfn
  * @skip_isolation:	the flag to skip the pageblock isolation in second
  *			isolate_single_pageblock()
+ * @migratetype:	migrate type to set in error recovery.
  *
  * Free and in-use pages can be as big as MAX_ORDER-1 and contain more than one
  * pageblock. When not all pageblocks within a page are isolated at the same
@@ -302,9 +303,9 @@  __first_valid_page(unsigned long pfn, unsigned long nr_pages)
  * the in-use page then splitting the free page.
  */
 static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
-			gfp_t gfp_flags, bool isolate_before, bool skip_isolation)
+			gfp_t gfp_flags, bool isolate_before, bool skip_isolation,
+			int migratetype)
 {
-	unsigned char saved_mt;
 	unsigned long start_pfn;
 	unsigned long isolate_pageblock;
 	unsigned long pfn;
@@ -328,13 +329,13 @@  static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
 	start_pfn  = max(ALIGN_DOWN(isolate_pageblock, MAX_ORDER_NR_PAGES),
 				      zone->zone_start_pfn);
 
-	saved_mt = get_pageblock_migratetype(pfn_to_page(isolate_pageblock));
+	if (skip_isolation) {
+		int mt = get_pageblock_migratetype(pfn_to_page(isolate_pageblock));
 
-	if (skip_isolation)
-		VM_BUG_ON(!is_migrate_isolate(saved_mt));
-	else {
-		ret = set_migratetype_isolate(pfn_to_page(isolate_pageblock), saved_mt, flags,
-				isolate_pageblock, isolate_pageblock + pageblock_nr_pages);
+		VM_BUG_ON(!is_migrate_isolate(mt));
+	} else {
+		ret = set_migratetype_isolate(pfn_to_page(isolate_pageblock), migratetype,
+				flags, isolate_pageblock, isolate_pageblock + pageblock_nr_pages);
 
 		if (ret)
 			return ret;
@@ -475,7 +476,7 @@  static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
 failed:
 	/* restore the original migratetype */
 	if (!skip_isolation)
-		unset_migratetype_isolate(pfn_to_page(isolate_pageblock), saved_mt);
+		unset_migratetype_isolate(pfn_to_page(isolate_pageblock), migratetype);
 	return -EBUSY;
 }
 
@@ -537,7 +538,8 @@  int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
 	bool skip_isolation = false;
 
 	/* isolate [isolate_start, isolate_start + pageblock_nr_pages) pageblock */
-	ret = isolate_single_pageblock(isolate_start, flags, gfp_flags, false, skip_isolation);
+	ret = isolate_single_pageblock(isolate_start, flags, gfp_flags, false,
+			skip_isolation, migratetype);
 	if (ret)
 		return ret;
 
@@ -545,7 +547,8 @@  int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
 		skip_isolation = true;
 
 	/* isolate [isolate_end - pageblock_nr_pages, isolate_end) pageblock */
-	ret = isolate_single_pageblock(isolate_end, flags, gfp_flags, true, skip_isolation);
+	ret = isolate_single_pageblock(isolate_end, flags, gfp_flags, true,
+			skip_isolation, migratetype);
 	if (ret) {
 		unset_migratetype_isolate(pfn_to_page(isolate_start), migratetype);
 		return ret;