Re: [PATCH v5 6/7] KVM: s390: Introduce gmap_enable_cmm()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] KVM: s390: missing srcu lock in gmap_enable_cmm_already_locked()
- [High] KVM: s390: gmap_enable_cmm_already_locked() fails to clear hpage flags
- [Medium] KVM: s390: premature GMAP_FLAG_USES_CMM in gmap_enable_cmm_already_locked()
- [Medium] KVM: s390: stale uses_skeys() cached in gmap_enable_cmm_already_locked()
--

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

KVM: s390: Introduce gmap_enable_cmm()

This commit introduces gmap_enable_cmm_already_locked() and its locking
wrapper gmap_enable_cmm() to allocate all 4k page tables of a running guest
and split existing large page mappings on a best-effort basis when enabling
CMMA.

> diff --git a/arch/s390/kvm/gmap/gmap.c b/arch/s390/kvm/gmap/gmap.c
> index b9d92bef5459d..e0c584ec69069 100644
> --- a/arch/s390/kvm/gmap/gmap.c
> +++ b/arch/s390/kvm/gmap/gmap.c
> @@ -1524,3 +1524,40 @@ struct gmap *gmap_create_shadow(struct kvm_s390_mmu_cache *mc, struct gmap *pare
>  	}
>  	return new;
>  }
> +
> +/* Bail out for any allocation failure. This is only a best effort to allocate all page tables. */
> +void gmap_enable_cmm_already_locked(struct kvm_s390_mmu_cache *mc, struct gmap *gmap)
> +{
> +	struct kvm_memory_slot *ms;
> +	struct kvm_memslots *slots;
> +	gfn_t gfn, start, end;
> +	union crste *crstep;
> +	union pte *ptep;
> +	int flags, bkt;
> +
> +	if (uses_cmm(gmap))
> +		return;
> +
> +	set_bit(GMAP_FLAG_USES_CMM, &gmap->flags);

[Severity: Medium]
Should GMAP_FLAG_USES_CMM be set only after the memslot walk finishes?

gmap_enable_cmm() in arch/s390/kvm/gmap/gmap.h checks uses_cmm(gmap) before
acquiring gmap->kvm->slots_arch_lock:

arch/s390/kvm/gmap/gmap.h:gmap_enable_cmm() {
	if (uses_cmm(gmap))
		return;

	guard(mutex)(&gmap->kvm->slots_arch_lock);

	gmap_enable_cmm_already_locked(mc, gmap);
}

Because gmap_enable_cmm_already_locked() sets GMAP_FLAG_USES_CMM before
walking the memslots, and then drops gmap->kvm->mmu_lock and calls
kvm_s390_mmu_cache_topup() on every _PAGE_ENTRIES iteration, a concurrent
caller of gmap_enable_cmm() (such as another vCPU in handle_essa()) or a
reader of uses_cmm(gmap) (such as the KVM_REQ_STOP_MIGRATION handler) can
observe uses_cmm(gmap) as true and proceed while the first caller is still
allocating 4k page tables and splitting large pages.

In addition, if slots are empty or kvm_s390_mmu_cache_topup(mc) fails early,
GMAP_FLAG_USES_CMM remains permanently set, preventing any later call to
gmap_enable_cmm() or gmap_enable_cmm_already_locked() from retrying.

[Severity: High]
Also, what prevents gmap_link() from continuing to request 1M or 2G mappings
after GMAP_FLAG_USES_CMM is set if GMAP_FLAG_ALLOW_HPAGE_1M or
GMAP_FLAG_ALLOW_HPAGE_2G was enabled on gmap->flags?

Unlike kvm_s390_pv_init_vm(), gmap_enable_cmm_already_locked() does not clear
GMAP_FLAG_ALLOW_HPAGE_1M or GMAP_FLAG_ALLOW_HPAGE_2G on gmap->flags, and
gmap_1m_allowed() / gmap_2g_allowed() do not check !uses_cmm(gmap):

arch/s390/kvm/gmap/gmap.c:gmap_link() {
	if (f->page) {
		order = folio_order(page_folio(f->page));
		if (order >= get_order(_REGION3_SIZE) && gmap_2g_allowed(gmap, f, slot))
			level = TABLE_TYPE_REGION3;
		else if (order >= get_order(_SEGMENT_SIZE) && gmap_1m_allowed(gmap, f, slot))
			level = TABLE_TYPE_SEGMENT;
	}
	return _gmap_link(mc, gmap, level, f);
}

If GMAP_FLAG_ALLOW_HPAGE_2G is set on gmap->flags and
gmap_enable_cmm_already_locked() splits a 2G region3 entry into a segment
table on the first _PAGE_ENTRIES step, but then bails out on a subsequent
step in the same 2G range when kvm_s390_mmu_cache_topup(mc) fails (or races
with a fault while mmu_lock is dropped between iterations), the remaining
segment entries in that 2G range are still empty (i = 1) or 1M mappings
(fc = 1).

When a fault subsequently runs gmap_link() -> _gmap_link() ->
dat_entry_walk() with walk_level == TABLE_TYPE_REGION3 and
DAT_WALK_ALLOC_CONTINUE, dat_entry_walk() sees the split region3 table entry
and clears allocate:

arch/s390/kvm/gmap/dat.c:dat_entry_walk() {
		if (walk_level == TABLE_TYPE_REGION3 &&
		    continue_anyway && !entry.pud.h.fc && !entry.h.i) {
			walk_level = TABLE_TYPE_PAGE_TABLE;
			allocate = false;
		}
}

At the segment level, dat_entry_walk() then returns -ENOENT (for empty
segment entries because allocate is false) or -EFBIG (for 1M segment entries
because split is false), which _gmap_link() translates into -EAGAIN, causing
kvm_s390_faultin_gfn() to retry in an unbounded loop.

Similarly, if kvm_s390_mmu_cache_topup(mc) fails before a region is split, or
if a memslot is later created or moved via
s390_kvm_mmu_prepare_memory_region() -> dat_create_slot(), subsequent faults
will still see GMAP_FLAG_ALLOW_HPAGE_1M or GMAP_FLAG_ALLOW_HPAGE_2G set and
install 1M or 2G large mappings without PGSTEs.

> +
> +	slots = kvm_memslots(gmap->kvm);
> +	if (!slots || kvm_memslots_empty(slots))
> +		return;
> +
> +	flags = DAT_WALK_ALLOC | DAT_WALK_SPLIT | (uses_skeys(gmap) ? DAT_WALK_USES_SKEYS : 0);
> +
> +	kvm_for_each_memslot(ms, bkt, slots) {

[Severity: High]
Can kvm_memslots(gmap->kvm) and kvm_for_each_memslot() be called here without
holding gmap->kvm->srcu or gmap->kvm->slots_lock?

kvm_memslots() calls __kvm_memslots(), which checks that either kvm->srcu or
kvm->slots_lock is held:

include/linux/kvm_host.h:__kvm_memslots() {
	as_id = array_index_nospec(as_id, KVM_MAX_NR_ADDRESS_SPACES);
	return srcu_dereference_check(kvm->memslots[as_id], &kvm->srcu,
			lockdep_is_held(&kvm->slots_lock) ||
			!refcount_read(&kvm->users_count));
}

gmap_enable_cmm() only acquires gmap->kvm->slots_arch_lock, and callers such
as KVM_S390_SET_CMMA_BITS -> kvm_s390_set_cmma_bits() also hold only
kvm->slots_arch_lock without acquiring kvm->srcu.

Holding slots_arch_lock alone does not satisfy srcu_dereference_check() or
protect kvm->memslots against concurrent memslot updates (unlike
kvm_s390_vm_set_migration(), which acquires both guard(srcu)(&kvm->srcu) and
guard(mutex)(&kvm->slots_arch_lock)). Should gmap_enable_cmm_already_locked()
acquire guard(srcu)(&gmap->kvm->srcu)?

> +		start = ALIGN(ms->base_gfn, _PAGE_ENTRIES);
> +		end = ALIGN_DOWN(ms->base_gfn + ms->npages, _PAGE_ENTRIES);
> +
> +		for (gfn = start; gfn < end; gfn += _PAGE_ENTRIES) {
> +			if (kvm_s390_mmu_cache_topup(mc))
> +				return;
> +
> +			guard(write_lock)(&gmap->kvm->mmu_lock);
> +
> +			dat_entry_walk(mc, gfn, gmap->asce, flags, TABLE_TYPE_PAGE_TABLE,
> +				       &crstep, &ptep);

[Severity: Medium]
Could evaluating uses_skeys(gmap) only once before the memslot loop leave
flags with a stale value across mmu_lock drops?

gmap_enable_skeys() -> _gmap_enable_skeys() sets GMAP_FLAG_USES_SKEYS under
mmap_write_lock(gmap->kvm->mm) and gmap->kvm->mmu_lock without acquiring
gmap->kvm->slots_arch_lock.

If a concurrent vCPU executing a storage-key instruction or a
KVM_S390_SET_SKEYS ioctl enables GMAP_FLAG_USES_SKEYS while
gmap_enable_cmm_already_locked() has dropped gmap->kvm->mmu_lock between
iterations, subsequent dat_entry_walk() calls in this loop will still pass
flags without DAT_WALK_USES_SKEYS.

When dat_split_ste() splits an invalidated large PMD (old.h.fc == 1 and
old.h.i == 1) with uses_skeys == false:

arch/s390/kvm/gmap/dat.c:dat_split_ste() {
			pgste_init.pcl = uses_skeys && init.h.i;
			dat_init_pgstes(pt, pgste_init.val);
		} else {
			dat_init_page_table(pt, init.val, 0);
		}

		if (dat_pmdp_xchg_atomic(pmdp, old, new, gfn, asce)) {
			if (!pgste_init.pcl)
				return 0;
			for (i = 0; i < _PAGE_ENTRIES; i++) {
				union pgste pgste = pt->pgstes[i];

				pgste = dat_save_storage_key_into_pgste(pt->ptes[i], pgste);
				pgste_set_unlock(pt->ptes + i, pgste);
			}
			return 0;
		}
}

pgste_init.pcl will be false and dat_save_storage_key_into_pgste() will be
skipped, leaving the new PGSTEs zeroed so that subsequent storage key reads
or minor faults lose the guest storage keys on those pages.

Should uses_skeys(gmap) be checked inside the write_lock(&gmap->kvm->mmu_lock)
critical section on each iteration?

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