Re: [PATCH v8 13/15] s390/mm: Batch PTE updates in lazy MMU mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] s390/mm: NULL pointer deref on CPU hotplug lazy MMU rollback failure
- [High] s390/mm: Hardirq torn read of ipte_range bounds causes OOB memory write
- [High] s390/mm: ptep_get_and_clear_full() bypasses fullmm optimization in lazy MMU mode
- [Medium] s390/mm: ptep_get_and_clear_full() bypasses fullmm TLB optimization
--

commit 93b412133f9ff3548a7d0d31445ca8ff8b51da29
Author: Alexander Gordeev <agordeev@linux.ibm.com>

s390/mm: Batch PTE updates in lazy MMU mode

This patch introduces a per-CPU cache to batch PTE updates (via the IPTE
instruction) during lazy MMU mode. This reduces CEC-wide quiesce traffic
for memory-management system calls like mprotect(), munmap(), and mremap(),
yielding significant performance improvements.

> diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
> index c47264f3abf21..decce76fac743 100644
> --- a/arch/s390/include/asm/pgtable.h
> +++ b/arch/s390/include/asm/pgtable.h
[ ... ]
> @@ -1263,11 +1383,13 @@ static inline pte_t ptep_get_and_clear_full(struct mm_struct *mm,
>  {
>  	pte_t res;
>  
> -	if (full) {
> -		res = ptep_get(ptep);
> -		set_pte(ptep, __pte(_PAGE_INVALID));
> -	} else {
> -		res = ptep_xchg_lazy(mm, addr, ptep, __pte(_PAGE_INVALID));
> +	if (!lazy_mmu_ptep_get_and_clear(addr, ptep, &res)) {
> +		if (full) {
> +			res = __ptep_get(ptep);
> +			__set_pte(ptep, __pte(_PAGE_INVALID));
> +		} else {
> +			res = ptep_xchg_lazy(mm, addr, ptep, __pte(_PAGE_INVALID));
> +		}
>  	}

[Severity: High]
Does intercepting the clear operation via lazy_mmu_ptep_get_and_clear() before
checking the full parameter bypass the fullmm optimization?

During process teardown (e.g. exit_mmap()), fullmm is set to 1 to skip
individual IPTE (TLB flush) instructions because a full MM TLB flush will be
performed at the end. By unconditionally caching the clear operation in the
lazy MMU cache here, leave_ipte_range() will later issue batch IPTE
instructions for these entries when lazy MMU mode is disabled. This appears
to degrade the O(1) TLB teardown into O(N) CEC-wide synchronizing IPTE
instructions.

[ ... ]
> diff --git a/arch/s390/mm/lazy_mmu.c b/arch/s390/mm/lazy_mmu.c
> new file mode 100644
> index 0000000000000..8c1a62f237362
> --- /dev/null
> +++ b/arch/s390/mm/lazy_mmu.c
[ ... ]
> +static void enter_ipte_range(struct mm_struct *mm,
> +			     unsigned long addr, unsigned long end, hw_pte_t *pte)
> +{
> +	struct ipte_range *range;
> +
> +	if (!static_branch_likely(&lazy_mmu_enabled))
> +		return;
> +
> +	range = get_cpu_var(ipte_range);

[Severity: High]
If lazy_mmu_alloc() fails during CPU hotplug, could this retrieve a NULL
ipte_range pointer?

When a lazy_mmu_alloc() failure triggers a CPU hotplug rollback,
lazy_mmu_cpu_offline() frees the memory but does not disable the globally
active lazy_mmu_enabled static branch. Subsequent lazy MMU entries on that
CPU would bypass the static branch check above, retrieve a NULL ipte_range,
and dereference it below.

> +	local_bh_disable();
> +	get_lowcore()->lazy_mmu_count++;
> +
> +	if (mm_is_protected(mm)) {
> +		local_bh_enable();
> +		return;
> +	}
> +
> +	range->mm = mm;
> +	range->base_addr = addr;
> +	range->base_end = end;

[Severity: High]
Can a hardirq observe a torn state here?

The base_addr and base_end fields are updated sequentially. If a hardirq or
NMI strikes after base_addr is set but while base_end is still 0, a
concurrent execution of this_ipte_range() would observe base_end == 0.

This would cause the calculation:
nr_ptes = (range->base_end - range->base_addr) / PAGE_SIZE;

to underflow to a massive value, bypassing the bounds validation and allowing
an out-of-bounds memory write to range->cache[idx].

> +	range->base_pte = pte;
> +
> +	local_bh_enable();
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791365932.git.agordeev@linux.ibm.com?part=13