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 354FDCD4847 for ; Fri, 22 Sep 2023 17:09:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A83576B02F4; Fri, 22 Sep 2023 13:09:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A0CD16B02F5; Fri, 22 Sep 2023 13:09:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8B8D86B02F6; Fri, 22 Sep 2023 13:09:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 77A976B02F4 for ; Fri, 22 Sep 2023 13:09:50 -0400 (EDT) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 4318F401A1 for ; Fri, 22 Sep 2023 17:09:50 +0000 (UTC) X-FDA: 81264870540.13.86CC735 Received: from mail-ej1-f50.google.com (mail-ej1-f50.google.com [209.85.218.50]) by imf20.hostedemail.com (Postfix) with ESMTP id 2F7C11C0036 for ; Fri, 22 Sep 2023 17:09:47 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=hMaxFflK; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf20.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.218.50 as permitted sender) smtp.mailfrom=axelrasmussen@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1695402588; 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=YZMr1kN15ref1wUYr4E6kS3JTBkLGuuuWxsn4v7voMQ=; b=uL5gSeyQ2+ErCa3dZ1g6b5LqQIzIoVABXt89b8nbTQGtdAwSa3869lxuHAHtd9OzZjdG1c kIPfVuVbb7zlUJSiw7otboxyaAaHpohPFsHmKDUXJ9kJ6s8hlzp7spCBFK2ZZ9CaVDvWqv ctF5WgvyXDbJbibIF/naqkH3OGlTXVc= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=hMaxFflK; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf20.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.218.50 as permitted sender) smtp.mailfrom=axelrasmussen@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1695402588; a=rsa-sha256; cv=none; b=tbdYX//YcgINOzl13lB09EXhcmhjT84tA5Fo0IbHaQLcE40NyLjjSQzskitMi2fDwgFTj/ EP9+U96v+wAkGiTh9oarT1WB0jXu1kLFMBPIl0urIYeyfYpnlu5+FET430rOTGUl8QzS9r zUy3quuUH8D7fMhNmDAb8SFNsBAKi5A= Received: by mail-ej1-f50.google.com with SMTP id a640c23a62f3a-9ae75ece209so112306066b.3 for ; Fri, 22 Sep 2023 10:09:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1695402587; x=1696007387; darn=kvack.org; 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=YZMr1kN15ref1wUYr4E6kS3JTBkLGuuuWxsn4v7voMQ=; b=hMaxFflKzk1yQzzD5TN8HpqTmnjLFmuNc/2w89wXxPJki568c0vSpAe1w+Q5GbaQLx AfVNYn98KU0TGhxZcjY8d23Unt2WoDVAr+jvbNp0DP8E6KMrnEFQRXwEsAoT0gHdvEgT D9aK84m7adP9HOOKsOK0w6PHPTuo03M9kjKuryarsK3P6f+s8ejklu58tEeurB0NAkjZ lTXtjCl1eebaduz5tgXDMqYj7Is9SuN+pw2xgo0hkYitPXnQqmgyhvYd44jtu961cCIl J2MCVZSZWuKzFzvdt1DKYEP/8C6kGeqXyo813C9dNJiqKcaX/R46MKEIPwqJStX2hs20 JmSg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695402587; x=1696007387; 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=YZMr1kN15ref1wUYr4E6kS3JTBkLGuuuWxsn4v7voMQ=; b=E7b+Mf1psLiOVCA2b4XIwhyhYfGwNPGcLQOgWNLW8ILW/Ih+FyHaK1ssU69nULnWFw azpXQvIqAhCnzWMfGASr4zb9UtTSr1kylU21+H93HjD5EQgBqxO1pCxrM8v76R34fND9 Gi5QpzSddxikXaF5+CdRdVD9GVVbRFezsv4G6E6Xgf4Ip7tOWD93xLgxhKHPB7+sIzwL rvjtt6zQqEboo5zhAgJC+iQvfv+wuAUf2IOQvEZMgI+yn61+gQlGNlS6nhDOl8ouIKYW XmeNoa+/NLbenNCYFDxVEh/t4eQMUA0QA+Pn5bZFcl/yK3MwHfpW5DiAUiVPyv2UOj2t vDrw== X-Gm-Message-State: AOJu0Yxr5RRrFsjeKyOQdRs+m9lNm39qZ+j9HbT0n811sih0hxJvCu+E hGBi64Hy8tOkVZu8u15VbmRr5pw2BVTg+jF6Po6XcA== X-Google-Smtp-Source: AGHT+IH4/kuj4pc7LLnOeIo2zssiCuHuC210f94Y24I1r6FA5L9LiUH2koVLT71RUpM7wLJf6p3XQNKYGbF6emeCse8= X-Received: by 2002:a17:907:985b:b0:9a2:1ce5:1243 with SMTP id jj27-20020a170907985b00b009a21ce51243mr7710566ejc.60.1695402586369; Fri, 22 Sep 2023 10:09:46 -0700 (PDT) MIME-Version: 1.0 References: <20230922115804.2043771-1-ryan.roberts@arm.com> <20230922115804.2043771-3-ryan.roberts@arm.com> <20230922161404.GA23332@willie-the-truck> <32052cb7-91f1-461d-a226-2cd2fcf34ad2@arm.com> In-Reply-To: <32052cb7-91f1-461d-a226-2cd2fcf34ad2@arm.com> From: Axel Rasmussen Date: Fri, 22 Sep 2023 10:09:08 -0700 Message-ID: Subject: Re: [PATCH v2 2/2] arm64: hugetlb: Fix set_huge_pte_at() to work with all swap entries To: Ryan Roberts Cc: Will Deacon , 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 , 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 2F7C11C0036 X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: nxj8c1jjpa61rufqcxs3qq6exq7w6iag X-HE-Tag: 1695402587-984804 X-HE-Meta: U2FsdGVkX1/OoioS5svnwr1sYihFsONobWqcWvmfHusiLUWm2pLGWzaQ6pV5JAX+lTWqY0vxYc3w3Jmgp7MhXhc0+JSLPzVjLjJR9WBEJt1yoaMxBnlxkE7DPbRbCP1V8cWT1aR8EpWrPYzvulVSRaSb+x0zzuGo+n4O9XcajcrrN+4R9Dh/sxTGqeUA915I3rsxMAr+sHSCIUDiKS4Sw4Equ3FGSc2BXuEcSnVk81VWYoew+Mw/r446Oce8sNy8HT0op9QwpOhST+LSEyis+F06sChun1DUMR6lzlTTayA+ZQbjwLpRmVorN5/zzlYMqGFdlM848196Uwc1DjZVNL6aIpzu9V5E2i7YAwLFgGk1xXS7lI74Au2BQIZHSvd+5dS073E/8L8WHQ3w8Q6b/OoZJ/MtBS2tbkC8OGpmlDvPHYY4A7pjNAoMF3RxU78834chLLbSfBChHgn3Ja2Cj5u0PKQP7nQDE5mMlU7u1VXo8qbXrL4v2j4EECCJSspvLgWqtIrmFkxxD4ZrwB496T1P19jWwW/6mqfEfajB0Zx6mn6NglygF8KyxN8WAjHZ0si2BAK0O0HVgA7YFinRUvC5HqCD2IsOdYPTgb9BMsMdCOLuDyVOG/RI5ol6/NXwgpEW+ocs9MzbzuTiUN9NyaE7sjQj3ObBGL4NYyJbn/dVctZA0CNQbJ1nccUgFtbUM50coLa5SdvO4ZuDMLA0ylhEV3dYOSFuV8DrEK7Sy+A5pNCEoxrRNHLSpWK6sEbxm5SebDpgMnhmSPOmdbHSolTw294XmbVjOW2kyj7mp2FcWARMbyrchQe3GTn450W6K2UZtI+Ypxv73NXyf0cFONZU2/ft0iy3vAgzlQC9C3T8FllT0uURgNYOet2jnOFBeu8BwjCdiJbUnAgdDRR3yjRnAHDYr4syiq9y9UQ/4z6KI+7SeLJ2O7+JbfmR1JVsFZJg2qCGcBL6lnU6Bic VuCgrqkZ STv5SDRxp3wv5XrogclTRcTeyPXf3T3VjUaqvx3uPKU0dIyuqv7jDYUbLoO4/cFOe8Om1VxTjGFL4uzayru0hc8sx0QoUx/sgqG5q9jJ3MfUsN40vbO0aa/n/PEqN3GyXGtV2x6WfInHfLayULx6fFdyy/1+tuzkM4DzDFFN3lbRbE25+lFehltKMDtGIyG94UFMGtcIYT04hD8umwS9Gq/8yUgGpltXXnywOoa4IZG/0Goyr+nlrAsok0JJdsS20/o6VGL7dkLx2PP8nStiZ3QFBwzjx/6SMnuYUrdKeDdsVaS1j6zSZaXuHtSkMoSCoRIHP77uv7xdgQlTvYIHnuKMH7Z26Kw/DR7cSXLdYly3k18cEsICsCTHUpurs69FwSmki3VUdscfW+fdrtUMttUUx17Z7752JId9O 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: Looks correct to me - thanks for the fix! Reviewed-by: Axel Rasmussen On Fri, Sep 22, 2023 at 9:41=E2=80=AFAM Ryan Roberts = wrote: > > 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 implementatio= n > >> 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 addre= ss > >> and subsequent panic. > >> > >> arm64's huge pte implementation supports multiple huge page sizes, som= e > >> 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 pt= e > >> 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 typ= es > >> 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 se= t > >> 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, i= t > >> did break the interface because it couldn't handle swap entries withou= t > >> 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 huget= lbfs") > >> 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 e= ntry) > >> -{ > >> - 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, unsig= ned long addr, > >> unsigned long pfn, dpfn; > >> pgprot_t hugeprot; > >> > >> - if (!pte_present(pte)) { > >> - struct folio *folio; > >> - > >> - folio =3D hugetlb_swap_entry_to_folio(pte_to_swp_entry(pt= e)); > >> - ncontig =3D num_contig_ptes(folio_size(folio), &pgsize); > >> + ncontig =3D num_contig_ptes(sz, &pgsize); > >> > >> - for (i =3D 0; i < ncontig; i++, ptep++) > >> + if (!pte_present(pte)) { > >> + for (i =3D 0; i < ncontig; i++, ptep++, addr +=3D 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 us= e 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 >