Re: [PATCH RESEND 3/3] arm64: Add PUD THP support for DAX and pfnmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] arm64: mm: local-only TLB flush in pudp_set_access_flags() causes fault livelock
- [High] arm64: mm: missing pudp_set_wrprotect() loses HAFDBS dirty bits and triggers warning
- [High] arm64: mm: missing pudp_huge_get_and_clear() loses concurrent HAFDBS A/D updates
- [Low] arm64: mm: redefinition of pud_trans_huge() when CONFIG_TRANSPARENT_HUGEPAGE=n
--

commit 7dc812ccee714c3818da8b998f785b9073deeacd
Author: Jinjiang Tu <tujinjiang@huawei.com>

arm64: Add PUD THP support for DAX and pfnmap

This patch adds PUD transparent hugepage page table helpers on arm64 and
enables HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD for 4KB page size configurations to
support PUD THP for DAX and pfnmap mappings.

> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 7de2487e4d353..feca481b20795 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h

[ ... ]

> @@ -826,6 +838,13 @@ static inline bool pud_leaf(pud_t pud)
>  #define pud_user(pud)		pte_user(pud_pte(pud))
>  #define pud_user_exec(pud)	pte_user_exec(pud_pte(pud))
>  
> +#ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
> +static inline int pud_trans_huge(pud_t pud)
> +{
> +	return pud_present(pud) && !pud_table(__pud(pud_val(pud) | PTE_VALID));
> +}
> +#endif

[Severity: Low]
Will this cause a redefinition error for pud_trans_huge() when building with
CONFIG_PAGE_SIZE_4KB=y and CONFIG_TRANSPARENT_HUGEPAGE=n (for example,
allnoconfig or tinyconfig)?

In arch/arm64/Kconfig, config ARM64 selects
HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD whenever PAGE_SIZE_4KB is enabled, even
when CONFIG_TRANSPARENT_HUGEPAGE is disabled:

	select HAVE_ARCH_TRANSPARENT_HUGEPAGE
	select HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD if PAGE_SIZE_4KB

However, include/linux/pgtable.h includes asm/pgtable.h and then provides a
fallback definition of pud_trans_huge() whenever CONFIG_TRANSPARENT_HUGEPAGE
is not defined:

#if !defined(CONFIG_TRANSPARENT_HUGEPAGE) || \
	!defined(CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD)
static inline int pud_trans_huge(pud_t pud)
{
	return 0;
}
#endif

Should pud_trans_huge() in arch/arm64/include/asm/pgtable.h also be guarded by
CONFIG_TRANSPARENT_HUGEPAGE (matching pmd_trans_huge() in the same file)?

[ ... ]

> @@ -1207,6 +1231,16 @@ static inline int pmdp_set_access_flags(struct vm_area_struct *vma,
>  }
>  #endif
>  
> +#ifdef CONFIG_ARCH_SUPPORTS_PUD_PFNMAP
> +static inline int pudp_set_access_flags(struct vm_area_struct *vma,
> +					unsigned long address, pud_t *pudp,
> +					pud_t entry, int dirty)
> +{
> +	return __ptep_set_access_flags_anysz(vma, address, (pte_t *)pudp,
> +					     pud_pte(entry), dirty, PUD_SIZE);
> +}
> +#endif

[Severity: High]
Can delegating pudp_set_access_flags() to __ptep_set_access_flags_anysz() lead
to an infinite spurious write page-fault livelock on remote CPUs?

In arch/arm64/mm/fault.c, __ptep_set_access_flags_anysz() only invalidates the
local CPU's TLB via TLBF_NOBROADCAST when upgrading a read-only entry to
writable/dirty, relying on remote CPUs to invalidate their stale read-only TLB
entries on a subsequent permission fault:

	if (pte_same(pte, entry))
		return 0;
	...
	/*
	 * Invalidate the local stale read-only entry.  Remote stale entries
	 * may still cause page faults and be invalidated via
	 * flush_tlb_fix_spurious_fault().
	 */
	if (dirty) {
		...
		__flush_tlb_range(vma, address, address + pgsize, pgsize, level,
				  TLBF_NOWALKCACHE | TLBF_NOBROADCAST);
	}

Suppose two CPUs fault in a shared PUD mapping as hardware read-only
(PTE_RDONLY=1, such as on an initial read fault for fsdax, a PFNMAP/devdax
mapping on a CPU without hardware DBM, or after copy_huge_pud() write-protects
the PUD), and CPU A then writes to the mapping. CPU A updates *pudp via
wp_huge_pud() -> insert_pud() -> pudp_set_access_flags() or
huge_pud_set_accessed() -> touch_pud() -> pudp_set_access_flags(), flushing
only CPU A's local TLB.

When CPU B subsequently writes to the same PUD range, it takes a permission
fault from its stale read-only local TLB entry and enters:

__handle_mm_fault()
  huge_pud_set_accessed()
    touch_pud()
      pudp_set_access_flags()
        __ptep_set_access_flags_anysz()

Because *pudp in memory already matches entry (with PTE_AF, PTE_DIRTY, and
PTE_WRITE set and PTE_RDONLY cleared), pte_same(pte, entry) is true and
__ptep_set_access_flags_anysz() returns 0 without flushing CPU B's TLB.

Unlike the PTE and PMD paths, which call flush_tlb_fix_spurious_fault() and
flush_tlb_fix_spurious_fault_pmd() when ptep_set_access_flags() or
pmdp_set_access_flags() returns 0, neither touch_pud(),
huge_pud_set_accessed(), __handle_mm_fault(), nor insert_pud() performs a
local TLB flush on a spurious PUD fault. Wouldn't CPU B return to userspace
with the stale read-only PUD TLB entry still in place and fault in an infinite
loop?

[ ... ]

> @@ -1281,6 +1315,14 @@ static inline bool pmdp_test_and_clear_young(struct vm_area_struct *vma,
>  }
>  #endif /* CONFIG_TRANSPARENT_HUGEPAGE || CONFIG_ARCH_HAS_NONLEAF_PMD_YOUNG */
>  
> +#ifdef CONFIG_ARCH_SUPPORTS_PUD_PFNMAP
> +static inline bool pudp_test_and_clear_young(struct vm_area_struct *vma,
> +		unsigned long address, pud_t *pudp)
> +{
> +	return __ptep_test_and_clear_young(vma, address, (pte_t *)pudp);
> +}
> +#endif
> +
>  static inline pte_t __ptep_get_and_clear_anysz(struct mm_struct *mm,
>  					       unsigned long address,
>  					       pte_t *ptep,

[Severity: High]
Should arm64 also define __HAVE_ARCH_PUDP_HUGE_GET_AND_CLEAR and implement
pudp_huge_get_and_clear() via __ptep_get_and_clear_anysz(..., PUD_SIZE)?

__ptep_get_and_clear_anysz() in arch/arm64/include/asm/pgtable.h already uses
an atomic xchg_relaxed() and has a PUD_SIZE case for
page_table_check_pud_clear(). Without an arch definition of
pudp_huge_get_and_clear(), arm64 falls back to the generic implementation in
include/linux/pgtable.h:

#ifndef __HAVE_ARCH_PUDP_HUGE_GET_AND_CLEAR
static inline pud_t pudp_huge_get_and_clear(struct mm_struct *mm,
					    unsigned long address,
					    pud_t *pudp)
{
	pud_t pud = *pudp;

	pud_clear(pudp);
	page_table_check_pud_clear(mm, address, pud);

	return pud;
}
#endif

When splitting a DAX PUD via split_huge_pud() -> __split_huge_pud_locked() ->
pudp_huge_clear_flush() -> pudp_huge_get_and_clear(), __split_huge_pud_locked()
propagates the dirty and young bits from old_pud to the folio:

	old_pud = pudp_huge_clear_flush(vma, haddr, pud);

	if (!vma_is_dax(vma))
		return;

	page = pud_page(old_pud);
	folio = page_folio(page);

	if (!folio_test_dirty(folio) && pud_dirty(old_pud))
		folio_mark_dirty(folio);
	if (!folio_test_referenced(folio) && pud_young(old_pud))
		folio_set_referenced(folio);

On systems with FEAT_HAFDBS, couldn't a concurrent hardware access or dirty
state update between the non-atomic read of *pudp and pud_clear(pudp) be lost?

[ ... ]

> @@ -1443,6 +1485,15 @@ static inline pmd_t pmdp_establish(struct vm_area_struct *vma,
>  }
>  #endif
>  
> +#ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
> +static inline pud_t pudp_establish(struct vm_area_struct *vma,
> +		unsigned long address, pud_t *pudp, pud_t pud)
> +{
> +	page_table_check_pud_set(vma->vm_mm, address, pudp, pud);
> +	return __pud(xchg_relaxed(&pud_val(*pudp), pud_val(pud)));
> +}
> +#endif

[Severity: High]
Is __HAVE_ARCH_PUDP_SET_WRPROTECT with pudp_set_wrprotect() also needed here,
matching pmdp_set_wrprotect() right above pmdp_establish()?

Without an architecture-specific pudp_set_wrprotect() wrapping
__ptep_set_wrprotect(), arm64 uses the generic fallback in
include/linux/pgtable.h:

static inline void pudp_set_wrprotect(struct mm_struct *mm,
				      unsigned long address, pud_t *pudp)
{
	pud_t old_pud = *pudp;

	set_pud_at(mm, address, pudp, pud_wrprotect(old_pud));
}

When fork() copies a COW PUD via copy_page_range() -> copy_p4d_range() ->
copy_pud_range() -> copy_huge_pud() -> pudp_set_wrprotect():

1. The non-atomic read of *pudp followed by set_pud_at() races with hardware
   AF/DBM updates (FEAT_HAFDBS), unlike __ptep_set_wrprotect() which uses a
   cmpxchg_relaxed() loop to atomically transfer hardware dirty state to
   PTE_DIRTY while setting PTE_RDONLY and clearing PTE_WRITE.

2. Updating a live valid PUD in place via set_pud_at() -> __set_ptes_anysz()
   calls __check_safe_pte_update() in arch/arm64/mm/mmu.c, which triggers
   VM_WARN_ONCE() when the entry is old or writable-clean on a live mm:

	VM_WARN_ONCE(!pte_young(pte),
		     "%s: racy access flag clearing: %s -> %s",
		     __func__, pte_str_old, pte_str);
	VM_WARN_ONCE(pte_write(old_pte) && !pte_dirty(pte),
		     "%s: racy dirty state clearing: %s -> %s",
		     __func__, pte_str_old, pte_str);

3. set_pud_at() also runs __sync_cache_and_tags() across PUD_SIZE >> PAGE_SHIFT
   (262,144) pages unnecessarily when only write-protecting an existing entry.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028121404.853944-1-tujinjiang@huawei.com?part=3