Re: [PATCH v2 4/7] KVM: s390: cmma: Fix dirty tracking when removing memslot

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Data race and double-decrement of `cmma_dirty_pages` due to two-pass CMMA cleanup
--

--- Patch [4]: [PATCH v2 4/7] KVM: s390: cmma: Fix dirty tracking when removing memslot ---
commit 119121acc5129e77726627587bbb91e16dea4543
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: cmma: Fix dirty tracking when removing memslot

This patch fixes inaccurate dirty tracking during migration by appropriately
decrementing the cmma-dirty counter when a memslot is removed. It does this
by walking the page tables and decrementing the counter prior to deleting
the slot.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 9e3b8b5c6aa66..5c2408fc5a8cb 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c

[ ... ]

> @@ -5796,11 +5796,19 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  	return 0;
>  }
>  
> +static long cmma_d_count_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_walk *walk)
> +{
> +	if (pgste_of(ptep)->cmma_d)
> +		atomic64_dec(walk->priv);
> +	return 0;
> +}
> +

[Severity: High]
Can this two-pass CMMA cleanup cause a double-decrement of the global counter?

The first pass in cmma_d_count_pte() locklessly reads cmma_d and decrements
the counter, but leaves cmma_d = 1.

>  void kvm_arch_commit_memory_region(struct kvm *kvm,
>  				struct kvm_memory_slot *old,
>  				const struct kvm_memory_slot *new,
>  				enum kvm_mr_change change)
>  {
> +	const struct dat_walk_ops ops = { .pte_entry = cmma_d_count_pte, };
>  	struct kvm_s390_mmu_cache *mc = NULL;
>  	int rc = 0;
>  
> @@ -5814,6 +5822,12 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>  	}
>  
>  	scoped_guard(write_lock, &kvm->mmu_lock) {
> +		if (kvm->arch.migration_mode && kvm->arch.use_cmma) {
> +			_dat_walk_gfn_range(old->base_gfn, old->base_gfn + old->npages,
> +					    kvm->arch.gmap->asce, &ops, DAT_WALK_IGN_HOLES,
> +					    &kvm->arch.cmma_dirty_pages);
> +		}
> +
>  		switch (change) {
>  		case KVM_MR_DELETE:
>  			rc = dat_delete_slot(mc, kvm->arch.gmap->asce, old->base_gfn, old->npages);

[Severity: High]
The wide window between the first walk here (decrementing) and the second walk
in dat_delete_slot() (which clears cmma_d) appears to allow a concurrent
KVM_S390_GET_CMMA_BITS ioctl to intervene.

Since the ioctl takes slots_arch_lock but not mmu_lock, could it acquire the
PGSTE lock, observe cmma_d == 1, clear it, and decrement the
cmma_dirty_pages counter a second time for the same page?

If this happens, the counter would underflow and wrap to UINT64_MAX, causing
KVM_S390_GET_CMMA_BITS to permanently report pending dirty pages and break VM
live migration.

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