Re: [PATCH v4 04/13] KVM: s390: cmma: Fix dirty tracking when removing memslot

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Double-decrement of `cmma_dirty_pages` counter on `-ENOMEM` retry paths
- [Medium] Mixing goto-based error handling and split definition-initialization with scope-based cleanup helpers (`__free` and `guard`)
--

--- Patch [4]: [PATCH v4 04/13] KVM: s390: cmma: Fix dirty tracking when removing memslot ---
commit 335967b9cb5dc29aafc1953486ce5288ed03df4d
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: cmma: Fix dirty tracking when removing memslot
    
This patch fixes cmma-dirty tracking by appropriately decrementing the
cmma-dirty counter when a memslot is removed. It also opportunistically
switches kvm_arch_commit_memory_region() to use __free() for the MMU
cache.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 1952f592a80a1..5046de39d739e 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
[ ... ]
> @@ -5803,14 +5803,24 @@ 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;
> +}
> +
>  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)
>  {
> -	struct kvm_s390_mmu_cache *mc = NULL;
> +	const struct dat_walk_ops ops = { .pte_entry = cmma_d_count_pte, };
> +	struct kvm_s390_mmu_cache *mc __free(kvm_s390_mmu_cache) = NULL;

[Severity: Medium]
This isn't a bug, but does mixing goto-based error handling and split
definition-initialization with scope-based cleanup helpers go against the
cleanup subsystem guidelines?

In kvm_arch_commit_memory_region(), mc is initialized to NULL at the
function start. Later, the code continues to handle errors with a goto:

    mc = kvm_s390_new_mmu_cache();
    if (!mc)
        goto out;

Could the goto out; be replaced with a direct return to avoid mixing
cleanup patterns, and could the variable be declared at the point of
allocation?

>  	int rc = 0;
>  
> +	guard(mutex)(&kvm->slots_arch_lock);
> +
>  	if (change == KVM_MR_FLAGS_ONLY)
>  		return;
[ ... ]
> @@ -5821,6 +5831,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 && old) {
> +			_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]
Can this result in a double-decrement of the cmma_dirty_pages counter?

Looking at kvm_arch_commit_memory_region(), this block executes inside a
retry loop:

    retry:
        scoped_guard(write_lock, &kvm->mmu_lock) {
            if (kvm->arch.migration_mode && kvm->arch.use_cmma && old) {
                _dat_walk_gfn_range(..., &ops, ...);
            }
            
            switch (change) {
            case KVM_MR_DELETE:
                rc = dat_delete_slot(...);
                ...
        }
        if (rc == -ENOMEM) {
            rc = kvm_s390_mmu_cache_topup(mc);
            if (!rc)
                goto retry;
        }

If dat_delete_slot() clears cmma_d on some PTEs and then fails with
-ENOMEM (e.g. running out of MMU cache while splitting a page table), we 
top up the cache and jump back to the retry label. The pre-pass
_dat_walk_gfn_range() runs again, but cmma_d_count_pte() doesn't clear the
cmma_d bit on the unhandled PTEs during its traversal.

Would the second pass decrement the counter again for the PTEs that weren't
processed before the -ENOMEM failure, potentially causing the counter to
underflow?

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