Re: [PATCH v2 03/15] riscv/mm: implement Svnapot contpte read-side helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Missing single-PTE modifier overrides for NAPOT blocks break contiguous encoding, causing Use-After-Free and lockups.
- [High] Unbounded retry loop in `napotpte_ptep_get_lockless()` causes same-CPU deadlock if interrupted by NMI/IRQ.
- [High] `page_table_check` refcount corruption during Svnapot block conversion/clearing.
- [Medium] `ptep_get` and `ptep_get_lockless` strip the `_PAGE_NAPOT` bit, breaking `pte_leaf_size()` for generic MM.
--

--- Patch [3]: [PATCH v2 03/15] riscv/mm: implement Svnapot contpte read-side helpers ---
commit 5e07f7373fe29103de43866f47711b800fb7a33e
Author: Yunhui Cui <cuiyunhui@bytedance.com>

riscv/mm: implement Svnapot contpte read-side helpers

This patch introduces read-side helpers for Svnapot contiguous mappings to
allow generic MM code to operate on the logical sub-PTEs. The public PTE
getters are routed through these helpers.

> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index a0f64e980e981..fce6d04ea65db 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -706,37 +706,37 @@ static inline pte_t __ptep_get_lockless(pte_t *ptep)
>  	return __ptep_get(ptep);
>  }
>  
> +#ifdef CONFIG_RISCV_ISA_SVNAPOT
> +
> +#define ptep_get ptep_get
> +static inline pte_t ptep_get(pte_t *ptep)
> +{
> +	pte_t pte = __ptep_get(ptep);
> +
> +	if (likely(!pte_present_napot(pte)))
> +		return pte;
> +
> +	return napotpte_ptep_get(ptep, pte);
> +}
> +
> +#define ptep_get_lockless ptep_get_lockless
> +static inline pte_t ptep_get_lockless(pte_t *ptep)
> +{
> +	pte_t pte = __ptep_get_lockless(ptep);
> +
> +	if (likely(!pte_present_napot(pte)))
> +		return pte;
> +
> +	return napotpte_ptep_get_lockless(ptep);
> +}
> +

[Severity: Critical]
Does this implementation miss overriding ptep_get_and_clear and 
ptep_modify_prot_start for NAPOT blocks?

If a NAPOT block is partially unmapped or modified via madvise() or
mprotect(), the generic MM code will call ptep_get_and_clear() on a single
PTE. Because these functions are not overridden, they fall back to the raw
__ptep_get_and_clear().

This clears only one PTE, leaving the other 15 with _PAGE_NAPOT still set,
which breaks hardware rules. Additionally, it returns the raw NAPOT PTE where
pte_pfn() evaluates to the base PFN, causing the core MM to drop the
mapcount and refcount on the base sub-page instead of the actual targeted
sub-page. Could this lead to data corruption or a use-after-free?

[ ... ]

> diff --git a/arch/riscv/mm/contpte.c b/arch/riscv/mm/contpte.c
> new file mode 100644
> index 0000000000000..43ea76e424492
> --- /dev/null
> +++ b/arch/riscv/mm/contpte.c
> @@ -0,0 +1,273 @@

[ ... ]

> +static inline pte_t napotpte_subpte(pte_t *ptep, pte_t pte)
> +{
> +	unsigned long pfn;
> +	pgprot_t prot;
> +
> +	if (!pte_present_napot(pte))
> +		return pte;
> +
> +	pfn = pte_pfn(pte) + (ptep - napot_align_ptep(ptep));
> +	prot = __pgprot(pte_protval_no_pfn_no_napot(pte));
> +
> +	return pfn_pte(pfn, prot);
> +}

[Severity: Medium]
Since this helper strips the _PAGE_NAPOT bit by using 
pte_protval_no_pfn_no_napot(pte), does it break pte_leaf_size() for generic MM?

If pte_leaf_size() expects the Svnapot bit to be present to return the
contiguous block size, returning a subpte without it will cause callers like
perf_get_page_size() to falsely evaluate the mapping as PAGE_SIZE.

> +
> +static inline pte_t __napot_ptep_get_and_clear(struct mm_struct *mm,
> +					       unsigned long addr, pte_t *ptep)
> +{
> +	pte_t pte;
> +
> +	pte = __pte(atomic_long_xchg((atomic_long_t *)ptep, 0));
> +	page_table_check_pte_clear(mm, addr, pte_mknonnapot(pte, addr));
> +
> +	return pte;
> +}

[Severity: High]
Can this cause page_table_check refcount corruption during Svnapot block
conversion?

pte_mknonnapot() returns the raw PTE unmodified. Because the raw Svnapot PFN
encoded in all 16 PTEs is identically base_pfn | 0x8, pte_pfn() inside
page_table_check_pte_clear() evaluates to the exact same base_pfn for all
16 iterations. 

Will this cause the refcount for the base PFN to be decremented 16 times
while leaking the remaining 15 mapped sub-PFNs?

> +
> +static void napotpte_convert(struct mm_struct *mm, unsigned long addr,
> +			     pte_t *ptep, pte_t target)
> +{
> +	unsigned long start_addr, end, ptent_addr;
> +	pte_t *start_ptep;
> +	pte_t ptent, pte;
> +	unsigned int i, nr;
> +
> +	start_addr = napot_align_addr(addr);
> +	start_ptep = napot_align_ptep(ptep);
> +	nr = napotpte_pte_num();
> +	end = start_addr + napotpte_size();
> +
> +	for (i = 0; i < nr; i++) {
> +		ptent_addr = start_addr + i * PAGE_SIZE;
> +		ptent = __napot_ptep_get_and_clear(mm, ptent_addr,
> +						   start_ptep + i);

[Severity: High]
Does this create a preemption window that can lead to a CPU deadlock?

napotpte_convert() modifies NAPOT blocks by clearing their 16 PTEs sequentially
while holding the PTL, but it does not disable local interrupts.

[ ... ]

> +pte_t napotpte_ptep_get_lockless(pte_t *orig_ptep)
> +{
> +	pte_t orig_pte, pte;
> +	pte_t *ptep;
> +	unsigned int i, nr;
> +
> +	if (!napot_hw_supported())
> +		return READ_ONCE(*orig_ptep);
> +
> +	nr = napotpte_pte_num();
> +
> +retry:
> +	orig_pte = READ_ONCE(*orig_ptep);
> +	if (!pte_present_napot(orig_pte))
> +		return orig_pte;
> +
> +	ptep = napot_align_ptep(orig_ptep);
> +
> +	for (i = 0; i < nr; i++, ptep++) {
> +		pte = READ_ONCE(*ptep);
> +
> +		if (!napotpte_is_consistent(pte, orig_pte))
> +			goto retry;

[Severity: High]
If a perf NMI interrupts napotpte_convert() on the same CPU while the block is
partially cleared and attempts a lockless page table walk, won't this loop
spin indefinitely?

The getter detects the intermediate inconsistency, jumps to retry, and spins
forever because the writer is interrupted on the exact same CPU and cannot
resume to restore consistency.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784201104.git.cuiyunhui@bytedance.com?part=3