Re: [PATCH v7 14/14] KVM: s390: Fix kvm_arch_commit_memory_region() when low on memory

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

New issues:
- [High] Dropping `kvm->mmu_lock` during the retry loop allows vCPUs to fault in memory that will be subsequently overwritten, destroying guest storage keys.

Pre-existing issues:
- [Medium] Missing VM Kill on memory allocation failure leaves VM in an inconsistent state.
--

--- Patch [14]: [PATCH v7 14/14] KVM: s390: Fix kvm_arch_commit_memory_region() when low on memory ---
commit 7328596d47af3b280bf92a137aeed10b8bbc3124
Author: Claudio Imbrenda <[email protected]>

KVM: s390: Fix kvm_arch_commit_memory_region() when low on memory

When low on memory, kvm_arch_commit_memory_region() might not perform
the required action completely, leaving the VM in an inconsistent state.
This patch fixes it by refilling the struct kvm_s390_mmu_cache and trying
again.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 518a69c55e85c..fd8badb0d571c 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c

[ ... ]

> @@ -5838,11 +5838,9 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>  		return;
>  
>  	mc = kvm_s390_new_mmu_cache();
> -	if (!mc) {
> -		rc = -ENOMEM;
> +	if (!mc)
>  		goto out;
> -	}
> -
> +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,

[Severity: High]
Since kvm_set_memory_region() makes the new memslot visible to vCPUs before
calling kvm_arch_commit_memory_region(), can a guest vCPU fault in the new
memslot while KVM is committing it?

If dat_create_slot() hits -ENOMEM due to a large memslot exhausting the MMU
cache during KVM_MR_CREATE, the retry loop drops kvm->mmu_lock to top up the
cache. During this unlocked window, the guest vCPU could install valid page
tables and storage keys.

When the retry loop reacquires the lock, it restarts dat_create_slot() from
the beginning of the memslot.

If this happens, wouldn't the callback blindly overwrite the guest's valid
PTEs with an empty token and permanently destroy the valid storage keys?

arch/s390/kvm/dat.c:_dat_slot_pte() {
    ...
    pgste = pgste_get_lock(ptep);
    pgste = __dat_ptep_xchg(ptep, pgste, new_pte, gfn, walk->asce, false);
    pgste.cmma_d = 0;
    pgste_set_unlock(ptep, pgste);
    ...
}

[ ... ]

> @@ -5865,9 +5863,15 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>  		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;
> +	}
>  out:
>  	if (rc)
>  		pr_warn("failed to commit memory region\n");

[Severity: Medium]
This is a pre-existing issue, but if kvm_s390_mmu_cache_topup() fails and
returns -ENOMEM when host memory is exhausted, doesn't the code fall through
to the out label and only log a warning via pr_warn?

The commit message states that such a failure should kill the VM. Does this
actually kill the VM, or does it leave the VM running with partially deleted
or corrupted memory slot mappings since no KVM kill mechanism is invoked
here?

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