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

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

New issues:
- [High] Missing `DAT_WALK_ANY` flag skips swapped-out pages, leaking the CMMA dirty counter.

Pre-existing issues:
- [High] Silent failure in `kvm_arch_commit_memory_region` leaves KVM memslots and s390 DAT tables out of sync.
--

--- Patch [7]: [PATCH v5 07/13] KVM: s390: cmma: Fix dirty tracking when removing memslot ---
commit 209a02966d18973e5f9b2678eee5bc92d7952603
Author: Claudio Imbrenda <[email protected]>

KVM: s390: cmma: Fix dirty tracking when removing memslot
    
When a memslot is removed, all ptes that mapped the slot are cleared or
even deallocated. If this happens while the system is in migration
mode, and if cmma-dirty pages are removed, the cmma-dirty counter will
not reflect reality.
    
Fix by appropriately decrementing the cmma-dirty counter when removing
a memslot.
    
Opportunistically improve 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 a72ffd515c866..2ae431030621e 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c

[ ... ]

> @@ -5804,14 +5804,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;

[Severity: High]
This is a pre-existing issue, but if kvm_s390_new_mmu_cache() fails to
allocate memory and returns NULL later in this function, doesn't it
silently skip updating the DAT tables?

Since kvm_arch_commit_memory_region() returns void, KVM assumes the changes
are applied infallibly. Could this leave KVM memslots and the s390 hardware
DAT tables fatally out of sync under host memory pressure?

>  	int rc = 0;
>  
> +	guard(mutex)(&kvm->slots_arch_lock);
> +
>  	if (change == KVM_MR_FLAGS_ONLY)
>  		return;
>  
> @@ -5822,6 +5838,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);
> +		}

[Severity: High]
Does this walk miss swapped-out pages by omitting the DAT_WALK_ANY flag?

If DAT_WALK_ANY is not passed to _dat_walk_gfn_range(), holes are skipped
and cmma_d_count_pte() is never called for swapped-out pages, leaking the
cmma_dirty_pages count.

However, the subsequent dat_delete_slot() will process those same holes and
wipe their dirty status.

Will this cause an infinite loop in userspace migration wait loops if the
dirty count never drops to zero?

> +
>  		switch (change) {
>  		case KVM_MR_DELETE:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7