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 B2C41EB64D7 for ; Fri, 30 Jun 2023 10:08:08 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4A2DD8E0008; Fri, 30 Jun 2023 06:08:08 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 452C38D0001; Fri, 30 Jun 2023 06:08:08 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 342EF8E0008; Fri, 30 Jun 2023 06:08:08 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 24F458D0001 for ; Fri, 30 Jun 2023 06:08:08 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id E027A140EC8 for ; Fri, 30 Jun 2023 10:08:07 +0000 (UTC) X-FDA: 80958988614.27.22BBC33 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf27.hostedemail.com (Postfix) with ESMTP id 16FED40019 for ; Fri, 30 Jun 2023 10:08:04 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=none; spf=pass (imf27.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1688119685; 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; bh=Xut25gQxpPwA0RnpDnJb8BosQjJ7lxozTDO6At5FvdM=; b=LIrxk7PHjI/hJpjnxz/8P4wK680Vz1Qx6Z62PmZAV7GdwzW383JHisSagQEwstPGU07HOn dA6sq0HBTiIleVzycqlaD2ujcemYzcUkWAjTB0+bqaAQTRtO+6WZFi1d6Zliw2mJiAJkZM Dew++GXMZWby/MAi7cIXbNYx0rcAJsE= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=none; spf=pass (imf27.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1688119685; a=rsa-sha256; cv=none; b=KOTGRjK6H5HCSZdZ3pxVmAxMQgHE4A3DjRNz2h3ccuCYGUx1iKqzBnnL7oV2CuxYSsKzcf rb9UbN/10rCOzquIpTMgO1KAtGBa1+sGBmlbJYSLNjRO17ArBcuHRrjNAsxn6fQ9WOTBu5 LUn3uzuSPXKOdd7GNn8c3jYNStaN4Gs= Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6CD04D75; Fri, 30 Jun 2023 03:08:47 -0700 (PDT) Received: from [10.57.76.103] (unknown [10.57.76.103]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id CA1053F663; Fri, 30 Jun 2023 03:07:58 -0700 (PDT) Message-ID: <05ea6668-3dca-23ed-56c8-bbf8079d93cd@arm.com> Date: Fri, 30 Jun 2023 11:07:57 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [PATCH] mm/hugetlb.c: fix a bug within a BUG(): inconsistent pte comparison To: John Hubbard , Andrew Morton Cc: LKML , linux-mm@kvack.org, Adrian Hunter , Al Viro , Alex Williamson , Alexander Potapenko , Alexander Shishkin , Andrey Konovalov , Andrey Ryabinin , Christian Brauner , Christoph Hellwig , Daniel Vetter , Dave Airlie , Dimitri Sivanich , Dmitry Vyukov , Ian Rogers , Jason Gunthorpe , Jiri Olsa , Johannes Weiner , "Kirill A . Shutemov" , Lorenzo Stoakes , Mark Rutland , Matthew Wilcox , Miaohe Lin , Michal Hocko , Mike Kravetz , Mike Rapoport , Muchun Song , Namhyung Kim , Naoya Horiguchi , Oleksandr Tyshchenko , Pavel Tatashin , Roman Gushchin , SeongJae Park , Shakeel Butt , Uladzislau Rezki , Vincenzo Frascino , Yu Zhao References: <20230630013203.1955064-1-jhubbard@nvidia.com> From: Ryan Roberts In-Reply-To: <20230630013203.1955064-1-jhubbard@nvidia.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 16FED40019 X-Rspam-User: X-Stat-Signature: betkaos76mjfddh4hopsf8aurny5h668 X-Rspamd-Server: rspam01 X-HE-Tag: 1688119684-645365 X-HE-Meta: U2FsdGVkX18AdnzSF2o01U0J3DRpMR5ySnHZ8l2MiU1wHf2XmXJlzD6PHYKjR/5MZUrLAo1Q03BQP64sxqwnc0NJ+5rgYJ2nnOMd978L3s+uuaLpNSX1QrhQ/atjI9Gu7vtPuxd1FyB/4Zvt/xadu2pjGQIsKiHAKHGCD4frk40ES6dHazyFocYsJ/AD8+1aYAG80OJjRjNWOILIYCXkaadLG2OGnZBGY5FaTARXrp63vBUHO3VhfzsaUMg2kzXc3VDcI/nhBc53GL2a8TBBDOWO3uBzA7NjNJRRylqQ9dS4o4sRT0NjwiecCt9PXi6EBoqRGiCUxmI3S8a93ls1ZeS4eMuVmxRh6xGyPlBgPMZd6+jv/VMHPRL9a7X7akFYXFtRh1ET38Bc4SYQjW6csyUZhYiVAAyfNFHTq16Drhwv00Hk/ffD6+vgTxXDfXVI5bQK/pgTSnxMYDGHMuYXq+KZ54sQJydU/9O3Xm0pxC1wXBIfqGLXa3aCNWo45K+6nS8HwQqRL+cIpVVUYDa0eLDp5RLbwfTBKPLSVFIhfZiJYd90/GVwN2++O0+bIUXiD8aiiP72mlcFPqe8wk/b0H+huO3IyktrMzhoDuJmdiBzH6Jgslp2//bXP2d6ODI5uGw0f26uzouFZEV3lK/v6zZx75Ttdfz8YfKZ0m7joyoXOf+ENqDHrzF8kYNPnaZVPmOGTayIMy6t92Uaku434cdJHCxATuFw7LHS+bXpkCl9MiZrHSyD4VlG+Bj+aWtg4LzD3eWURi52d5rBerT6PhkC/xKYHUi6SbbGGON5CImL4NlMtVRO66JyejLMpqP2cggJU3tecLWfmnRjvgkk1J6Xfme/gpQQdElC/K1Of1k8hpOspHAovH8qjpNwzvd/ExzbJfj/qx7Q2SmpsHgLb9HQYSDYosDDefQQ44+2C5xKvo0qqwnLlRYBSI8Zr9YuuA9rcT/ErUyakzf/uDo EOOFWYJr hJi6HUATsRLfsypRq2JnfS1SK/bjJZl0BiKFmXB4cvhEQoYd6Xv7m58cVqpj1N2DUn7vPoafnBnIiop2ITXKNlfqepNyU4MSv+3aXzawau/rRCfsVCE0JrZRUcp30/xhokwHjdIx7ab0CcKoF53vMQMZLfHcUFX7nb+2+G2GwfYCk5dt4aeAspgrOWvE5VWqWfbz9pWcyovcSevDailXzqSxkmtKO/9hiOiQVguMT4axiid171DajYqY2QVBN0ySjqAN7EwxdgA3OpuW36okIjOaFWhk6ZlwBETGOF5IfPRFSQajSnO0PByvWyQ8z25kqudARGXuwoCVY+Vabgm/l6LcUEJCDRXgDLQERI+jatECavOpNV1ELPSqQjDJYsyxvd7toBwfeAQ4QoVP/AxRh7QrcDMV4FqiHyu9Y3lOAG9pFU5lhZEgfCf8kj09L5dNfelCxXF75Ve3O5JEkkjWI88PZFtkT59A36bvhAwiaL7Oyr4hJCt6L+Dc6s2wSe0dLhT0Y5kjMDMLXopthzZSw1hDz5fPV0aYSSeQTqRH9KJc4fXK3pFogZXW50Ug3x29QuApTyhtbMiD/VXl95m1y7tySb+8xgylTJRzjDqq3XdawAmQ+aFW7meUsRQ7TqhpkClN7eWRBIjWu63Y5Y2t+HziRvD5BG6YmjFVBw7dgD5NrhfXxixXoZO3/NSU6GxFxqSFTVit7vD1+2yPIDJXu6kPVVspWI5xgWNI5B98nPn+8EaBC4vVvh6U8fv6OmvlZrJQ94DIg2q5ryY5hWDPTg/BThHOcoy6qq3B0vav5+35fZfXkyU6VsAdDgmYuXIkUEagDqt6Da9QlgYw5kQCbGrrgKaYV4bhoJysJ2sNX7b67yocxJRfvCtGAoTZPf66YJ5UmroTrMW2J+MwtHdUgOrREFtqA6rlv8iTE9xYArbd8MAmyTLdaRtss9MBrQKXjD0ESAKjwc9A69Q0= 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 30/06/2023 02:32, John Hubbard wrote: > The following crash happens for me when running the -mm selftests > (below). Specifically, it happens while running the uffd-stress > subtests: > > kernel BUG at mm/hugetlb.c:7249! > invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > CPU: 0 PID: 3238 Comm: uffd-stress Not tainted 6.4.0-hubbard-github+ #109 > Hardware name: ASUS X299-A/PRIME X299-A, BIOS 1503 08/03/2018 > RIP: 0010:huge_pte_alloc+0x12c/0x1a0 > ... > Call Trace: > > ? __die_body+0x63/0xb0 > ? die+0x9f/0xc0 > ? do_trap+0xab/0x180 > ? huge_pte_alloc+0x12c/0x1a0 > ? do_error_trap+0xc6/0x110 > ? huge_pte_alloc+0x12c/0x1a0 > ? handle_invalid_op+0x2c/0x40 > ? huge_pte_alloc+0x12c/0x1a0 > ? exc_invalid_op+0x33/0x50 > ? asm_exc_invalid_op+0x16/0x20 > ? __pfx_put_prev_task_idle+0x10/0x10 > ? huge_pte_alloc+0x12c/0x1a0 > hugetlb_fault+0x1a3/0x1120 > ? finish_task_switch+0xb3/0x2a0 > ? lock_is_held_type+0xdb/0x150 > handle_mm_fault+0xb8a/0xd40 > ? find_vma+0x5d/0xa0 > do_user_addr_fault+0x257/0x5d0 > exc_page_fault+0x7b/0x1f0 > asm_exc_page_fault+0x22/0x30 > > That happens because a BUG() statement in huge_pte_alloc() attempts to > check that a pte, if present, is a hugetlb pte, but it does so in a > non-lockless-safe manner that leads to a false BUG() report. > > We got here due to a couple of bugs, each of which by itself was not > quite enough to cause a problem: > > First of all, before commit c33c794828f2("mm: ptep_get() conversion"), > the BUG() statement in huge_pte_alloc() was itself fragile: it relied > upon compiler behavior to only read the pte once, despite using it twice > in the same conditional. > > Next, commit c33c794828f2 ("mm: ptep_get() conversion") broke that > delicate situation, by causing all direct pte reads to be done via > READ_ONCE(). And so READ_ONCE() got called twice within the same BUG() > conditional, leading to comparing (potentially, occasionally) different > versions of the pte, and thus to false BUG() reports. Thanks for finding and fixing this - sorry for the issue. FWIW, I've re-reviewed the whole ptep_get conversion patch looking for other instances of this pattern - I didn't spot any other issues. > > Fix this by taking a single snapshot of the pte before using it in the > BUG conditional. > > Now, that commit is only partially to blame here but, people doing > bisections will invariably land there, so this will help them find a fix > for a real crash. And also, the previous behavior was unlikely to ever > expose this bug--it was fragile, yet not actually broken. > > So that's why I chose this commit for the Fixes tag, rather than the > commit that created the original BUG() statement. > > Fixes: c33c794828f2 ("mm: ptep_get() conversion") > Cc: Adrian Hunter > Cc: Al Viro > Cc: Alex Williamson > Cc: Alexander Potapenko > Cc: Alexander Shishkin > Cc: Andrew Morton > Cc: Andrey Konovalov > Cc: Andrey Ryabinin > Cc: Christian Brauner > Cc: Christoph Hellwig > Cc: Daniel Vetter > Cc: Dave Airlie > Cc: Dimitri Sivanich > Cc: Dmitry Vyukov > Cc: Ian Rogers > Cc: Jason Gunthorpe > Cc: Jiri Olsa > Cc: Johannes Weiner > Cc: Kirill A. Shutemov > Cc: Lorenzo Stoakes > Cc: Mark Rutland > Cc: Matthew Wilcox > Cc: Miaohe Lin > Cc: Michal Hocko > Cc: Mike Kravetz > Cc: Mike Rapoport (IBM) > Cc: Muchun Song > Cc: Namhyung Kim > Cc: Naoya Horiguchi > Cc: Oleksandr Tyshchenko > Cc: Pavel Tatashin > Cc: Roman Gushchin > Cc: Ryan Roberts > Cc: SeongJae Park > Cc: Shakeel Butt > Cc: Uladzislau Rezki (Sony) > Cc: Vincenzo Frascino > Cc: Yu Zhao > Signed-off-by: John Hubbard > --- > mm/hugetlb.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index bce28cca73a1..73fbeb8f979f 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -7246,7 +7246,12 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma, > pte = (pte_t *)pmd_alloc(mm, pud, addr); > } > } > - BUG_ON(pte && pte_present(ptep_get(pte)) && !pte_huge(ptep_get(pte))); > + > + if (pte) { > + pte_t pteval = ptep_get(pte); Given the PTL is not held here, I think this should technically be ptep_get_lockless()? Thanks, Ryan > + > + BUG_ON(pte_present(pteval) && !pte_huge(pteval)); > + } > > return pte; > } > > base-commit: bf1fa6f15553df04f2bdd06190ccd5f388ab0777