Re: [PATCH v6 14/14] KVM: s390: Move DAT table updates to kvm_arch_prepare_memory_region()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Moving DAT page table modifications and migration state updates to `kvm_arch_prepare_memory_region()` without rollback logic permanently corrupts guest state and causes an infinite loop (host DoS) on error paths.
--

--- Patch [14]: [PATCH v6 14/14] KVM: s390: Move DAT table updates to kvm_arch_prepare_memory_region() ---
commit 99c3a9b6f800205c1388216366b06b3eef1e58f0
Author: Claudio Imbrenda <[email protected]>

KVM: s390: Move DAT table updates to kvm_arch_prepare_memory_region()

This commit moves the whole page table update logic from
kvm_arch_commit_memory_region() to kvm_arch_prepare_memory_region().
This allows the memory allocation failures during DAT table updates to be
propagated to userspace as errors rather than leaving the system in an
inconsistent state.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 035476ec7fbc3..668272dffa18b 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
[ ... ]
> @@ -5788,55 +5807,27 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
[ ... ]
> +retry:
> +	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);
> +			break;
> +		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);
> +			break;
> +		case KVM_MR_FLAGS_ONLY:
> +			break;
> +		default:
> +			rc = 0;
> +			WARN(1, "Unknown KVM MR CHANGE: %d\n", change);
> +		}
> +	}
> +	if (rc == -ENOMEM) {
> +		rc = kvm_s390_mmu_cache_topup(mc);
> +		if (!rc)
> +			goto retry;
> +	}
> +	return rc;

[Severity: Critical]
If kvm_s390_mmu_cache_topup() fails here and returns -ENOMEM, does this leave
the DAT modifications intact without any rollback?

If this returns an error, the KVM core will catch it and restore the active
memslot metadata:

virt/kvm/kvm_main.c:kvm_set_memslot() {
    ...
    if (change == KVM_MR_DELETE || change == KVM_MR_MOVE) {
        kvm_activate_memslot(kvm, invalid_slot, old);
        kfree(invalid_slot);
    }
    ...
}

Will this cause KVM core to be permanently desynchronized from the hardware
DAT tree, leaving program check holes in the DAT tree?

If the guest subsequently accesses this out-of-sync memslot, can it trigger an
infinite loop in the fault handler?

Looking at kvm_s390_faultin_gfn(), if the memslot still exists in KVM core,
it resolves the PFN and calls gmap_link(). Because the DAT tree still has the
hole, gmap_link() returns -EAGAIN. The fault handler might loop infinitely:

arch/s390/kvm/faultin.c:kvm_s390_faultin_gfn() {
    ...
    while (rc == -EAGAIN) {
        ...
        if (!mmu_invalidate_retry_gfn(kvm, inv_seq, f->gfn)) {
            f->valid = true;
            rc = gmap_link(mc, kvm->arch.gmap, f, slot);
        }
    ...
}

Could an unprivileged userspace process under memory pressure intentionally
trigger this error path and cause a host denial of service?

> +}
> +
> +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)
> +{
>  }

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