linux-kernel - Re: [PATCH] mm/hwpoison: fix race between soft_offline_page and unpoison

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150814072649.GA31021@hori1.linux.bs1.fc.nec.co.jp>
Date:	Fri, 14 Aug 2015 07:26:50 +0000
From:	Naoya Horiguchi <n-horiguchi@...jp.nec.com>
To:	Wanpeng Li <wanpeng.li@...mail.com>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm/hwpoison: fix race between soft_offline_page and
 unpoison_memory

On Fri, Aug 14, 2015 at 01:03:53PM +0800, Wanpeng Li wrote:
> On 8/14/15 12:19 PM, Naoya Horiguchi wrote:
...
> >>>>> If I read correctly, the old migratetype approach has a few problems:
> >>>>>   1) it doesn't fix the problem completely, because
> >>>>>      set_migratetype_isolate() can fail to set MIGRATE_ISOLATE to the
> >>>>>      target page if the pageblock of the page contains one or more
> >>>>>      unmovable pages (i.e. has_unmovable_pages() returns true).
> >>>>>   2) the original code changes migratetype to MIGRATE_ISOLATE forcibly,
> >>>>>      and sets it to MIGRATE_MOVABLE forcibly after soft offline, regardless
> >>>>>      of the original migratetype state, which could impact other subsystems
> >>>>>      like memory hotplug or compaction.
> >>>> Maybe we can add a "FIXME" comment on the Migratetype stuff, since the
> >>>> current linus tree calltrace and it should be fixed immediately, and I
> >>>> don't see obvious bugs appear on migratetype stuffs at least currently,
> >>>> so "FIXME" is enough. :-)
> >>> Sorry if confusing, but my intention in saying about "FIXME" comment was
> >>> that we can find another solution for this race rather than just reverting,
> >>> so adding comment about the reported bug in current code (keeping code from
> >>> 4491f712606) is OK for very short term.
> >>> I understand that leaving a race window of BUG_ON is not the best thing, but
> >>> as I said, this race shouldn't affect end-users, so this is not an urgent bug.
> >>> # It's enough if testers know this.
> >> The 4.2 is coming, this patch can be applied as a temporal solution in
> >> order to fix the broken linus tree, and the any final solution can be
> >> figured out later.
> > I didn't reproduce this problem yet in my environment, but from code reading
> > I guess that checking PageHWPoison flag in unmap_and_move() like below could
> > avoid the problem. Could you testing with this, please?
> 
> I have already try to modify unmap_and_move() the same as what you do
> before I post migratetype stuff. It doesn't work and have other calltrace.

OK, then I rethink of handling the race in unpoison_memory().

Currently properly contained/hwpoisoned pages should have page refcount 1
(when the memory error hits LRU pages or hugetlb pages) or refcount 0
(when the memory error hits the buddy page.) And current unpoison_memory()
implicitly assumes this because otherwise the unpoisoned page has no place
to go and it's just leaked.
So to avoid the kernel panic, adding prechecks of refcount and mapcount
to limit the page to unpoison for only unpoisonable pages looks OK to me.
The page under soft offlining always has refcount >=2 and/or mapcount > 0,
so such pages should be filtered out.

Here's a patch. In my testing (run soft offline stress testing then repeat
unpoisoning in background,) the reported (or similar) bug doesn't happen.
Can I have your comments?

Thanks,
Naoya Horiguchi
---
From: Naoya Horiguchi <n-horiguchi@...jp.nec.com>
Subject: [PATCH] mm/hwpoison: don't unpoison for pinned or mapped page

Signed-off-by: Naoya Horiguchi <n-horiguchi@...jp.nec.com>
---
 mm/memory-failure.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index d1f85f6278ee..c6f14d2cd919 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1442,6 +1442,16 @@ int unpoison_memory(unsigned long pfn)
 		return 0;
 	}
 
+	if (page_count(page) > 1) {
+		pr_info("MCE: Someone grabs the hwpoison page %#lx\n", pfn);
+		return 0;
+	}
+
+	if (page_mapped(page)) {
+		pr_info("MCE: Someone maps the hwpoison page %#lx\n", pfn);
+		return 0;
+	}
+
 	/*
 	 * unpoison_memory() can encounter thp only when the thp is being
 	 * worked by memory_failure() and the page lock is not held yet.
-- 
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/