If memory is successfully allocated on the target node and the
function directly returns without value restore for nmask,
non-first migration operations in migrate_pages() by again label
may ignore the nmask settings, thereby allowing new memory
allocations for migration on any node.

Signed-off-by: wangchuanguo <wangchuanguo@inspur.com>
---
 mm/vmscan.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index b620d74b0f66..9467b2acef28 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1026,8 +1026,10 @@ struct folio *alloc_migrate_folio(struct folio *src, unsigned long private)
 	mtc->nmask = NULL;
 	mtc->gfp_mask |= __GFP_THISNODE;
 	dst = alloc_migration_target(src, (unsigned long)mtc);
-	if (dst)
+	if (dst) {
+		mtc->nmask = allowed_mask;
 		return dst;
+	}
 
 	mtc->gfp_mask &= ~__GFP_THISNODE;
 	mtc->nmask = allowed_mask;
-- 
2.39.3

On Wed, Mar 26, 2025 at 11:12:18AM +0800, wangchuanguo wrote:
> If memory is successfully allocated on the target node and the
> function directly returns without value restore for nmask,
> non-first migration operations in migrate_pages() by again label
> may ignore the nmask settings, thereby allowing new memory
> allocations for migration on any node.
> 
> Signed-off-by: wangchuanguo <wangchuanguo@inspur.com>

Unless I am missing something this looks reasonable, but I whonder why nobody
noticed it before.
It is a path that should be pretty exercised.


-- 
Oscar Salvador
SUSE Labs

On Wed, Mar 26, 2025 at 11:12:18AM +0800, wangchuanguo wrote:
> If memory is successfully allocated on the target node and the
> function directly returns without value restore for nmask,
> non-first migration operations in migrate_pages() by again label
> may ignore the nmask settings, thereby allowing new memory
> allocations for migration on any node.

I have no opinion on whether this is the right thing to do or not, but
if it is:

> +++ b/mm/vmscan.c
> @@ -1026,8 +1026,10 @@ struct folio *alloc_migrate_folio(struct folio *src, unsigned long private)
>  	mtc->nmask = NULL;
>  	mtc->gfp_mask |= __GFP_THISNODE;
>  	dst = alloc_migration_target(src, (unsigned long)mtc);
> -	if (dst)
> +	if (dst) {
> +		mtc->nmask = allowed_mask;
>  		return dst;
> +	}
>  
>  	mtc->gfp_mask &= ~__GFP_THISNODE;
>  	mtc->nmask = allowed_mask;

this should be:

	dst = alloc_migration_target(src, (unsigned long)mtc);
+	mtc->nmask = allowed_mask;
	if (dst)
		return dst;

	mtc->gfp_mask &= ~__GFP_THISNODE;
-	mtc->nmask = allowed_mask;