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 CE64EC433EF for ; Sat, 16 Apr 2022 04:25:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4C5DB6B0072; Sat, 16 Apr 2022 00:25:46 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 475BA6B0073; Sat, 16 Apr 2022 00:25:46 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 33D646B0074; Sat, 16 Apr 2022 00:25:46 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0250.hostedemail.com [216.40.44.250]) by kanga.kvack.org (Postfix) with ESMTP id 22C016B0072 for ; Sat, 16 Apr 2022 00:25:46 -0400 (EDT) Received: from smtpin30.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id D16CA1833B002 for ; Sat, 16 Apr 2022 04:25:45 +0000 (UTC) X-FDA: 79361453850.30.8CD67F4 Received: from out2.migadu.com (out2.migadu.com [188.165.223.204]) by imf10.hostedemail.com (Postfix) with ESMTP id 146ADC0003 for ; Sat, 16 Apr 2022 04:25:44 +0000 (UTC) Date: Sat, 16 Apr 2022 13:25:36 +0900 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1650083143; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4sCmTR67ANX5zgjllBNBvU/F0lbCm+PnkqjbDF5PCS4=; b=LU2o/zcZCHXeg7zlG8pVv+KuxnVqISCCdciw3nRoBfbRL8iYDGX7CTlWMoEKa9bbMceYOg GK7EnbEOKWpEcvlDBAaKxA7jYeTwh5E5iqxwKNrPZostoUOVLyTTfYOFKjBXP13W5Jtxrp C65UyNi1pJizuHmVzeIbiBg5fk9w4Ss= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Naoya Horiguchi To: John Hubbard , Miaohe Lin Cc: Matthew Wilcox , Christoph Hellwig , Andrew Morton , Jason Gunthorpe , William Kucharski , Naoya Horiguchi , linux-kernel@vger.kernel.org, Linux-MM Subject: Re: [PATCH v1] mm: recheck PageCompound() before printing out info of head page Message-ID: <20220416042536.GA1058054@u2004> References: <20220414235950.840409-1-naoya.horiguchi@linux.dev> <21807e0d-8bd1-7b1a-dd76-85a02267b424@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <21807e0d-8bd1-7b1a-dd76-85a02267b424@huawei.com> X-Migadu-Flow: FLOW_OUT X-Migadu-Auth-User: linux.dev X-Stat-Signature: xomg6cdyjcr48c3qfheshggidtrb8rgz X-Rspam-User: Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b="LU2o/zcZ"; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf10.hostedemail.com: domain of naoya.horiguchi@linux.dev designates 188.165.223.204 as permitted sender) smtp.mailfrom=naoya.horiguchi@linux.dev X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 146ADC0003 X-HE-Tag: 1650083144-51310 Content-Transfer-Encoding: quoted-printable 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 Sat, Apr 16, 2022 at 09:56:41AM +0800, Miaohe Lin wrote: > 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: > >> > >> =C2=A0=C2=A0 [ 1160.232771] page:00000000b36a8a0f refcount:1 mapcoun= t:0 mapping:0000000000000000 index:0x1 pfn:0x16a000 > >> =C2=A0=C2=A0 [ 1160.236916] page:00000000b36a8a0f refcount:0 mapcoun= t:0 mapping:0000000000000000 index:0x1 pfn:0x16a000 > >> =C2=A0=C2=A0 [ 1160.240684] flags: 0x57ffffc0800000(hwpoison|node=3D= 1|zone=3D2|lastcpupid=3D0x1fffff) > >> =C2=A0=C2=A0 [ 1160.243458] raw: 0057ffffc0800000 dead000000000100 d= ead000000000122 0000000000000000 > >> =C2=A0=C2=A0 [ 1160.246268] raw: 0000000000000001 0000000000000000 0= 0000000ffffffff 0000000000000000 > >> =C2=A0=C2=A0 [ 1160.249197] page dumped because: VM_BUG_ON_FOLIO(!fo= lio_test_large(folio)) > >> =C2=A0=C2=A0 [ 1160.251815] ------------[ cut here ]------------ > >> =C2=A0=C2=A0 [ 1160.253438] kernel BUG at include/linux/mm.h:788! > >> =C2=A0=C2=A0 [ 1160.256162] invalid opcode: 0000 [#1] PREEMPT SMP PT= I > >> =C2=A0=C2=A0 [ 1160.258172] CPU: 2 PID: 115368 Comm: mceinj.sh Taint= ed: G=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 E= =C2=A0=C2=A0=C2=A0=C2=A0 5.18.0-rc1-v5.18-rc1-220404-2353-005-g83111+ #3 > >> =C2=A0=C2=A0 [ 1160.262049] Hardware name: QEMU Standard PC (i440FX = + PIIX, 1996), BIOS 1.15.0-1.fc35 04/01/2014 > >> =C2=A0=C2=A0 [ 1160.265103] RIP: 0010:dump_page.cold+0x27e/0x2bd > >> =C2=A0=C2=A0 [ 1160.266757] Code: fe ff ff 48 c7 c6 81 f1 5a 98 e9 4= c 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 0= 4 24 a9 00 00 > >> =C2=A0=C2=A0 [ 1160.273180] RSP: 0018:ffffaa2c4d59fd18 EFLAGS: 00010= 292 > >> =C2=A0=C2=A0 [ 1160.274969] RAX: 000000000000003e RBX: 0000000000000= 001 RCX: 0000000000000000 > >> =C2=A0=C2=A0 [ 1160.277263] RDX: 0000000000000001 RSI: ffffffff98599= 5a1 RDI: 00000000ffffffff > >> =C2=A0=C2=A0 [ 1160.279571] RBP: ffffdc9c45a80000 R08: 0000000000000= 000 R09: 00000000ffffdfff > >> =C2=A0=C2=A0 [ 1160.281794] R10: ffffaa2c4d59fb08 R11: ffffffff98940= d08 R12: ffffdc9c45a80000 > >> =C2=A0=C2=A0 [ 1160.283920] R13: ffffffff985b6f94 R14: 0000000000000= 000 R15: ffffdc9c45a80000 > >> =C2=A0=C2=A0 [ 1160.286641] FS:=C2=A0 00007eff54ce1740(0000) GS:ffff= 99c67bd00000(0000) knlGS:0000000000000000 > >> =C2=A0=C2=A0 [ 1160.289498] CS:=C2=A0 0010 DS: 0000 ES: 0000 CR0: 00= 00000080050033 > >> =C2=A0=C2=A0 [ 1160.291106] CR2: 00005628381a5f68 CR3: 0000000104712= 003 CR4: 0000000000170ee0 > >> =C2=A0=C2=A0 [ 1160.293031] Call Trace: > >> =C2=A0=C2=A0 [ 1160.293724]=C2=A0 > >> =C2=A0=C2=A0 [ 1160.294334]=C2=A0 get_hwpoison_page+0x47d/0x570 > >> =C2=A0=C2=A0 [ 1160.295474]=C2=A0 memory_failure+0x106/0xaa0 > >> =C2=A0=C2=A0 [ 1160.296474]=C2=A0 ? security_capable+0x36/0x50 > >> =C2=A0=C2=A0 [ 1160.297524]=C2=A0 hard_offline_page_store+0x43/0x80 > >> =C2=A0=C2=A0 [ 1160.298684]=C2=A0 kernfs_fop_write_iter+0x11c/0x1b0 > >> =C2=A0=C2=A0 [ 1160.299829]=C2=A0 new_sync_write+0xf9/0x160 > >> =C2=A0=C2=A0 [ 1160.300810]=C2=A0 vfs_write+0x209/0x290 > >> =C2=A0=C2=A0 [ 1160.301835]=C2=A0 ksys_write+0x4f/0xc0 > >> =C2=A0=C2=A0 [ 1160.302718]=C2=A0 do_syscall_64+0x3b/0x90 > >> =C2=A0=C2=A0 [ 1160.303664]=C2=A0 entry_SYSCALL_64_after_hwframe+0x4= 4/0xae > >> =C2=A0=C2=A0 [ 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: > >> > >> =C2=A0=C2=A0 CPU 0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 CPU 1 > >> > >> =C2=A0=C2=A0=C2=A0=C2=A0 memory_failure > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 get_hwpoison_page > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 get_any_page > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 dump_pa= ge > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 compound =3D PageCompound > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fre= e_pages_prepare > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 page->flags &=3D ~PAGE_FLAGS_CHECK_AT_PREP > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 folio_entire_mapcount > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 VM_BUG_ON_FOLIO(!folio_test_large(folio)) > >> > >> So in order to avoid the BUG_ON, recheck PageCompound() before __dum= p_page() > >> prints out information about the head page.=C2=A0 Precisely saying, = this does > >> not close up the race, but mitigates the impact to just inaccurate i= nfo, > >> which should be acceptable. > >=20 > > 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, ri= ght? >=20 > 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= ? OK, so memory_failure() simply shouldn't call dump_page() before taking p= age refcnt. >=20 > >=20 > >> > >> Fixes: 74e8ee4708a8 ("mm: Turn head_compound_mapcount() into folio_e= ntire_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(), simpl= y > >> replacing the dump_page() with simple printk() might be a proper fix > >=20 > > 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. Thanks, so now I lean to the printk-approach. > >=20 > >> (I confirmed that that fix also works).=C2=A0 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. > >=20 > > *If* there are other races, they should be addressed separately, IMHO= . > > Because MCE should be a special case. I agree. Thanks, Naoya Horiguchi > >=20 > >> --- > >> =C2=A0 mm/debug.c | 2 +- > >> =C2=A0 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) > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_warn("page:%p refcount:%d mapcount= :%d mapping:%p index:%#lx pfn:%#lx\n", > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 page, page_ref_count(head), mapcount, mapping, > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 page_to_pgoff(page), page_to_pfn(page)); > >> -=C2=A0=C2=A0=C2=A0 if (compound) { > >> +=C2=A0=C2=A0=C2=A0 if (compound && PageCompound(page)) { > >=20 > > If people decide to keep this approach, then I'd go with something > > that makes it clear, like: > >=20 > > =C2=A0=C2=A0=C2=A0=C2=A0/* Re-check, in a semi-doomed attempt to avoi= d racing with MCE: */ > > =C2=A0=C2=A0=C2=A0=C2=A0compound =3D PageCompound(page); >=20 > Agree. A comment will be needed or the code might look obscure. >=20 > Thanks! >=20 > >=20 > > :) > >=20 > > thanks, >=20