Re: [PATCH v2 1/1] s390/mm: Fix handling of _PAGE_UNUSED pte bit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] `ptep_reset_dat_prot()` misses clearing the `_PAGE_UNUSED` bit, allowing used pages to be wrongly discarded.
- [Critical] Hardware Storage Key updates by guest accesses bypass host PTE modifications, allowing `_PAGE_UNUSED` to linger and cause guest corruption.
--

commit 4a7ab46235e9a4cd801253465aae645c11c76ebb
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

s390/mm: Fix handling of _PAGE_UNUSED pte bit

The patch introduces checks to clear the _PAGE_UNUSED bit in various ptep
exchange and modify functions to prevent used pages from being discarded.
It also ensures the bit is only set for present PTEs in the gmap helper.

> diff --git a/arch/s390/mm/gmap_helpers.c b/arch/s390/mm/gmap_helpers.c
> index 1cfe4724fbe2a..5a7d6b9790e22 100644
> --- a/arch/s390/mm/gmap_helpers.c
> +++ b/arch/s390/mm/gmap_helpers.c

[ ... ]

> @@ -180,8 +180,8 @@ void gmap_helper_try_set_pte_unused(struct mm_struct *mm, unsigned long vmaddr)
>  	ptep = try_get_locked_pte(mm, vmaddr, &ptl);
>  	if (IS_ERR_OR_NULL(ptep))
>  		return;
> -
> -	__atomic64_or(_PAGE_UNUSED, (long *)ptep);
> +	if (pte_present(*ptep))
> +		__atomic64_or(_PAGE_UNUSED, (long *)ptep);

[Severity: Critical]
This is a pre-existing issue, but does the approach of clearing _PAGE_UNUSED
during host PTE modifications handle hardware-driven updates?

In gmap_helper_try_set_pte_unused(), __atomic64_or() sets _PAGE_UNUSED
without clearing the present or writable bits in the host PTE. If the guest
subsequently writes to this page, the CPU hardware handles it entirely via
Dynamic Address Translation, updating the hardware Storage Key without ever
trapping into the host kernel or modifying the host PTE.

Because the host PTE is never modified, the clearance logic added in this
patch is completely bypassed.

If the host later reclaims this memory via a path that skips
folio_referenced() (such as madvise(MADV_PAGEOUT), which calls
reclaim_pages() with ignore_references = true), the PTE is passed to
try_to_unmap_one() with _PAGE_UNUSED still set:

mm/rmap.c:try_to_unmap_one() {
    ...
    } else if (likely(pte_present(pteval)) && pte_unused(pteval) &&
               !userfaultfd_armed(vma)) {
        dec_mm_counter(mm, mm_counter(folio));
    ...
}

Because try_to_unmap_one() evaluates pte_unused() before checking the
Storage Key's dirty state, won't this unconditionally discard the page and
permanently destroy the guest's modified data?

>  	pte_unmap_unlock(ptep, ptl);
>  }

[ ... ]

> diff --git a/arch/s390/mm/pgtable.c b/arch/s390/mm/pgtable.c
> index 4acd8b140c4bd..2acc79383e7df 100644
> --- a/arch/s390/mm/pgtable.c
> +++ b/arch/s390/mm/pgtable.c

[ ... ]

> @@ -175,6 +179,8 @@ pte_t ptep_modify_prot_start(struct vm_area_struct *vma, unsigned long addr,
>  void ptep_modify_prot_commit(struct vm_area_struct *vma, unsigned long addr,
>  			     pte_t *ptep, pte_t old_pte, pte_t pte)
>  {
> +	if (pte_present(pte))
> +		pte = clear_pte_bit(pte, __pgprot(_PAGE_UNUSED));
>  	set_pte(ptep, pte);
>  }

[Severity: Critical]
This is a pre-existing issue, but it looks like the fix might be incomplete.
Does ptep_reset_dat_prot() also need to clear the _PAGE_UNUSED bit?

Unlike ptep_xchg_direct(), ptep_reset_dat_prot() directly writes the new
PTE without clearing _PAGE_UNUSED:

arch/s390/mm/pgtable.c:ptep_reset_dat_prot() {
    ...
    set_pte(ptep, new);
    ...
}

When a KVM guest unmaps a page, it is marked with _PAGE_UNUSED. If the guest
subsequently writes to this page (for example, an RO to RW transition on a
clean page), ptep_set_access_flags() is called by the host page fault handler.

Because the modification only affects software bits and _PAGE_PROTECT,
pte_allow_rdp() evaluates to true, delegating the PTE update to the
architecture-specific fast path:

arch/s390/include/asm/pgtable.h:ptep_set_access_flags() {
    ...
    if (cpu_has_rdp() && pte_allow_rdp(*ptep, entry))
        ptep_reset_dat_prot(vma->vm_mm, addr, ptep, entry);
    ...
}

Since the new PTE was constructed from the old one, it inherits the
_PAGE_UNUSED flag. Won't the now-dirty page retain the unused flag and be
erroneously discarded without writeback during memory reclaim by
try_to_unmap_one()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615091741.76724-2-imbrenda@linux.ibm.com?part=1