Re: [PATCH v2 11/11] mm/migration: fix possible do_pages_stat_array racing with memory offline

Re: [PATCH v2 11/11] mm/migration: fix possible do_pages_stat_array racing with memory offline

[Huang, Ying]

Miaohe Lin <linmiaohe@huawei.com> writes:

> When follow_page peeks a page, the page could be migrated and then be
> offlined while it's still being used by the do_pages_stat_array().
> Use FOLL_GET to hold the page refcnt to fix this potential race.
>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Acked-by: "Huang, Ying" <ying.huang@intel.com>

> ---
>  mm/migrate.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index dbd91fbdb127..cd85ba0ab592 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1807,13 +1807,18 @@ static void do_pages_stat_array(struct mm_struct *mm, unsigned long nr_pages,
>  			goto set_status;
>  
>  		/* FOLL_DUMP to ignore special (like zero) pages */
> -		page = follow_page(vma, addr, FOLL_DUMP);
> +		page = follow_page(vma, addr, FOLL_GET | FOLL_DUMP);
>  
>  		err = PTR_ERR(page);
>  		if (IS_ERR(page))
>  			goto set_status;
>  
> -		err = page ? page_to_nid(page) : -ENOENT;
> +		if (page) {
> +			err = page_to_nid(page);
> +			put_page(page);
> +		} else {
> +			err = -ENOENT;
> +		}
>  set_status:
>  		*status = err;

Re: [PATCH v2 11/11] mm/migration: fix possible do_pages_stat_array racing with memory offline

[Muchun Song]

On Thu, Mar 17, 2022 at 10:42 PM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> When follow_page peeks a page, the page could be migrated and then be
> offlined while it's still being used by the do_pages_stat_array().
> Use FOLL_GET to hold the page refcnt to fix this potential race.
>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Re: [PATCH v2 06/11] mm/migration: use helper macro min in do_pages_stat

[Muchun Song]

On Thu, Mar 17, 2022 at 10:42 PM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> We could use helper macro min to help set the chunk_nr to simplify
> the code.
>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Re: [PATCH v2 09/11] mm/migration: fix potential page refcounts leak in migrate_pages

[Muchun Song]

On Thu, Mar 17, 2022 at 10:41 PM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> In -ENOMEM case, there might be some subpages of fail-to-migrate THPs
> left in thp_split_pages list. We should move them back to migration
> list so that they could be put back to the right list by the caller
> otherwise the page refcnt will be leaked here. Also adjust nr_failed
> and nr_thp_failed accordingly to make vm events account more accurate.
>
> Fixes: b5bade978e9b ("mm: migrate: fix the return value of migrate_pages()")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> Reviewed-by: Zi Yan <ziy@nvidia.com>
> Reviewed-by: "Huang, Ying" <ying.huang@intel.com>
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

[PATCH v2 00/11] A few cleanup and fixup patches for migration

[Miaohe Lin]

Hi everyone,
This series contains a few patches to remove unneeded variables, jump
label and use helper to simplify the code. Also we fix some bugs such
as page refcounts leak , invalid node access and so on. More details
can be found in the respective changelogs. Thanks!

---
v1->v2:
  collect reviewed-by tag
  fix some commit log
  remove unneeded nr_failed assignment per Huang Ying
  getting the types correct to use min() per Andrew
  delay some patches to make this easier to move forward
  Thanks Muchun, Baolin, Zi Yan,Huang Ying, Andrew, Alistair for review!
---
Miaohe Lin (11):
  mm/migration: remove unneeded local variable mapping_locked
  mm/migration: remove unneeded out label
  mm/migration: remove unneeded local variable page_lru
  mm/migration: fix the confusing PageTransHuge check
  mm/migration: use helper function vma_lookup() in
    add_page_for_migration
  mm/migration: use helper macro min in do_pages_stat
  mm/migration: avoid unneeded nodemask_t initialization
  mm/migration: remove some duplicated codes in migrate_pages
  mm/migration: fix potential page refcounts leak in migrate_pages
  mm/migration: fix potential invalid node access for reclaim-based
    migration
  mm/migration: fix possible do_pages_stat_array racing with memory
    offline

 mm/migrate.c | 94 +++++++++++++++++++++++-----------------------------
 1 file changed, 42 insertions(+), 52 deletions(-)

-- 
2.23.0

[PATCH v2 01/11] mm/migration: remove unneeded local variable mapping_locked

[Miaohe Lin]

When mapping_locked is true, TTU_RMAP_LOCKED is always set to ttu. We can
check ttu instead so mapping_locked can be removed. And ttu is either 0
or TTU_RMAP_LOCKED now. Change '|=' to '=' to reflect this.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Reviewed-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/migrate.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 42e3a944f35c..50bc62d85eaf 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1230,7 +1230,6 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
 		goto put_anon;
 
 	if (page_mapped(hpage)) {
-		bool mapping_locked = false;
 		enum ttu_flags ttu = 0;
 
 		if (!PageAnon(hpage)) {
@@ -1244,14 +1243,13 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
 			if (unlikely(!mapping))
 				goto unlock_put_anon;
 
-			mapping_locked = true;
-			ttu |= TTU_RMAP_LOCKED;
+			ttu = TTU_RMAP_LOCKED;
 		}
 
 		try_to_migrate(src, ttu);
 		page_was_mapped = 1;
 
-		if (mapping_locked)
+		if (ttu & TTU_RMAP_LOCKED)
 			i_mmap_unlock_write(mapping);
 	}
 
-- 
2.23.0

[PATCH v2 02/11] mm/migration: remove unneeded out label

[Miaohe Lin]

We can do prep_transhuge_page when newpage is not NULL. Thus we can remove
out label to simplify the code.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Reviewed-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/migrate.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 50bc62d85eaf..bc1867a5706c 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2025,12 +2025,9 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page,
 
 	newpage = alloc_pages_node(nid, (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE),
 				   HPAGE_PMD_ORDER);
-	if (!newpage)
-		goto out;
+	if (newpage)
+		prep_transhuge_page(newpage);
 
-	prep_transhuge_page(newpage);
-
-out:
 	return newpage;
 }
 
-- 
2.23.0

[PATCH v2 03/11] mm/migration: remove unneeded local variable page_lru

[Miaohe Lin]

We can use page_is_file_lru() directly to help account the isolated
pages to simplify the code a bit.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/migrate.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index bc1867a5706c..0ea555e4eaae 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2033,7 +2033,6 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page,
 
 static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 {
-	int page_lru;
 	int nr_pages = thp_nr_pages(page);
 	int order = compound_order(page);
 
@@ -2060,8 +2059,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 	if (isolate_lru_page(page))
 		return 0;
 
-	page_lru = page_is_file_lru(page);
-	mod_node_page_state(page_pgdat(page), NR_ISOLATED_ANON + page_lru,
+	mod_node_page_state(page_pgdat(page), NR_ISOLATED_ANON + page_is_file_lru(page),
 			    nr_pages);
 
 	/*
-- 
2.23.0

[PATCH v2 04/11] mm/migration: fix the confusing PageTransHuge check

[Miaohe Lin]

prep_transhuge_page should be called when PageTransHuge(page) is true.
The newly allocated new_page is not yet PageTransHuge though it could
pass the check as PageTransHuge only checks PageHead now.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Reviewed-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/migrate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 0ea555e4eaae..b16c561a9a4b 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1560,7 +1560,7 @@ struct page *alloc_migration_target(struct page *page, unsigned long private)
 
 	new_page = __alloc_pages(gfp_mask, order, nid, mtc->nmask);
 
-	if (new_page && PageTransHuge(new_page))
+	if (new_page && PageTransHuge(page))
 		prep_transhuge_page(new_page);
 
 	return new_page;
-- 
2.23.0

[PATCH v2 05/11] mm/migration: use helper function vma_lookup() in add_page_for_migration

[Miaohe Lin]

We could use helper function vma_lookup() to lookup the needed vma to
simplify the code.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/migrate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index b16c561a9a4b..eb1c736750da 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1614,8 +1614,8 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
 
 	mmap_read_lock(mm);
 	err = -EFAULT;
-	vma = find_vma(mm, addr);
-	if (!vma || addr < vma->vm_start || !vma_migratable(vma))
+	vma = vma_lookup(mm, addr);
+	if (!vma || !vma_migratable(vma))
 		goto out;
 
 	/* FOLL_DUMP to ignore special (like zero) pages */
-- 
2.23.0

[PATCH v2 06/11] mm/migration: use helper macro min in do_pages_stat

[Miaohe Lin]

We could use helper macro min to help set the chunk_nr to simplify
the code.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/migrate.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index eb1c736750da..92068d090db8 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1853,16 +1853,12 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
 			 const void __user * __user *pages,
 			 int __user *status)
 {
-#define DO_PAGES_STAT_CHUNK_NR 16
+#define DO_PAGES_STAT_CHUNK_NR 16UL
 	const void __user *chunk_pages[DO_PAGES_STAT_CHUNK_NR];
 	int chunk_status[DO_PAGES_STAT_CHUNK_NR];
 
 	while (nr_pages) {
-		unsigned long chunk_nr;
-
-		chunk_nr = nr_pages;
-		if (chunk_nr > DO_PAGES_STAT_CHUNK_NR)
-			chunk_nr = DO_PAGES_STAT_CHUNK_NR;
+		unsigned long chunk_nr = min(nr_pages, DO_PAGES_STAT_CHUNK_NR);
 
 		if (in_compat_syscall()) {
 			if (get_compat_pages_array(chunk_pages, pages,
-- 
2.23.0

[PATCH v2 07/11] mm/migration: avoid unneeded nodemask_t initialization

[Miaohe Lin]

Avoid unneeded next_pass and this_pass initialization as they're always
set before using to save possible cpu cycles when there are plenty of
nodes in the system.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Reviewed-by: Muchun Song <songmuchun@bytedance.com>
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/migrate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 92068d090db8..bb36f6b40f18 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2379,8 +2379,8 @@ static int establish_migrate_target(int node, nodemask_t *used,
  */
 static void __set_migration_target_nodes(void)
 {
-	nodemask_t next_pass	= NODE_MASK_NONE;
-	nodemask_t this_pass	= NODE_MASK_NONE;
+	nodemask_t next_pass;
+	nodemask_t this_pass;
 	nodemask_t used_targets = NODE_MASK_NONE;
 	int node, best_distance;
 
-- 
2.23.0

[PATCH v2 08/11] mm/migration: remove some duplicated codes in migrate_pages

[Miaohe Lin]

Remove the duplicated codes in migrate_pages to simplify the code. Minor
readability improvement. No functional change intended.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Reviewed-by: Zi Yan <ziy@nvidia.com>
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/migrate.c | 36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index bb36f6b40f18..63a87ef0996f 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1414,14 +1414,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 						nr_thp_split++;
 						goto retry;
 					}
-
-					nr_failed_pages += nr_subpages;
-					break;
-				}
-
 				/* Hugetlb migration is unsupported */
-				if (!no_subpage_counting)
+				} else if (!no_subpage_counting) {
 					nr_failed++;
+				}
+
 				nr_failed_pages += nr_subpages;
 				break;
 			case -ENOMEM:
@@ -1436,28 +1433,22 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 						nr_thp_split++;
 						goto retry;
 					}
-
-					nr_failed_pages += nr_subpages;
-					goto out;
+				} else if (!no_subpage_counting) {
+					nr_failed++;
 				}
 
-				if (!no_subpage_counting)
-					nr_failed++;
 				nr_failed_pages += nr_subpages;
 				goto out;
 			case -EAGAIN:
-				if (is_thp) {
+				if (is_thp)
 					thp_retry++;
-					break;
-				}
-				retry++;
+				else
+					retry++;
 				break;
 			case MIGRATEPAGE_SUCCESS:
 				nr_succeeded += nr_subpages;
-				if (is_thp) {
+				if (is_thp)
 					nr_thp_succeeded++;
-					break;
-				}
 				break;
 			default:
 				/*
@@ -1466,14 +1457,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 				 * removed from migration page list and not
 				 * retried in the next outer loop.
 				 */
-				if (is_thp) {
+				if (is_thp)
 					nr_thp_failed++;
-					nr_failed_pages += nr_subpages;
-					break;
-				}
-
-				if (!no_subpage_counting)
+				else if (!no_subpage_counting)
 					nr_failed++;
+
 				nr_failed_pages += nr_subpages;
 				break;
 			}
-- 
2.23.0

[PATCH v2 09/11] mm/migration: fix potential page refcounts leak in migrate_pages

[Miaohe Lin]

In -ENOMEM case, there might be some subpages of fail-to-migrate THPs
left in thp_split_pages list. We should move them back to migration
list so that they could be put back to the right list by the caller
otherwise the page refcnt will be leaked here. Also adjust nr_failed
and nr_thp_failed accordingly to make vm events account more accurate.

Fixes: b5bade978e9b ("mm: migrate: fix the return value of migrate_pages()")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Reviewed-by: Zi Yan <ziy@nvidia.com>
Reviewed-by: "Huang, Ying" <ying.huang@intel.com>
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/migrate.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/mm/migrate.c b/mm/migrate.c
index 63a87ef0996f..97dfd1f4870d 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1438,6 +1438,14 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 				}
 
 				nr_failed_pages += nr_subpages;
+				/*
+				 * There might be some subpages of fail-to-migrate THPs
+				 * left in thp_split_pages list. Move them back to migration
+				 * list so that they could be put back to the right list by
+				 * the caller otherwise the page refcnt will be leaked.
+				 */
+				list_splice_init(&thp_split_pages, from);
+				nr_thp_failed += thp_retry;
 				goto out;
 			case -EAGAIN:
 				if (is_thp)
-- 
2.23.0

[PATCH v2 10/11] mm/migration: fix potential invalid node access for reclaim-based migration

[Miaohe Lin]

If we failed to setup hotplug state callbacks for mm/demotion:online in
some corner cases, node_demotion will be left uninitialized. Invalid node
might be returned from the next_demotion_node() when doing reclaim-based
migration. Use kcalloc to allocate node_demotion to fix the issue.

Fixes: ac16ec835314 ("mm: migrate: support multiple target nodes demotion")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Reviewed-by: "Huang, Ying" <ying.huang@intel.com>
---
 mm/migrate.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 97dfd1f4870d..dbd91fbdb127 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2534,9 +2534,9 @@ static int __init migrate_on_reclaim_init(void)
 {
 	int ret;
 
-	node_demotion = kmalloc_array(nr_node_ids,
-				      sizeof(struct demotion_nodes),
-				      GFP_KERNEL);
+	node_demotion = kcalloc(nr_node_ids,
+				sizeof(struct demotion_nodes),
+				GFP_KERNEL);
 	WARN_ON(!node_demotion);
 
 	ret = cpuhp_setup_state_nocalls(CPUHP_MM_DEMOTION_DEAD, "mm/demotion:offline",
-- 
2.23.0

[PATCH v2 11/11] mm/migration: fix possible do_pages_stat_array racing with memory offline

[Miaohe Lin]

When follow_page peeks a page, the page could be migrated and then be
offlined while it's still being used by the do_pages_stat_array().
Use FOLL_GET to hold the page refcnt to fix this potential race.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/migrate.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index dbd91fbdb127..cd85ba0ab592 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1807,13 +1807,18 @@ static void do_pages_stat_array(struct mm_struct *mm, unsigned long nr_pages,
 			goto set_status;
 
 		/* FOLL_DUMP to ignore special (like zero) pages */
-		page = follow_page(vma, addr, FOLL_DUMP);
+		page = follow_page(vma, addr, FOLL_GET | FOLL_DUMP);
 
 		err = PTR_ERR(page);
 		if (IS_ERR(page))
 			goto set_status;
 
-		err = page ? page_to_nid(page) : -ENOENT;
+		if (page) {
+			err = page_to_nid(page);
+			put_page(page);
+		} else {
+			err = -ENOENT;
+		}
 set_status:
 		*status = err;
 
-- 
2.23.0