Re: [PATCH 1/4] mm,hwpoison: Take free pages off the buddy freelists
From: Oscar Salvador
Date: Fri Sep 04 2020 - 07:40:59 EST
On Fri, Sep 04, 2020 at 02:02:05AM +0000, HORIGUCHI NAOYA(堀口 直也) wrote:
> 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.
Yeah, I figured we would need something like:
action_result(pfn, MF_MSG_BUDDY_2ND, MF_FAILED) ?
Although checking other parts of the code, MF_IGNORED can suit here as well,
and it is probably better.
> 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?
I __think__ we do not need the following anymore:
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;
}
We have this block because shake_page() (among other things) sends pcp-pages
back to the buddy freelists, so we could have the chance to handle the page.
Since we will be doing that in get_hwpoison_page, and such scenario should
have been handled in the block from above, I think it is safe to skip that
block.
But I think that the skape_page() has to stay as it flushes pages from pagevecs
to their respective LRU lists, otherwise we could do the wrong thing in:
if (!PageTransTail(p) && !PageLRU(p))
goto identify_page_state;
So, I will send V2 with a 5th patch cleaning that up as I do not want to get
it convulated with the fix itself.
thanks Naoya
--
Oscar Salvador
SUSE L3