Re: [PATCH v5 7/9] drivers/base/memory: count inherited poisoned frames into the block

[Breno Leitao <leitao@debian.org>]

Hello David,

On Tue, Sep 22, 2026 at 01:33:51PM +0200, David Hildenbrand (Arm) wrote:
> On 9/21/26 16:31, Breno Leitao wrote:
> > On Fri, Sep 18, 2026 at 10:16:25PM +0200, David Hildenbrand (Arm) wrote:
> >> On 9/18/26 17:22, Breno Leitao wrote:
> >>>
> >>> Right, we have two source for poisoned page information, today.
> >>>
> >>> 1) LINUX_EFI_POISONED_MEMORY: Used to track memory block that got
> >>>    poisioned, and will be passed around during kexec.
> >>> 2) PG_hwpoison on struct page: Used by the memory subsystem to avoid
> >>>    touching it.
> >>
> >> How are both kept in sync? See below.
> > 
> > The EFI table is only written when there is a memory failure. That is
> > the only thing that writes to it:
> > 
> > 	action_result() -> efi_hwpoison_record_pfn() -> set_bit()
> > 
> > You can see it on patch "mm/memory-failure: efi: record
> > hardware-poisoned frames into the poisoned-memory table"
> > 
> > Then, when the kernel kexecs into a second kernel, the EFI config table
> > is queried and the pages are poisoned from it at boot, as they are
> > getting into the buddy allocator, in __free_pages_core().
> 
> I am not sure that is really the right place. That means we only poison free
> memory.

Right, and that is a real gap. A frame only reaches __free_pages_core()
if it is handed to the buddy allocator, so anything this kernel already
took from memblock -- the memmap itself, early page tables, percpu --
can be sitting on a recorded frame with nobody noticing.

> Shouldn't we poison as soon as we initialize the memmap, and check
> whether any memblock allocations ended up on that poisoned memory and bail out?


That is close to what I had until v2, where the bitmap was walked at EFI
parse time and every set unit was memblock_reserve()d:

	for (off = 0; off < bitmap_size; off += PAGE_SIZE) {
		...
		for_each_set_bit(bit, map, nbits) {
			u64 unit = off * BITS_PER_BYTE + bit;

			memblock_reserve(phys_base + unit * unit_size, unit_size);
			nr_units++;
		}
		...
	}

Set the full v2 patch at
https://lore.kernel.org/all/20260821-hwpoison-kho-v2-6-5743791e48e6@debian.org/

Kiryl raised some concerns, such as x86 does not keep memblock past
boot, and kexec picks placement from iomem_resource via
walk_system_ram_res(), which memblock_reserve() does not touch. So the
frames stay out of buddy, but the next image can still be placed on
them, and since nothing sets PG_hwpoison none of the existing hwpoison
users see them either.

Most of the discussion happened here:
https://lore.kernel.org/all/aog_uH5fTsJzXjjx@thinkstation/

v3 then poisoned the frames from mm_core_init(), right after
memblock_free_all(), taking them back off buddy. That breaks with
CONFIG_DEFERRED_STRUCT_PAGE_INIT, where the struct pages above
first_deferred_pfn do not exist yet at that point, and Kiryl's other
point was that the shape was wrong regardless: flag the frame on its
first add to buddy, not after buddy is up.

See the discussion at:
https://lore.kernel.org/all/apGbDOnPm5VXP5ez@thinkstation/

Which is how it ended up in __free_pages_core().

> >> The inconsistency is what worries me.
> >>
> >> And that we have pages we are told are hwpoisoned but we seem to ignore that and
> >> carry on with our kernel letting it boot?
> > 
> > The walk exists precisely because of that inconsistency: the counter has
> > to match the frames that actually carry the flag, and the bitmap says
> > more than that.
> > 
> > If we drop the page walk, then there is change for inconsistency, but,
> > as-is, there is no inconsistency.
> > 
> > So, I am planning to keep the page walk above, when there is a poisoned
> > page in the memory block, avoiding any inconsitency.
>
> How about we keep it very simple and don't mix information from two different
> sources? That is, remove that bitmap scan here entirely. We should process the
> bitmap exactly once when initializing the memmap.
> 
> From that point on, the memmap should be our reliable source of information.

Sure, is this approach better?

  - the bitmap is read as the struct pages are initialised, and the only
    thing it does there is SetPageHWPoison();

  - __free_pages_core() keeps the hook but tests PageHWPoison() instead
    of the table, so a recorded frame is still held back before it can
    enter buddy;

  - nothing else queries the table.

One caveat on "exactly once": if that means one pass at one point in
boot, it is what v3[1] did -- hwpoison_init_boot() from mm_core_init(),
right after memblock_free_all() -- and it does not work. With
CONFIG_DEFERRED_STRUCT_PAGE_INIT the struct pages above
first_deferred_pfn are not there yet at that point.

Link: https://lore.kernel.org/all/20260826-hwpoison-kho-v3-5-6f79c4b605bc@debian.org/

So I _think_ we want to do it once per frame, as that frame's struct
page comes up. __init_single_page() is the single function they all pass
through. Is __init_single_page() the right place to query the bitmap and
mark is as poisoned?

> For this code here, just remember globally whether we hwpoisoned any page. If
> so, just walk the memmap. If not (the 99.9999% of all systems, no need to scan
> anything).

Yes, and the counter already exists: num_poisoned_pages and we can query
it. I am also planning to add something similar in the EFI table, like
an ->empty field, which will be cleared when a bit is set, avoiding
doing bitmap walk on the happy path (not a single poisoned page).

Anyway, range_hwpoison() short-circuits on it today,

      if (!size || !atomic_long_read(&num_poisoned_pages))
              return poison;

so memblk_nr_poison_init() can gate on the same thing and walk the
memmap, with no EFI call left in drivers/base/memory.c at all. That
disposes of the misleading name there too, which was where this
started.

Now, the half I am less sure about is checking whether a memblock
allocation landed on a recorded frame. Poisoning at memmap init cannot
prevent that -- the memmap is itself a memblock allocation, so the
struct pages do not exist while the early allocations are being made --
it can only detect it afterwards, as a pass over memblock.reserved
before memblock_free_all().

The alternative is to bring v2's memblock_reserve() back purely to keep
the early allocator off those frames, and still poison at memmap init
so the flag is there for everyone else. That gets both properties, at
the cost of frames that are Reserved and HWPoison and never in buddy,
which I have not thought through for unpoison.

Thanks for the suggestions!
--breno