Re: [GIT PULL v1 10/12] KVM: s390: Fix cmma dirty tracking

[sashiko-bot@kernel.org]

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

New issues:
- [High] Race between `kvm_s390_vm_stop_migration()` and `handle_essa()` causes permanent corruption of the `cmma_dirty_pages` counter.
- [High] Unbounded guest address space scan in `dat_get_cmma()` while holding the `mmu_lock` read lock causes soft lockups.
- [High] The `cmma_dirty_pages` counter is not decremented when PGSTEs are zapped, leading to a permanent counter leak.

Pre-existing issues:
- [High] Decoupled locking in `__do_essa()` causes `cmma_dirty_pages` to temporarily underflow, exposing a massive `remaining` value to userspace.
- [High] `KVM_S390_GET_CMMA_BITS` leaks uninitialized kernel heap memory to userspace due to unpopulated array elements.
--

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

KVM: s390: Fix cmma dirty tracking

It is possible that some guest memory areas have not been touched yet
when starting migration mode, and thus have no ptes allocated. Only
existing and allocated ptes should count toward the total of dirty cmma
entries.

[ ... ]

> diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
> index cffac7782c4bf..0ad4ebc80ebaf 100644
> --- a/arch/s390/kvm/dat.c
> +++ b/arch/s390/kvm/dat.c
> @@ -1253,6 +1253,9 @@ int dat_get_cmma(union asce asce, gfn_t *start, unsigned int *count, u8 *values,
>  	};
>  
>  	_dat_walk_gfn_range(*start, asce_end(asce), asce, &ops, DAT_WALK_IGN_HOLES, &state);
> +	/* If no dirty pages were found, wrap around and continue searching */
> +	if (*start && state.start == -1)
> +		_dat_walk_gfn_range(0, *start, asce, &ops, DAT_WALK_IGN_HOLES, &state);

[Severity: High]
Can this wrap-around scan trigger a host soft lockup?

Since both the initial and wrap-around scans execute entirely under the
mmu_lock read lock, preemption is disabled. If a guest has a massive
memory address space that is fully populated but contains no dirty CMMA
pages, dat_get_cmma could iterate over millions of page table entries in
a tight loop without ever checking need_resched or yielding.

[Severity: High]
This is a pre-existing issue, but does dat_get_cmma leak uninitialized
kernel heap memory to userspace here?

When KVM_S390_GET_CMMA_BITS is invoked, the values array is allocated using
vmalloc without zero-initialization. If dat_get_cmma encounters clean pages
or unpopulated holes interleaved with dirty pages, __dat_get_cmma_pte skips
those array indices. The subsequent copy_to_user in kvm_s390_get_cmma_bits
then copies the entire contiguous array back to userspace, taking those
uninitialized vmalloc bytes with it.

>  	if (state.start == -1) {
>  		*count = 0;

[ ... ]

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 221b2fb199d4e..9ad6bd4edbce3 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
[ ... ]
> @@ -1210,28 +1210,54 @@ static int kvm_s390_vm_start_migration(struct kvm *kvm)
>  	kvm_for_each_memslot(ms, bkt, slots) {
>  		if (!ms->dirty_bitmap)
>  			return -EINVAL;
> -		ram_pages += ms->npages;
>  	}
> -	/* mark all the pages as dirty */
> -	gmap_set_cmma_all_dirty(kvm->arch.gmap);
> -	atomic64_set(&kvm->arch.cmma_dirty_pages, ram_pages);
> -	kvm->arch.migration_mode = 1;
> +	/*
> +	 * Set the flag and let KVM handle ESSA manually, potentially setting
> +	 * the cmma_d bit in some PGSTEs and increasing cmma_dirty_pages.
> +	 * At this point cmma_dirty_pages is still 0, and all existing PGSTEs
> +	 * have their cmma_d bit set to 0.
> +	 * Any newly allocated page table has its entries marked as cmma-clean,
> +	 * which is fine because the CMMA values are not dirty.
> +	 */
> +	WRITE_ONCE(kvm->arch.migration_mode, 1);
>  	kvm_s390_sync_request_broadcast(kvm, KVM_REQ_START_MIGRATION);
> +	/*
> +	 * Mark all PGSTEs as cmma-dirty, increasing cmma_dirty_pages as needed,
> +	 * but without double-counting pages that have become dirty on their own
> +	 * in the meantime.
> +	 * At this point some pages might have become dirty on their own already
> +	 * and cmma_dirty_pages might therefore be non-zero.
> +	 */
> +	gmap_set_cmma_all_dirty(kvm->arch.gmap);

[Severity: High]
Could this lead to a permanent leak in the cmma_dirty_pages counter?

This establishes an invariant that the cmma_dirty_pages counter mirrors the
global number of PGSTEs with the cmma_d bit set. However, if host userspace
unmaps memory or deletes a memslot while migration mode is active,
dat_free_pt destroys the page tables and associated PGSTEs without
decrementing cmma_dirty_pages. This leaves the counter artificially inflated,
potentially trapping the VMM in an infinite migration loop.

>  	return 0;
>  }
>  
>  /*
> - * Must be called with kvm->slots_lock to avoid races with ourselves and
> - * kvm_s390_vm_start_migration.
> + * Must be called with kvm->slots_lock to avoid races with ourselves,
> + * kvm_s390_vm_start_migration() and kvm_s390_get_cmma_bits().
>   */
>  static int kvm_s390_vm_stop_migration(struct kvm *kvm)
>  {
>  	/* migration mode already disabled */
>  	if (!kvm->arch.migration_mode)
>  		return 0;
> -	kvm->arch.migration_mode = 0;
> +	/*
> +	 * Unset the flag and propagate to all vCPUs. From now on the cmma_d
> +	 * bit will not be touched on any PGSTE.
> +	 * At this point cmma_dirty_pages is possibly non-zero, and thus some
> +	 * PGSTEs might have cmma_d set.
> +	 */
> +	WRITE_ONCE(kvm->arch.migration_mode, 0);
>  	if (kvm->arch.use_cmma)
>  		kvm_s390_sync_request_broadcast(kvm, KVM_REQ_STOP_MIGRATION);
> +	/* Clear cmma_d on all existing PGSTEs and set cmma_dirty_pages to 0. */
> +	gmap_set_cmma_all_clean(kvm->arch.gmap);
> +	atomic64_set(&kvm->arch.cmma_dirty_pages, 0);

[Severity: High]
Is there a race condition here with concurrent ESSA instructions?

kvm_s390_sync_request_broadcast does not wait for vCPUs currently executing
host code. If a vCPU executing handle_essa reads migration_mode as 1, it
will proceed to __do_essa. Because gmap_set_cmma_all_clean and __do_essa
can overlap, the stop-migration thread might clear cmma_dirty_pages to 0
right before __do_essa calls atomic64_inc. This would leave the counter at
1 with no actual dirty PGSTEs, breaking migration invariants.

> +	/*
> +	 * At this point the system has the expected state: migration_mode is 0,
> +	 * cmma_dirty_pages is 0, and all existing PGSTEs have their cmma_d bit
> +	 * set to 0.
> +	 */
>  	return 0;
>  }

[ ... ]

> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index 9bc6fd02ff777..ad0ddc433a73c 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -1236,7 +1236,7 @@ static int handle_essa(struct kvm_vcpu *vcpu)
>  						: ESSA_SET_STABLE_IF_RESIDENT))
>  		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
>  
> -	if (!vcpu->kvm->arch.migration_mode) {
> +	if (!READ_ONCE(vcpu->kvm->arch.migration_mode)) {

[Severity: High]
This is a pre-existing issue, but does the decoupling of the PGSTE lock
and the global counter increment in __do_essa open a race window?

In __do_essa, dat_perform_essa sets the cmma_d bit under the PGSTE lock,
but the global cmma_dirty_pages counter is incremented outside of that
critical section. Concurrently, dat_get_cmma could acquire the same PGSTE
lock, see the bit set, clear it, and decrement cmma_dirty_pages before
__do_essa has incremented it. If the counter starts at 0, it temporarily
underflows to -1. If kvm_s390_get_cmma_bits reads it at that moment, it
exposes ULLONG_MAX to userspace as the remaining count.

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