mm/vmscan: Fix NR_ISOLATED_FILE corruption on 64-bit

PATCH Applied Nicholas Piggin 3 participants 4 messages Oct 29, 2020 – Oct 30, 2020

Subsystems (3)

Memory management MGLRU MM reclaim

lore

Share & tools

lore

Nicholas Piggin

author

2020-10-29 03:23 UTC #0 lore raw

PATCH 1/1 mm/vmscan: Fix NR_ISOLATED_FILE corruption on 64-bit

Previously the negated unsigned long would be cast back to signed long which would have the correct negative value. After commit 730ec8c01a2b ("mm/vmscan.c: change prototype for shrink_page_list"), the large unsigned int converts to a large positive signed long.

Symptoms include CMA allocations hanging forever holding the cma_mutex due to alloc_contig_range->...->isolate_migratepages_block waiting forever in "while (unlikely(too_many_isolated(pgdat)))".

Cc [email protected]

Cc Maninder Singh <[email protected]>

Cc Vaneet Narang <[email protected]>

Cc Maninder Singh <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Amit Sahrawat <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Vlastimil Babka <[email protected]>

Fixes 730ec8c01a2b ("mm/vmscan.c: change prototype for shrink_page_list")

Signed-off-by Nicholas Piggin <[email protected]>

---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/vmscan.c +1 -1

index 1b8f0e059767..92c507bacf09 100644
@@ -1516,7 +1516,7 @@ unsigned int reclaim_clean_pages_from_list(struct zone *zone,
 	nr_reclaimed = shrink_page_list(&clean_pages, zone->zone_pgdat, &sc,
 			TTU_IGNORE_ACCESS, &stat, true);
 	list_splice(&clean_pages, page_list);
-	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, -nr_reclaimed);
+	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, -(long)nr_reclaimed);
 	/*
 	 * Since lazyfree pages are isolated from file LRU from the beginning,
 	 * they will rotate back to anonymous LRU in the end if it failed to

Signature · 1 line

2.23.0

Michal Hocko

2020-10-29 13:11 UTC #1 lore raw

Quoted 8 lines · Nicholas Piggin

Previously the negated unsigned long would be cast back to signed long which would have the correct negative value. After commit 730ec8c01a2b ("mm/vmscan.c: change prototype for shrink_page_list"), the large unsigned int converts to a large positive signed long.
Symptoms include CMA allocations hanging forever holding the cma_mutex due to alloc_contig_range->...->isolate_migratepages_block waiting forever in "while (unlikely(too_many_isolated(pgdat)))".

Nasty.

Quoted 19 lines

Cc [email protected]
Cc Maninder Singh <[email protected]>
Cc Vaneet Narang <[email protected]>
Cc Maninder Singh <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Amit Sahrawat <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Vlastimil Babka <[email protected]>
Fixes 730ec8c01a2b ("mm/vmscan.c: change prototype for shrink_page_list")
Signed-off-by Nicholas Piggin <[email protected]>
---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/vmscan.c +0 -0
index 1b8f0e059767..92c507bacf09 100644

mm/vmscan.c unsigned int reclaim_clean_pages_from_list(struct zone *zone, · line 1522

Quoted 8 lines

mm/vmscan.c +1 -1

@@ -1516,7 +1516,7 @@ unsigned int reclaim_clean_pages_from_list(struct zone *zone,
 	nr_reclaimed = shrink_page_list(&clean_pages, zone->zone_pgdat, &sc,
 			TTU_IGNORE_ACCESS, &stat, true);
 	list_splice(&clean_pages, page_list);
-	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, -nr_reclaimed);
+	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, -(long)nr_reclaimed);
 	/*
 	 * Since lazyfree pages are isolated from file LRU from the beginning,
 	 * they will rotate back to anonymous LRU in the end if it failed to

You want the same also for -stat.nr_lazyfree_fail right?

Signature · 2 lines

Michal Hocko
SUSE Labs

Andrew Morton

maintainer

2020-10-30 05:25 UTC #2 lore raw

Quoted 26 lines · On Thu, 29 Oct 2020 14:11:03 +0100 Michal Hocko <[email protected]> wrote:

Quoted 8 lines · Nicholas Piggin
Previously the negated unsigned long would be cast back to signed long which would have the correct negative value. After commit 730ec8c01a2b ("mm/vmscan.c: change prototype for shrink_page_list"), the large unsigned int converts to a large positive signed long.
Symptoms include CMA allocations hanging forever holding the cma_mutex due to alloc_contig_range->...->isolate_migratepages_block waiting forever in "while (unlikely(too_many_isolated(pgdat)))".
Nasty.
Quoted 14 lines
Cc [email protected]
Cc Maninder Singh <[email protected]>
Cc Vaneet Narang <[email protected]>
Cc Maninder Singh <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Amit Sahrawat <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Vlastimil Babka <[email protected]>
Fixes 730ec8c01a2b ("mm/vmscan.c: change prototype for shrink_page_list")
Signed-off-by Nicholas Piggin <[email protected]>
---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 15 lines

mm/vmscan.c unsigned int reclaim_clean_pages_from_list(struct zone *zone, · line 1522

Quoted 13 lines

mm/vmscan.c +1 -1

index 1b8f0e059767..92c507bacf09 100644
@@ -1516,7 +1516,7 @@ unsigned int reclaim_clean_pages_from_list(struct zone *zone,
 	nr_reclaimed = shrink_page_list(&clean_pages, zone->zone_pgdat, &sc,
 			TTU_IGNORE_ACCESS, &stat, true);
 	list_splice(&clean_pages, page_list);
-	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, -nr_reclaimed);
+	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, -(long)nr_reclaimed);
 	/*
 	 * Since lazyfree pages are isolated from file LRU from the beginning,
 	 * they will rotate back to anonymous LRU in the end if it failed to

You want the same also for -stat.nr_lazyfree_fail right?

I did the below, and added a cc:stable.

mm/vmscan.c renamed from mm/vmscan.c~mm-vmscan-fix-nr_isolated_file-corruption-on-64-bit-fix +3 -2

@@ -1516,7 +1516,8 @@ unsigned int reclaim_clean_pages_from_li
 	nr_reclaimed = shrink_page_list(&clean_pages, zone->zone_pgdat, &sc,
 			TTU_IGNORE_ACCESS, &stat, true);
 	list_splice(&clean_pages, page_list);
-	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, -(long)nr_reclaimed);
+	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE,
+			    -(long)nr_reclaimed);
 	/*
 	 * Since lazyfree pages are isolated from file LRU from the beginning,
 	 * they will rotate back to anonymous LRU in the end if it failed to

@@ -1526,7 +1527,7 @@ unsigned int reclaim_clean_pages_from_li
 	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON,
 			    stat.nr_lazyfree_fail);
 	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE,
-			    -stat.nr_lazyfree_fail);
+			    -(long)stat.nr_lazyfree_fail);
 	return nr_reclaimed;
 }

Michal Hocko

Acked-by

2020-10-30 07:00 UTC #3 lore raw

Quoted 59 lines · Andrew Morton

Quoted 26 lines · On Thu, 29 Oct 2020 14:11:03 +0100 Michal Hocko <[email protected]> wrote:
Quoted 8 lines · Nicholas Piggin
Previously the negated unsigned long would be cast back to signed long which would have the correct negative value. After commit 730ec8c01a2b ("mm/vmscan.c: change prototype for shrink_page_list"), the large unsigned int converts to a large positive signed long.
Symptoms include CMA allocations hanging forever holding the cma_mutex due to alloc_contig_range->...->isolate_migratepages_block waiting forever in "while (unlikely(too_many_isolated(pgdat)))".
Nasty.
Quoted 14 lines
Cc [email protected]
Cc Maninder Singh <[email protected]>
Cc Vaneet Narang <[email protected]>
Cc Maninder Singh <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Amit Sahrawat <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Vlastimil Babka <[email protected]>
Fixes 730ec8c01a2b ("mm/vmscan.c: change prototype for shrink_page_list")
Signed-off-by Nicholas Piggin <[email protected]>
---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines
mm/vmscan.c unsigned int reclaim_clean_pages_from_list(struct zone *zone, · line 1522
Quoted 13 lines
mm/vmscan.c +1 -1
index 1b8f0e059767..92c507bacf09 100644
@@ -1516,7 +1516,7 @@ unsigned int reclaim_clean_pages_from_list(struct zone *zone,
 	nr_reclaimed = shrink_page_list(&clean_pages, zone->zone_pgdat, &sc,
 			TTU_IGNORE_ACCESS, &stat, true);
 	list_splice(&clean_pages, page_list);
-	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, -nr_reclaimed);
+	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, -(long)nr_reclaimed);
 	/*
 	 * Since lazyfree pages are isolated from file LRU from the beginning,
 	 * they will rotate back to anonymous LRU in the end if it failed to
You want the same also for -stat.nr_lazyfree_fail right?
I did the below, and added a cc:stable.
mm/vmscan.c renamed from mm/vmscan.c~mm-vmscan-fix-nr_isolated_file-corruption-on-64-bit-fix +2 -1
@@ -1516,7 +1516,8 @@ unsigned int reclaim_clean_pages_from_li
 	nr_reclaimed = shrink_page_list(&clean_pages, zone->zone_pgdat, &sc,
 			TTU_IGNORE_ACCESS, &stat, true);
 	list_splice(&clean_pages, page_list);
-	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, -(long)nr_reclaimed);
+	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE,
+			    -(long)nr_reclaimed);
 	/*
 	 * Since lazyfree pages are isolated from file LRU from the beginning,
 	 * they will rotate back to anonymous LRU in the end if it failed to

mm/vmscan.c unsigned int reclaim_clean_pages_from_li · line 1532

Quoted 7 lines

mm/vmscan.c renamed from mm/vmscan.c~mm-vmscan-fix-nr_isolated_file-corruption-on-64-bit-fix +1 -1

@@ -1526,7 +1527,7 @@ unsigned int reclaim_clean_pages_from_li
 	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON,
 			    stat.nr_lazyfree_fail);
 	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE,
-			    -stat.nr_lazyfree_fail);
+			    -(long)stat.nr_lazyfree_fail);
 	return nr_reclaimed;
 }

Acked-by Michal Hocko <[email protected]>

Btw. regular reclaim paths are OK because nr_taken is unsigned long.

Btw #2 I was quite surprised about the inconsistency for !SMP case where __mod_node_page_state uses int for the delta.

Thanks!

Signature · 2 lines

Michal Hocko
SUSE Labs