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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id B5DCFC433F5 for ; Tue, 22 Mar 2022 00:47:02 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DEB6B6B0072; Mon, 21 Mar 2022 20:47:01 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D6F8F6B0073; Mon, 21 Mar 2022 20:47:01 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BE88B6B0074; Mon, 21 Mar 2022 20:47:01 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.hostedemail.com [64.99.140.28]) by kanga.kvack.org (Postfix) with ESMTP id B05466B0072 for ; Mon, 21 Mar 2022 20:47:01 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 7438D23A7D for ; Tue, 22 Mar 2022 00:47:01 +0000 (UTC) X-FDA: 79270182642.07.871B381 Received: from mail-pj1-f54.google.com (mail-pj1-f54.google.com [209.85.216.54]) by imf26.hostedemail.com (Postfix) with ESMTP id E0638140033 for ; Tue, 22 Mar 2022 00:47:00 +0000 (UTC) Received: by mail-pj1-f54.google.com with SMTP id mz9-20020a17090b378900b001c657559290so966829pjb.2 for ; Mon, 21 Mar 2022 17:47:00 -0700 (PDT) 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=ZEAhtCrJUXhBfx1FoGP14Q/p+PdLnLauLi6Y8sm/760=; b=o/BMgl9zNdPqkzoshOb4BN3pBKErNJDh3XzX1/tMZ9NzCkJ/ZbXNDx26fRZgONGgQy UHlOMFasMuUeUcWLKEbe+OlDIcbP6nEfc+/uMl+ih9Xgk/10ZsAsbEMK6qnY7FBj9brx CZL0gZHUnWtjpudj595id8UEmGdVUCifVxfIfg10s3tXb3VVDy73Y9XoYtCGXSsTwX3o TQkD2fvSZXY0uTfQFneIYANdAi/zbH86xjyVbhgvMPX0Q1uzSdeJIQW9DuCxS6c1lhSv FxoVabG48tP91Ynm+cGuhUJaZDUVXxolY6AOQWrYoZ/+kIUKCCBwrgIExCu+d1wthw1T YwpA== 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=ZEAhtCrJUXhBfx1FoGP14Q/p+PdLnLauLi6Y8sm/760=; b=J8xwSHHw9TJLyYORURAZydZp27yFD6gdLzMOrvFwaYOKpg4iCmdn8Vs8zYzs1tMjos fe4n6tMPot7ZaAtP7VZXaT2kOJR2LLoTjweUViPdwDL/oma1u7Dq0TIehLJWlqzsNrg2 Ysn+kynQ1O6k/AbfgnvDEIwVuGdtfQXIHC3c+eTG+iz7COyy+DTaa73SGug9qZ7Ji5GZ c/PicIlr9BpRK/0e7Y98OLyJ4vQybEMOfPfZy78YwT5rF7N+iV/EYU4jrqWYEHGLzYZ6 ZcX7Jf3rnfHaE/1uWHMUWae4gp65Mok97d5A1HvDYlzpZy1O5FBqiNGwldEI4W4ATkSi n2KA== X-Gm-Message-State: AOAM531VTbYQES6oHSckUoUfjokCH4YVQwVk24VZ52kjnTuDn47uxaUk g6MrqJ6xDd1P7fqUXSTF33RZOkJidn30N36lTqk= X-Google-Smtp-Source: ABdhPJwIxH6y1BkKtneP0BlJrp7ZyjYudITDvla33DF12Q0ZgQbsw+L0huwBaOcV9h8Zl89NIPIxey9yQ1dG9Uy7V5M= X-Received: by 2002:a17:903:1cd:b0:154:5edf:5704 with SMTP id e13-20020a17090301cd00b001545edf5704mr7063631plh.26.1647910019578; Mon, 21 Mar 2022 17:46:59 -0700 (PDT) MIME-Version: 1.0 References: <20220318051612.271802-1-naoya.horiguchi@linux.dev> In-Reply-To: <20220318051612.271802-1-naoya.horiguchi@linux.dev> From: Yang Shi Date: Mon, 21 Mar 2022 17:46:48 -0700 Message-ID: Subject: Re: [PATCH v5] mm/hwpoison: fix race between hugetlb free/demotion and memory_failure_hugetlb() To: Naoya Horiguchi Cc: Linux MM , Andrew Morton , Mike Kravetz , Miaohe Lin , Naoya Horiguchi , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b="o/BMgl9z"; spf=pass (imf26.hostedemail.com: domain of shy828301@gmail.com designates 209.85.216.54 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: E0638140033 X-Stat-Signature: trdjy1bdaw1ie5zud5pkbrmgsew3jbio X-HE-Tag: 1647910020-880400 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 Thu, Mar 17, 2022 at 10:16 PM Naoya Horiguchi wrote: > > From: Naoya Horiguchi > > There is a race condition between memory_failure_hugetlb() and hugetlb > free/demotion, which causes setting PageHWPoison flag on the wrong page. > The one simple result is that wrong processes can be killed, but another > (more serious) one is that the actual error is left unhandled, so no one > prevents later access to it, and that might lead to more serious results > like consuming corrupted data. > > Think about the below race window: > > CPU 1 CPU 2 > memory_failure_hugetlb > struct page *head = compound_head(p); > hugetlb page might be freed to > buddy, or even changed to another > compound page. > > get_hwpoison_page -- page is not what we want now... > > The compound_head is called outside hugetlb_lock, so the head is not > reliable. > > So set PageHWPoison flag after passing prechecks. And to detect > potential violation, this patch also introduces a new action type > MF_MSG_DIFFERENT_PAGE_SIZE. > > Reported-by: Mike Kravetz > Signed-off-by: Naoya Horiguchi > Signed-off-by: Miaohe Lin > Cc: > --- > ChangeLog v4 -> v5: > - call TestSetPageHWPoison() when page_handle_poison() fails. > - call TestSetPageHWPoison() for unhandlable cases (MF_MSG_UNKNOWN and > MF_MSG_DIFFERENT_PAGE_SIZE). > - Set PageHWPoison on the head page only when the error page is surely > a hugepage, otherwise set the flag on the raw page. > - rebased onto v5.17-rc8-mmotm-2022-03-16-17-42 > > ChangeLog v3 -> v4: > - squash with "mm/memory-failure.c: fix race with changing page > compound again". > - update patch subject and description based on it. > > ChangeLog v2 -> v3: > - rename the patch because page lock is not the primary factor to > solve the reported issue. > - updated description in the same manner. > - call page_handle_poison() instead of __page_handle_poison() for > free hugepage case. > - reorder put_page and unlock_page (thanks to Miaohe Lin) > > ChangeLog v1 -> v2: > - pass subpage to get_hwpoison_huge_page() instead of head page. > - call compound_head() in hugetlb_lock to avoid race with hugetlb > demotion/free. > --- > mm/hugetlb.c | 8 +++-- > mm/memory-failure.c | 75 +++++++++++++++++++++++++++------------------ > 2 files changed, 51 insertions(+), 32 deletions(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index fbf598bbc4e3..d8ef67c049e4 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -6777,14 +6777,16 @@ bool isolate_huge_page(struct page *page, struct list_head *list) > > int get_hwpoison_huge_page(struct page *page, bool *hugetlb) > { > + struct page *head; > int ret = 0; > > *hugetlb = false; > spin_lock_irq(&hugetlb_lock); > - if (PageHeadHuge(page)) { > + head = compound_head(page); > + if (PageHeadHuge(head)) { > *hugetlb = true; > - if (HPageFreed(page) || HPageMigratable(page)) > - ret = get_page_unless_zero(page); > + if (HPageFreed(head) || HPageMigratable(head)) > + ret = get_page_unless_zero(head); > else > ret = -EBUSY; > } > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index e939719c0765..9323a5653dec 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -1194,7 +1194,7 @@ static int __get_hwpoison_page(struct page *page, unsigned long flags) > int ret = 0; > bool hugetlb = false; > > - ret = get_hwpoison_huge_page(head, &hugetlb); > + ret = get_hwpoison_huge_page(page, &hugetlb); > if (hugetlb) > return ret; > > @@ -1281,11 +1281,10 @@ static int get_any_page(struct page *p, unsigned long flags) > > 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); > + ret = get_hwpoison_huge_page(page, &hugetlb); > if (hugetlb) > return ret; > > @@ -1504,39 +1503,38 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags) > struct page *head = compound_head(p); > int res; > unsigned long page_flags; > - > - if (TestSetPageHWPoison(head)) { > - pr_err("Memory failure: %#lx: already hardware poisoned\n", > - pfn); > - res = -EHWPOISON; > - if (flags & MF_ACTION_REQUIRED) > - res = kill_accessing_process(current, page_to_pfn(head), flags); > - return res; > - } > - > - num_poisoned_pages_inc(); > + bool put = false; > + unsigned long already_hwpoisoned = 0; > > if (!(flags & MF_COUNT_INCREASED)) { > res = get_hwpoison_page(p, flags); > if (!res) { > lock_page(head); > if (hwpoison_filter(p)) { > - if (TestClearPageHWPoison(head)) > - num_poisoned_pages_dec(); > unlock_page(head); > return -EOPNOTSUPP; > } > unlock_page(head); > - res = MF_FAILED; > - if (__page_handle_poison(p)) { > - page_ref_inc(p); > + if (page_handle_poison(p, true, false)) { > res = MF_RECOVERED; > + } else { > + if (TestSetPageHWPoison(head)) > + already_hwpoisoned = page_to_pfn(head); > + else > + num_poisoned_pages_inc(); > + res = MF_FAILED; > } > action_result(pfn, MF_MSG_FREE_HUGE, res); > - return res == MF_RECOVERED ? 0 : -EBUSY; > + res = res == MF_RECOVERED ? 0 : -EBUSY; > + goto out; > } else if (res < 0) { > + if (TestSetPageHWPoison(p)) > + already_hwpoisoned = pfn; > + else > + num_poisoned_pages_inc(); > action_result(pfn, MF_MSG_UNKNOWN, MF_IGNORED); > - return -EBUSY; > + res = -EBUSY; > + goto out; > } > } > > @@ -1547,21 +1545,31 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags) > * If this happens just bail out. > */ > if (!PageHuge(p) || compound_head(p) != head) { > + if (TestSetPageHWPoison(p)) > + already_hwpoisoned = pfn; > + else > + num_poisoned_pages_inc(); > action_result(pfn, MF_MSG_DIFFERENT_PAGE_SIZE, MF_IGNORED); The commit log says "this patch also introduces a new action type MF_MSG_DIFFERENT_PAGE_SIZE", but it is not defined in the patch and it is called here. Did I miss something? > res = -EBUSY; > - goto out; > + goto unlock_page; > } > > page_flags = head->flags; > > if (hwpoison_filter(p)) { > - if (TestClearPageHWPoison(head)) > - num_poisoned_pages_dec(); > - put_page(p); > + put = true; > res = -EOPNOTSUPP; > - goto out; > + goto unlock_page; > + } > + > + if (TestSetPageHWPoison(head)) { > + put = true; > + already_hwpoisoned = page_to_pfn(head); > + goto unlock_page; > } > > + num_poisoned_pages_inc(); > + > /* > * TODO: hwpoison for pud-sized hugetlb doesn't work right now, so > * simply disable it. In order to make it work properly, we need > @@ -1574,18 +1582,27 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags) > if (huge_page_size(page_hstate(head)) > PMD_SIZE) { > action_result(pfn, MF_MSG_NON_PMD_HUGE, MF_IGNORED); > res = -EBUSY; > - goto out; > + goto unlock_page; > } > > if (!hwpoison_user_mappings(p, pfn, flags, head)) { > action_result(pfn, MF_MSG_UNMAP_FAILED, MF_IGNORED); > res = -EBUSY; > - goto out; > + goto unlock_page; > } > > return identify_page_state(pfn, p, page_flags); > -out: > +unlock_page: > unlock_page(head); > +out: > + if (put) > + put_page(p); > + if (already_hwpoisoned) { > + pr_err("Memory failure: %#lx: already hardware poisoned\n", pfn); > + res = -EHWPOISON; > + if (flags & MF_ACTION_REQUIRED) > + res = kill_accessing_process(current, already_hwpoisoned, flags); > + } > return res; > } > > -- > 2.25.1 >