Re: [PATCH v2 13/15] riscv/mm: remove redundant TLB flush in napotpte_convert

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing the TLB flush in napotpte_convert() allows stale 4KB TLB entries to survive when folding base PTEs into a NAPOT block, leading to silent data corruption during partial-block TLB flushes.
--

--- Patch [13]: [PATCH v2 13/15] riscv/mm: remove redundant TLB flush in napotpte_convert ---
commit 63217ab7f83ce36005df21143bf66fc315027a26
Author: Yunhui Cui <cuiyunhui@bytedance.com>

riscv/mm: remove redundant TLB flush in napotpte_convert

This commit removes the TLB flush during the conversion between a NAPOT
block and its equivalent base PTEs in napotpte_convert(). The justification
is that since the effective translations (PFNs and permissions) are unchanged,
TLB invalidation can be deferred to operations that actually change the
effective mapping.

Link: https://lists.riscv.org/g/tech-privileged/message/2950

> diff --git a/arch/riscv/mm/contpte.c b/arch/riscv/mm/contpte.c
> index b0b8ff0aade3a..8289f39d6e188 100644
> --- a/arch/riscv/mm/contpte.c
> +++ b/arch/riscv/mm/contpte.c

[ ... ]

> @@ -149,8 +149,15 @@ static void napotpte_convert(struct mm_struct *mm, unsigned long addr,
>  			target = pte_mkyoung(target);
>  	}
>  
> -	flush_tlb_mm_range(mm, start_addr, end, PAGE_SIZE);
> -

[Severity: High]
Does removing this TLB flush allow stale 4KB TLB entries to survive when
folding base PTEs into a NAPOT block?

When set_ptes() maps a single page that completes a 16-page aligned block
backed by a large folio, __napotpte_try_fold() folds the 16 base PTEs into
a 64KB NAPOT block via napotpte_convert(). The CPU may have already cached
separate 4KB TLB entries for the existing 15 pages. Without the TLB flush
in napotpte_convert(), these 4KB TLB entries can survive the folding process.

Could this lead to silent data corruption during partial-block TLB flushes?

Later, if an operation like madvise(MADV_FREE) targets a single 4KB subpage
of this NAPOT block, it calls split_folio(), which splits the underlying
folio but does not alter the NAPOT page table entries. On retry, the loop
processes the now order-0 folio with nr = 1 and calls
clear_young_dirty_ptes() with nr = 1.

Because the PTE still has the NAPOT bit set, napotpte_clear_young_dirty_ptes()
correctly expands the operation to clear the young and dirty bits for all 16
PTEs in the page table to maintain the NAPOT encoding invariant. However,
madvise_free_pte_range() then queues only a 1-page TLB flush:

	tlb_remove_tlb_entries(tlb, pte, 1, addr);

If the TLB held a 64KB NAPOT entry, the single-page sfence.vma would
successfully invalidate the entire 64KB block. But because the old 4KB TLB
entries survived the folding, the flush only invalidates the single targeted
4KB entry, leaving the other 15 4KB TLB entries valid.

Since their dirty bits were cleared in the page table but they are still
cached as dirty in the TLB, subsequent writes to these 15 pages will not set
the hardware dirty bit in the page table. Will the kernel later view these
modified pages as clean and discard them without writeback during memory
reclaim, dropping dirty data?

> +	/*
> +	 * This conversion only changes the representation of the same
> +	 * effective translations: NAPOT encoding <-> equivalent base PTEs.
> +	 * A flush here would not replace the TLB maintenance required by a
> +	 * later permission downgrade, because writable base-PTE translations
> +	 * could be refilled after this conversion and before that downgrade.
> +	 * Leave TLB invalidation to the operation that changes the effective
> +	 * mapping.
> +	 */
>  	page_table_check_ptes_set(mm, start_addr, start_ptep, target, nr);
>  	if (pte_napot(target)) {

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