Re: [PATCH v3 04/10] 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:
- [Critical] Unconditional NULL pointer dereference of the `old` memslot during `KVM_MR_CREATE` when migration mode is active.
- [Low] Build failure due to `goto` jumping over a `guard()` declaration, violating C cleanup attribute semantics.

Pre-existing issues:
- [High] Memory exhaustion and guest memory corruption during `KVM_MR_MOVE` due to shared MMU cache.
--

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

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

This patch updates CMMA dirty tracking to decrement the counter when a
memslot is removed. It walks the memslot's GFN range during 
kvm_arch_commit_memory_region to clear the dirty count.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index e154a8ad6c735..0a44351722702 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
[ ... ]
> @@ -5817,7 +5825,15 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>  		goto out;
>  	}

[Severity: Low]
Will this compile correctly? The goto statement here jumps over the guard()
declaration below, which typically causes a compiler error since C
restricts jumping past the initialization of __attribute__((cleanup))
variables.

>  
> +	guard(mutex)(&kvm->slots_arch_lock);
> +
>  	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,

[Severity: Critical]
Can this code trigger a NULL pointer dereference when a new memory slot is 
created? 

If change == KVM_MR_CREATE, the old memslot pointer is NULL, but this 
block unconditionally dereferences old->base_gfn and old->npages before 
the switch statement checks the change type.

> +					    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]
This is a pre-existing issue, but could this lead to memory exhaustion and 
guest memory corruption during KVM_MR_MOVE? 

Looking at the KVM_MR_MOVE case below this 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);

The code falls through from dat_delete_slot() to dat_create_slot() and 
shares the single kvm_s390_mmu_cache (mc). If the cache runs out of space 
for the second operation, it will fail midway, leaving the DAT tables out 
of sync with the memslots since this function cannot return an error to 
userspace.

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