Re: [PATCH v6 18/33] KVM: s390: Prepare gmap for a second KVM implementation

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

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but an unrecoverable failure in s390_kvm_mmu_commit_memory_region() leaves hardware page tables out of sync with KVM memslots.
- [High] This problem wasn't introduced by this patch, but non-atomic clear of dirty_bitmap in s390_kvm_mmu_get_dirty_log() causes dirty page tracking loss.
--

--- Patch [18]: [PATCH v6 18/33] KVM: s390: Prepare gmap for a second KVM implementation ---
commit e0cdf68355be56510fa59547630f7974c617a6a1
Author: Steffen Eiden <[email protected]>

KVM: s390: Prepare gmap for a second KVM implementation
    
Refactor gmap code such that a second s390 (host) KVM implementation can
use the gmap code as well. Move mmu code from s390 to gmap so the other
KVM implementation can use it as well.
    
No functional change.

> diff --git a/arch/s390/kvm/gmap/kvm_mmu.c b/arch/s390/kvm/gmap/kvm_mmu.c
> new file mode 100644
> index 0000000000000..b08b8229bb6ff
> --- /dev/null
> +++ b/arch/s390/kvm/gmap/kvm_mmu.c
> @@ -0,0 +1,133 @@
[ ... ]
> +/*
> + * Get (and clear) the dirty memory log for a memory slot.
> + */
> +int s390_kvm_mmu_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
> +{
[ ... ]
> +	/* Clear the dirty log */
> +	if (is_dirty) {
> +		n = kvm_dirty_bitmap_bytes(memslot);
> +		memset(memslot->dirty_bitmap, 0, n);

[Severity: High]
This isn't a bug introduced by this patch, but in s390_kvm_mmu_get_dirty_log()
is it safe to use a non-atomic memset() here? If mark_page_dirty() sets bits
concurrently (e.g. via DMA or adapter interrupts) using atomic set_bit_le(),
this non-atomic memset() might overwrite those newly dirtied bits. Could this
lead to lost dirty page tracking and data corruption post-migration?

> +	}
> +	r = 0;
> +out:
> +	mutex_unlock(&kvm->slots_lock);
> +	return r;
> +}
[ ... ]
> +void s390_kvm_mmu_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 __free(kvm_s390_mmu_cache) = NULL;
> +	int rc = 0;
> +
> +	guard(mutex)(&kvm->slots_arch_lock);
> +
> +	if (change == KVM_MR_FLAGS_ONLY)
> +		return;
> +
> +	mc = kvm_s390_new_mmu_cache();
> +	if (!mc) {
> +		rc = -ENOMEM;
> +		goto out;

[Severity: Critical]
This is a pre-existing issue, but in s390_kvm_mmu_commit_memory_region(),
could this early return on allocation failure leave hardware page tables out of
sync with KVM memslots? If an unprivileged userspace process deletes a
memslot and triggers KVM_MR_DELETE, jumping to the out label skips
dat_delete_slot(). The generic KVM code removes the memslot, but the guest
hardware could retain the physical memory mapping, potentially allowing the
guest to access stale or reallocated host memory.

> +	}
> +
> +	scoped_guard(write_lock, &kvm->mmu_lock) {
> +		kvm_s390_update_cmma_dirty(kvm, old);
> +		switch (change) {
> +		case KVM_MR_DELETE:
> +			rc = dat_delete_slot(mc, kvm->arch.gmap->asce, old->base_gfn, old->npages);
> +			break;

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