From: Qi Zheng <zhengqi.arch@bytedance.com>
To: Ryan Roberts <ryan.roberts@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will@kernel.org>,
"James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>,
Helge Deller <deller@gmx.de>, Nicholas Piggin <npiggin@gmail.com>,
Christophe Leroy <christophe.leroy@csgroup.eu>,
Paul Walmsley <paul.walmsley@sifive.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Albert Ou <aou@eecs.berkeley.edu>,
Heiko Carstens <hca@linux.ibm.com>,
Vasily Gorbik <gor@linux.ibm.com>,
Alexander Gordeev <agordeev@linux.ibm.com>,
Christian Borntraeger <borntraeger@linux.ibm.com>,
Sven Schnelle <svens@linux.ibm.com>,
Gerald Schaefer <gerald.schaefer@linux.ibm.com>,
"David S. Miller" <davem@davemloft.net>,
Arnd Bergmann <arnd@arndb.de>,
Mike Kravetz <mike.kravetz@oracle.com>,
Muchun Song <muchun.song@linux.dev>,
SeongJae Park <sj@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
Uladzislau Rezki <urezki@gmail.com>,
Christoph Hellwig <hch@infradead.org>,
Lorenzo Stoakes <lstoakes@gmail.com>,
Anshuman Khandual <anshuman.khandual@arm.com>,
Peter Xu <peterx@redhat.com>,
Axel Rasmussen <axelrasmussen@google.com>,
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
Subject: Re: [PATCH v1 8/8] arm64: hugetlb: Fix set_huge_pte_at() to work with all swap entries
Date: Fri, 22 Sep 2023 17:58:11 +0800 [thread overview]
Message-ID: <41b6524a-effe-c600-962c-2b6e32526dc8@bytedance.com> (raw)
In-Reply-To: <9e8d66fb-1d8d-4ce0-86a7-4c8b04557cca@arm.com>
Hi Ryan,
On 2023/9/22 17:35, Ryan Roberts wrote:
> On 22/09/2023 08:54, Qi Zheng wrote:
>> Hi Ryan,
>>
>> On 2023/9/22 15:40, Ryan Roberts wrote:
>>> On 22/09/2023 03:54, Qi Zheng wrote:
>>>> Hi Ryan,
>>>>
>>>> On 2023/9/22 00:20, 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 contiguous mappings. 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 does 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 sets a PTE_MARKER_POISONED swap entry. But review shows there are
>>>>> other places too (PTE_MARKER_UFFD_WP).
>>>>>
>>>>> So the root cause is 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_swap_pte_at() where
>>>>> the size was inferred from the folio, as descibed above. While that
>>>>> commit didn't break anything at the time,
>>>>
>>>> If it didn't break anything at that time, then shouldn't the Fixes tag
>>>> be added to this commit?
>>>>
>>>>> 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.
>>>>
>>>> So the Fixes tag should be added only to the commit that introduces the
>>>> first new callers?
>>>
>>> Well I guess it's a matter of point of view; My view is that 18f3962953e4 is the
>>> buggy change because it broke the interface to not be able to handle swap
>>> entries which do not contain PFNs. The fact that there were no callers that used
>>> the interface in this way at the time of the commit is irrelevant in my view.
>>
>> I understand your point of view.
>>
>> But IIUC, the Fixes tag is used to indicate the version that needs to
>> backport, but the version where the commit 18f3962953e4 is located
>> does not need to backport this bugfix patch.
>>
>>> But I already added 2 fixes tags; one for the buggy commit, and the other for
>>> the commit containing the new user of the interface.
>>
>> I think 2 fixes tags will cause inconvenience to the maintainers.
>>
>
> I did some Archaeology:
Nice! Thanks for doing this.
>
> $ git rev-list --no-walk=sorted --pretty=oneline \
> 05e90bd05eea33fc77d6b11e121e2da01fee379f \
> 60dfaad65aa97fb6755b9798a6b3c9e79bcd5930 \
> 8a13897fb0daa8f56821f263f0c63661e1c6acae \
> 18f3962953e40401b7ed98e8524167282c3e626e \
> v6.5 v5.18 v5.17 v5.19 v6.5-rc6 v6.5-rc7
>
> 2dde18cd1d8fac735875f2e4987f11817cc0bc2c Linux 6.5
> 706a741595047797872e669b3101429ab8d378ef Linux 6.5-rc7
> 8a13897fb0daa8f56821f263f0c63661e1c6acae mm: userfaultfd: support UFFDIO_POISON for hugetlbfs
> 2ccdd1b13c591d306f0401d98dedc4bdcd02b421 Linux 6.5-rc6
> 3d7cb6b04c3f3115719235cc6866b10326de34cd Linux 5.19
> 18f3962953e40401b7ed98e8524167282c3e626e mm: hugetlb: kill set_huge_swap_pte_at()
> 4b0986a3613c92f4ec1bdc7f60ec66fea135991f Linux 5.18
> 05e90bd05eea33fc77d6b11e121e2da01fee379f mm/hugetlb: only drop uffd-wp special pte if required
> 60dfaad65aa97fb6755b9798a6b3c9e79bcd5930 mm/hugetlb: allow uffd wr-protect none ptes
> f443e374ae131c168a065ea1748feac6b2e76613 Linux 5.17
>
>
> So it turns out that the PTE_MARKER_UFFD_WP markers were added first, using
> set_huge_pte_at(). At the time, this should have used set_huge_swap_pte_at(), so
> was arguably buggy for that reason. However, arm64 does not support UFFD_WP so
> none of the call sites that set the PTE_MARKER_UFFD_WP marker to the pte ever
> trigger on arm64.
>
> Then "mm: hugetlb: kill set_huge_swap_pte_at()" came along and "broke" the
> interface, but there were no callers relying on the behavoir that was broken.
>
> Then "mm: userfaultfd: support UFFDIO_POISON for hugetlbfs" came along in
> v6.5-rc7 and started relying on the broken behaviour of set_huge_pte_at().
Got it.
>
> So on that basis, I agree that the first commit where broken behaviour is
> observable is "mm: userfaultfd: support UFFDIO_POISON for hugetlbfs". So I will
> tag that one as "Fixes". (Although if set_huge_pte_at() was an exported symbol,
> then we would want to mark "mm: hugetlb: kill set_huge_swap_pte_at()").
Agree. I just checked the time point when 18f3962953e4 was added,
neither set_huge_pte_at() nor set_huge_swap_pte_at() are exported
symbols.
Thanks,
Qi
>
> Thanks,
> Ryan
>
>
>
>
>> Thanks,
>> Qi
>>
>>>
>>>>
>>>> Other than that, LGTM.
>>>
>>> Thanks!
>>>
>>>>
>>>> Thanks,
>>>> Qi
>>>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-09-22 9:59 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-21 16:19 [PATCH v1 0/8] Fix set_huge_pte_at() panic on arm64 Ryan Roberts
2023-09-21 16:20 ` [PATCH v1 1/8] parisc: hugetlb: Convert set_huge_pte_at() to take vma Ryan Roberts
2023-09-21 16:20 ` [PATCH v1 2/8] powerpc: " Ryan Roberts
2023-09-21 18:43 ` Christophe Leroy
2023-09-22 6:44 ` Christophe Leroy
2023-09-22 7:19 ` Ryan Roberts
2023-09-22 6:56 ` Christophe Leroy
2023-09-22 7:33 ` Ryan Roberts
2023-09-22 8:10 ` Christophe Leroy
2023-09-22 8:41 ` Ryan Roberts
2023-09-22 9:14 ` Christophe Leroy
2023-09-22 9:37 ` Ryan Roberts
2023-09-21 16:20 ` [PATCH v1 3/8] riscv: " Ryan Roberts
2023-09-22 7:54 ` Alexandre Ghiti
2023-09-22 8:36 ` Ryan Roberts
2023-09-21 16:20 ` [PATCH v1 4/8] s390: " Ryan Roberts
2023-09-21 16:20 ` [PATCH v1 5/8] sparc: " Ryan Roberts
2023-09-21 16:20 ` [PATCH v1 6/8] mm: " Ryan Roberts
2023-09-22 1:37 ` SeongJae Park
2023-09-21 16:20 ` [PATCH v1 7/8] arm64: " Ryan Roberts
2023-09-21 16:20 ` [PATCH v1 8/8] arm64: hugetlb: Fix set_huge_pte_at() to work with all swap entries Ryan Roberts
2023-09-22 2:54 ` Qi Zheng
2023-09-22 7:40 ` Ryan Roberts
2023-09-22 7:54 ` Qi Zheng
2023-09-22 9:35 ` Ryan Roberts
2023-09-22 9:58 ` Qi Zheng [this message]
2023-09-21 16:30 ` [PATCH v1 0/8] Fix set_huge_pte_at() panic on arm64 Andrew Morton
2023-09-21 16:35 ` Ryan Roberts
2023-09-21 17:38 ` Catalin Marinas
2023-09-22 7:41 ` Ryan Roberts
2023-09-22 9:23 ` Greg Kroah-Hartman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=41b6524a-effe-c600-962c-2b6e32526dc8@bytedance.com \
--to=zhengqi.arch@bytedance.com \
--cc=James.Bottomley@HansenPartnership.com \
--cc=agordeev@linux.ibm.com \
--cc=akpm@linux-foundation.org \
--cc=anshuman.khandual@arm.com \
--cc=aou@eecs.berkeley.edu \
--cc=arnd@arndb.de \
--cc=axelrasmussen@google.com \
--cc=borntraeger@linux.ibm.com \
--cc=catalin.marinas@arm.com \
--cc=christophe.leroy@csgroup.eu \
--cc=davem@davemloft.net \
--cc=deller@gmx.de \
--cc=gerald.schaefer@linux.ibm.com \
--cc=gor@linux.ibm.com \
--cc=hca@linux.ibm.com \
--cc=hch@infradead.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-parisc@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=linux-s390@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=lstoakes@gmail.com \
--cc=mike.kravetz@oracle.com \
--cc=muchun.song@linux.dev \
--cc=npiggin@gmail.com \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=peterx@redhat.com \
--cc=ryan.roberts@arm.com \
--cc=sj@kernel.org \
--cc=sparclinux@vger.kernel.org \
--cc=stable@vger.kernel.org \
--cc=svens@linux.ibm.com \
--cc=urezki@gmail.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).