Re: [PATCH v6 07/14] KVM: s390: cmma: Fix dirty tracking when removing memslot

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Mixing goto-based error handling with scope-based cleanup (__free and guard) in the same function.

Pre-existing issues:
- [Critical] Silent ENOMEM failures in commit_memory_region leave stale guest page tables, causing host Use-After-Free.
- [High] KVM_MR_MOVE operations can exhaust the MMU cache (`mc`), leading to silent memslot corruption.
--

--- Patch [7]: [PATCH v6 07/14] KVM: s390: cmma: Fix dirty tracking when removing memslot ---
commit fbfd75bf3afca84e6e947282c2cd405f559d2839
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

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

This patch updates memslot removal to appropriately decrement the
cmma-dirty counter to keep it accurate when the system is in migration
mode. It also updates kvm_arch_commit_memory_region() to use __free()
for the struct kvm_s390_mmu_cache.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 24c3bfcd5db26..c1984fe2f9a40 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
[ ... ]
> @@ -5809,14 +5809,30 @@ 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)
> +{
[ ... ]
> +}
> +
>  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;
>  	int rc = 0;
>  
> +	guard(mutex)(&kvm->slots_arch_lock);
> +

[Severity: Medium]
Does this mix scope-based cleanup with goto-based error handling?

The patch adds __free() and guard() here, but kvm_arch_commit_memory_region()
still retains legacy goto-based error handling shortly after this:

	mc = kvm_s390_new_mmu_cache();
	if (!mc) {
		rc = -ENOMEM;
		goto out;
	}

The cleanup guidelines in include/linux/cleanup.h state that a function
must use scope-based cleanup entirely or not at all, to prevent subtle
resource leaks when cleanup order (LIFO) is misunderstood.


[Severity: Critical]
This is a pre-existing issue, but can this silent ENOMEM failure leave stale
guest page tables?

If kvm_s390_new_mmu_cache() fails in the snippet above, the deletion of
guest page tables is skipped.

Since KVM's generic memslot management removes the memslot and allows host
userspace to unmap the backing memory, the guest's hardware page tables
(gmap) would retain valid PTE mappings to those physical pages, potentially
leading to a host use-after-free.

>  	if (change == KVM_MR_FLAGS_ONLY)
>  		return;
>  
> @@ -5827,6 +5843,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);
> @@ -5848,7 +5870,6 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>  out:
>  	if (rc)
>  		pr_warn("failed to commit memory region\n");
> -	kvm_s390_free_mmu_cache(mc);
>  	return;
>  }

[Severity: High]
This is a pre-existing issue, but can a KVM_MR_MOVE operation exhaust the
MMU cache?

Looking at the KVM_MR_MOVE case in kvm_arch_commit_memory_region():

		case KVM_MR_MOVE:
			rc = dat_delete_slot(mc, kvm->arch.gmap->asce, old->base_gfn, old->npages);
			if (rc)
				break;
			fallthrough;
		case KVM_MR_CREATE:
			rc = dat_create_slot(mc, kvm->arch.gmap->asce, new->base_gfn, new->npages);

dat_delete_slot() and dat_create_slot() are called sequentially without
replenishing the MMU cache (mc) in between.

If both operations require splitting large pages, the cache could be
exhausted, and dat_create_slot() might fail with -ENOMEM. The generic KVM
code would assume the move succeeded, but the new guest page tables would be
unmapped, leading to a guest crash on access.

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