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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM: s390: hole PTE clobber and stale pgste.zero in _gmap_unmap_pmd_fast()
- [High] KVM: s390: missing compiler barriers around PTE loop in _gmap_unmap_pmd_fast()
--

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

KVM: s390: Improve unmapping of large ranges

This commit adds a fast path for unmapping guest address ranges larger than
one segment by detaching the pmd via idte_crste(), clearing the pte entries
in the detached page table while saving storage keys, and reattaching the
pmd.

> diff --git a/arch/s390/kvm/gmap/gmap.c b/arch/s390/kvm/gmap/gmap.c
> index c10681f882c2d..6700a87a420a8 100644
> --- a/arch/s390/kvm/gmap/gmap.c
> +++ b/arch/s390/kvm/gmap/gmap.c

[ ... ]

> @@ -465,6 +456,103 @@ bool gmap_unmap_gfn_range(struct gmap *gmap, struct kvm_memory_slot *slot, gfn_t
>  	return false;
>  }
>  
> +static long _gmap_unmap_pud_fast(union crste *crstep, gfn_t gfn, gfn_t next, struct dat_walk *w)
> +{

[ ... ]

> +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;
> +	}

[Severity: High]
Can this loop in _gmap_unmap_pmd_fast() clobber hole ptes and leave stale
pgste.zero bits set?

gmap_unmap_gfn_range_fast() passes DAT_WALK_IGN_HOLES to
_dat_walk_gfn_range(). In standard pte walks, dat_pte_walk_range() skips
entries where pte_hole() is true when DAT_WALK_IGN_HOLES is set without
DAT_WALK_ANY:

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 non-segment-aligned memslot is deleted or moved via
kvm_set_memslot() -> kvm_invalidate_memslot() ->
kvm_prepare_memory_region() -> s390_kvm_mmu_prepare_memory_region(),
dat_delete_slot() -> _dat_slot_pte() marks the ptes in partially covered
boundary segments as _PTE_TOK(_DAT_TOKEN_PIC, PGM_ADDRESSING) and drops
kvm->mmu_lock while invalid_slot is still present in the active hva tree:

arch/s390/kvm/gmap/dat.c:_dat_slot_pte() {
	new_pte = walk->priv ? _PTE_EMPTY : _PTE_TOK(_DAT_TOKEN_PIC, PGM_ADDRESSING);
}

If an mmu notifier invalidation larger than _PAGES_PER_SEGMENT runs on that
hva range before invalid_slot is removed, kvm_handle_hva_range() ->
kvm_unmap_gfn_range() -> gmap_unmap_gfn_range_fast() ->
_gmap_unmap_pmd_fast() iterates over [first, last] and unconditionally
writes _PTE_EMPTY, overwriting those PGM_ADDRESSING hole markers. Would
subsequent storage key or cmma operations (such as dat_get_storage_key(),
dat_set_storage_key(), dat_reset_reference_bit(), or dat_perform_essa()) on
those unmapped gfns then succeed instead of returning PGM_ADDRESSING?

Also, the slow path (_gmap_unmap_pte() -> gmap_ptep_xchg() ->
_gmap_ptep_xchg()) clears pgste.zero:

arch/s390/kvm/gmap/gmap.h:_gmap_ptep_xchg() {
	pgste.zero = 0;
	return __dat_ptep_xchg(ptep, pgste, newpte, gfn, gmap->datt);
}

Since _gmap_unmap_pmd_fast() only clears vsie_notif and vsie_gmem, could
stale pgste.zero bits remain set and be observed by ESSA_GET_STATE or
ESSA_SET_POT_VOLATILE?

> +
> +	/* Reattach the modified page table */
> +	WRITE_ONCE(*crstep, old);

[Severity: High]
What prevents the compiler from reordering the pt->ptes[i] and
pt->pgstes[i] accesses outside the detached pmd window in
_gmap_unmap_pmd_fast()?

In arch/s390/kvm/gmap/dat.h, idte_crste() only clobbers *crstep and "cc",
without a "memory" clobber:

arch/s390/kvm/gmap/dat.h:idte_crste() {
	if (!machine_has_tlb_guest()) {
		/* flush without guest asce */
		asm volatile("idte	%[table_origin],0,%[gaddr]"
			: "+m" (*crstep)
			: [table_origin] "a" (table_origin), [gaddr] "a" (gaddr)
			: "cc");
	} else {
		/* flush with guest asce */
		asm volatile("idte %[table_origin],%[asce],%[gaddr_opt]"
			: "+m" (*crstep)
			: [table_origin] "a" (table_origin),
			  [gaddr_opt] "a" (gaddr | IDTE_GUEST_ASCE),
			  [asce] "a" (asce.val)
			: "cc");
	}
}

Neither page_get_storage_key() nor WRITE_ONCE(*crstep, old) includes a
compiler memory barrier either (unlike pgste_set_unlock() in
arch/s390/kvm/gmap/dat.h, which places barrier() before WRITE_ONCE()).

Because pt->ptes[i] and pt->pgstes[i] are plain non-volatile accesses to
types distinct from *crstep (union crste), could the compiler reorder the
pte/pgste updates before idte_crste() or after WRITE_ONCE(*crstep, old),
allowing running vcpus to retain stale tlb entries to unmapped pages or
update hardware storage keys after dat_save_storage_key_into_pgste() has
read them?

> +
> +	return 0;
> +}
> +
> +static bool gmap_unmap_gfn_range_fast(struct gmap *gmap, gfn_t start, gfn_t end)
> +{
> +	const struct dat_walk_ops ops = {
> +		.pud_entry = _gmap_unmap_pud_fast,
> +		.pmd_entry = _gmap_unmap_pmd_fast,
> +	};
> +	bool failed;
> +
> +	lockdep_assert_held_write(&gmap->kvm->mmu_lock);
> +
> +	gmap_unmap_prefix(gmap, ALIGN_DOWN(start, _PAGE_ENTRIES), ALIGN(end, _PAGE_ENTRIES));
> +
> +	failed = _dat_walk_gfn_range(start, end, gmap->datt, &ops, DAT_WALK_IGN_HOLES, gmap);
> +	KVM_BUG_ON(failed, gmap->kvm);
> +	return false;
> +}

[ ... ]

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