Re: [PATCH 32/46] mm/memory_hotplug: avoid races in pfn_to_online_page() with dying pgmap

[Lance Yang <lance.yang@linux.dev>]

On Tue, Oct 27, 2026 at 12:17:52AM +0100, David Hildenbrand (Arm) wrote:
>From: Josef Bacik <josef@toxicpanda.com>
>
>If ZONE_DEVICE memory shares a memory section with ordinary memory, the
>section is online and the pfns of the device are valid.
>pfn_to_online_page() tells them apart by asking get_dev_pagemap() whether
>the pfn belongs to a dev_pagemap.
>
>get_dev_pagemap() only returns a dev_pagemap whose reference is alive.
>memunmap_pages() kills the reference first, waits until all pages are
>idle, and then removes the memmap. During all that time the function
>finds nothing, and pfn_to_online_page() hands out the struct pages of the
>device as if they were online memory. A pfn walker reads them while they
>are poisoned and freed.
>
>The entry in pgmap_array is only deleted after the pages have been
>removed. Look at that instead of trying to get a reference.
>
>This is probably not worth CCing stable.
>
>Fix generated by an LLM and sent by Josef Bacik in private.
>
>Fixes: 1f90a3477df3 ("mm: teach pfn_to_online_page() about ZONE_DEVICE section collisions")
>Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
>---
> include/linux/memremap.h |  6 ++++++
> mm/memory_hotplug.c      | 13 +++++--------
> mm/memremap.c            | 13 +++++++++++++
> 3 files changed, 24 insertions(+), 8 deletions(-)
>
>diff --git a/include/linux/memremap.h b/include/linux/memremap.h
>index e3c2ccf872a8a..b5e5e626c7a59 100644
>--- a/include/linux/memremap.h
>+++ b/include/linux/memremap.h
>@@ -231,6 +231,7 @@ void memunmap_pages(struct dev_pagemap *pgmap);
> void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap);
> void devm_memunmap_pages(struct device *dev, struct dev_pagemap *pgmap);
> struct dev_pagemap *get_dev_pagemap(unsigned long pfn);
>+bool pfn_has_dev_pagemap(unsigned long pfn);
> bool pgmap_pfn_valid(struct dev_pagemap *pgmap, unsigned long pfn);
> 
> unsigned long memremap_compat_align(void);
>@@ -284,6 +285,11 @@ static inline struct dev_pagemap *get_dev_pagemap(unsigned long pfn)
> 	return NULL;
> }
> 
>+static inline bool pfn_has_dev_pagemap(unsigned long pfn)
>+{
>+	return false;
>+}
>+
> static inline bool pgmap_pfn_valid(struct dev_pagemap *pgmap, unsigned long pfn)
> {
> 	return false;
>diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>index 796af1028ee23..5a972d7a9e478 100644
>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -342,7 +342,6 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages)
> struct page *pfn_to_online_page(unsigned long pfn)
> {
> 	unsigned long nr = pfn_to_section_nr(pfn);
>-	struct dev_pagemap *pgmap;
> 	struct mem_section *ms;
> 
> 	BUILD_BUG_ON(IS_ENABLED(CONFIG_HAVE_ARCH_PFN_VALID));
>@@ -363,14 +362,12 @@ struct page *pfn_to_online_page(unsigned long pfn)
> 	/*
> 	 * Slowpath: when ZONE_DEVICE collides with
> 	 * ZONE_{NORMAL,MOVABLE} within the same section some pfns in
>-	 * the section may be 'offline' but 'valid'. Only
>-	 * get_dev_pagemap() can determine sub-section online status.
>+	 * the section may be 'offline' but 'valid'. Only the presence of a
>+	 * pgmap can determine sub-section online status: it indicates a
>+	 * ZONE_DEVICE offline pfn. That includes a pgmap whose reference has
>+	 * been killed, its memmap is about to go away.
> 	 */
>-	pgmap = get_dev_pagemap(pfn);
>-	put_dev_pagemap(pgmap);
>-
>-	/* The presence of a pgmap indicates ZONE_DEVICE offline pfn */
>-	if (pgmap)
>+	if (pfn_has_dev_pagemap(pfn))
> 		return NULL;
> 
> 	return pfn_to_page(pfn);


LLM stumbled upon a possible race in kpage_read() ... 

Say the reader passes pfn_section_valid() for a device PFN in a section
shared with online RAM, gets preempted, and resumes after
pgmap_array_delete(). We'd then find no pgmap and return pfn_to_page(pfn),
even though the subsection is no longer valid ...

Could we take the memory hotplug read lock in kpage_read(), covering both
the lookup and the metadata reads? We can drop it before put_user(), once
we're done with the page.

Maybe along these lines?

---8<---
diff --git a/fs/proc/page.c b/fs/proc/page.c
index 6aa99e42ec22..12bed36d76dc 100644
--- a/fs/proc/page.c
+++ b/fs/proc/page.c
@@ -12,6 +12,7 @@
 #include <linux/hugetlb.h>
 #include <linux/memremap.h>
 #include <linux/memcontrol.h>
+#include <linux/memory_hotplug.h>
 #include <linux/mmu_notifier.h>
 #include <linux/page_idle.h>
 #include <linux/kernel-page-flags.h>
@@ -77,6 +78,8 @@ static ssize_t kpage_read(struct file *file, char __user *buf,
 	count = min_t(unsigned long, count, (max_dump_pfn * KPMSIZE) - src);

 	while (count > 0) {
+		/* Prevent memory removal while inspecting the memmap. */
+		get_online_mems();
 		/*
 		 * TODO: ZONE_DEVICE support requires to identify
 		 * memmaps that were actually initialized.
@@ -101,6 +104,8 @@ static ssize_t kpage_read(struct file *file, char __user *buf,
 		} else
 			info = 0;

+		put_online_mems();
+
 		if (put_user(info, out)) {
 			ret = -EFAULT;
 			break;
--

Cheers, Lance