Re: [PATCH 0/6] mm: fix pfn walker races with memory offline

["David Hildenbrand (Arm)" <david@kernel.org>]

On 10/27/26 12:31, Gregory Price (Meta) wrote:
> On 2026-10-27 09:57 +0100, David Hildenbrand (Arm) wrote:
>> On 10/27/26 05:21, Gregory Price (Meta) wrote:
>>> Several pfn walkers check that a struct page is online, then use it
>>> after memory offline has poisoned it (CONFIG_DEBUG_VM) or removal has
>>> freed the vmemmap.  Five of the races crash the kernel; the sixth leaves
>>> an offline spinning with the device hotplug lock held.
>>
>> I have roughly the exact same list of AI garbage fixes here from a certain company.
>>
>> And I consider this all extremely low priority.
>>
>> So this is something to discuss throughout the next months and do it properly.
>>
> 
> I actually have a reason for these fixes other than "bot said bad".

So you found them on production kernels?

We did not have a single real report of that for probably a decade or
even longer in production.

Recently, we had the single first report about the compaction case from a
stress test (no production):

https://lore.kernel.org/all/20260903095543.3752338-1-yuan1.liu@intel.com/

"We are not sure whether this race is reachable under realistic
workloads or only under this synthetic stress test."

Where I motivated that we need something clean.

> 
> We leverage ballooning and hotplugging as VM memory overcommit tools
> and are finding that some of these race conditions crash them with
> large MTBF.

yeah, that makes sense.

> 
> While most of these are insanely narrow, they are not as unlikely to
> happen as you think. A vCPU can take a vmexit in these windows much
> more easily than a host can take an NMI or other interrupt.

Jup, that's what I would expect.

> 
> Thankfully none of these (or the other series) appear to be obvious
> security issues, but they are stability issues.

Right. And DEBUG_VM reports are irrelevant in production.

The thing is that the race between offlining and actual memory removal
is extremely long. Poisoning is just a mechanism to find these theoretical
issues that could cause issues faster.

And with all the other things going on, this has lower priority, and I
wanted to get back to the AI patches I have once the other thing is sorted.

> 
> I have a reproducer for every single one of these (and the other
> series) - but a handful are unreachable due to a 2-statement read
> being emitted as a single read by the compiler (which can be undone
> by a targeted compiler flag). Those were included for completeness.


In general, I think we want something that will protect all
pfn_to_online_page() + zone->contig thingies, not just selected ones.
We discussed in the past using some sort of rcu for that.

get_online_mems is usually the wrong approach.

The infrastructure for that should be added in a clean separate patch, not
mangled into a patch that fixes a DEBUG_VM splat.

This is nothing to rush in or to schlop around.

The AI patch for compaction I have here does something similar to your first
patch, with a bit less churn, and converts a handful more users.

E.g.,


+/*
+ * Nothing keeps a section online once pfn_to_online_page() has returned. A pfn
+ * walker that holds no reference to the page does the check and looks at the
+ * memmap under this guard: remove_pfn_range_from_zone() waits for it before the
+ * memmap is poisoned and can be freed. That wait cannot be interrupted, and
+ * whatever a walker sleeps on under the guard, memory offlining waits for as
+ * well: leave the guard before copying to user space. mem_hotplug_lock is held
+ * during the wait: never take it under the guard.
+ */
+extern struct srcu_struct memmap_srcu;
+#define memmap_walk_guard()    guard(srcu)(&memmap_srcu)


and then


diff --git a/fs/proc/page.c b/fs/proc/page.c
index 260772b20bd99..c5f6d7fe12d9a 100644
--- a/fs/proc/page.c
+++ b/fs/proc/page.c
@@ -58,13 +58,39 @@ static u64 get_kpage_count(const struct page *page)
        return ret;
 }

+static u64 kpage_info(unsigned long pfn, enum kpage_operation op)
+{
+       struct page *page;
+
+       /* The page is not ours: keep its memmap until we are done with it. */
+       memmap_walk_guard();
+
+       /*
+        * TODO: ZONE_DEVICE support requires to identify
+        * memmaps that were actually initialized.
+        */
+       page = pfn_to_online_page(pfn);
+       if (!page)
+               return 0;
+
+       switch (op) {
+       case KPAGE_FLAGS:
+               return stable_page_flags(page);
+       case KPAGE_COUNT:
+               return get_kpage_count(page);
+       case KPAGE_CGROUP:
+               return page_cgroup_ino(page);
+       default:
+               return 0;
+       }
+}
+
 static ssize_t kpage_read(struct file *file, char __user *buf,
                size_t count, loff_t *ppos,
                enum kpage_operation op)
 {
        const unsigned long max_dump_pfn = get_max_dump_pfn();
        u64 __user *out = (u64 __user *)buf;
-       struct page *page;
        unsigned long src = *ppos;
        unsigned long pfn;
        ssize_t ret = 0;
@@ -78,29 +104,7 @@ 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) {
-               /*
-                * TODO: ZONE_DEVICE support requires to identify
-                * memmaps that were actually initialized.
-                */
-               page = pfn_to_online_page(pfn);
-
-               if (page) {
-                       switch (op) {
-                       case KPAGE_FLAGS:
-                               info = stable_page_flags(page);
-                               break;
-                       case KPAGE_COUNT:
-                               info = get_kpage_count(page);
-                               break;
-                       case KPAGE_CGROUP:
-                               info = page_cgroup_ino(page);
-                               break;
-                       default:
-                               info = 0;
-                               break;
-                       }
-               } else
-                       info = 0;
+               info = kpage_info(pfn, op);

                if (put_user(info, out)) {


protecting individual pfn_to_online_page is just whack a mole. Likely we want a
way to check whether pfn_to_online_page() is called from the wrong context, and
maybe we need a variant that can be called from "wrong context" for a good reason.

-- 
Cheers,

David