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 6D22BC6FD1D for ; Fri, 31 Mar 2023 01:33:11 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4CF936B0071; Thu, 30 Mar 2023 21:33:10 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4581B6B0072; Thu, 30 Mar 2023 21:33:10 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2D19E6B0074; Thu, 30 Mar 2023 21:33:10 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 1739E6B0071 for ; Thu, 30 Mar 2023 21:33:10 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id D409E1402D4 for ; Fri, 31 Mar 2023 01:33:09 +0000 (UTC) X-FDA: 80627470098.11.EE2AF15 Received: from mail-pl1-f172.google.com (mail-pl1-f172.google.com [209.85.214.172]) by imf21.hostedemail.com (Postfix) with ESMTP id 06E1E1C0004 for ; Fri, 31 Mar 2023 01:33:06 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=Y8Ry0QCY; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf21.hostedemail.com: domain of zokeefe@google.com designates 209.85.214.172 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=1680226387; 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=R5BfCm5LJSVqy2GRHOsUJLl8pmivkWNOPbogWlIH6Rc=; b=JUm+pllN6ACXvf5osHlg4Oh3F/D+mnzLlZ6wFvQV6l2BhXWGQHLJlLf0rFKPEOb5pR47AH I0A4Ya7HN2Py6HWQTs6TNErbLpPOv8qdQoBcw1EeRy4lE8BNBO1mdQ+yGh1YTKzoq7Es0Z Wd5yUMwu5zStQPQB/skShLu1Cn7aI5w= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=Y8Ry0QCY; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf21.hostedemail.com: domain of zokeefe@google.com designates 209.85.214.172 as permitted sender) smtp.mailfrom=zokeefe@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1680226387; a=rsa-sha256; cv=none; b=gZFGAgLRgeV/SYGbtfL1JLWhptDSUK8+iJheGglPockcBMAfD1dK8IPRJIn226Kr+WttJZ ANNA3b2nI4Gh/Xs68rTIHszWanTUbS1eHEZBWE7JPD6SzVePL+vYd2x7v4IiH6NShRj32+ tdORLzBUOY88Ayo702Q7ocAGkoxHqVQ= Received: by mail-pl1-f172.google.com with SMTP id d9443c01a7336-1a2104d8b00so193525ad.1 for ; Thu, 30 Mar 2023 18:33:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680226386; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=R5BfCm5LJSVqy2GRHOsUJLl8pmivkWNOPbogWlIH6Rc=; b=Y8Ry0QCYqWVPclUT/1jUKuJYd+7uWZaq74QJQqc8efWHkb900SDo2HMu0kIugifNRh LWqUvZJC63jDus2mCRJPyzprpKkvqn3v1ianxtQ9baMza5eYGYq4dfC70/6SAVp43XXl JvOZzMtCkNHHJGAi+uoHRRxCf8KPTvwcNUbRfQrls8oh0DAEl4DYJsQqSmCZIbTCZEcZ H4aBlgXWvXufx5zMklBmShASI3SDFY70ClUDfDDriq4q9NuHQClzg9OjDyPnPqV1JP2R 2C2FxeiTHQSXA+IizhpWOOa43pmaj20t3y3lsM9jf/HOdUXXVZBvMTnNEe3vdEJNzjPk TCNA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680226386; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=R5BfCm5LJSVqy2GRHOsUJLl8pmivkWNOPbogWlIH6Rc=; b=taOeRQosDhf6cFFjbDsEZga5dAE0sEKoCF9IIh7cbzVZBhLZQZwuJ0ZWF9A3+LRWg2 Os4fCj86Aoy/y606JMakLysw/zhNkwVQA3nL2VLVbWNQ2zaGfHvACf2J6Ah/Cl98RVqh JgNwwB+7VwNYbkERJdfr03w91Osnx8zD0KLwUfG/bKL2YEbgd9cD0TXAH9BASTzuwatL yQPhl0hHaINaTL6BOTsmHwMlTFpIpJNsd2hu6FCU6bY4UH3njR+KQozSoqwQw6v0V54a XAJ8qGfMjfVpWG8UAFMFI2cnPmA7beIMxs4VUyezy3+GJZiI0BXB3NvyPuU9Qer7+ekT R6gw== X-Gm-Message-State: AAQBX9cc/1V1bJ7647sKVgL0AUIIalHkux8GP4k5ka6ys1zpzn2FyRUL 4slPOpT9ueCw0FvGdG8NxysTQQ== X-Google-Smtp-Source: AKy350a+u7aNz6PUTYlfRL+pBCp4Ct/UTl/zfYVGu9lLcjxUhgffncrwJ7qIFLd4w1puQdArVVoXtw== X-Received: by 2002:a17:902:c20c:b0:1a2:4bc:5420 with SMTP id 12-20020a170902c20c00b001a204bc5420mr133571pll.10.1680226385545; Thu, 30 Mar 2023 18:33:05 -0700 (PDT) Received: from google.com (33.5.83.34.bc.googleusercontent.com. [34.83.5.33]) by smtp.gmail.com with ESMTPSA id 13-20020aa7924d000000b006262520ac59sm469774pfp.127.2023.03.30.18.33.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 30 Mar 2023 18:33:04 -0700 (PDT) Date: Thu, 30 Mar 2023 18:33:01 -0700 From: Zach O'Keefe To: Ivan Orlov Cc: 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, shy828301@gmail.com, syzbot+9578faa5475acb35fa50@syzkaller.appspotmail.com Subject: Re: [PATCH v2] mm: khugepaged: Fix kernel BUG in hpage_collapse_scan_file Message-ID: <20230331013301.ecgkjymaf3ws6rfb@google.com> References: <20230330155305.423051-1-ivan.orlov0322@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230330155305.423051-1-ivan.orlov0322@gmail.com> X-Rspamd-Queue-Id: 06E1E1C0004 X-Rspamd-Server: rspam09 X-Rspam-User: X-Stat-Signature: myjo3agebqqqih4z7rstdzrptby4xc9a X-HE-Tag: 1680226386-622286 X-HE-Meta: U2FsdGVkX1+ja7EgVTqzN5LwKu8L8qTV4M7MvEzpgw7RQ8pNjIM7vy3bYkM4EqGMDriI0UBY93OOFnLfPg2x+xYZHpu+Ok4tDprCaUWBCWKvhgVXeQZyPYyr5BxMAL1iZF34DiT8HsyZ2QKmDxldzwB4oA8MqrPugmU5n4E5wdV3MrilWjQrr58JpNdgFFJVFJo0dFEmoU+DWECUIpvIXuZsgMds1TifRk/NolSZd4BFKvFmLI3pUrP+zz9OYzDpztnxfteMohxWzCl+jXjnlN4sOnYtL/7v8XdrW5l7LUu8ZzyxzKu9Pxm7JBiqVai/d63igqtHBqWz2+I99oILUsmriL+6Lchr9Z+/C7413bPmAD7ZHMvuwJBYrJpRteHK5CAthDKQI5NGnM8dL0pEQTkkiQRbjp4m0PpONDUX7qI4biAP/eIRunr8Y6scnsljm8Q8g29O3lYVOnbDR9b+BYgJ46YS6YmbIZCf8GgStAl+n+jAHXcAUNfrSbUhdfgTiTqerQb5lww1i/Bi5lkVCRXuf3SjtfGC+//ikoahP9VaF1fGkpm285cmoE/T2dUyVDfMZOpChTXiQsXEy0eiDl3lbaL3jyZkwhJa2E8zyG4vda3/9R3H7Cy2B6KWSEZVQhh/W7/lRShUx58gR0fF0XYHByyo5dP7ECjHNcBJ/qhyIhvxSZuEuGE+ViCKF6bPjly67K7p1i2OrQ8vZP4mBUOYK4o+VWVOel1DqXndmtkVwrHdSMrQbZyVw58R1bafWDgoI+djMo4CTILn5a1gq46/L1DjMJXCwus5f8aL6xXzBFf+GlluVVvGr4ySVdXiIXUZp/INK3hVe2P+v2GRyRY6nvbCW7WDpWCeh+6afi0WGiKrxWNQyq1kTWyHjt4U7Y471izSwerILtttKj/0aO6ZZzscKtbluz07a8meKWN6gUq4HdMWdfwqH31c3q69xoMmVNyN6iaoVH7uiMv Dge2fLj1 MtE11mQ+D8eaXS3CMX6T6GqKDpD4AvEfzpykXTi3wZlf4QvM8DRujNFT8Y8e3Po3obkIMhEgU07sm4zenPddtp3LjsJ59M7LkbjNMjT6Q2jJAu/KS5ZxfDAQzogDabBdbNvYoVqXrHotBf84gSgYgcK/HO9odBGzIML0tN01dd9dviO0+vFnp79eBia0Y8R7OZOZrxnbwkLn09pceT5aqPMgW2lo1KehX2rKFHSgeFOa2YLegA/WxY4PmgBTfdi6mnozwBmLzJc7s27TmuvOq3qVh5yc3yx7e+WRXEzVQHNOOmTh/yUMVOCqT3go0/cInFIaQ7EAH5vWt23BlVY5GBGFJFNxFCi5dF99pnSXYOPgNgtnfKrNojEWK7XcNzWsu+VD4HoW9gzVsdyp5qg9MWzXzi3T4I1vh2CkXKbX0v9v/A87bNWZOkEpGWaWHFLuEqK7ooe6s2o3Pj19RHjVRqxnuoTlZpfT7NFH5itgs7YwPzx5MhzDOGQxQlvprafz12MEOBXeAcz81dMrWodQ6Q0mzZ9uss2GXArEKsjxFmZ6r3NTC7vSn4+NUSj9ZzKRSxc/e1KzF6YdBuwk9dj5vmMvDLaT+sDborWc0hDIPvB9Sl5dcnVEXEc3J2xvKAdGaY0Ggq7SkgTpTDrkw3EySIEagVmwTf6KwPmxG 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 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 case 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 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 slots 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 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). 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 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 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 that comment can be augmented). > Tested via syzbot. > > Reported-by: syzbot+9578faa5475acb35fa50@syzkaller.appspotmail.com > Link: https://syzkaller.appspot.com/bug?id=7d6bb3760e026ece7524500fe44fb024a0e959fc > 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 go 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 performed > + * 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 performed + * in the previous condition + */ Though, checkpatch.pl --strict didn't seem to care. > + mapping->nrpages--; > + shmem_uncharge(mapping->host, 1); > + result = 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 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, 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 >