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 411BFC433F5 for ; Sat, 16 Apr 2022 01:56:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B88D26B0072; Fri, 15 Apr 2022 21:56:45 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B35F66B0073; Fri, 15 Apr 2022 21:56:45 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A24896B0074; Fri, 15 Apr 2022 21:56:45 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.hostedemail.com [64.99.140.27]) by kanga.kvack.org (Postfix) with ESMTP id 92D156B0072 for ; Fri, 15 Apr 2022 21:56:45 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay12.hostedemail.com (Postfix) with ESMTP id 7DAA6120808 for ; Sat, 16 Apr 2022 01:56:45 +0000 (UTC) X-FDA: 79361078370.12.E4D7019 Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by imf16.hostedemail.com (Postfix) with ESMTP id 56D93180003 for ; Sat, 16 Apr 2022 01:56:44 +0000 (UTC) Received: from canpemm500002.china.huawei.com (unknown [172.30.72.53]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4KgGX82ftWzRRf7; Sat, 16 Apr 2022 09:56:40 +0800 (CST) Received: from [10.174.177.76] (10.174.177.76) by canpemm500002.china.huawei.com (7.192.104.244) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Sat, 16 Apr 2022 09:56:41 +0800 Subject: Re: [PATCH v1] mm: recheck PageCompound() before printing out info of head page To: John Hubbard , Naoya Horiguchi , Matthew Wilcox CC: Christoph Hellwig , Andrew Morton , Jason Gunthorpe , William Kucharski , Naoya Horiguchi , , Linux-MM References: <20220414235950.840409-1-naoya.horiguchi@linux.dev> From: Miaohe Lin Message-ID: <21807e0d-8bd1-7b1a-dd76-85a02267b424@huawei.com> Date: Sat, 16 Apr 2022 09:56:41 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [10.174.177.76] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To canpemm500002.china.huawei.com (7.192.104.244) X-CFilter-Loop: Reflected X-Rspam-User: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 56D93180003 X-Stat-Signature: 6f7pu95ymj5o8o56nd3dz8zgbn7859af Authentication-Results: imf16.hostedemail.com; dkim=none; spf=pass (imf16.hostedemail.com: domain of linmiaohe@huawei.com designates 45.249.212.188 as permitted sender) smtp.mailfrom=linmiaohe@huawei.com; dmarc=pass (policy=quarantine) header.from=huawei.com X-HE-Tag: 1650074204-829414 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 2022/4/15 9:08, John Hubbard wrote: > On 4/14/22 16:59, Naoya Horiguchi wrote: >> From: Naoya Horiguchi >> >> The following VM_BUG_ON_FOLIO() is triggered when memory error event >> happens on the (thp/folio) pages which are about to be freed: >> >>    [ 1160.232771] page:00000000b36a8a0f refcount:1 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x16a000 >>    [ 1160.236916] page:00000000b36a8a0f refcount:0 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x16a000 >>    [ 1160.240684] flags: 0x57ffffc0800000(hwpoison|node=1|zone=2|lastcpupid=0x1fffff) >>    [ 1160.243458] raw: 0057ffffc0800000 dead000000000100 dead000000000122 0000000000000000 >>    [ 1160.246268] raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000 >>    [ 1160.249197] page dumped because: VM_BUG_ON_FOLIO(!folio_test_large(folio)) >>    [ 1160.251815] ------------[ cut here ]------------ >>    [ 1160.253438] kernel BUG at include/linux/mm.h:788! >>    [ 1160.256162] invalid opcode: 0000 [#1] PREEMPT SMP PTI >>    [ 1160.258172] CPU: 2 PID: 115368 Comm: mceinj.sh Tainted: G            E     5.18.0-rc1-v5.18-rc1-220404-2353-005-g83111+ #3 >>    [ 1160.262049] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 04/01/2014 >>    [ 1160.265103] RIP: 0010:dump_page.cold+0x27e/0x2bd >>    [ 1160.266757] Code: fe ff ff 48 c7 c6 81 f1 5a 98 e9 4c fe ff ff 48 c7 c6 a1 95 59 98 e9 40 fe ff ff 48 c7 c6 50 bf 5a 98 48 89 ef e8 9d 04 6d ff <0f> 0b 41 f7 c4 ff 0f 00 00 0f 85 9f fd ff ff 49 8b 04 24 a9 00 00 >>    [ 1160.273180] RSP: 0018:ffffaa2c4d59fd18 EFLAGS: 00010292 >>    [ 1160.274969] RAX: 000000000000003e RBX: 0000000000000001 RCX: 0000000000000000 >>    [ 1160.277263] RDX: 0000000000000001 RSI: ffffffff985995a1 RDI: 00000000ffffffff >>    [ 1160.279571] RBP: ffffdc9c45a80000 R08: 0000000000000000 R09: 00000000ffffdfff >>    [ 1160.281794] R10: ffffaa2c4d59fb08 R11: ffffffff98940d08 R12: ffffdc9c45a80000 >>    [ 1160.283920] R13: ffffffff985b6f94 R14: 0000000000000000 R15: ffffdc9c45a80000 >>    [ 1160.286641] FS:  00007eff54ce1740(0000) GS:ffff99c67bd00000(0000) knlGS:0000000000000000 >>    [ 1160.289498] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >>    [ 1160.291106] CR2: 00005628381a5f68 CR3: 0000000104712003 CR4: 0000000000170ee0 >>    [ 1160.293031] Call Trace: >>    [ 1160.293724]  >>    [ 1160.294334]  get_hwpoison_page+0x47d/0x570 >>    [ 1160.295474]  memory_failure+0x106/0xaa0 >>    [ 1160.296474]  ? security_capable+0x36/0x50 >>    [ 1160.297524]  hard_offline_page_store+0x43/0x80 >>    [ 1160.298684]  kernfs_fop_write_iter+0x11c/0x1b0 >>    [ 1160.299829]  new_sync_write+0xf9/0x160 >>    [ 1160.300810]  vfs_write+0x209/0x290 >>    [ 1160.301835]  ksys_write+0x4f/0xc0 >>    [ 1160.302718]  do_syscall_64+0x3b/0x90 >>    [ 1160.303664]  entry_SYSCALL_64_after_hwframe+0x44/0xae >>    [ 1160.304981] RIP: 0033:0x7eff54b018b7 >> >> As shown in the RIP address, this VM_BUG_ON in folio_entire_mapcount() is >> called from dump_page("hwpoison: unhandlable page") in get_any_page(). >> The below explains the mechanism of the race: >> >>    CPU 0                                       CPU 1 >> >>      memory_failure >>        get_hwpoison_page >>          get_any_page >>            dump_page >>              compound = PageCompound >>                                                  free_pages_prepare >>                                                    page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP >>              folio_entire_mapcount >>                VM_BUG_ON_FOLIO(!folio_test_large(folio)) >> >> So in order to avoid the BUG_ON, recheck PageCompound() before __dump_page() >> prints out information about the head page.  Precisely saying, this does >> not close up the race, but mitigates the impact to just inaccurate info, >> which should be acceptable. > > Actually, I don't think it really closes the race even to that extent, > because a machine check (MCE, on x86) could come in at any point, so > rechecking (without locks) won't really completely fix such a bug, right? It might also be unsafe to do dump_mapping or dump_page_owner when page could be freed at any time or we might come across some use-after-free issues? > >> >> Fixes: 74e8ee4708a8 ("mm: Turn head_compound_mapcount() into folio_entire_mapcount()") >> Signed-off-by: Naoya Horiguchi >> --- >> I'm actually not sure how widely the race condition could affect. >> If we can consider this as a bug specific to memory_failure(), simply >> replacing the dump_page() with simple printk() might be a proper fix > > I think we should have separate, limited, special case functions that > are safe for use by exotic things like the MCE handler. And so just > doing a printk there seems better to me. > >> (I confirmed that that fix also works).  But if any other dump_page()'s >> caller could race with freeing thp/folio, the more generic fix >> (I'm suggesting in this patch) would be desirable. > > *If* there are other races, they should be addressed separately, IMHO. > Because MCE should be a special case. > >> --- >>   mm/debug.c | 2 +- >>   1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/debug.c b/mm/debug.c >> index bef329bf28f0..fce63b200bea 100644 >> --- a/mm/debug.c >> +++ b/mm/debug.c >> @@ -93,7 +93,7 @@ static void __dump_page(struct page *page) >>       pr_warn("page:%p refcount:%d mapcount:%d mapping:%p index:%#lx pfn:%#lx\n", >>               page, page_ref_count(head), mapcount, mapping, >>               page_to_pgoff(page), page_to_pfn(page)); >> -    if (compound) { >> +    if (compound && PageCompound(page)) { > > If people decide to keep this approach, then I'd go with something > that makes it clear, like: > >     /* Re-check, in a semi-doomed attempt to avoid racing with MCE: */ >     compound = PageCompound(page); Agree. A comment will be needed or the code might look obscure. Thanks! > > :) > > thanks,