On Wed, Sep 02, 2020 at 11:45:07AM +0200, Oscar Salvador wrote: > The crux of the matter is that historically we left poisoned pages > in the buddy system because we have some checks in place when > allocating a page that a gatekeeper for poisoned pages. > Unfortunately, we do have other users (e.g: compaction [1]) that scan > buddy freelists and try to get a page from there without checking > whether the page is HWPoison. > > As I stated already, I think it is fundamentally wrong to keep > HWPoison pages within the buddy systems, checks in place or not. > > Let us fix this we same way we did for soft_offline [2], and take > the page off the buddy freelist, so it is completely unreachable. > > Note that this is fairly simple to trigger, as we only need > to poison free buddy pages (madvise MADV_HWPOISON) and then we need > to run some sort of memory stress system. > > Just for a matter of reference, I put a dump_page in compaction_alloc > to trigger for HWPoison patches: > > kernel: page:0000000012b2982b refcount:1 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x1d5db > kernel: flags: 0xfffffc0800000(hwpoison) > kernel: raw: 000fffffc0800000 ffffea00007573c8 ffffc90000857de0 0000000000000000 > kernel: raw: 0000000000000001 0000000000000000 00000001ffffffff 0000000000000000 > kernel: page dumped because: compaction_alloc > > kernel: CPU: 4 PID: 123 Comm: kcompactd0 Tainted: G E 5.9.0-rc2-mm1-1-default+ #5 > kernel: Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014 > kernel: Call Trace: > kernel: dump_stack+0x6d/0x8b > kernel: compaction_alloc+0xb2/0xc0 > kernel: migrate_pages+0x2a6/0x12a0 > kernel: ? isolate_freepages+0xc80/0xc80 > kernel: ? __ClearPageMovable+0xb0/0xb0 > kernel: compact_zone+0x5eb/0x11c0 > kernel: ? finish_task_switch+0x74/0x300 > kernel: ? lock_timer_base+0xa8/0x170 > kernel: proactive_compact_node+0x89/0xf0 > kernel: ? kcompactd+0x2d0/0x3a0 > kernel: kcompactd+0x2d0/0x3a0 > kernel: ? finish_wait+0x80/0x80 > kernel: ? kcompactd_do_work+0x350/0x350 > kernel: kthread+0x118/0x130 > kernel: ? kthread_associate_blkcg+0xa0/0xa0 > kernel: ret_from_fork+0x22/0x30 Thanks for spotting this. > After that, if e.g: someone faults in the page, that someone will get killed > unexpectedly. > > [1] https://lore.kernel.org/linux-mm/20190826104144.GA7849@linux/T/#u > [2] https://patchwork.kernel.org/patch/11694847/ > > Signed-off-by: Oscar Salvador <osalvador@xxxxxxx> > --- > mm/memory-failure.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 696505f56910..d349dcb45056 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -1325,6 +1325,7 @@ int memory_failure(unsigned long pfn, int flags) > struct dev_pagemap *pgmap; > int res; > unsigned long page_flags; > + bool retry = true; > > if (!sysctl_memory_failure_recovery) > panic("Memory failure on page %lx", pfn); > @@ -1364,10 +1365,20 @@ int memory_failure(unsigned long pfn, int flags) > * In fact it's dangerous to directly bump up page count from 0, > * that may make page_ref_freeze()/page_ref_unfreeze() mismatch. > */ > +try_again: > if (!(flags & MF_COUNT_INCREASED) && !get_hwpoison_page(p)) { > if (is_free_buddy_page(p)) { > - action_result(pfn, MF_MSG_BUDDY, MF_DELAYED); > - return 0; > + if (take_page_off_buddy(p)) { > + action_result(pfn, MF_MSG_BUDDY, MF_DELAYED); > + return 0; > + } else { > + /* We lost the race, try again */ > + if (retry) { > + retry = false; > + goto try_again; > + } You might need add calling action_result() here. > + return -EBUSY; > + } > } else { > action_result(pfn, MF_MSG_KERNEL_HIGH_ORDER, MF_IGNORED); > return -EBUSY; And the following block also handles buddy pages, so you could also call take_page_off_buddy() here? /* * We ignore non-LRU pages for good reasons. * - PG_locked is only well defined for LRU pages and a few others * - to avoid races with __SetPageLocked() * - to avoid races with __SetPageSlab*() (and more non-atomic ops) * The check (unnecessarily) ignores LRU pages being isolated and * walked by the page reclaim code, however that's not a big loss. */ shake_page(p, 0); /* shake_page could have turned it free. */ if (!PageLRU(p) && is_free_buddy_page(p)) { if (flags & MF_COUNT_INCREASED) action_result(pfn, MF_MSG_BUDDY, MF_DELAYED); else action_result(pfn, MF_MSG_BUDDY_2ND, MF_DELAYED); return 0; } Or if get_hwpoison_page() handles retry inside it as will be done in patch 3/4, we don't need this block any more? Thanks, Naoya Horiguchi
