Message ID | 20221018200125.848471-1-jthoughton@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4ac7:0:0:0:0:0 with SMTP id y7csp2138395wrs; Tue, 18 Oct 2022 13:06:24 -0700 (PDT) X-Google-Smtp-Source: AMsMyM5FxZnW3dW8pA+qJPuWFNf2qgU5wXie0y1EWl92EsR2cCDupmLdo2DPhV5oWrYuX/ecTa0n X-Received: by 2002:a63:4e66:0:b0:456:b3a7:7a80 with SMTP id o38-20020a634e66000000b00456b3a77a80mr3940819pgl.467.1666123584218; Tue, 18 Oct 2022 13:06:24 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666123584; cv=none; d=google.com; s=arc-20160816; b=RgNqLI+cQsyZDv/HDMi2GR2TEQFDo2Ilb7gJtBT2L7R5BTckV8f6RSlZg049peU0lD f6GaVK2awt5L6zhME+lfdzCbAzZFL3XVk4AdDJRZ9LQWHbmoKlgV2cTCPl+o4MUmRDb4 CsFMJ/F6RvaKzTxJXRLoHDoUjIgnKBQl6b7WmcN+gyGHkWWgPie7+nTVjYjvGyp32J8o IKKgwEEO9i48qYlyak/UVZRdDPe0xtssCJFxo8e9BIOMAbD3L30mJyn/1fVHLPzPqzSq Hti7dKfNFC7rEbezIg1NXlrsMffJixEsYIbFgw/wjvtwb95x68w+nDpwrpqNj31g7owc AXng== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:mime-version:date :dkim-signature; bh=gWINlDuaY2L8iLCd06Hh2mBWo/vkOwHDew9Pwd1Mff0=; b=DP39931BeYjLY5YzdB98SEiVXm8abAFgH+ppebWIZ3vz3MI3yqbyC8OlR1hKXokZMU 1os9kfOW2o6k+EjIftovH+icla13H1ognfQ+y1CivQ2oq2kV6UXCw194n2YIyO6XNcG7 OyMUz0/wvVLAlWLojaEni9t7Drh4OoULDfYyn0w+t7qU0Duh2fxe2C/PvOv49Nbz67pq 8w4AAzpn1BXGWCqQltGFAcwNHHd3DXarxjw+I36iYbrpRSH1UQ2VjM6VuNdFsWMVlAFE x/GI8cySk3BAB1a12b9R2XdZCBg95Ihw2sr6jyWxI0cEWwyJ0ym9yeiHmY9VadpHW8lo tQ4w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=X2mLtVik; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id n16-20020a170903111000b00184c33ddeb3si19299676plh.41.2022.10.18.13.05.46; Tue, 18 Oct 2022 13:06:24 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=X2mLtVik; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230028AbiJRUBh (ORCPT <rfc822;toshivichauhan@gmail.com> + 99 others); Tue, 18 Oct 2022 16:01:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46824 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229991AbiJRUBf (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 18 Oct 2022 16:01:35 -0400 Received: from mail-yw1-x114a.google.com (mail-yw1-x114a.google.com [IPv6:2607:f8b0:4864:20::114a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C22089D50A for <linux-kernel@vger.kernel.org>; Tue, 18 Oct 2022 13:01:34 -0700 (PDT) Received: by mail-yw1-x114a.google.com with SMTP id 00721157ae682-33dc888dc62so149550597b3.4 for <linux-kernel@vger.kernel.org>; Tue, 18 Oct 2022 13:01:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:from:subject:message-id:mime-version:date:from:to:cc:subject :date:message-id:reply-to; bh=gWINlDuaY2L8iLCd06Hh2mBWo/vkOwHDew9Pwd1Mff0=; b=X2mLtVikXakn4hxHFaq5nvCpGMMIKzqg3vy+vui5ke+ILw0deOuNgwKM1a/6a2ucxr GyieO1X4Q+l8LPgHbxJqxtzLU7CCTsM6/dZV01yipi3H88nNem4nHrCjA7LHUpqWFqDz q57Rdzz1h2zTCUkTdI0psI+kPDK9nMq9apT8gRuofGMLElnRw5FE1tLG+yehmAXGbwqh 8W3RXe7c+5mFoVHOPi8KUjehorqwVUpCe1J7SBD8AJmpVZunyIjsMoyq497A8fbPX975 1+SODuMBIi1s1C45CO8wwl+K1HYsDdGhXJA3ngFwLFXZxiqiZUqMaG/cljW4TUcnPrEp ugSg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:from:subject:message-id:mime-version:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=gWINlDuaY2L8iLCd06Hh2mBWo/vkOwHDew9Pwd1Mff0=; b=mbCWUVcTflMe0XOiiriKB0Bd8XkqenERGzuY2GdX4/nwOT75UxeH68zTO/3IiFZd38 RzeTi8pB9DIvtDw1ak0Un759zUMo0hJGVosin9uYlcGkYVt0Yb0V8MnP1RdJNY/lvWQ8 ZgfZpULDDpORhFYNSBWec+7adoER7gj6QNFK+xaZ2VXMFWCFax7X+hieZpSU+bSpAE9g fuzNt/FYhL9DXGRXLg3SMTXomwqSwAeBcld3BsNiTPoojLBtfBCKBGN8zFXKZo6rDqhh +VDHpFJWV9pE0Tn1XDYW0U+r3ShGkRgUqt+ov7pUp7dfdEsrFuPj0EkLJpx9PhnG18n4 DmRQ== X-Gm-Message-State: ACrzQf2WBPgHQRG7bdeRnQBsOWr//ivR1WaIkO1H44BT7DitnDMvH+Kr qKVDxmiSmyPzLMDUgvzsF0K/c6/o8FEW8mGI X-Received: from jthoughton.c.googlers.com ([fda3:e722:ac3:cc00:14:4d90:c0a8:2a4f]) (user=jthoughton job=sendgmr) by 2002:a0d:d5c8:0:b0:356:6d6c:7b83 with SMTP id x191-20020a0dd5c8000000b003566d6c7b83mr3920713ywd.439.1666123294095; Tue, 18 Oct 2022 13:01:34 -0700 (PDT) Date: Tue, 18 Oct 2022 20:01:25 +0000 Mime-Version: 1.0 X-Mailer: git-send-email 2.38.0.413.g74048e4d9e-goog Message-ID: <20221018200125.848471-1-jthoughton@google.com> Subject: [PATCH] hugetlbfs: don't delete error page from pagecache From: James Houghton <jthoughton@google.com> To: Mike Kravetz <mike.kravetz@oracle.com>, Muchun Song <songmuchun@bytedance.com>, Naoya Horiguchi <naoya.horiguchi@nec.com>, Miaohe Lin <linmiaohe@huawei.com> Cc: Andrew Morton <akpm@linux-foundation.org>, Yang Shi <shy828301@gmail.com>, Axel Rasmussen <axelrasmussen@google.com>, linux-mm@kvack.org, linux-kernel@vger.kernel.org, James Houghton <jthoughton@google.com> Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747057203389166008?= X-GMAIL-MSGID: =?utf-8?q?1747057203389166008?= |
Series |
hugetlbfs: don't delete error page from pagecache
|
|
Commit Message
James Houghton
Oct. 18, 2022, 8:01 p.m. UTC
This change is very similar to the change that was made for shmem [1],
and it solves the same problem but for HugeTLBFS instead.
Currently, when poison is found in a HugeTLB page, the page is removed
from the page cache. That means that attempting to map or read that
hugepage in the future will result in a new hugepage being allocated
instead of notifying the user that the page was poisoned. As [1] states,
this is effectively memory corruption.
The fix is to leave the page in the page cache. If the user attempts to
use a poisoned HugeTLB page with a syscall, the syscall will fail with
EIO, the same error code that shmem uses. For attempts to map the page,
the thread will get a BUS_MCEERR_AR SIGBUS.
[1]: commit a76054266661 ("mm: shmem: don't truncate page if memory failure happens")
Signed-off-by: James Houghton <jthoughton@google.com>
---
fs/hugetlbfs/inode.c | 13 ++++++-------
mm/hugetlb.c | 4 ++++
mm/memory-failure.c | 5 ++++-
3 files changed, 14 insertions(+), 8 deletions(-)
Comments
On Tue, Oct 18, 2022 at 1:01 PM James Houghton <jthoughton@google.com> wrote: > > This change is very similar to the change that was made for shmem [1], > and it solves the same problem but for HugeTLBFS instead. > > Currently, when poison is found in a HugeTLB page, the page is removed > from the page cache. That means that attempting to map or read that > hugepage in the future will result in a new hugepage being allocated > instead of notifying the user that the page was poisoned. As [1] states, > this is effectively memory corruption. > > The fix is to leave the page in the page cache. If the user attempts to > use a poisoned HugeTLB page with a syscall, the syscall will fail with > EIO, the same error code that shmem uses. For attempts to map the page, > the thread will get a BUS_MCEERR_AR SIGBUS. > > [1]: commit a76054266661 ("mm: shmem: don't truncate page if memory failure happens") > > Signed-off-by: James Houghton <jthoughton@google.com> Thanks for the patch. Yes, we should do the same thing for hugetlbfs. When I was working on shmem I did look into hugetlbfs too. But the problem is we actually make the whole hugetlb page unavailable even though just one 4K sub page is hwpoisoned. It may be fine to 2M hugetlb page, but a lot of memory may be a huge waste for 1G hugetlb page, particular for the page fault path. So I discussed this with Mike offline last year, and I was told Google was working on PTE mapped hugetlb page. That should be able to solve the problem. And we'd like to have the high-granularity hugetlb mapping support as the predecessor. There were some other details, but I can't remember all of them, I have to refresh my memory by rereading the email discussions... > --- > fs/hugetlbfs/inode.c | 13 ++++++------- > mm/hugetlb.c | 4 ++++ > mm/memory-failure.c | 5 ++++- > 3 files changed, 14 insertions(+), 8 deletions(-) > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > index fef5165b73a5..7f836f8f9db1 100644 > --- a/fs/hugetlbfs/inode.c > +++ b/fs/hugetlbfs/inode.c > @@ -328,6 +328,12 @@ static ssize_t hugetlbfs_read_iter(struct kiocb *iocb, struct iov_iter *to) > } else { > unlock_page(page); > > + if (PageHWPoison(page)) { > + put_page(page); > + retval = -EIO; > + break; > + } > + > /* > * We have the page, copy it to user space buffer. > */ > @@ -1111,13 +1117,6 @@ static int hugetlbfs_migrate_folio(struct address_space *mapping, > static int hugetlbfs_error_remove_page(struct address_space *mapping, > struct page *page) > { > - struct inode *inode = mapping->host; > - pgoff_t index = page->index; > - > - hugetlb_delete_from_page_cache(page_folio(page)); > - if (unlikely(hugetlb_unreserve_pages(inode, index, index + 1, 1))) > - hugetlb_fix_reserve_counts(inode); > - > return 0; > } > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 97896165fd3f..5120a9ccbf5b 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -6101,6 +6101,10 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, > > ptl = huge_pte_lock(h, dst_mm, dst_pte); > > + ret = -EIO; > + if (PageHWPoison(page)) > + goto out_release_unlock; > + > /* > * We allow to overwrite a pte marker: consider when both MISSING|WP > * registered, we firstly wr-protect a none pte which has no page cache > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 145bb561ddb3..bead6bccc7f2 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -1080,6 +1080,7 @@ static int me_huge_page(struct page_state *ps, struct page *p) > int res; > struct page *hpage = compound_head(p); > struct address_space *mapping; > + bool extra_pins = false; > > if (!PageHuge(hpage)) > return MF_DELAYED; > @@ -1087,6 +1088,8 @@ static int me_huge_page(struct page_state *ps, struct page *p) > mapping = page_mapping(hpage); > if (mapping) { > res = truncate_error_page(hpage, page_to_pfn(p), mapping); > + /* The page is kept in page cache. */ > + extra_pins = true; > unlock_page(hpage); > } else { > unlock_page(hpage); > @@ -1104,7 +1107,7 @@ static int me_huge_page(struct page_state *ps, struct page *p) > } > } > > - if (has_extra_refcount(ps, p, false)) > + if (has_extra_refcount(ps, p, extra_pins)) > res = MF_FAILED; > > return res; > -- > 2.38.0.413.g74048e4d9e-goog >
On Wed, Oct 19, 2022 at 11:31 AM Yang Shi <shy828301@gmail.com> wrote: > > On Tue, Oct 18, 2022 at 1:01 PM James Houghton <jthoughton@google.com> wrote: > > > > This change is very similar to the change that was made for shmem [1], > > and it solves the same problem but for HugeTLBFS instead. > > > > Currently, when poison is found in a HugeTLB page, the page is removed > > from the page cache. That means that attempting to map or read that > > hugepage in the future will result in a new hugepage being allocated > > instead of notifying the user that the page was poisoned. As [1] states, > > this is effectively memory corruption. > > > > The fix is to leave the page in the page cache. If the user attempts to > > use a poisoned HugeTLB page with a syscall, the syscall will fail with > > EIO, the same error code that shmem uses. For attempts to map the page, > > the thread will get a BUS_MCEERR_AR SIGBUS. > > > > [1]: commit a76054266661 ("mm: shmem: don't truncate page if memory failure happens") > > > > Signed-off-by: James Houghton <jthoughton@google.com> > > Thanks for the patch. Yes, we should do the same thing for hugetlbfs. > When I was working on shmem I did look into hugetlbfs too. But the > problem is we actually make the whole hugetlb page unavailable even > though just one 4K sub page is hwpoisoned. It may be fine to 2M > hugetlb page, but a lot of memory may be a huge waste for 1G hugetlb > page, particular for the page fault path. Right -- it is wasted until a hole is punched or the file is truncated. Although we're wasting the rest of the hugepage for a little longer with this patch, I think it's worth it to have correct behavior. > > So I discussed this with Mike offline last year, and I was told Google > was working on PTE mapped hugetlb page. That should be able to solve > the problem. And we'd like to have the high-granularity hugetlb > mapping support as the predecessor. > > There were some other details, but I can't remember all of them, I > have to refresh my memory by rereading the email discussions... Yes! I am working on this. :) I will send up a series in the coming weeks that implements basic support for high-granularity mapping (HGM). This patch is required for hwpoison semantics to work properly for high-granularity mapping (and, as the patch states, for shared HugeTLB mappings generally). For HGM, if we partially map a hugepage and find poison, faulting on the unmapped bits of it will allocate a new hugepage. By keeping the poisoned page in the pagecache, we correctly give userspace a SIGBUS. I didn't mention this in the commit description because I think this patch is correct on its own. I haven't implemented PAGE_SIZE poisoning of HugeTLB pages yet, but high-granularity mapping unblocks this work. Hopefully that will be ready in the coming months. :) - James Houghton > > > --- > > fs/hugetlbfs/inode.c | 13 ++++++------- > > mm/hugetlb.c | 4 ++++ > > mm/memory-failure.c | 5 ++++- > > 3 files changed, 14 insertions(+), 8 deletions(-) > > > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > > index fef5165b73a5..7f836f8f9db1 100644 > > --- a/fs/hugetlbfs/inode.c > > +++ b/fs/hugetlbfs/inode.c > > @@ -328,6 +328,12 @@ static ssize_t hugetlbfs_read_iter(struct kiocb *iocb, struct iov_iter *to) > > } else { > > unlock_page(page); > > > > + if (PageHWPoison(page)) { > > + put_page(page); > > + retval = -EIO; > > + break; > > + } > > + > > /* > > * We have the page, copy it to user space buffer. > > */ > > @@ -1111,13 +1117,6 @@ static int hugetlbfs_migrate_folio(struct address_space *mapping, > > static int hugetlbfs_error_remove_page(struct address_space *mapping, > > struct page *page) > > { > > - struct inode *inode = mapping->host; > > - pgoff_t index = page->index; > > - > > - hugetlb_delete_from_page_cache(page_folio(page)); > > - if (unlikely(hugetlb_unreserve_pages(inode, index, index + 1, 1))) > > - hugetlb_fix_reserve_counts(inode); > > - > > return 0; > > } > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > index 97896165fd3f..5120a9ccbf5b 100644 > > --- a/mm/hugetlb.c > > +++ b/mm/hugetlb.c > > @@ -6101,6 +6101,10 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, > > > > ptl = huge_pte_lock(h, dst_mm, dst_pte); > > > > + ret = -EIO; > > + if (PageHWPoison(page)) > > + goto out_release_unlock; > > + > > /* > > * We allow to overwrite a pte marker: consider when both MISSING|WP > > * registered, we firstly wr-protect a none pte which has no page cache > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > > index 145bb561ddb3..bead6bccc7f2 100644 > > --- a/mm/memory-failure.c > > +++ b/mm/memory-failure.c > > @@ -1080,6 +1080,7 @@ static int me_huge_page(struct page_state *ps, struct page *p) > > int res; > > struct page *hpage = compound_head(p); > > struct address_space *mapping; > > + bool extra_pins = false; > > > > if (!PageHuge(hpage)) > > return MF_DELAYED; > > @@ -1087,6 +1088,8 @@ static int me_huge_page(struct page_state *ps, struct page *p) > > mapping = page_mapping(hpage); > > if (mapping) { > > res = truncate_error_page(hpage, page_to_pfn(p), mapping); > > + /* The page is kept in page cache. */ > > + extra_pins = true; > > unlock_page(hpage); > > } else { > > unlock_page(hpage); > > @@ -1104,7 +1107,7 @@ static int me_huge_page(struct page_state *ps, struct page *p) > > } > > } > > > > - if (has_extra_refcount(ps, p, false)) > > + if (has_extra_refcount(ps, p, extra_pins)) > > res = MF_FAILED; > > > > return res; > > -- > > 2.38.0.413.g74048e4d9e-goog > >
On 10/19/22 11:31, Yang Shi wrote: > On Tue, Oct 18, 2022 at 1:01 PM James Houghton <jthoughton@google.com> wrote: > > > > This change is very similar to the change that was made for shmem [1], > > and it solves the same problem but for HugeTLBFS instead. > > > > Currently, when poison is found in a HugeTLB page, the page is removed > > from the page cache. That means that attempting to map or read that > > hugepage in the future will result in a new hugepage being allocated > > instead of notifying the user that the page was poisoned. As [1] states, > > this is effectively memory corruption. > > > > The fix is to leave the page in the page cache. If the user attempts to > > use a poisoned HugeTLB page with a syscall, the syscall will fail with > > EIO, the same error code that shmem uses. For attempts to map the page, > > the thread will get a BUS_MCEERR_AR SIGBUS. > > > > [1]: commit a76054266661 ("mm: shmem: don't truncate page if memory failure happens") > > > > Signed-off-by: James Houghton <jthoughton@google.com> > > Thanks for the patch. Yes, we should do the same thing for hugetlbfs. > When I was working on shmem I did look into hugetlbfs too. But the > problem is we actually make the whole hugetlb page unavailable even > though just one 4K sub page is hwpoisoned. It may be fine to 2M > hugetlb page, but a lot of memory may be a huge waste for 1G hugetlb > page, particular for the page fault path. One thing that complicated this a bit is the vmemmap optimizations for hugetlb. However, I believe Naoya may have addressed this recently. > So I discussed this with Mike offline last year, and I was told Google > was working on PTE mapped hugetlb page. That should be able to solve > the problem. And we'd like to have the high-granularity hugetlb > mapping support as the predecessor. Yes, I went back in my notes and noticed it had been one year. No offense intended to James and his great work on HGM. However, in hindsight we should have fixed this in some way without waiting for a HGM based. > There were some other details, but I can't remember all of them, I > have to refresh my memory by rereading the email discussions... I think the complicating factor was vmemmap optimization. As mentioned above, this may have already been addressed by Naoya in patches to indicate which sub-page(s) had the actual error. As Yang Shi notes, this patch makes the entire hugetlb page inaccessible. With some work, we could allow reads to everything but the sub-page with error. However, this should be much easier with HGM. And, we could potentially even do page faults everywhere but the sub-page with error. I still think it may be better to wait for HGM instead of trying to do read access to all but sub-page with error now. But, entirely open to other opinions. I plan to do a review of this patch a little later.
On 10/18/22 20:01, James Houghton wrote: > This change is very similar to the change that was made for shmem [1], > and it solves the same problem but for HugeTLBFS instead. > > Currently, when poison is found in a HugeTLB page, the page is removed > from the page cache. That means that attempting to map or read that > hugepage in the future will result in a new hugepage being allocated > instead of notifying the user that the page was poisoned. As [1] states, > this is effectively memory corruption. > > The fix is to leave the page in the page cache. If the user attempts to > use a poisoned HugeTLB page with a syscall, the syscall will fail with > EIO, the same error code that shmem uses. For attempts to map the page, > the thread will get a BUS_MCEERR_AR SIGBUS. > > [1]: commit a76054266661 ("mm: shmem: don't truncate page if memory failure happens") > > Signed-off-by: James Houghton <jthoughton@google.com> > --- > fs/hugetlbfs/inode.c | 13 ++++++------- > mm/hugetlb.c | 4 ++++ > mm/memory-failure.c | 5 ++++- > 3 files changed, 14 insertions(+), 8 deletions(-) Thanks James! Code looks correct. One observation below, but I do not suggest changing. Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > index fef5165b73a5..7f836f8f9db1 100644 > --- a/fs/hugetlbfs/inode.c > +++ b/fs/hugetlbfs/inode.c > @@ -328,6 +328,12 @@ static ssize_t hugetlbfs_read_iter(struct kiocb *iocb, struct iov_iter *to) > } else { > unlock_page(page); > > + if (PageHWPoison(page)) { > + put_page(page); > + retval = -EIO; > + break; > + } > + > /* > * We have the page, copy it to user space buffer. > */ > @@ -1111,13 +1117,6 @@ static int hugetlbfs_migrate_folio(struct address_space *mapping, > static int hugetlbfs_error_remove_page(struct address_space *mapping, > struct page *page) > { > - struct inode *inode = mapping->host; > - pgoff_t index = page->index; > - > - hugetlb_delete_from_page_cache(page_folio(page)); > - if (unlikely(hugetlb_unreserve_pages(inode, index, index + 1, 1))) > - hugetlb_fix_reserve_counts(inode); > - > return 0; > } > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 97896165fd3f..5120a9ccbf5b 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -6101,6 +6101,10 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, > > ptl = huge_pte_lock(h, dst_mm, dst_pte); > > + ret = -EIO; > + if (PageHWPoison(page)) > + goto out_release_unlock; > + > /* > * We allow to overwrite a pte marker: consider when both MISSING|WP > * registered, we firstly wr-protect a none pte which has no page cache > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 145bb561ddb3..bead6bccc7f2 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -1080,6 +1080,7 @@ static int me_huge_page(struct page_state *ps, struct page *p) > int res; > struct page *hpage = compound_head(p); > struct address_space *mapping; > + bool extra_pins = false; > > if (!PageHuge(hpage)) > return MF_DELAYED; > @@ -1087,6 +1088,8 @@ static int me_huge_page(struct page_state *ps, struct page *p) > mapping = page_mapping(hpage); > if (mapping) { > res = truncate_error_page(hpage, page_to_pfn(p), mapping); > + /* The page is kept in page cache. */ That looks a bit silly as we are know truncate_error_page() is a noop and call it anyway. I suppose we could just set 'res = MF_RECOVERED'. However, it really should be left as is in case more non-hugetlb functionality is added to truncate_error_page() in the future.
On Wed, Oct 19, 2022 at 11:42 AM James Houghton <jthoughton@google.com> wrote: > > On Wed, Oct 19, 2022 at 11:31 AM Yang Shi <shy828301@gmail.com> wrote: > > > > On Tue, Oct 18, 2022 at 1:01 PM James Houghton <jthoughton@google.com> wrote: > > > > > > This change is very similar to the change that was made for shmem [1], > > > and it solves the same problem but for HugeTLBFS instead. > > > > > > Currently, when poison is found in a HugeTLB page, the page is removed > > > from the page cache. That means that attempting to map or read that > > > hugepage in the future will result in a new hugepage being allocated > > > instead of notifying the user that the page was poisoned. As [1] states, > > > this is effectively memory corruption. > > > > > > The fix is to leave the page in the page cache. If the user attempts to > > > use a poisoned HugeTLB page with a syscall, the syscall will fail with > > > EIO, the same error code that shmem uses. For attempts to map the page, > > > the thread will get a BUS_MCEERR_AR SIGBUS. > > > > > > [1]: commit a76054266661 ("mm: shmem: don't truncate page if memory failure happens") > > > > > > Signed-off-by: James Houghton <jthoughton@google.com> > > > > Thanks for the patch. Yes, we should do the same thing for hugetlbfs. > > When I was working on shmem I did look into hugetlbfs too. But the > > problem is we actually make the whole hugetlb page unavailable even > > though just one 4K sub page is hwpoisoned. It may be fine to 2M > > hugetlb page, but a lot of memory may be a huge waste for 1G hugetlb > > page, particular for the page fault path. > > Right -- it is wasted until a hole is punched or the file is > truncated. Although we're wasting the rest of the hugepage for a > little longer with this patch, I think it's worth it to have correct > behavior. > > > > > So I discussed this with Mike offline last year, and I was told Google > > was working on PTE mapped hugetlb page. That should be able to solve > > the problem. And we'd like to have the high-granularity hugetlb > > mapping support as the predecessor. > > > > There were some other details, but I can't remember all of them, I > > have to refresh my memory by rereading the email discussions... > > Yes! I am working on this. :) I will send up a series in the coming > weeks that implements basic support for high-granularity mapping > (HGM). This patch is required for hwpoison semantics to work properly > for high-granularity mapping (and, as the patch states, for shared > HugeTLB mappings generally). For HGM, if we partially map a hugepage > and find poison, faulting on the unmapped bits of it will allocate a > new hugepage. By keeping the poisoned page in the pagecache, we > correctly give userspace a SIGBUS. I didn't mention this in the commit > description because I think this patch is correct on its own. I don't mean it is not correct. I'm not sure which one (this patch or HGM) would go first. But it sounds like you thought this patch should be the predecessor of HGM since it could deliver userspace the correct signal? > > I haven't implemented PAGE_SIZE poisoning of HugeTLB pages yet, but > high-granularity mapping unblocks this work. Hopefully that will be > ready in the coming months. :) > > - James Houghton > > > > > > --- > > > fs/hugetlbfs/inode.c | 13 ++++++------- > > > mm/hugetlb.c | 4 ++++ > > > mm/memory-failure.c | 5 ++++- > > > 3 files changed, 14 insertions(+), 8 deletions(-) > > > > > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > > > index fef5165b73a5..7f836f8f9db1 100644 > > > --- a/fs/hugetlbfs/inode.c > > > +++ b/fs/hugetlbfs/inode.c > > > @@ -328,6 +328,12 @@ static ssize_t hugetlbfs_read_iter(struct kiocb *iocb, struct iov_iter *to) > > > } else { > > > unlock_page(page); > > > > > > + if (PageHWPoison(page)) { > > > + put_page(page); > > > + retval = -EIO; > > > + break; > > > + } > > > + > > > /* > > > * We have the page, copy it to user space buffer. > > > */ > > > @@ -1111,13 +1117,6 @@ static int hugetlbfs_migrate_folio(struct address_space *mapping, > > > static int hugetlbfs_error_remove_page(struct address_space *mapping, > > > struct page *page) > > > { > > > - struct inode *inode = mapping->host; > > > - pgoff_t index = page->index; > > > - > > > - hugetlb_delete_from_page_cache(page_folio(page)); > > > - if (unlikely(hugetlb_unreserve_pages(inode, index, index + 1, 1))) > > > - hugetlb_fix_reserve_counts(inode); > > > - > > > return 0; > > > } > > > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > > index 97896165fd3f..5120a9ccbf5b 100644 > > > --- a/mm/hugetlb.c > > > +++ b/mm/hugetlb.c > > > @@ -6101,6 +6101,10 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, > > > > > > ptl = huge_pte_lock(h, dst_mm, dst_pte); > > > > > > + ret = -EIO; > > > + if (PageHWPoison(page)) > > > + goto out_release_unlock; > > > + > > > /* > > > * We allow to overwrite a pte marker: consider when both MISSING|WP > > > * registered, we firstly wr-protect a none pte which has no page cache > > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > > > index 145bb561ddb3..bead6bccc7f2 100644 > > > --- a/mm/memory-failure.c > > > +++ b/mm/memory-failure.c > > > @@ -1080,6 +1080,7 @@ static int me_huge_page(struct page_state *ps, struct page *p) > > > int res; > > > struct page *hpage = compound_head(p); > > > struct address_space *mapping; > > > + bool extra_pins = false; > > > > > > if (!PageHuge(hpage)) > > > return MF_DELAYED; > > > @@ -1087,6 +1088,8 @@ static int me_huge_page(struct page_state *ps, struct page *p) > > > mapping = page_mapping(hpage); > > > if (mapping) { > > > res = truncate_error_page(hpage, page_to_pfn(p), mapping); > > > + /* The page is kept in page cache. */ > > > + extra_pins = true; > > > unlock_page(hpage); > > > } else { > > > unlock_page(hpage); > > > @@ -1104,7 +1107,7 @@ static int me_huge_page(struct page_state *ps, struct page *p) > > > } > > > } > > > > > > - if (has_extra_refcount(ps, p, false)) > > > + if (has_extra_refcount(ps, p, extra_pins)) > > > res = MF_FAILED; > > > > > > return res; > > > -- > > > 2.38.0.413.g74048e4d9e-goog > > >
On Wed, Oct 19, 2022 at 11:55 AM Mike Kravetz <mike.kravetz@oracle.com> wrote: > > On 10/19/22 11:31, Yang Shi wrote: > > On Tue, Oct 18, 2022 at 1:01 PM James Houghton <jthoughton@google.com> wrote: > > > > > > This change is very similar to the change that was made for shmem [1], > > > and it solves the same problem but for HugeTLBFS instead. > > > > > > Currently, when poison is found in a HugeTLB page, the page is removed > > > from the page cache. That means that attempting to map or read that > > > hugepage in the future will result in a new hugepage being allocated > > > instead of notifying the user that the page was poisoned. As [1] states, > > > this is effectively memory corruption. > > > > > > The fix is to leave the page in the page cache. If the user attempts to > > > use a poisoned HugeTLB page with a syscall, the syscall will fail with > > > EIO, the same error code that shmem uses. For attempts to map the page, > > > the thread will get a BUS_MCEERR_AR SIGBUS. > > > > > > [1]: commit a76054266661 ("mm: shmem: don't truncate page if memory failure happens") > > > > > > Signed-off-by: James Houghton <jthoughton@google.com> > > > > Thanks for the patch. Yes, we should do the same thing for hugetlbfs. > > When I was working on shmem I did look into hugetlbfs too. But the > > problem is we actually make the whole hugetlb page unavailable even > > though just one 4K sub page is hwpoisoned. It may be fine to 2M > > hugetlb page, but a lot of memory may be a huge waste for 1G hugetlb > > page, particular for the page fault path. > > One thing that complicated this a bit is the vmemmap optimizations for > hugetlb. However, I believe Naoya may have addressed this recently. > > > So I discussed this with Mike offline last year, and I was told Google > > was working on PTE mapped hugetlb page. That should be able to solve > > the problem. And we'd like to have the high-granularity hugetlb > > mapping support as the predecessor. > > Yes, I went back in my notes and noticed it had been one year. No offense > intended to James and his great work on HGM. However, in hindsight we should > have fixed this in some way without waiting for a HGM based. > > > There were some other details, but I can't remember all of them, I > > have to refresh my memory by rereading the email discussions... > > I think the complicating factor was vmemmap optimization. As mentioned > above, this may have already been addressed by Naoya in patches to > indicate which sub-page(s) had the actual error. > > As Yang Shi notes, this patch makes the entire hugetlb page inaccessible. > With some work, we could allow reads to everything but the sub-page with > error. However, this should be much easier with HGM. And, we could > potentially even do page faults everywhere but the sub-page with error. > > I still think it may be better to wait for HGM instead of trying to do > read access to all but sub-page with error now. But, entirely open to > other opinions. I have no strong preference about which goes first. > > I plan to do a review of this patch a little later. > -- > Mike Kravetz
On Tue, Oct 18, 2022 at 08:01:25PM +0000, James Houghton wrote: > This change is very similar to the change that was made for shmem [1], > and it solves the same problem but for HugeTLBFS instead. > > Currently, when poison is found in a HugeTLB page, the page is removed > from the page cache. That means that attempting to map or read that > hugepage in the future will result in a new hugepage being allocated > instead of notifying the user that the page was poisoned. As [1] states, > this is effectively memory corruption. > > The fix is to leave the page in the page cache. If the user attempts to > use a poisoned HugeTLB page with a syscall, the syscall will fail with > EIO, the same error code that shmem uses. For attempts to map the page, > the thread will get a BUS_MCEERR_AR SIGBUS. > > [1]: commit a76054266661 ("mm: shmem: don't truncate page if memory failure happens") > > Signed-off-by: James Houghton <jthoughton@google.com> I did some testing and found no issue. So I agree with this patch. Thank you very much. Tested-by: Naoya Horiguchi <naoya.horiguchi@nec.com> Reviewed-by: Naoya Horiguchi <naoya.horiguchi@nec.com> As for whether to go with HGM patchset or not, I have no strong opinion. As you stated in another email this patch is correct without HGM patch, so it's OK to me to make this merged first. Thanks, Naoya Horiguchi
On 11/04/22 02:10, HORIGUCHI NAOYA(堀口 直也) wrote: > On Tue, Oct 18, 2022 at 08:01:25PM +0000, James Houghton wrote: > > This change is very similar to the change that was made for shmem [1], > > and it solves the same problem but for HugeTLBFS instead. > > > > Currently, when poison is found in a HugeTLB page, the page is removed > > from the page cache. That means that attempting to map or read that > > hugepage in the future will result in a new hugepage being allocated > > instead of notifying the user that the page was poisoned. As [1] states, > > this is effectively memory corruption. > > > > The fix is to leave the page in the page cache. If the user attempts to > > use a poisoned HugeTLB page with a syscall, the syscall will fail with > > EIO, the same error code that shmem uses. For attempts to map the page, > > the thread will get a BUS_MCEERR_AR SIGBUS. > > > > [1]: commit a76054266661 ("mm: shmem: don't truncate page if memory failure happens") > > > > Signed-off-by: James Houghton <jthoughton@google.com> > > I did some testing and found no issue. So I agree with this patch. > Thank you very much. > > Tested-by: Naoya Horiguchi <naoya.horiguchi@nec.com> > Reviewed-by: Naoya Horiguchi <naoya.horiguchi@nec.com> > > As for whether to go with HGM patchset or not, I have no strong opinion. > As you stated in another email this patch is correct without HGM patch, > so it's OK to me to make this merged first. Thanks Naoya. This is a late thought, but ... Should this patch and Yang Shi's shmem patch be backported to stable releases? Both address potential data corruption/loss, so it certainly seems like stable material.
On Fri, Nov 04, 2022 at 06:27:44AM -0700, Mike Kravetz wrote: > On 11/04/22 02:10, HORIGUCHI NAOYA(堀口 直也) wrote: > > On Tue, Oct 18, 2022 at 08:01:25PM +0000, James Houghton wrote: > > > This change is very similar to the change that was made for shmem [1], > > > and it solves the same problem but for HugeTLBFS instead. > > > > > > Currently, when poison is found in a HugeTLB page, the page is removed > > > from the page cache. That means that attempting to map or read that > > > hugepage in the future will result in a new hugepage being allocated > > > instead of notifying the user that the page was poisoned. As [1] states, > > > this is effectively memory corruption. > > > > > > The fix is to leave the page in the page cache. If the user attempts to > > > use a poisoned HugeTLB page with a syscall, the syscall will fail with > > > EIO, the same error code that shmem uses. For attempts to map the page, > > > the thread will get a BUS_MCEERR_AR SIGBUS. > > > > > > [1]: commit a76054266661 ("mm: shmem: don't truncate page if memory failure happens") > > > > > > Signed-off-by: James Houghton <jthoughton@google.com> > > > > I did some testing and found no issue. So I agree with this patch. > > Thank you very much. > > > > Tested-by: Naoya Horiguchi <naoya.horiguchi@nec.com> > > Reviewed-by: Naoya Horiguchi <naoya.horiguchi@nec.com> > > > > As for whether to go with HGM patchset or not, I have no strong opinion. > > As you stated in another email this patch is correct without HGM patch, > > so it's OK to me to make this merged first. > > Thanks Naoya. > > This is a late thought, but ... > Should this patch and Yang Shi's shmem patch be backported to stable releases? > Both address potential data corruption/loss, so it certainly seems like > stable material. Yes, I agree that backporting these could be helpful. So I think that I'll try to backport commit a7605426666 and its dependencies to 5.15 (and older LTS if possible). For this patch, just adding "Cc: stable" should be enough for now. Thanks, Naoya Horiguchi
On 11/07/22 04:25, HORIGUCHI NAOYA(堀口 直也) wrote: > On Fri, Nov 04, 2022 at 06:27:44AM -0700, Mike Kravetz wrote: > > On 11/04/22 02:10, HORIGUCHI NAOYA(堀口 直也) wrote: > > > On Tue, Oct 18, 2022 at 08:01:25PM +0000, James Houghton wrote: > > > > This change is very similar to the change that was made for shmem [1], > > > > and it solves the same problem but for HugeTLBFS instead. > > > > > > > > Currently, when poison is found in a HugeTLB page, the page is removed > > > > from the page cache. That means that attempting to map or read that > > > > hugepage in the future will result in a new hugepage being allocated > > > > instead of notifying the user that the page was poisoned. As [1] states, > > > > this is effectively memory corruption. > > > > > > > > The fix is to leave the page in the page cache. If the user attempts to > > > > use a poisoned HugeTLB page with a syscall, the syscall will fail with > > > > EIO, the same error code that shmem uses. For attempts to map the page, > > > > the thread will get a BUS_MCEERR_AR SIGBUS. > > > > > > > > [1]: commit a76054266661 ("mm: shmem: don't truncate page if memory failure happens") > > > > > > > > Signed-off-by: James Houghton <jthoughton@google.com> > > > > > > I did some testing and found no issue. So I agree with this patch. > > > Thank you very much. > > > > > > Tested-by: Naoya Horiguchi <naoya.horiguchi@nec.com> > > > Reviewed-by: Naoya Horiguchi <naoya.horiguchi@nec.com> > > > > > > As for whether to go with HGM patchset or not, I have no strong opinion. > > > As you stated in another email this patch is correct without HGM patch, > > > so it's OK to me to make this merged first. > > > > Thanks Naoya. > > > > This is a late thought, but ... > > Should this patch and Yang Shi's shmem patch be backported to stable releases? > > Both address potential data corruption/loss, so it certainly seems like > > stable material. > > Yes, I agree that backporting these could be helpful. > > So I think that I'll try to backport commit a7605426666 and its dependencies > to 5.15 (and older LTS if possible). For this patch, just adding "Cc: stable" > should be enough for now. We have now actually seen users impacted by this issue. I suggest adding the following tags to this patch. Fixes: 78bb920344b8 ("mm: hwpoison: dissolve in-use hugepage in unrecoverable memory error") Cc: <stable@vger.kernel.org>
On Sun, Nov 6, 2022 at 8:25 PM HORIGUCHI NAOYA(堀口 直也) <naoya.horiguchi@nec.com> wrote: > > On Fri, Nov 04, 2022 at 06:27:44AM -0700, Mike Kravetz wrote: > > On 11/04/22 02:10, HORIGUCHI NAOYA(堀口 直也) wrote: > > > On Tue, Oct 18, 2022 at 08:01:25PM +0000, James Houghton wrote: > > > > This change is very similar to the change that was made for shmem [1], > > > > and it solves the same problem but for HugeTLBFS instead. > > > > > > > > Currently, when poison is found in a HugeTLB page, the page is removed > > > > from the page cache. That means that attempting to map or read that > > > > hugepage in the future will result in a new hugepage being allocated > > > > instead of notifying the user that the page was poisoned. As [1] states, > > > > this is effectively memory corruption. > > > > > > > > The fix is to leave the page in the page cache. If the user attempts to > > > > use a poisoned HugeTLB page with a syscall, the syscall will fail with > > > > EIO, the same error code that shmem uses. For attempts to map the page, > > > > the thread will get a BUS_MCEERR_AR SIGBUS. > > > > > > > > [1]: commit a76054266661 ("mm: shmem: don't truncate page if memory failure happens") > > > > > > > > Signed-off-by: James Houghton <jthoughton@google.com> > > > > > > I did some testing and found no issue. So I agree with this patch. > > > Thank you very much. > > > > > > Tested-by: Naoya Horiguchi <naoya.horiguchi@nec.com> > > > Reviewed-by: Naoya Horiguchi <naoya.horiguchi@nec.com> > > > > > > As for whether to go with HGM patchset or not, I have no strong opinion. > > > As you stated in another email this patch is correct without HGM patch, > > > so it's OK to me to make this merged first. > > > > Thanks Naoya. > > > > This is a late thought, but ... > > Should this patch and Yang Shi's shmem patch be backported to stable releases? > > Both address potential data corruption/loss, so it certainly seems like > > stable material. > > Yes, I agree that backporting these could be helpful. +1 > > So I think that I'll try to backport commit a7605426666 and its dependencies > to 5.15 (and older LTS if possible). For this patch, just adding "Cc: stable" > should be enough for now. Thanks, Naoya. > > Thanks, > Naoya Horiguchi
On Mon, 7 Nov 2022 08:55:43 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote: > We have now actually seen users impacted by this issue. I suggest > adding the following tags to this patch. > > Fixes: 78bb920344b8 ("mm: hwpoison: dissolve in-use hugepage in unrecoverable memory error") > Cc: <stable@vger.kernel.org> I've made those changes.
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index fef5165b73a5..7f836f8f9db1 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -328,6 +328,12 @@ static ssize_t hugetlbfs_read_iter(struct kiocb *iocb, struct iov_iter *to) } else { unlock_page(page); + if (PageHWPoison(page)) { + put_page(page); + retval = -EIO; + break; + } + /* * We have the page, copy it to user space buffer. */ @@ -1111,13 +1117,6 @@ static int hugetlbfs_migrate_folio(struct address_space *mapping, static int hugetlbfs_error_remove_page(struct address_space *mapping, struct page *page) { - struct inode *inode = mapping->host; - pgoff_t index = page->index; - - hugetlb_delete_from_page_cache(page_folio(page)); - if (unlikely(hugetlb_unreserve_pages(inode, index, index + 1, 1))) - hugetlb_fix_reserve_counts(inode); - return 0; } diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 97896165fd3f..5120a9ccbf5b 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -6101,6 +6101,10 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, ptl = huge_pte_lock(h, dst_mm, dst_pte); + ret = -EIO; + if (PageHWPoison(page)) + goto out_release_unlock; + /* * We allow to overwrite a pte marker: consider when both MISSING|WP * registered, we firstly wr-protect a none pte which has no page cache diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 145bb561ddb3..bead6bccc7f2 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -1080,6 +1080,7 @@ static int me_huge_page(struct page_state *ps, struct page *p) int res; struct page *hpage = compound_head(p); struct address_space *mapping; + bool extra_pins = false; if (!PageHuge(hpage)) return MF_DELAYED; @@ -1087,6 +1088,8 @@ static int me_huge_page(struct page_state *ps, struct page *p) mapping = page_mapping(hpage); if (mapping) { res = truncate_error_page(hpage, page_to_pfn(p), mapping); + /* The page is kept in page cache. */ + extra_pins = true; unlock_page(hpage); } else { unlock_page(hpage); @@ -1104,7 +1107,7 @@ static int me_huge_page(struct page_state *ps, struct page *p) } } - if (has_extra_refcount(ps, p, false)) + if (has_extra_refcount(ps, p, extra_pins)) res = MF_FAILED; return res;