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 D6053CD4846 for ; Fri, 22 Sep 2023 16:41:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 391756B02E4; Fri, 22 Sep 2023 12:41:07 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3199D6B02E5; Fri, 22 Sep 2023 12:41:07 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1E1666B02E6; Fri, 22 Sep 2023 12:41:07 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 085766B02E4 for ; Fri, 22 Sep 2023 12:41:07 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id C20B640251 for ; Fri, 22 Sep 2023 16:41:06 +0000 (UTC) X-FDA: 81264798132.26.4A464D0 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf23.hostedemail.com (Postfix) with ESMTP id 036E7140028 for ; Fri, 22 Sep 2023 16:41:03 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf23.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1695400864; 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=wKfdN1QdOQoZI3/pQHXjzmk1KUwwCctdNwSy1vUj0OA=; b=aaY20ZkbjUxhvG7te911mh1HShh3W+0Ff6a1lebO5zyDp9JT8gT1bFzOFz6MTmMsnWuIUO Lp7sz062leWf9gchrw3AYMtggmOH4Huh2SV4GsgCdBFwJOyaprb4/JpAIfo7yGVIXA6FZZ sM+EIIJQb+R2uaqtHzWfRvTrgAncoko= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf23.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1695400864; a=rsa-sha256; cv=none; b=qZwq0pLSMYKxk5x6cVfbRUemBC+xOm7ihn9rOR7sfhCnBU4FKgyUiZfVrqjdNCg1Dc9Mti /T9L+EebmBSVbgCY6U4MvDFYwjrcuEnx0UmjhgXfDJQ0hBUvRi/1YsACt2QkLBww7pfHtS v+8KdoMCEOqJW28RmMvcnHsVS4W2z/g= 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 0B4B2DA7; Fri, 22 Sep 2023 09:41:40 -0700 (PDT) Received: from [10.57.65.11] (unknown [10.57.65.11]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 43E173F67D; Fri, 22 Sep 2023 09:40:58 -0700 (PDT) Message-ID: <32052cb7-91f1-461d-a226-2cd2fcf34ad2@arm.com> Date: Fri, 22 Sep 2023 17:40:56 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 2/2] arm64: hugetlb: Fix set_huge_pte_at() to work with all swap entries Content-Language: en-GB To: Will Deacon Cc: Catalin Marinas , "James E.J. Bottomley" , Helge Deller , Nicholas Piggin , Christophe Leroy , Paul Walmsley , Palmer Dabbelt , Albert Ou , Heiko Carstens , Vasily Gorbik , Alexander Gordeev , Christian Borntraeger , Sven Schnelle , Gerald Schaefer , "David S. Miller" , Arnd Bergmann , Mike Kravetz , Muchun Song , SeongJae Park , Andrew Morton , Uladzislau Rezki , Christoph Hellwig , Lorenzo Stoakes , Anshuman Khandual , Peter Xu , Axel Rasmussen , Qi Zheng , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-parisc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-riscv@lists.infradead.org, linux-s390@vger.kernel.org, sparclinux@vger.kernel.org, linux-mm@kvack.org, stable@vger.kernel.org References: <20230922115804.2043771-1-ryan.roberts@arm.com> <20230922115804.2043771-3-ryan.roberts@arm.com> <20230922161404.GA23332@willie-the-truck> From: Ryan Roberts In-Reply-To: <20230922161404.GA23332@willie-the-truck> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspam-User: X-Stat-Signature: batehrfk3cwghtuk176wdqnx1wkjeqiw X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 036E7140028 X-HE-Tag: 1695400863-786036 X-HE-Meta: U2FsdGVkX18M9A1TDvQAuUcINVfWidIgbWY2TldUO7KG65Ne0xvHRKBxTDk+QhmRV6IXA+UqAWJsb0JzWGptZWYTYg4t+c5ZsqBbLGKQaCrgFPPQfoolK9t73t0moJ2IeSZVdPFuA0NjkJPxIjJl1dWX0arMCbXTn+SqXij91Jev3Xbll2r1xI2gxG7nYm37/rCaL3uCb5tK+Oin+w7c/goDDMU4AZk631MYXjAoawSeSfWVzyjKcRHJAO58KTK4PPiuaOn6YLBHTzK7eHyXur6v9DuyndlerWnxqnCkBViQQ4NHl9McFSFUCviqmU9Om+QmR9jmdKnr1odPx7ZNx9PnUQGWE8Ay+UMVKcxaY8WBOdOUHd5q0xU7ZEtupnf7XYeMKBD13AnfnRIJrpfgnIH2o6j3Ska2VyFXWalGMkQjp+dbLHHOqM2Mn5N3GVoHSJzmF3MQOYrjRcQRB9GORky6SGLJYqy0GSiev1jA9I/iECYX7DwLACOZFEYqawkBR411zr4/hiedahcD3tWEDSR6Asc8ShWgi/rtXQiiop1ojUbYRDYqDfIaOvEHp8yOvD3bd/+479t9meyNFKSo9QDRJN4Jlx6QGd6Gu6qL63x6IYT7QVkMuJNMHsBVrikQzNr/zzW+4iddRicmHti0eEfxv3ibel65R6leQ2wilyZOx3a26TpsQraseXMw9OjNYhdEnTXaYUIXrkL4ie4y73MbLIkG51P6qScgp/UzpvhikqkCJ5uaqBFIA7ZP2rDH48PYRbyH2PaWke46IIJWwBvHpyMkkj//toxfm2a34PKGLXNIVyivMSowWSdWUz3KgukaQ10ljX9RZBV8nbRyQ3c0ez3kavQumB3tlw1sRm2hPB4Fp9q2MgFvVVSmGSKIfjf9MFc/d9nHbRbpSHXbJhi4w9rzFia0nofkHm/pNM0vrCRfmsT3lR1iMRJ9vMRBZdaThS7SF7H6Li8PNdB fJajFhJe wPB+eh8tL6243B85C88n/7rRwvKcHOkiEoUsW4ibcDwVWpyM7rTTgxbBrk9sXL3dipJH8q9wyvl5g6fn+g79LyNyBy72V2+wpVeDHiboWe+a3c/1iWfAlvLJAR3sOb7xGJy5OiuzraRsvKpCeQoL1C2kRlt3nxYrmdQFkigeJ8+0Swygeyadmaf1uXmyqokZlF3bkgT2S4dgRBKtnH4sPokRFOxvRELWeuJPSAGluvqnscW1FGFnTQD5wewP98vxcghgMWZH8MA8K6tSCcG0GDS5Yq+s0+StLVYtxagGaF+0VSIPfhY+pVJaMSUNyHdR+4yVn 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 22/09/2023 17:14, Will Deacon wrote: > On Fri, Sep 22, 2023 at 12:58:04PM +0100, Ryan Roberts wrote: >> When called with a swap entry that does not embed a PFN (e.g. >> PTE_MARKER_POISONED or PTE_MARKER_UFFD_WP), the previous implementation >> of set_huge_pte_at() would either cause a BUG() to fire (if >> CONFIG_DEBUG_VM is enabled) or cause a dereference of an invalid address >> and subsequent panic. >> >> arm64's huge pte implementation supports multiple huge page sizes, some >> of which are implemented in the page table with multiple contiguous >> entries. So set_huge_pte_at() needs to work out how big the logical pte >> is, so that it can also work out how many physical ptes (or pmds) need >> to be written. It previously did this by grabbing the folio out of the >> pte and querying its size. >> >> However, there are cases when the pte being set is actually a swap >> entry. But this also used to work fine, because for huge ptes, we only >> ever saw migration entries and hwpoison entries. And both of these types >> of swap entries have a PFN embedded, so the code would grab that and >> everything still worked out. >> >> But over time, more calls to set_huge_pte_at() have been added that set >> swap entry types that do not embed a PFN. And this causes the code to go >> bang. The triggering case is for the uffd poison test, commit >> 99aa77215ad0 ("selftests/mm: add uffd unit test for UFFDIO_POISON"), >> which causes a PTE_MARKER_POISONED swap entry to be set, coutesey of >> commit 8a13897fb0da ("mm: userfaultfd: support UFFDIO_POISON for >> hugetlbfs") - added in v6.5-rc7. Although review shows that there are >> other call sites that set PTE_MARKER_UFFD_WP (which also has no PFN), >> these don't trigger on arm64 because arm64 doesn't support UFFD WP. >> >> Arguably, the root cause is really due to commit 18f3962953e4 ("mm: >> hugetlb: kill set_huge_swap_pte_at()"), which aimed to simplify the >> interface to the core code by removing set_huge_swap_pte_at() (which >> took a page size parameter) and replacing it with calls to >> set_huge_pte_at() where the size was inferred from the folio, as >> descibed above. While that commit didn't break anything at the time, it >> did break the interface because it couldn't handle swap entries without >> PFNs. And since then new callers have come along which rely on this >> working. But given the brokeness is only observable after commit >> 8a13897fb0da ("mm: userfaultfd: support UFFDIO_POISON for hugetlbfs"), >> that one gets the Fixes tag. >> >> Now that we have modified the set_huge_pte_at() interface to pass the >> huge page size in the previous patch, we can trivially fix this issue. >> >> Signed-off-by: Ryan Roberts >> Fixes: 8a13897fb0da ("mm: userfaultfd: support UFFDIO_POISON for hugetlbfs") >> Cc: # 6.5+ >> --- >> arch/arm64/mm/hugetlbpage.c | 17 +++-------------- >> 1 file changed, 3 insertions(+), 14 deletions(-) >> >> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c >> index a7f8c8db3425..13fd592228b1 100644 >> --- a/arch/arm64/mm/hugetlbpage.c >> +++ b/arch/arm64/mm/hugetlbpage.c >> @@ -241,13 +241,6 @@ static void clear_flush(struct mm_struct *mm, >> flush_tlb_range(&vma, saddr, addr); >> } >> >> -static inline struct folio *hugetlb_swap_entry_to_folio(swp_entry_t entry) >> -{ >> - VM_BUG_ON(!is_migration_entry(entry) && !is_hwpoison_entry(entry)); >> - >> - return page_folio(pfn_to_page(swp_offset_pfn(entry))); >> -} >> - >> void set_huge_pte_at(struct mm_struct *mm, unsigned long addr, >> pte_t *ptep, pte_t pte, unsigned long sz) >> { >> @@ -257,13 +250,10 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr, >> unsigned long pfn, dpfn; >> pgprot_t hugeprot; >> >> - if (!pte_present(pte)) { >> - struct folio *folio; >> - >> - folio = hugetlb_swap_entry_to_folio(pte_to_swp_entry(pte)); >> - ncontig = num_contig_ptes(folio_size(folio), &pgsize); >> + ncontig = num_contig_ptes(sz, &pgsize); >> >> - for (i = 0; i < ncontig; i++, ptep++) >> + if (!pte_present(pte)) { >> + for (i = 0; i < ncontig; i++, ptep++, addr += pgsize) >> set_pte_at(mm, addr, ptep, pte); > > Our set_pte_at() doesn't use 'addr' for anything and the old code didn't > even bother to increment it here! I'm fine adding that, but it feels > unrelated to the issue which this patch is actually fixing. True. I agree its not strictly necessary and will presumably be optimized out. But I'm not sure that having knowledge that the implementation doesn't use it is a good reason not to call the interface correctly. I'll leave it as I've done it if that's ok. > > Either way: > > Acked-by: Will Deacon Thanks! > > Will