From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3181CC433EF for ; Mon, 15 Nov 2021 19:17:34 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id B5F3160234 for ; Mon, 15 Nov 2021 19:17:33 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org B5F3160234 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 49A0F6B007B; Mon, 15 Nov 2021 14:17:33 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 449226B007D; Mon, 15 Nov 2021 14:17:33 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 311D16B007E; Mon, 15 Nov 2021 14:17:33 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0196.hostedemail.com [216.40.44.196]) by kanga.kvack.org (Postfix) with ESMTP id 21E496B007B for ; Mon, 15 Nov 2021 14:17:33 -0500 (EST) Received: from smtpin06.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id D18748477D for ; Mon, 15 Nov 2021 19:17:32 +0000 (UTC) X-FDA: 78812123544.06.9AAFB0A Received: from mail-ed1-f52.google.com (mail-ed1-f52.google.com [209.85.208.52]) by imf06.hostedemail.com (Postfix) with ESMTP id 38C16801A89B for ; Mon, 15 Nov 2021 19:17:32 +0000 (UTC) Received: by mail-ed1-f52.google.com with SMTP id w1so10461056edc.6 for ; Mon, 15 Nov 2021 11:17:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=V0Ve19ZRiN+HEb7FgnykGLArE+cyMvKMte5Sm1A4UfI=; b=Jv5zNde4KksxoMpqcJyhkUsu4MRIuH7stt6fPPHWuK8jIc3i8AJ27AcqMeRPXjydIT 2BmTSSZFcwnUwwo66ChRKtxZOgTB4LA89yZBybubdcXvXwufok6m5Nhow8gnvQFz4NJG +xj3jxVq0Ao1G6egWCBv/v35fTT96Q3w4dwokgm39V65V2O42PMf4IvQkmwkx7Z8aYE1 2EpiCrGpPluMrCCAvoJ69dxVG6hox/w5uo5QjfpKQxDDiHGAShVmwTZVU7I2VRW45YXX EjbFNPlQiCB5n2uTM/IoMjddnXGeOxEm/pZ2x/9ZZ4HWWT8vjrTt+odjc+/UEv3MmLDW zzbA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=V0Ve19ZRiN+HEb7FgnykGLArE+cyMvKMte5Sm1A4UfI=; b=0w4W1zil2hdYdhQF9xjbh/ad5qZLQy+bhTG4mbgTQ1TS18Ez1cFMcqjp6xw9JrlNzB /IFLvmNeiYEvj5BNXz4lCHC3k96qcAykcVtOH3MydxFKkGY1Z+JlDyB+y4qr5txU8p8W O/ebNVBzT8gUEusXP0gly8QSWtGDUqQNEtXMcUdc+mrdbUlef5nqZg6JtHHTWyh32QxE +CcfIwfmYWzAoIRh5Qv13qe21xWbXoj9QcaJksMVQGMXpJ12ypjNNNbQime1AyG9wgM6 rV2Htnb70pge27OVd0yOiP4/4PpftgBovGiVvcKc6I+9onGWw59rk0avABDgeUd0rPm+ d7XQ== X-Gm-Message-State: AOAM531QrmaK14eokNj/Wp6xCXJWxOI3OfWairQK9Qy4HPN7GdYcMn6v wQqvRQE2qJ3VcqeY0Dd4UAYK5FgVmgsGXVSbx4OND+VR X-Google-Smtp-Source: ABdhPJzUPWJOp8IzVNkzPDf7iQNrQ5ZYTythcn+H/qWxYk+QKM4pyPvfaDBYxMpzho3PD0hPh2Ev3HMXDFdo5LbAbiM= X-Received: by 2002:a50:a6ca:: with SMTP id f10mr1427178edc.81.1637003452353; Mon, 15 Nov 2021 11:10:52 -0800 (PST) MIME-Version: 1.0 References: <20211115084006.3728254-1-naoya.horiguchi@linux.dev> <20211115084006.3728254-4-naoya.horiguchi@linux.dev> In-Reply-To: <20211115084006.3728254-4-naoya.horiguchi@linux.dev> From: Yang Shi Date: Mon, 15 Nov 2021 11:10:40 -0800 Message-ID: Subject: Re: [PATCH v4 3/3] mm/hwpoison: fix unpoison_memory() To: Naoya Horiguchi Cc: Linux MM , Andrew Morton , David Hildenbrand , Oscar Salvador , Michal Hocko , Ding Hui , Tony Luck , "Aneesh Kumar K.V" , Miaohe Lin , Peter Xu , Naoya Horiguchi , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 38C16801A89B X-Stat-Signature: zeypgmcjmxtkd1if7nbhs3sdu6kh4irk Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=Jv5zNde4; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf06.hostedemail.com: domain of shy828301@gmail.com designates 209.85.208.52 as permitted sender) smtp.mailfrom=shy828301@gmail.com X-HE-Tag: 1637003852-855311 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Nov 15, 2021 at 12:40 AM Naoya Horiguchi wrote: > > From: Naoya Horiguchi > > After recent soft-offline rework, error pages can be taken off from > buddy allocator, but the existing unpoison_memory() does not properly > undo the operation. Moreover, due to the recent change on > __get_hwpoison_page(), get_page_unless_zero() is hardly called for > hwpoisoned pages. So __get_hwpoison_page() highly likely returns -EBUSY > (meaning to fail to grab page refcount) and unpoison just clears > PG_hwpoison without releasing a refcount. That does not lead to a > critical issue like kernel panic, but unpoisoned pages never get back to > buddy (leaked permanently), which is not good. > > To (partially) fix this, we need to identify "taken off" pages from > other types of hwpoisoned pages. We can't use refcount or page flags > for this purpose, so a pseudo flag is defined by hacking ->private > field. Someone might think that put_page() is enough to cancel > taken-off pages, but the normal free path contains some operations not > suitable for the current purpose, and can fire VM_BUG_ON(). > > Note that unpoison_memory() is now supposed to be cancel hwpoison events > injected only by madvise() or /sys/devices/system/memory/{hard,soft}_offline_page, > not by MCE injection, so please don't try to use unpoison when testing > with MCE injection. > > [lkp@intel.com: report build failure for ARCH=i386] > Signed-off-by: Naoya Horiguchi > --- > ChangeLog v4: > - use integer value for MAGIC_HWPOISON to avoid compile error for ARCH=i386 > - close race in unpoison_taken_off_page() Reviewed-by: Yang Shi > > ChangeLog v3: > - fix description > - add PageTable check in unpoison_memory() > - fix return value of clear_page_hwpoison() > - pass page instead head to PageHWPoisonTakenOff check. > - rename take_page_back_buddy() with put_page_back_buddy() > > ChangeLog v2: > - unpoison_memory() returns as commented > - explicitly avoids unpoisoning slab pages > - separates internal pinning function into __get_unpoison_page() > --- > include/linux/mm.h | 1 + > include/linux/page-flags.h | 4 ++ > mm/memory-failure.c | 109 ++++++++++++++++++++++++++++++------- > mm/page_alloc.c | 27 +++++++++ > 4 files changed, 122 insertions(+), 19 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 7941bca871dc..8bbb7205ef9f 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -3220,6 +3220,7 @@ enum mf_flags { > MF_ACTION_REQUIRED = 1 << 1, > MF_MUST_KILL = 1 << 2, > MF_SOFT_OFFLINE = 1 << 3, > + MF_UNPOISON = 1 << 4, > }; > extern int memory_failure(unsigned long pfn, int flags); > extern void memory_failure_queue(unsigned long pfn, int flags); > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > index 52ec4b5e5615..a4fe056910bb 100644 > --- a/include/linux/page-flags.h > +++ b/include/linux/page-flags.h > @@ -522,7 +522,11 @@ PAGEFLAG_FALSE(Uncached, uncached) > PAGEFLAG(HWPoison, hwpoison, PF_ANY) > TESTSCFLAG(HWPoison, hwpoison, PF_ANY) > #define __PG_HWPOISON (1UL << PG_hwpoison) > +#define MAGIC_HWPOISON 0x48575053U /* HWPS */ > +extern void SetPageHWPoisonTakenOff(struct page *page); > +extern void ClearPageHWPoisonTakenOff(struct page *page); > extern bool take_page_off_buddy(struct page *page); > +extern bool put_page_back_buddy(struct page *page); > #else > PAGEFLAG_FALSE(HWPoison, hwpoison) > #define __PG_HWPOISON 0 > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 722036539b44..0f8b798cba69 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -1160,6 +1160,22 @@ static int page_action(struct page_state *ps, struct page *p, > return (result == MF_RECOVERED || result == MF_DELAYED) ? 0 : -EBUSY; > } > > +static inline bool PageHWPoisonTakenOff(struct page *page) > +{ > + return PageHWPoison(page) && page_private(page) == MAGIC_HWPOISON; > +} > + > +void SetPageHWPoisonTakenOff(struct page *page) > +{ > + set_page_private(page, MAGIC_HWPOISON); > +} > + > +void ClearPageHWPoisonTakenOff(struct page *page) > +{ > + if (PageHWPoison(page)) > + set_page_private(page, 0); > +} > + > /* > * Return true if a page type of a given page is supported by hwpoison > * mechanism (while handling could fail), otherwise false. This function > @@ -1262,6 +1278,27 @@ static int get_any_page(struct page *p, unsigned long flags) > return ret; > } > > +static int __get_unpoison_page(struct page *page) > +{ > + struct page *head = compound_head(page); > + int ret = 0; > + bool hugetlb = false; > + > + ret = get_hwpoison_huge_page(head, &hugetlb); > + if (hugetlb) > + return ret; > + > + /* > + * PageHWPoisonTakenOff pages are not only marked as PG_hwpoison, > + * but also isolated from buddy freelist, so need to identify the > + * state and have to cancel both operations to unpoison. > + */ > + if (PageHWPoisonTakenOff(page)) > + return -EHWPOISON; > + > + return get_page_unless_zero(page) ? 1 : 0; > +} > + > /** > * get_hwpoison_page() - Get refcount for memory error handling > * @p: Raw error page (hit by memory error) > @@ -1278,18 +1315,26 @@ static int get_any_page(struct page *p, unsigned long flags) > * extra care for the error page's state (as done in __get_hwpoison_page()), > * and has some retry logic in get_any_page(). > * > + * When called from unpoison_memory(), the caller should already ensure that > + * the given page has PG_hwpoison. So it's never reused for other page > + * allocations, and __get_unpoison_page() never races with them. > + * > * Return: 0 on failure, > * 1 on success for in-use pages in a well-defined state, > * -EIO for pages on which we can not handle memory errors, > * -EBUSY when get_hwpoison_page() has raced with page lifecycle > - * operations like allocation and free. > + * operations like allocation and free, > + * -EHWPOISON when the page is hwpoisoned and taken off from buddy. > */ > static int get_hwpoison_page(struct page *p, unsigned long flags) > { > int ret; > > zone_pcp_disable(page_zone(p)); > - ret = get_any_page(p, flags); > + if (flags & MF_UNPOISON) > + ret = __get_unpoison_page(p); > + else > + ret = get_any_page(p, flags); > zone_pcp_enable(page_zone(p)); > > return ret; > @@ -1942,6 +1987,28 @@ core_initcall(memory_failure_init); > pr_info(fmt, pfn); \ > }) > > +static inline int clear_page_hwpoison(struct ratelimit_state *rs, struct page *p) > +{ > + if (TestClearPageHWPoison(p)) { > + unpoison_pr_info("Unpoison: Software-unpoisoned page %#lx\n", > + page_to_pfn(p), rs); > + num_poisoned_pages_dec(); > + return 1; > + } > + return 0; > +} > + > +static inline int unpoison_taken_off_page(struct ratelimit_state *rs, > + struct page *p) > +{ > + if (put_page_back_buddy(p)) { > + unpoison_pr_info("Unpoison: Software-unpoisoned page %#lx\n", > + page_to_pfn(p), rs); > + return 0; > + } > + return -EBUSY; > +} > + > /** > * unpoison_memory - Unpoison a previously poisoned page > * @pfn: Page number of the to be unpoisoned page > @@ -1958,9 +2025,7 @@ int unpoison_memory(unsigned long pfn) > { > struct page *page; > struct page *p; > - int freeit = 0; > - int ret = 0; > - unsigned long flags = 0; > + int ret = -EBUSY; > static DEFINE_RATELIMIT_STATE(unpoison_rs, DEFAULT_RATELIMIT_INTERVAL, > DEFAULT_RATELIMIT_BURST); > > @@ -1996,24 +2061,30 @@ int unpoison_memory(unsigned long pfn) > goto unlock_mutex; > } > > - if (!get_hwpoison_page(p, flags)) { > - if (TestClearPageHWPoison(p)) > - num_poisoned_pages_dec(); > - unpoison_pr_info("Unpoison: Software-unpoisoned free page %#lx\n", > - pfn, &unpoison_rs); > + if (PageSlab(page) || PageTable(page)) > goto unlock_mutex; > - } > > - if (TestClearPageHWPoison(page)) { > - unpoison_pr_info("Unpoison: Software-unpoisoned page %#lx\n", > - pfn, &unpoison_rs); > - num_poisoned_pages_dec(); > - freeit = 1; > - } > + ret = get_hwpoison_page(p, MF_UNPOISON); > + if (!ret) { > + if (clear_page_hwpoison(&unpoison_rs, page)) > + ret = 0; > + else > + ret = -EBUSY; > + } else if (ret < 0) { > + if (ret == -EHWPOISON) { > + ret = unpoison_taken_off_page(&unpoison_rs, p); > + } else > + unpoison_pr_info("Unpoison: failed to grab page %#lx\n", > + pfn, &unpoison_rs); > + } else { > + int freeit = clear_page_hwpoison(&unpoison_rs, p); > > - put_page(page); > - if (freeit && !(pfn == my_zero_pfn(0) && page_count(p) == 1)) > put_page(page); > + if (freeit && !(pfn == my_zero_pfn(0) && page_count(p) == 1)) { > + put_page(page); > + ret = 0; > + } > + } > > unlock_mutex: > mutex_unlock(&mf_mutex); > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index c5952749ad40..d9ba6c1b9f43 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -9448,6 +9449,7 @@ bool take_page_off_buddy(struct page *page) > del_page_from_free_list(page_head, zone, page_order); > break_down_buddy_pages(zone, page_head, page, 0, > page_order, migratetype); > + SetPageHWPoisonTakenOff(page); > if (!is_migrate_isolate(migratetype)) > __mod_zone_freepage_state(zone, -1, migratetype); > ret = true; > @@ -9459,4 +9461,29 @@ bool take_page_off_buddy(struct page *page) > spin_unlock_irqrestore(&zone->lock, flags); > return ret; > } > + > +/* > + * Cancel takeoff done by take_page_off_buddy(). > + */ > +bool put_page_back_buddy(struct page *page) > +{ > + struct zone *zone = page_zone(page); > + unsigned long pfn = page_to_pfn(page); > + unsigned long flags; > + int migratetype = get_pfnblock_migratetype(page, pfn); > + bool ret = false; > + > + spin_lock_irqsave(&zone->lock, flags); > + if (put_page_testzero(page)) { > + ClearPageHWPoisonTakenOff(page); > + __free_one_page(page, pfn, zone, 0, migratetype, FPI_NONE); > + if (TestClearPageHWPoison(page)) { > + num_poisoned_pages_dec(); > + ret = true; > + } > + } > + spin_unlock_irqrestore(&zone->lock, flags); > + > + return ret; > +} > #endif > -- > 2.25.1 >