Re: [PATCH v5 2/4] 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:
- [Critical] Violation of Ultravisor architecture constraints by deferring PTE invalidation while calling `uv_convert_from_secure_pte()`.
- [High] `ptep_get_and_clear_full()` bypasses the `full` optimization during process teardown, causing massive redundant IPTE traffic.
- [High] Deferred page table memory updates break lockless page table walkers (like GUP-fast).
- [Low] Dead declarations for IPTE range modify protection functions.
--

--- Patch [2]: [PATCH v5 2/4] s390/mm: Batch PTE updates in lazy MMU mode ---
commit dedb613a05cdcf9218ef386e46c14b5cce4aa714
Author: Alexander Gordeev <agordeev@linux.ibm.com>

s390/mm: Batch PTE updates in lazy MMU mode

This commit introduces a per-CPU cache to batch PTE updates while in
lazy MMU mode. It intercepts PTE updates to store them in a software
cache, flushing them to the hardware page tables in a single operation
upon exiting lazy MMU mode.

> diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
> --- a/arch/s390/include/asm/pgtable.h
> +++ b/arch/s390/include/asm/pgtable.h
> [ ... ]
> @@ -1220,7 +1308,8 @@ static inline pte_t ptep_get_and_clear(struct mm_struct *mm,
>  {
>  	pte_t res;
>  
> -	res = ptep_xchg_lazy(mm, addr, ptep, __pte(_PAGE_INVALID));
> +	if (!lazy_mmu_ptep_get_and_clear(addr, ptep, &res))
> +		res = ptep_xchg_lazy(mm, addr, ptep, __pte(_PAGE_INVALID));
>  	page_table_check_pte_clear(mm, addr, res);
>  	/* At this point the reference through the mapping is still present */
>  	if (mm_is_protected(mm) && pte_present(res))

[Severity: Critical]
Does this sequence violate Ultravisor constraints?

When unmapping secure execution guest memory, this path caches the
invalidation but leaves the hardware page table unmodified. The function
then immediately calls uv_convert_from_secure_pte() while the real PTE in
memory is still present.

Could concurrent hardware walks encounter a secure PTE mapping a now
non-secure page and trigger a hardware fault?

> [ ... ]
> @@ -1229,9 +1318,34 @@ static inline pte_t ptep_get_and_clear(struct mm_struct *mm,
> [ ... ]
> +bool ipte_range_ptep_modify_prot_start(struct vm_area_struct *vma,
> +				       unsigned long addr, pte_t *ptep, pte_t *res);
> +bool ipte_range_ptep_modify_prot_commit(struct vm_area_struct *vma,
> +					unsigned long addr, pte_t *ptep,
> +					pte_t old_pte, pte_t pte);

[Severity: Low]
Are these declarations for ipte_range_ptep_modify_prot_start() and
ipte_range_ptep_modify_prot_commit() used anywhere?

It looks like they are declared here but never implemented, as the code
instead provides __lazy_mmu_ptep_modify_prot_start().

> [ ... ]
> @@ -1261,11 +1375,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 this bypass the full optimization during process teardown?

During exit_mmap(), zap_pte_range() passes full=1 to skip individual IPTE
flushes since the entire address space is being destroyed.

By unconditionally intercepting the call via lazy_mmu_ptep_get_and_clear(),
this bypasses the full optimization fast path. Will this cause
leave_ipte_range() to subsequently issue redundant IPTE instructions for
every single valid PTE?

> diff --git a/arch/s390/mm/lazy_mmu.c b/arch/s390/mm/lazy_mmu.c
> --- /dev/null
> +++ b/arch/s390/mm/lazy_mmu.c
> [ ... ]
> +bool __lazy_mmu_ptep_get_and_clear(unsigned long addr, pte_t *ptep, pte_t *res)
> +{
> +	struct ipte_range *range;
> +	pte_t pte, old;
> +
> +	range = this_ipte_range(ptep);
> +	if (!range)
> +		return false;
> +
> +	old = __ipte_range_ptep_get(range, ptep);
> +	pte = __pte(_PAGE_INVALID);
> +	__ipte_range_set_pte(range, ptep, pte);
> +	*res = old;
> +
> +	return true;
> +}

[Severity: High]
Can this deferred page table update cause issues with lockless page table
walkers like get_user_pages_fast()?

When mprotect() uses lazy MMU mode and calls ptep_modify_prot_start(),
this function updates the local software cache to invalid but leaves the
hardware PTE fully valid and writable.

Since lockless walkers read the hardware page tables directly without
taking the page table lock, could they see a present, writable PTE and
successfully pin the page for writing, completely ignoring the ongoing
mprotect()?

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