Re: [PATCH v4 7/7] KVM: s390: Allocate page tables when CMMA is used

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] KVM: s390: gmap_allocate_all_page_tables_best_effort() drops skeys on split
- [High] KVM: s390: hugepage flags left enabled when enabling CMMA in handle_essa()
- [Medium] KVM: s390: race in handle_essa() setting GMAP_FLAG_USES_CMM before allocation
- [Medium] KVM: s390: page tables allocated after dat_set_cmma_bits() in kvm_s390_set_cmma_bits()
--

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

KVM: s390: Allocate page tables when CMMA is used

This commit calls gmap_allocate_all_page_tables_best_effort() in
handle_essa() and kvm_s390_set_cmma_bits() when GMAP_FLAG_USES_CMM is first
set on the gmap, so that page tables are pre-allocated and existing large
page mappings are split when CMMA is used.

> diff --git a/arch/s390/kvm/s390/priv.c b/arch/s390/kvm/s390/priv.c
> index 856939acf012f..4f0dd3173ab91 100644
> --- a/arch/s390/kvm/s390/priv.c
> +++ b/arch/s390/kvm/s390/priv.c
> @@ -1248,7 +1248,10 @@ static int handle_essa(struct kvm_vcpu *vcpu)
>  		 * value really needs to be written to; if the value is
>  		 * already correct, we do nothing and avoid the lock.
>  		 */
> -		set_bit(GMAP_FLAG_USES_CMM, &vcpu->arch.gmap->flags);
> +		if (!uses_cmm(vcpu->arch.gmap)) {
> +			set_bit(GMAP_FLAG_USES_CMM, &vcpu->arch.gmap->flags);
> +			gmap_allocate_all_page_tables_best_effort(vcpu->arch.gmap);
> +		}

[Severity: Medium]
What happens if two vCPUs in a multi-vCPU guest intercept on ESSA
concurrently in handle_essa() when !READ_ONCE(vcpu->kvm->arch.migration_mode)?

In this path only vcpu->mutex and the kvm->srcu read lock are held. When the
first vCPU sees !uses_cmm(vcpu->arch.gmap), it immediately sets
GMAP_FLAG_USES_CMM via set_bit() and then enters
gmap_allocate_all_page_tables_best_effort(), which iterates over all memslots
while repeatedly dropping gmap->kvm->mmu_lock and sleeping in
kvm_s390_mmu_cache_topup():

arch/s390/kvm/gmap/gmap.c:gmap_allocate_all_page_tables_best_effort() {
    ...
		for (gfn = start; gfn < end; gfn += _PAGE_ENTRIES) {
			if (kvm_s390_mmu_cache_topup(mc))
				break;

			guard(write_lock)(&gmap->kvm->mmu_lock);

			dat_entry_walk(mc, gfn, gmap->asce, DAT_WALK_ALLOC | DAT_WALK_SPLIT,
				       TABLE_TYPE_PAGE_TABLE, &crstep, &ptep);
		}
}

If a second vCPU enters handle_essa() while the first vCPU is still running
gmap_allocate_all_page_tables_best_effort(), the second vCPU will observe
uses_cmm(vcpu->arch.gmap) == true, skip the allocation, set
vcpu->arch.sie_block->ecb2 |= ECB2_CMMA, and retry ESSA in SIE before all
page tables have been allocated and all large pages have been split.
Conversely, because !uses_cmm() and set_bit() are not atomic or serialized by
a VM-wide lock such as kvm->slots_arch_lock, two vCPUs can both pass
!uses_cmm() and run duplicate allocation walks across all memslots.

Should the check, flag update, and page table allocation be serialized under
a VM-wide lock so other vCPUs wait until allocation finishes before re-entering
SIE with ECB2_CMMA enabled?

[Severity: Critical]
Can calling gmap_allocate_all_page_tables_best_effort() here (and in
kvm_s390_set_cmma_bits()) corrupt guest storage keys when splitting aged
1M or 2G large pages?

If storage keys are already enabled (GMAP_FLAG_USES_SKEYS) and a 1M large
page has been aged via gmap_age_gfn() -> gmap_clear_young_crste() so that
crste.h.i is set while the storage keys remain in the physical pages,
gmap_allocate_all_page_tables_best_effort() calls dat_entry_walk() without
DAT_WALK_USES_SKEYS:

arch/s390/kvm/gmap/gmap.c:gmap_allocate_all_page_tables_best_effort()
  -> dat_entry_walk(mc, gfn, gmap->asce, DAT_WALK_ALLOC | DAT_WALK_SPLIT, ...)
     -> dat_split_ste()

Inside dat_split_ste() in arch/s390/kvm/gmap/dat.c, saving the physical
storage keys into the newly allocated PGSTEs for an invalid segment entry is
gated on uses_skeys from the walk flags:

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);
			}
    ...
}

Because DAT_WALK_USES_SKEYS is not passed, pgste_init.pcl is false and the new
PGSTEs are left with zeroed storage keys while pt->ptes[i].h.i == 1. When
those 4K pages are later faulted back in via _gmap_handle_minor_pte_fault() ->
__dat_ptep_xchg() (which does pass uses_skeys(gmap)), old.h.i && !new.h.i
triggers dat_set_storage_key_from_pgste() and overwrites the guest's hardware
storage keys with zero.

Should gmap_allocate_all_page_tables_best_effort() include
(uses_skeys(gmap) ? DAT_WALK_USES_SKEYS : 0) in the flags passed to
dat_entry_walk()?

[Severity: High]
Could leaving GMAP_FLAG_ALLOW_HPAGE_1M and GMAP_FLAG_ALLOW_HPAGE_2G set on
gmap->flags when enabling CMMA lead to a guest vCPU infinite fault loop?

When userspace enables KVM_CAP_S390_HPAGE_1M or KVM_CAP_S390_HPAGE_2G and
also enables KVM_S390_VM_MEM_ENABLE_CMMA, neither handle_essa(),
kvm_s390_set_cmma_bits(), nor gmap_allocate_all_page_tables_best_effort()
clears GMAP_FLAG_ALLOW_HPAGE_1M or GMAP_FLAG_ALLOW_HPAGE_2G, and
gmap_1m_allowed() / gmap_2g_allowed() in arch/s390/kvm/gmap/gmap.c do not
check !uses_cmm(gmap):

arch/s390/kvm/gmap/gmap.c:gmap_1m_allowed() {
	return test_bit(GMAP_FLAG_ALLOW_HPAGE_1M, &gmap->flags) &&
	       !((f->gfn ^ f->pfn) & ~_SEGMENT_FR_MASK) &&
	       slot->base_gfn <= ALIGN_DOWN(f->gfn, _PAGES_PER_SEGMENT) &&
	       slot->base_gfn + slot->npages >= ALIGN(f->gfn + 1, _PAGES_PER_SEGMENT);
}

If gmap_allocate_all_page_tables_best_effort() bails out early on an
allocation failure, if another vCPU faults on a segment not yet visited while
mmu_lock is dropped between segments, or if a memslot is added or moved after
GMAP_FLAG_USES_CMM is set (where s390_kvm_mmu_prepare_memory_region() ->
dat_create_slot() installs _CRSTE_EMPTY entries without page tables),
gmap_link() will install a valid 1M or 2G large CRSTE.

When a vCPU with ECB2_CMMA set subsequently faults on that valid large page,
kvm_s390_faultin_gfn() -> gmap_try_fixup_minor() ->
gmap_handle_minor_crste_fault() sees:

arch/s390/kvm/gmap/gmap.c:gmap_handle_minor_crste_fault() {
    ...
	/* Appropriate permissions already (race with another handler), nothing to do. */
	if (!oldcrste.h.i && !(f->write_attempt && oldcrste.h.p))
		return 0;
    ...
}

and returns 0 without splitting the large page, causing the vCPU to fault
indefinitely. Should GMAP_FLAG_ALLOW_HPAGE_1M and GMAP_FLAG_ALLOW_HPAGE_2G be
cleared (or checked against uses_cmm(gmap)) before splitting existing large
pages?

> diff --git a/arch/s390/kvm/s390/s390.c b/arch/s390/kvm/s390/s390.c
> index 18fb200f854a8..739e8c23c3ae0 100644
> --- a/arch/s390/kvm/s390/s390.c
> +++ b/arch/s390/kvm/s390/s390.c
> @@ -2359,7 +2359,10 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
>  		}
>  	} while (r == -ENOMEM);
>  
> -	set_bit(GMAP_FLAG_USES_CMM, &kvm->arch.gmap->flags);
> +	if (!uses_cmm(kvm->arch.gmap)) {
> +		set_bit(GMAP_FLAG_USES_CMM, &kvm->arch.gmap->flags);
> +		gmap_allocate_all_page_tables_best_effort(kvm->arch.gmap);
> +	}

[Severity: Medium]
In kvm_s390_set_cmma_bits(), is it intentional that
gmap_allocate_all_page_tables_best_effort() is called after
dat_set_cmma_bits() rather than before it?

Inside dat_set_cmma_bits() in arch/s390/kvm/gmap/dat.c, dat_entry_walk() is
called under read_lock(&kvm->mmu_lock) with DAT_WALK_ALLOC and without
DAT_WALK_SPLIT:

arch/s390/kvm/gmap/dat.c:dat_set_cmma_bits() {
    ...
	for (cur = ALIGN_DOWN(gfn, _PAGE_ENTRIES); cur < gfn + count; cur += _PAGE_ENTRIES) {
		rc = dat_entry_walk(mc, cur, asce, DAT_WALK_ALLOC, TABLE_TYPE_PAGE_TABLE,
				    &crstep, &ptep);
		if (rc)
			return rc;
	}
    ...
}

If any 1M or 2G large page mappings already exist in the target GFN range
when KVM_S390_SET_CMMA_BITS is first invoked, dat_set_cmma_bits() will hit
the unsplit large page entry in dat_entry_walk() and fail with -EFBIG before
gmap_allocate_all_page_tables_best_effort() has had a chance to split it.

In addition, because the !uses_cmm(kvm->arch.gmap) block runs unconditionally
after the loop even when r != 0, any error from dat_set_cmma_bits() (such as
-EFBIG or PGM_ADDRESSING on an invalid GFN range) will still set
GMAP_FLAG_USES_CMM and allocate page tables across all memslots before
returning the error.

Should the !uses_cmm(kvm->arch.gmap) check and
gmap_allocate_all_page_tables_best_effort() happen before the
dat_set_cmma_bits() loop (or be gated on success if placed after)?

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