Re: [PATCH v2 08/10] KVM: s390: Improve unmapping of large ranges

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: s390: _gmap_unmap_pmd_fast() overwrites pte_hole() entries with _PTE_EMPTY
--

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

KVM: s390: Improve unmapping of large ranges

This patch optimizes unmapping of guest ranges larger than 1M by
detaching the PMD, overwriting PTEs in bulk, and reattaching the PMD
rather than invalidating leaf DAT entries individually.

> diff --git a/arch/s390/kvm/gmap/gmap.c b/arch/s390/kvm/gmap/gmap.c
> index c10681f882c2d..01d25aa1f605f 100644
> --- a/arch/s390/kvm/gmap/gmap.c
> +++ b/arch/s390/kvm/gmap/gmap.c
> @@ -465,6 +456,105 @@ bool gmap_unmap_gfn_range(struct gmap *gmap, struct kvm_memory_slot *slot, gfn_t

[ ... ]

> +static long _gmap_unmap_pmd_fast(union crste *crstep, gfn_t gfn, gfn_t next, struct dat_walk *w)
> +{
> +	union crste old = *crstep;
> +	struct page_table *pt;
> +	gfn_t first, last;
> +	unsigned int i;
> +
> +	if (old.h.fc) {
> +		if (old.s.fc1.vsie_notif)
> +			gmap_handle_vsie_unshadow_event(w->priv, gfn);
> +
> +		idte_crste(crstep, gfn, w->datt.asce);
> +		WRITE_ONCE(*crstep, _CRSTE_EMPTY(old.h.tt));
> +
> +		return 0;
> +	}
> +
> +	if (old.h.i)
> +		return 0;
> +
> +	pt = dereference_pmd(old.pmd);
> +	first = max(gfn, w->start) & (PTRS_PER_PTE - 1);
> +	last = (min(next, w->end) - 1) & (PTRS_PER_PTE - 1);
> +
> +	for (i = first; i <= last; i++)
> +		if (pte_needs_unshadow(pt->ptes[i], _PTE_EMPTY, pt->pgstes[i]))
> +			gmap_handle_vsie_unshadow_event(w->priv, gfn + i);
> +
> +	/* Detach the page table */
> +	idte_crste(crstep, gfn, w->datt.asce);
> +
> +	for (i = first; i <= last; i++) {
> +		if (!pt->ptes[i].h.i && w->datt.uses_skeys)
> +			pt->pgstes[i] = dat_save_storage_key_into_pgste(pt->ptes[i], pt->pgstes[i]);
> +		pt->ptes[i] = _PTE_EMPTY;
> +		pt->pgstes[i].vsie_notif = 0;
> +		pt->pgstes[i].vsie_gmem = 0;
> +		pt->pgstes[i].zero = 0;
> +	}

[Severity: High]
Can _gmap_unmap_pmd_fast() overwrite pte_hole() entries with _PTE_EMPTY?

In gmap_unmap_gfn_range_fast(), _dat_walk_gfn_range() is called with
DAT_WALK_IGN_HOLES, which causes dat_crste_walk_range() to skip CRSTE holes.
However, _gmap_unmap_pmd_fast() open-codes the PTE loop over
pt->ptes[first..last] instead of going through dat_pte_walk_range(), which
would normally skip PTE holes when DAT_WALK_IGN_HOLES is set:

arch/s390/kvm/gmap/dat.c:dat_pte_walk_range() {
	for ( ; gfn < end; idx++, gfn++) {
		if (pte_hole(READ_ONCE(table->ptes[idx]))) {
			if (!(w->flags & DAT_WALK_IGN_HOLES))
				return -EFAULT;
			if (!(w->flags & DAT_WALK_ANY))
				continue;
		}
}

When a memslot larger than 1M whose start or end GFN is not 1M-aligned
(_PAGES_PER_SEGMENT) is deleted or moved via kvm_set_memslot(),
kvm_invalidate_memslot() installs invalid_slot into the active
slots->hva_tree before kvm_prepare_memory_region() ->
s390_kvm_mmu_prepare_memory_region() calls dat_delete_slot() under
kvm->mmu_lock:

arch/s390/kvm/gmap/kvm_mmu.c:s390_kvm_mmu_prepare_memory_region() {
	scoped_guard(write_lock, &kvm->mmu_lock) {
		kvm_s390_update_cmma_dirty(kvm, old);
		if (change == KVM_MR_DELETE || change == KVM_MR_MOVE)
			rc = dat_delete_slot(kvm->arch.gmap->datt, old->base_gfn, old->npages);
		if (!rc && (change == KVM_MR_MOVE || change == KVM_MR_CREATE))
			rc = dat_create_slot(mc, kvm->arch.gmap->datt, new->base_gfn, new->npages);
	}
}

For any boundary 1M segment not completely covered by the deleted memslot,
_dat_slot_crste() leaves the existing page table attached (!crste.h.fc &&
!crste.h.i) and _dat_slot_pte() marks the deleted GFNs' PTEs as pte_hole()
(_PTE_TOK(_DAT_TOKEN_PIC, PGM_ADDRESSING)).

After s390_kvm_mmu_prepare_memory_region() drops kvm->mmu_lock and before
kvm_delete_memslot() or kvm_move_memslot() removes invalid_slot from
slots->hva_tree, a concurrent mmu_notifier invalidation
(kvm_mmu_notifier_invalidate_range_start() -> kvm_mmu_unmap_gfn_range() ->
gmap_unmap_gfn_range()) covering more than _PAGES_PER_SEGMENT pages will run
_gmap_unmap_pmd_fast() on that boundary segment and overwrite the pte_hole()
entries with _PTE_EMPTY.

Since dat_entry_walk() checks pte_hole() to return PGM_ADDRESSING for
unmapped GFNs inside a split segment, wouldn't overwriting those hole tokens
with _PTE_EMPTY cause subsequent storage key (ISKE/SSKE/RRBE), ESSA, and CMMA
operations on those unmapped GFNs to treat them as valid guest pages instead
of raising PGM_ADDRESSING or returning -EFAULT?

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