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 <ryan.roberts@xxxxxxx>
>> Fixes: 8a13897fb0da ("mm: userfaultfd: support UFFDIO_POISON for hugetlbfs")
>> Cc: <stable@xxxxxxxxxxxxxxx> # 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 <will@xxxxxxxxxx>
Thanks!
>
> Will
