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 E2387C7619A for ; Wed, 5 Apr 2023 16:43:53 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3596F6B0071; Wed, 5 Apr 2023 12:43:53 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3099A6B0074; Wed, 5 Apr 2023 12:43:53 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1D1646B0075; Wed, 5 Apr 2023 12:43:53 -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 0A5356B0071 for ; Wed, 5 Apr 2023 12:43:53 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id D919DA0BD0 for ; Wed, 5 Apr 2023 16:43:52 +0000 (UTC) X-FDA: 80647909104.04.8D5AEB4 Received: from mail-ed1-f52.google.com (mail-ed1-f52.google.com [209.85.208.52]) by imf19.hostedemail.com (Postfix) with ESMTP id 02E061A0018 for ; Wed, 5 Apr 2023 16:43:49 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=BJSatbPa; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf19.hostedemail.com: domain of zokeefe@google.com designates 209.85.208.52 as permitted sender) smtp.mailfrom=zokeefe@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1680713030; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=5TPofhCRqsaDy+QKQvLq1nSwBkzlsiF3K1oEcaeWETM=; b=uRHxdRZBvYhkhH0wx9K9HHkhvyYo8lKuPajiSqgE3E9eVrGmiP2ZMF6z4fz9Id2ILxmsn1 wStRY66Z7WMcWH4yOYxBIPORU7Zyn3g7HmYvb80Mtk+3ZmErC5z2ZGgwVxd5Jw0zA/uFDh xMSFwAXczobq6CmLeVfJgPLO0lJV2jY= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=BJSatbPa; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf19.hostedemail.com: domain of zokeefe@google.com designates 209.85.208.52 as permitted sender) smtp.mailfrom=zokeefe@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1680713030; a=rsa-sha256; cv=none; b=vb2g6fwLW6h22bgotbzpXmKcyMSOKVhOR0nlqLLRwAGa4FsKWt2QYInivLuLjBhOVJGHeW IOG0cUJb6uqxjJ6LZV4VnaBGRQo8X8XV1/whSzFZjBvdhwxGyvp4+FPu2yIRM/LpSdU0FM Tzw0YyiFff5ZNGwKnfJEgCRNvkd1L9c= Received: by mail-ed1-f52.google.com with SMTP id b20so142621237edd.1 for ; Wed, 05 Apr 2023 09:43:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680713028; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=5TPofhCRqsaDy+QKQvLq1nSwBkzlsiF3K1oEcaeWETM=; b=BJSatbPaHofJrC6zTpy2LSmzhLS6wGpgUE/yxHwMHt14zaVJ7j2hMM4hcIWsUMyniM C0zc+hvP3Mn1uhlwIYzgoVXROsKYNv1hiERfBFYGH1qcCoH6S4O0sJOZp/Kzx7rnNj9e vT0kMTBLAkF3VLAU/zwcey/VZwB61sCjj1VUW9Xc6RZJfxPPVYX3ZCyj2lPNH3oaSU8y jSbe2k2D8fQwAW63JjsL5GA3ETZesnfDiqRa+JinPNm8iMxmDwabdy0sXMx6KzmRiDtC KtwDL4llhQV4WC/oFSO7rzs+7vMvyfh3+rF1fCEYZXl0gl71+wEJKfBhYVXXbRsiA0/5 CvOg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680713028; h=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=5TPofhCRqsaDy+QKQvLq1nSwBkzlsiF3K1oEcaeWETM=; b=agmWjkYCDfM/qaNdQNNfsjsebSE0s4HcR6ix+GuxbNXB/xlDGTa1Y+fzC0O3oWCt5H 6IQWRg+X/uV52k+BF6kcMxnb6pM/ienT/BB8IxEkPcvZudc6nRlXUxh2TGVsfLUKjuLI N3Eio7QtwLJjYYabSqPh77UNQ18t1PGWkXfgFihWZNQK9Os5+Xe9q36TYgBDFu8Rxqf7 pDsLR+Njzo4g2OYo0vHFk8Nd6aijasQYer95yM9lQqeXnIUJrUoRoaEr69B5hMiJaTWN SSOux4IhmF18K69ercGhlPUiKLhydQ+CmO/vIkWPX9JXfvQcGqNta3SByrQeL3eXpnvF ncLw== X-Gm-Message-State: AAQBX9fBm//ROvH5O8tFUPyVpVd5nP3WavS4MKt5GJzAbfDALSZlZOpZ K0iyvEdKBmwb5DOgQu9qoM8a4op7G+2p56ul25Y1Wg== X-Google-Smtp-Source: AKy350brlMtCY3MiHnXwsnAhEHJalJ563bCxczrxwMKUFenq7zwkD9t+eK36V53WCL82GEcl+rfePCbO5dT/2PpAUWU= X-Received: by 2002:a17:907:8b92:b0:92a:a75e:6b9 with SMTP id tb18-20020a1709078b9200b0092aa75e06b9mr1773679ejc.11.1680713027992; Wed, 05 Apr 2023 09:43:47 -0700 (PDT) MIME-Version: 1.0 References: <20230330155305.423051-1-ivan.orlov0322@gmail.com> <20230331013301.ecgkjymaf3ws6rfb@google.com> In-Reply-To: From: "Zach O'Keefe" Date: Wed, 5 Apr 2023 09:43:36 -0700 Message-ID: Subject: Re: [PATCH v2] mm: khugepaged: Fix kernel BUG in hpage_collapse_scan_file To: Yang Shi Cc: Ivan Orlov , akpm@linux-foundation.org, himadrispandya@gmail.com, linux-kernel@vger.kernel.org, linux-kernel-mentees@lists.linuxfoundation.org, linux-mm@kvack.org, skhan@linuxfoundation.org, syzbot+9578faa5475acb35fa50@syzkaller.appspotmail.com Content-Type: multipart/alternative; boundary="000000000000018df005f8997fc3" X-Rspamd-Queue-Id: 02E061A0018 X-Rspamd-Server: rspam09 X-Rspam-User: X-Stat-Signature: b1ixt899o8kkqaar4boykjxzdpo5bt5m X-HE-Tag: 1680713029-906345 X-HE-Meta: U2FsdGVkX197igYxT4wMesfZgj7cBSmuIg6qF+Jd0G74c+GzE5Clo4epv+mPS6BEihk+rifDA0qEHXcz2MXzwSazmAjKZKKK/J+ogrA12FO2MnThafkJJD3yt6mkfynwoqPVEGB5PlljQeMFt56QkuXOiMFOAfG0HYf5cCqyRxh0j/YgmkkaaNKXvvZV9GE5gvSopuOMsxSwxZgN0Rx2/w+axfzNaQrui2kBsJUr2QcuvpbM3/QSEQ7+qrALCCrs+kPoX8nl3/X96nPSlfM2UoSTwc2hC5h85PHqpF5xni3IoHMyhu5uaiyrtQ5k0amVPoHPfIsT2LLXoJwoyymPY7uMdYh+5tagn+qJWIdXB7vYmBYUufwCt+eM8pFWK/fkIVEli85SZ8icbYvakZhQbgjmOVFPqVez66iqF3C7ot8K/VMFOUHMxnF7EOJFD8aFPOEqpvcA/hkRLLrKl9OYhnLRFSaz2SMUgnl7EFVrsCQAdgZcBVZyCe97P/5L2HoOGGdfTxUZIvyynjZ7PZzPrxro3wQliKYOPqUMPD8Wdi5RoVdHqABvqcEkoavIiMCMmmjkq4LezVNCVI/jF3HLa88+E17nQe3KPqwEfqfWEg0TyzxDsUeudAkpDmrAKD2Mz2C71sZvS6eZQgDSCy0WX0cO9g+GEAwhg1GWCLvDyd6uEyP63iT14mSeIcq86v7QfqoOhyhF1+pSfhS3yfFEsd06wGdVwepzDnmyCkc2Fz2EVNTZhivuDZk0YadyO1kYjof4xBFh3SmGXrSDv+YeSM0g6ubBmplWfQER8G4EdBXOGunP3l06jT284CputEqgtvSjqDMRKkGGYdD+eqFtDsjauVT81r5QPjMG5lADpTwNUHF1ZDlIq1v5qsTORyWElXp8mc/2nyADPE2no0XydSkszIzvLtc98MpKLgAYZsKexz1Qzug59dEIQSxST8JFKA+/QMXsV3RnpXse5+w pFSxuhDQ omFxlV2n7InfIK/dMkwdD7Me7lhyB+FzNq0fzGwAsThiu8ntJhYRSjFUzs8j/nH/E+SMHQXWLiFCMxgqaLO/7piRlKxAV9JjsnRegDxMmAaMaWpiDIdxRCQ87yogAKsr/IB2o1FQjoz3QuhXMk4+Cpp2plF0G/NiQTJYHcraRLhs/3E9aKjZK8ttFbG9Eg/U+gbCFw26AWInG8nnAv69JMEx5XudxOSBDQrmLVbTO+JJGkITjbkDCRCMnvjFaPWgLzwwQE8dlfgjm7rIWAN0VJGuDp5j6iK1uEkHkct8k4dc0cm3v8JwlzgMYjIhwMtB9C3PO80SrYJwp5G+9f3X3/4Xi9thH8F2euwduyoCccNaGFqRJ2eN6fIp1Wh1ll/iHfRmTbXK00ll0bX5SbrAfG7emQKIxmJeeSrjd3tYJGzHgE7z1RTeQjZze7IKwlqw+BJKn9Y6r5ccnHVvvEufrW3iaqA9ARe9BslIhiI9aCocASMLAW1coGF1PuP3/68m5m0D4eCni3m3azW+TH/5RgPLzIsdcynTZujJSKiewv4JlHVSVAQPfvC1nszEcZ71xjWQl90FYI6vAupHjEBweJwrVen9eS8+xvbMUZEk6YzWo3o9n+OoRJhIQa1WlMEzQwzGekSGDSIleLw8sCDPSE8dlHn1UqsyfcqtckvM27JlSqfnlkZYo+sjI6eD3vfHrZPCf/gjFAvVHCHbLHyzBO7kVKg== 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: --000000000000018df005f8997fc3 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Apr 3, 2023 at 5:58 PM Yang Shi wrote: > On Thu, Mar 30, 2023 at 6:33=E2=80=AFPM Zach O'Keefe = wrote: > > > > 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-13154-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:0000000000000000 > > > 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 cas= e > was > > also susceptible to a similar race, and I *think* it might be. > Unfortunately, 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 > condition. > > The nice do-while loop near the top of collapse_file() attempts to avoi= d > 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 > slots > > then need to be allocated again -- which under OOM condition is failabl= e. > > > > 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 > allocated a > > new slot in our mapping pointing to the new page. We *then* locks the > page. 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. > Ugh. Duh. You=E2=80=99re right, and I=E2=80=99m not sure how I missed that = implication; we *did* succeed in locking the page in page cache. Well, the above rant just looks foolish now. Anyways, sorry for the false alarm, Ivan, and thanks for keeping me honest, Yang! > > > > Maybe I'm just low on caffeine at the end of the day, and am missing > something, > > but if I had more time, I'd be looking into the file-side some more to > verify. > > Apologies that hasn't occurred to me until now ; I was looking at one o= f > 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 > more > > details about the specifics of the race we are fixing and/or guarding > against to > > help ppl understand how that -ENOMEM comes about if the do-while loop h= as > > "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=3D7d6bb3760e026ece7524500fe44fb024a0= e959fc > > > 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 > quite 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 g= o > that > > route. > > > > > > > > #define CREATE_TRACE_POINTS > > > @@ -1840,6 +1841,15 @@ static int collapse_file(struct mm_struct *mm, > unsigned long addr, > > > goto xa_locked; > > > } > > > xas_store(&xas, hpage); > > > + if (xas_error(&xas)) { > > > + /* revert shmem_charge performe= d > > > + * in the previous condition > > > + */ > > > > Nit: Here, and following, I think standard convention for multiline > comment is > > to have an empty first and last line, eg: > > > > + /* > > + * revert shmem_charge performe= d > > + * 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, > unsigned long addr, > > > > > > /* Finally, replace with the new page. */ > > > xas_store(&xas, hpage); > > > + /* We can't get an ENOMEM here (because the allocation > happened before) > > > + * but let's check for errors (XArray implementation ca= n > 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 th= e > file > > side. > > > > > continue; > > > out_unlock: > > > unlock_page(page); > > > @@ -2029,6 +2044,11 @@ static int collapse_file(struct mm_struct *mm, > unsigned 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 > the poor > > timing, for raising issues, then disappearing. Hopefully I'm wrong and > the > > file-side isn't a concern. > > > > Best, > > Zach > > > > > xa_locked: > > > xas_unlock_irq(&xas); > > > xa_unlocked: > > > -- > > > 2.34.1 > > > > > --000000000000018df005f8997fc3 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Mon, Apr 3, 2023 at 5:58 PM Yang Shi <shy828301@gmail.com> wrote:
On Thu, Mar 30, 2023 at 6:33=E2=80=AFPM Zach O'Keefe <zokeefe@google.com&= gt; wrote:
>
> 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-syzkalle= r-13154-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&g= t; 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:=C2=A0 00007fdbae5ee700(0000) GS:ffff8880b9900000(0000) knlGS:= 0000000000000000
> > CS:=C2=A0 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:
> >=C2=A0 <TASK>
> >=C2=A0 madvise_collapse+0x721/0xf50 mm/khugepaged.c:2693
> >=C2=A0 madvise_vma_behavior mm/madvise.c:1086 [inline]
> >=C2=A0 madvise_walk_vmas mm/madvise.c:1260 [inline]
> >=C2=A0 do_madvise+0x9e5/0x4680 mm/madvise.c:1439
> >=C2=A0 __do_sys_madvise mm/madvise.c:1452 [inline]
> >=C2=A0 __se_sys_madvise mm/madvise.c:1450 [inline]
> >=C2=A0 __x64_sys_madvise+0xa5/0xb0 mm/madvise.c:1450
> >=C2=A0 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> >=C2=A0 do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
> >=C2=A0 entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >
>
> Thanks, Ivan.
>
> In the process of reviewing this, I starting thinking if the !shmem ca= se was
> also susceptible to a similar race, and I *think* it might be. Unfortu= nately, my
> time has ran out, and I haven't been able to validate ; I'm le= ss familiar with
> the file-side of things.
>
> The underlying problem is race with truncation/hole-punch=C2=A0 under = OOM condition.
> The nice do-while loop near the top of collapse_file() attempts to avo= id this
> scenario by making sure enough slots are available. However, when we d= rop xarray
> lock, we open ourselves up to concurrent removal + slot deletion. Thos= e slots
> then need to be allocated again -- which under OOM condition is failab= le.
>
> The syzbot reproducer picks on shmem, but I think this can occur for f= ile as
> well. If we find a hole, we unlock the xarray and call
> page_cache_sync_readahead(), which if it succeeds, IIUC, will have all= ocated a
> new slot in our mapping pointing to the new page. We *then* locks the = page. Only
> after the page is locked are we protected from concurrent removal (Not= e: 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, protec= ting 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.

Ugh. Duh. You=E2= =80=99re right, and I=E2=80=99m not sure how I missed that implication; we = *did* succeed in locking the page in page cache. Well, the above rant just = looks foolish now.

Anywa= ys, sorry for the false alarm, Ivan, and thanks for keeping me honest, Yang= !




>
> Maybe I'm just low on caffeine at the end of the day, and am missi= ng something,
> but if I had more time, I'd be looking into the file-side some mor= e to verify.
> 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 follo= w:
>
> > The 'xas_store' call during page cache scanning can poten= tially
> > translate 'xas' into the error state (with the reproducer= provided
> > by the syzkaller the error code is -ENOMEM). However, there are n= o
> > further checks after the 'xas_store', and the next call o= f 'xas_next'
> > at the start of the scanning cycle doesn't increase the xa_in= dex,
> > and the issue occurs.
> >
> > This patch will add the xarray state error checking after the
> > 'xas_store' and the corresponding result error code. It w= ill
> > 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 e= ven more
> details about the specifics of the race we are fixing and/or guarding = against to
> help ppl understand how that -ENOMEM comes about if the do-while loop = has
> "Ensured" we have slots available (additionally, I think tha= t comment can be
> augmented).
>
> > Tested via syzbot.
> >
> > Reported-by: syzbot+9578faa5475acb35fa50@syzkal= ler.appspotmail.com
> > Link: https= ://syzkaller.appspot.com/bug?id=3D7d6bb3760e026ece7524500fe44fb024a0e959fc<= /a>
> > Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> > ---
> > V1 -> V2: Add WARN_ON_ONCE error checking and comments
> >
> >=C2=A0 mm/khugepaged.c | 20 ++++++++++++++++++++
> >=C2=A0 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 {
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0SCAN_CGROUP_CHARGE_FAIL,
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0SCAN_TRUNCATED,
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0SCAN_PAGE_HAS_PRIVATE,
> > +=C2=A0 =C2=A0 =C2=A0SCAN_STORE_FAILED,
> >=C2=A0 };
>
> I'm still reluctant to add a new error code for this as this seems= like quite a
> rare race that requires OOM to trigger. I'd be happier just reusin= g 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.
>
> >
> >=C2=A0 #define CREATE_TRACE_POINTS
> > @@ -1840,6 +1841,15 @@ static int collapse_file(struct mm_struct = *mm, unsigned long addr,
> >=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=A0go= to xa_locked;
> >=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=A0xas_store(&xas, hpage); > > +=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=A0if (xas_error(&xas)) {
> > +=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/* rev= ert shmem_charge performed
> > +=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 * in = the previous condition
> > +=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 */ >
> Nit: Here, and following, I think standard convention for multiline co= mment is
> to have an empty first and last line, eg:
>
>=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* revert shmem_charge performed
>=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* in the previous condition
>=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*/
>
> Though, checkpatch.pl --strict didn't seem to care.
>
> > +=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=A0mappin= g->nrpages--;
> > +=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=A0shmem_= uncharge(mapping->host, 1);
> > +=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=A0result= =3D SCAN_STORE_FAILED;
> > +=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=A0goto x= a_locked;
> > +=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=A0nr_none++;
> >=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=A0continue;
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0}
> > @@ -1992,6 +2002,11 @@ static int collapse_file(struct mm_struct = *mm, unsigned long addr,
> >
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Finally,= replace with the new page. */
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0xas_store(&= amp;xas, hpage);
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* We can't = get an ENOMEM here (because the allocation happened before)
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * but let's= check for errors (XArray implementation can be
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * changed in th= e future)
> > +=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=A0WARN_ON_ONCE(xas= _error(&xas));
>
> Nit: it's not just that allocation happened before -- need some gu= arantee we've
> been protected from concurrent removal. This is what made me look at t= he file
> side.
>
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0continue; > >=C2=A0 out_unlock:
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unlock_page= (page);
> > @@ -2029,6 +2044,11 @@ static int collapse_file(struct mm_struct = *mm, unsigned long addr,
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Join all the small entries into a si= ngle multi-index entry */
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0xas_set_order(&xas, start, HPAGE_PM= D_ORDER);
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0xas_store(&xas, hpage);
> > +=C2=A0 =C2=A0 =C2=A0/* Here we can't get an ENOMEM (because = entries were
> > +=C2=A0 =C2=A0 =C2=A0 * previously allocated) But let's check= for errors
> > +=C2=A0 =C2=A0 =C2=A0 * (XArray implementation can be changed in = the future)
> > +=C2=A0 =C2=A0 =C2=A0 */
> > +=C2=A0 =C2=A0 =C2=A0WARN_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.=C2=A0 Sor= ry for the poor
> timing, for raising issues, then disappearing. Hopefully I'm wrong= and the
> file-side isn't a concern.
>
> Best,
> Zach
>
> >=C2=A0 xa_locked:
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0xas_unlock_irq(&xas);
> >=C2=A0 xa_unlocked:
> > --
> > 2.34.1
> >

--000000000000018df005f8997fc3--