[PATCH] mm/migrate: snapshot the source nid before the misplaced migration

[PATCH] mm/migrate: snapshot the source nid before the misplaced migration

[Yafang Shao]

A successful migration frees the source folio, so the folio_nid() read
in the promotion counter update can pick up a reallocated page's nid.
Snapshot the nid before the migration, next to the memcg snapshot the
function already takes.

Fixes: e39bb6be9f2b ("NUMA Balancing: add page promotion counter")
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: "Huang, Ying" <ying.huang@intel.com>
---
 mm/migrate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

---

This was found while implementing the BPF-NUMAB series. No real issues occurred.

diff --git a/mm/migrate.c b/mm/migrate.c
index 15b45832bcfa..91ba064e1bf4 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2757,6 +2757,7 @@ int migrate_misplaced_folio(struct folio *folio, int node)
 	pg_data_t *pgdat = NODE_DATA(node);
 	int nr_remaining;
 	unsigned int nr_succeeded;
+	int src_node = folio_nid(folio);
 	LIST_HEAD(migratepages);
 	struct mem_cgroup *memcg = get_mem_cgroup_from_folio(folio);
 	struct lruvec *lruvec = mem_cgroup_lruvec(memcg, pgdat);
@@ -2771,7 +2772,7 @@ int migrate_misplaced_folio(struct folio *folio, int node)
 		count_vm_numa_events(NUMA_PAGE_MIGRATE, nr_succeeded);
 		count_memcg_events(memcg, NUMA_PAGE_MIGRATE, nr_succeeded);
 		if ((sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING)
-		    && !node_is_toptier(folio_nid(folio))
+		    && !node_is_toptier(src_node)
 		    && node_is_toptier(node))
 			mod_lruvec_state(lruvec, PGPROMOTE_SUCCESS, nr_succeeded);
 	}
-- 
2.52.0

Re: [PATCH] mm/migrate: snapshot the source nid before the misplaced migration

[Matthew Wilcox]

On Fri, Oct 23, 2026 at 10:32:26AM +0800, Yafang Shao wrote:
> A successful migration frees the source folio, so the folio_nid() read
> in the promotion counter update can pick up a reallocated page's nid.
> Snapshot the nid before the migration, next to the memcg snapshot the
> function already takes.

I don't understand how a node ID can change even after the folio has
been freed.  I don't think this patch is wrong, but I don't think
there's actually a bug here that's being fixed.

Re: [PATCH] mm/migrate: snapshot the source nid before the misplaced migration

[Yafang Shao]

On Fri, Oct 23, 2026 at 11:29 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Fri, Oct 23, 2026 at 10:32:26AM +0800, Yafang Shao wrote:
> > A successful migration frees the source folio, so the folio_nid() read
> > in the promotion counter update can pick up a reallocated page's nid.
> > Snapshot the nid before the migration, next to the memcg snapshot the
> > function already takes.
>
> I don't understand how a node ID can change even after the folio has
> been freed.  I don't think this patch is wrong, but I don't think
> there's actually a bug here that's being fixed.

You are right.
The node bits in page->flags encode the physical node and the
allocator does not touch them. There is no observable bug here.

-- 
Regards
Yafang

Re: [PATCH] mm/migrate: snapshot the source nid before the misplaced migration

[Zi Yan]

On Thu Oct 22, 2026 at 11:28 PM EDT, Matthew Wilcox wrote:
> On Fri, Oct 23, 2026 at 10:32:26AM +0800, Yafang Shao wrote:
>> A successful migration frees the source folio, so the folio_nid() read
>> in the promotion counter update can pick up a reallocated page's nid.
>> Snapshot the nid before the migration, next to the memcg snapshot the
>> function already takes.
>
> I don't understand how a node ID can change even after the folio has
> been freed.  I don't think this patch is wrong, but I don't think

I agree with you in the current code base.

> there's actually a bug here that's being fixed.

Strictly speaking, it can be seen as a benign UAF. It works, because

1. folio->flags happens to be the same as page->flags,
2. folio/page nid is stored in the ->flags and never changed.

In memdesc world, after a folio is freed, I assume folio->flags would be
zeroed since struct folio is freed and calling folio_nid() on a freed
folio would give a bogus result (yeah, it can be mitigated by not
zeroing node field and each node has its own struct folio pool, but that
makes folio_nid() implementation dependent).

IMHO, this patch is still worth getting in.

-- 
Best Regards,
Yan, Zi

Re: [PATCH] mm/migrate: snapshot the source nid before the misplaced migration

[Yafang Shao]

On Fri, Oct 23, 2026 at 10:11 PM Zi Yan <ziy@nvidia.com> wrote:
>
> On Thu Oct 22, 2026 at 11:28 PM EDT, Matthew Wilcox wrote:
> > On Fri, Oct 23, 2026 at 10:32:26AM +0800, Yafang Shao wrote:
> >> A successful migration frees the source folio, so the folio_nid() read
> >> in the promotion counter update can pick up a reallocated page's nid.
> >> Snapshot the nid before the migration, next to the memcg snapshot the
> >> function already takes.
> >
> > I don't understand how a node ID can change even after the folio has
> > been freed.  I don't think this patch is wrong, but I don't think
>
> I agree with you in the current code base.
>
> > there's actually a bug here that's being fixed.
>
> Strictly speaking, it can be seen as a benign UAF. It works, because
>
> 1. folio->flags happens to be the same as page->flags,
> 2. folio/page nid is stored in the ->flags and never changed.
>
> In memdesc world, after a folio is freed, I assume folio->flags would be
> zeroed since struct folio is freed and calling folio_nid() on a freed
> folio would give a bogus result (yeah, it can be mitigated by not
> zeroing node field and each node has its own struct folio pool, but that
> makes folio_nid() implementation dependent).
>
> IMHO, this patch is still worth getting in.

Thanks for the careful review. I will refine the commit message
and send a new version.

-- 
Regards
Yafang

Re: [PATCH] mm/migrate: snapshot the source nid before the misplaced migration

[Matthew Wilcox]

On Fri, Oct 23, 2026 at 10:11:34AM -0400, Zi Yan wrote:
> On Thu Oct 22, 2026 at 11:28 PM EDT, Matthew Wilcox wrote:
> > On Fri, Oct 23, 2026 at 10:32:26AM +0800, Yafang Shao wrote:
> >> A successful migration frees the source folio, so the folio_nid() read
> >> in the promotion counter update can pick up a reallocated page's nid.
> >> Snapshot the nid before the migration, next to the memcg snapshot the
> >> function already takes.
> >
> > I don't understand how a node ID can change even after the folio has
> > been freed.  I don't think this patch is wrong, but I don't think
> 
> I agree with you in the current code base.
> 
> > there's actually a bug here that's being fixed.
> 
> Strictly speaking, it can be seen as a benign UAF. It works, because
> 
> 1. folio->flags happens to be the same as page->flags,
> 2. folio/page nid is stored in the ->flags and never changed.
> 
> In memdesc world, after a folio is freed, I assume folio->flags would be
> zeroed since struct folio is freed and calling folio_nid() on a freed
> folio would give a bogus result (yeah, it can be mitigated by not
> zeroing node field and each node has its own struct folio pool, but that
> makes folio_nid() implementation dependent).

Oh, once struct folio is separately allocated, it's a UAF.  It could be
recycled to a different NID or maybe the slab containing the folio could
be freed (assuming we're not holding the RCU read lock at this point in
migration, and I don't see why we would be).

With a better commit message, I have no objection to this patch.