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 36B1CC76196 for ; Tue, 4 Apr 2023 00:58:24 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id BB9506B0075; Mon, 3 Apr 2023 20:58:23 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B68A3900002; Mon, 3 Apr 2023 20:58:23 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A09956B007B; Mon, 3 Apr 2023 20:58:23 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 8C3A56B0075 for ; Mon, 3 Apr 2023 20:58:23 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 5EA371204B4 for ; Tue, 4 Apr 2023 00:58:23 +0000 (UTC) X-FDA: 80641897686.28.03E10F6 Received: from mail-pl1-f171.google.com (mail-pl1-f171.google.com [209.85.214.171]) by imf18.hostedemail.com (Postfix) with ESMTP id 995991C0016 for ; Tue, 4 Apr 2023 00:58:21 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=ew5ohV9V; spf=pass (imf18.hostedemail.com: domain of shy828301@gmail.com designates 209.85.214.171 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1680569901; h=from:from:sender: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:dkim-signature; bh=6mPWw+cp8u38n//2LxCVukXUZphVvDxZkQ7SxSLKPdg=; b=nw//EBg5+sabVAXwXY99fTil+JXhb/6pAEhabB+iMpgwvzHXFxcQAmQTLqrQuF5nwN8Pfn JzoTM98ajBOs/1S+oVKJrBfpuX6g94DJgcCbFdfTJuA/xJ6rOT+eipNjJ+oxZAqBe3BPFS B0Q0egaKU01rvkuPeyExSpoLtn5jLak= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=ew5ohV9V; spf=pass (imf18.hostedemail.com: domain of shy828301@gmail.com designates 209.85.214.171 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1680569901; a=rsa-sha256; cv=none; b=2vTM+x+toEdOW+CV+8SWvBFv7YKC1gw8aAJ0t0VOZBWxwJYqYwVuqbCRgvjXmRvpyIkG5e my1r7xih98I09ijHFEyTNdk2HK9fDIGyqaL9USW39ltTNOL+Gc4t1GN3thcczBVnkObM2K MIYOOs+3BlC1K543K6byUnaFKCfk99Y= Received: by mail-pl1-f171.google.com with SMTP id o11so29817007ple.1 for ; Mon, 03 Apr 2023 17:58:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1680569900; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=6mPWw+cp8u38n//2LxCVukXUZphVvDxZkQ7SxSLKPdg=; b=ew5ohV9V7tjDVx7tuFLPhpfxaYqNpO+SXfuREqneN2fOMRoZLAFP8Uayeio9arNyeZ G8D7WJRk/tdSoifp8dD3r/NinW4Y997taX6+0C2PmMirlWoSSvYmmG5temfWiBOKfGl6 j1uIVHxK/lZnDed1sNhRtxOO8YuRhO2hq10wO7fMdIjOYRmytr3dD9XuZfT8Xvfm4c19 UEmVJ5J1eWQF+lIqAiTKUKkPeIGq7gRAxAMoyaHpReqWNoiI88a9n6kcoxlqfLIkPqaf 53QeXcaVpY/aP0kDHqqkxEVQRXsW4H8hwZAOvusviaknI/kQCldA8xeX9eXD+fSUZAPQ SzSw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680569900; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=6mPWw+cp8u38n//2LxCVukXUZphVvDxZkQ7SxSLKPdg=; b=tKDQdbJtj7fDQpwMx9kAcxBMEPO3JWMTXcMN02rLtVjxmTf7yThbdg47OVEZ/lRWNH bgksjakq6JpvIgEcv7xZtldVSruXrZylFitb6WMTIx0fBwOjlEEjH5WQryaLiWKTF4vJ sV2i/v9oAnBAV1GC2AaAr+o3F2yXSMKM+deictTXfYVbg7pOgmX5F4sYq8YTxpkQUy0O Hx51eL2xdjOuln0qU9CaOPN8NSWZ4Qlz3q5JxnJ+W3/hlvtqe++c0k4Flpfo+VpZQb1A 1lZjHh/o4U3kRP8yX7jXVjlk8i/HTiL3YM15hk4i2rSxMC/YDktrX1UkcpstYdvOy3PV SZDQ== X-Gm-Message-State: AAQBX9eEcroP9TnvCrqH2RQwQ4AB5Fe8WRaAvU8maDQYBbP5WP+ChQxY Pzyz4DNgYPyK6+FjHz2EhnD8aTtchWxlUBYlI/w= X-Google-Smtp-Source: AKy350aPu9QE0B9P21qh06MAkRNBNUK2EH+YclgqKNaEDf6n4sBfV37d1dmdAMJ8B+lw2FaVY5XL4TuNFU6ZhceBR+8= X-Received: by 2002:a17:90a:f283:b0:23d:4e0e:cf2c with SMTP id fs3-20020a17090af28300b0023d4e0ecf2cmr284098pjb.3.1680569900309; Mon, 03 Apr 2023 17:58:20 -0700 (PDT) MIME-Version: 1.0 References: <20230330155305.423051-1-ivan.orlov0322@gmail.com> <20230331013301.ecgkjymaf3ws6rfb@google.com> In-Reply-To: <20230331013301.ecgkjymaf3ws6rfb@google.com> From: Yang Shi Date: Mon, 3 Apr 2023 17:58:08 -0700 Message-ID: Subject: Re: [PATCH v2] mm: khugepaged: Fix kernel BUG in hpage_collapse_scan_file To: "Zach O'Keefe" Cc: Ivan Orlov , akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, himadrispandya@gmail.com, skhan@linuxfoundation.org, linux-kernel-mentees@lists.linuxfoundation.org, syzbot+9578faa5475acb35fa50@syzkaller.appspotmail.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam03 X-Stat-Signature: tbstq7hm6quiyqbzdgwpo46cpiyb714d X-Rspamd-Queue-Id: 995991C0016 X-HE-Tag: 1680569901-174045 X-HE-Meta: U2FsdGVkX19IJXTF1sV+xL8AhOZevm0IVr2d9FDmyhrPcx/9AO1VjXpv8Sz10qdQg8+nPFjBZucLhXiARh1GvOdtmFCyipl7CBTsh5vfbe7avbx75svuTp+nnx7AjoZT6sjLl38EMmUDwRJs6QJSBu9iBpBJa9t/IGLo7EEylroJgLyBfGnYFMUNpf1VyguLwLLsfONAqJKlgFNcIywdGJaiIgDSAUOUcDzC6jRAxuwzzThPRNUwXDiMs781qgaFkxdEPvMeuuFO1rAL9NGHCfKZq/CFtgx/aKWS2JNeB6OE99XV1n9V5FAHOcfEwc5MZIdqqIvwxcqqM49LP83Hc4ddeXj4yPGWWavVbF83PP0+YLeh9hHS3j/9+RVBndtlWXQUp4PnAPX2jMsg4NwP7PgfQ1FJv520k0+/JY/TcUh3crK/NzhMRYl3ohwDAcwaiG8i8zR5cyhUH184UwibkkSktXEjqCQZcKc6TK9iXFAsc9MfpU7O6MUJloZpTQjVutquZFrn6DoMd9Fz6Hy365Xb4RyfIyq5SV35Tfj6IkFyIxG0/iKIDFlqoIKY1vZIiWZym0qhvLKYbtZUflXJ/B11DUemUyezyb5iixbkQV3/vHjkDJULoVNlhfjax+V/n0jJxbCfWTJwQ1AJiteRUh76yx66jQIn2G/3c9Xv6rRqi8y4nqgACxBZfOtLXCM7lGnsmdvlC/hLHJYLaj5q+2QwFXzzc3veegjCQpTjmv2NCl9W1cKuUP/RRh04wcwqx6LBelhNkYq7kVohVaqh0Ow/YYCNy5CgBxEkQsgNcQssNNc4+ZLnKBUJQUg4KEWT2hUJUoyaIuINcjE5mIVAbjtrFF2+3FeEuDEQPMwk3iLjGwfJfiCDA/QZ+vRHUshMNfaDFD+U+NIz3MOMMplqtwET8SnOno+5vWGsITw4vmzzh9UMdW79bnO6+Av5CQG2l1KhXocMbOvDtRAatKN SK61OcvT BONaY2cUH9S3FBhWnOdR/822orvRor1G30cQ087cXxgvZGNT23J66Mo6xe3ZXoIbKi7rVQjZms+nAjVEjfwuezNpGusYVGyVZtcbmA3PsLLBgvmtgUVdfObZ/WEEkH7NWy1PiccAoSu9aH80XYPD0HO8ESWg4WTUgj+m70WM4JP654tqBfEW5nIyonQi9R+aRcSpmauvl+IfdZWyiMqR/gl6QDgpwtxnKVD11jNjtn2GHyhwFEvKXIpq59HN/rKUgETtxgx9awTabPVcV8fIs909qgMNh/OYAY9bvaqdkTXs5IJHJAxbFRPVdBk4ggDGYLELudog0e92yZ03a/uDZfBlNuIVSiVVLHnuk5+tvd4E4vYYf21QXv2SUzNxsidGZMLe7Fh6RkdFzDrDfXxnCKXMAzOyeVmD2rE4V1Q8dH1lPhfFIvecCD4Ko9oy981BRfN4YyA+jA1xzVwVPUn/8qH4Dt7caQ0XL8tMpW98jmJn+QmUoqKfvve8hJc0v5vTE//ajuxZltTVOGl8W+8cxxHxDJfMjRdCJXDjr4GhO7/mh0YA27wsViYowiekrl9tGTA67ih8u6devx+hJXcVfoc+07S65XJ2juRgYEKc39M3KsYHNPMgWAWvsDcqwFjFBY+NUlKGUakOg5XHOUUec37eDvYk/wVuM5XyyjbQc/z9qSo4= 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 30, 2023 at 6:33=E2=80=AFPM Zach O'Keefe w= rote: > > On Mar 30 19:53, Ivan Orlov wrote: > > Syzkaller reported the following issue: > > > > kernel BUG at mm/khugepaged.c:1823! > > invalid opcode: 0000 [#1] PREEMPT SMP KASAN > > CPU: 1 PID: 5097 Comm: syz-executor220 Not tainted 6.2.0-syzkaller-1315= 4-g857f1268a591 #0 > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS= Google 02/16/2023 > > RIP: 0010:collapse_file mm/khugepaged.c:1823 [inline] > > RIP: 0010:hpage_collapse_scan_file+0x67c8/0x7580 mm/khugepaged.c:2233 > > Code: 00 00 89 de e8 c9 66 a3 ff 31 ff 89 de e8 c0 66 a3 ff 45 84 f6 0f= 85 28 0d 00 00 e8 22 64 a3 ff e9 dc f7 ff ff e8 18 64 a3 ff <0f> 0b f3 0f = 1e fa e8 0d 64 a3 ff e9 93 f6 ff ff f3 0f 1e fa 4c 89 > > RSP: 0018:ffffc90003dff4e0 EFLAGS: 00010093 > > RAX: ffffffff81e95988 RBX: 00000000000001c1 RCX: ffff8880205b3a80 > > RDX: 0000000000000000 RSI: 00000000000001c0 RDI: 00000000000001c1 > > RBP: ffffc90003dff830 R08: ffffffff81e90e67 R09: fffffbfff1a433c3 > > R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000000 > > R13: ffffc90003dff6c0 R14: 00000000000001c0 R15: 0000000000000000 > > FS: 00007fdbae5ee700(0000) GS:ffff8880b9900000(0000) knlGS:00000000000= 00000 > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > CR2: 00007fdbae6901e0 CR3: 000000007b2dd000 CR4: 00000000003506e0 > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > > Call Trace: > > > > madvise_collapse+0x721/0xf50 mm/khugepaged.c:2693 > > madvise_vma_behavior mm/madvise.c:1086 [inline] > > madvise_walk_vmas mm/madvise.c:1260 [inline] > > do_madvise+0x9e5/0x4680 mm/madvise.c:1439 > > __do_sys_madvise mm/madvise.c:1452 [inline] > > __se_sys_madvise mm/madvise.c:1450 [inline] > > __x64_sys_madvise+0xa5/0xb0 mm/madvise.c:1450 > > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > > do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 > > entry_SYSCALL_64_after_hwframe+0x63/0xcd > > > > Thanks, Ivan. > > In the process of reviewing this, I starting thinking if the !shmem case = was > also susceptible to a similar race, and I *think* it might be. Unfortunat= ely, my > time has ran out, and I haven't been able to validate ; I'm less familiar= with > the file-side of things. > > The underlying problem is race with truncation/hole-punch under OOM cond= ition. > The nice do-while loop near the top of collapse_file() attempts to avoid = this > scenario by making sure enough slots are available. However, when we drop= xarray > lock, we open ourselves up to concurrent removal + slot deletion. Those s= lots > then need to be allocated again -- which under OOM condition is failable. > > The syzbot reproducer picks on shmem, but I think this can occur for file= as > well. If we find a hole, we unlock the xarray and call > page_cache_sync_readahead(), which if it succeeds, IIUC, will have alloca= ted a > new slot in our mapping pointing to the new page. We *then* locks the pag= e. Only > after the page is locked are we protected from concurrent removal (Note: = this is > what provides us protection in many of the xas_store() cases ; we've held= the > slot's contained page-lock since verifying the slot exists, protecting us= from > removal / reallocation races). IIUC, the find_lock_page() should be able to handle the race. It does check whether the page is truncated or not after locking the page. > > Maybe I'm just low on caffeine at the end of the day, and am missing some= thing, > but if I had more time, I'd be looking into the file-side some more to ve= rify. > Apologies that hasn't occurred to me until now ; I was looking at one of = your > comments and double-checked why I *thought* we were safe. > > Anyways, irrespective of that looming issues, some more notes to follow: > > > The 'xas_store' call during page cache scanning can potentially > > translate 'xas' into the error state (with the reproducer provided > > by the syzkaller the error code is -ENOMEM). However, there are no > > further checks after the 'xas_store', and the next call of 'xas_next' > > at the start of the scanning cycle doesn't increase the xa_index, > > and the issue occurs. > > > > This patch will add the xarray state error checking after the > > 'xas_store' and the corresponding result error code. It will > > also add xarray state error checking via WARN_ON_ONCE macros, > > to be sure that ENOMEM or other possible errors don't occur > > at the places they shouldn't. > > Thanks for the additions here. I think it's worthwhile providing even mor= e > details about the specifics of the race we are fixing and/or guarding aga= inst to > help ppl understand how that -ENOMEM comes about if the do-while loop has > "Ensured" we have slots available (additionally, I think that comment can= be > augmented). > > > Tested via syzbot. > > > > Reported-by: syzbot+9578faa5475acb35fa50@syzkaller.appspotmail.com > > Link: https://syzkaller.appspot.com/bug?id=3D7d6bb3760e026ece7524500fe4= 4fb024a0e959fc > > Signed-off-by: Ivan Orlov > > --- > > V1 -> V2: Add WARN_ON_ONCE error checking and comments > > > > mm/khugepaged.c | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > > index 92e6f56a932d..8b6580b13339 100644 > > --- a/mm/khugepaged.c > > +++ b/mm/khugepaged.c > > @@ -55,6 +55,7 @@ enum scan_result { > > SCAN_CGROUP_CHARGE_FAIL, > > SCAN_TRUNCATED, > > SCAN_PAGE_HAS_PRIVATE, > > + SCAN_STORE_FAILED, > > }; > > I'm still reluctant to add a new error code for this as this seems like q= uite a > rare race that requires OOM to trigger. I'd be happier just reusing SCAN_= FAIL, > or, something we might get some millage out of later: SCAN_OOM. > > Also, a reminder to update include/trace/events/huge_memory.h, if you go = that > route. > > > > > #define CREATE_TRACE_POINTS > > @@ -1840,6 +1841,15 @@ static int collapse_file(struct mm_struct *mm, u= nsigned long addr, > > goto xa_locked; > > } > > xas_store(&xas, hpage); > > + if (xas_error(&xas)) { > > + /* revert shmem_charge performed > > + * in the previous condition > > + */ > > Nit: Here, and following, I think standard convention for multiline comme= nt is > to have an empty first and last line, eg: > > + /* > + * revert shmem_charge performed > + * in the previous condition > + */ > > Though, checkpatch.pl --strict didn't seem to care. > > > + mapping->nrpages--; > > + shmem_uncharge(mapping->host, 1); > > + result =3D SCAN_STORE_FAILED; > > + goto xa_locked; > > + } > > nr_none++; > > continue; > > } > > @@ -1992,6 +2002,11 @@ static int collapse_file(struct mm_struct *mm, u= nsigned long addr, > > > > /* Finally, replace with the new page. */ > > xas_store(&xas, hpage); > > + /* We can't get an ENOMEM here (because the allocation ha= ppened before) > > + * but let's check for errors (XArray implementation can = be > > + * changed in the future) > > + */ > > + WARN_ON_ONCE(xas_error(&xas)); > > Nit: it's not just that allocation happened before -- need some guarantee= we've > been protected from concurrent removal. This is what made me look at the = file > side. > > > continue; > > out_unlock: > > unlock_page(page); > > @@ -2029,6 +2044,11 @@ static int collapse_file(struct mm_struct *mm, u= nsigned long addr, > > /* Join all the small entries into a single multi-index entry */ > > xas_set_order(&xas, start, HPAGE_PMD_ORDER); > > xas_store(&xas, hpage); > > + /* Here we can't get an ENOMEM (because entries were > > + * previously allocated) But let's check for errors > > + * (XArray implementation can be changed in the future) > > + */ > > + WARN_ON_ONCE(xas_error(&xas)); > > Ditto. > > Apologies I won't be around to see this change through -- I'm just out of= time, > and will be shutting my computer down tomorrow for 3 months. Sorry for t= he poor > timing, for raising issues, then disappearing. Hopefully I'm wrong and th= e > file-side isn't a concern. > > Best, > Zach > > > xa_locked: > > xas_unlock_irq(&xas); > > xa_unlocked: > > -- > > 2.34.1 > >