Re: [PATCH v4 15/27] KVM: S390: Prepare gmap for a second KVM implementation

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Fallible memory allocation during the infallible memslot commit phase causes silent Use-After-Free and guest crashes.
- [High] Non-atomic clearing of the memory slot dirty bitmap via `memset` can race with concurrent atomic `set_bit` operations, leading to lost dirty bits.
--

commit 2aa486852c957946a99960f65fceb0012466db06
Author: Steffen Eiden <seiden@linux.ibm.com>

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 relevant definitions into the shared
kvm_host.h. Move mmu code from s390 to gmap so the other KVM
implementation can use it as well.

> diff --git a/arch/s390/kvm/gmap/mmu.c b/arch/s390/kvm/gmap/mmu.c
> new file mode 100644
> index 0000000000000..5bb6609b974b2
> --- /dev/null
> +++ b/arch/s390/kvm/gmap/mmu.c
[ ... ]
> +int gmap_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 since the code is being
moved here, I noticed this clearing logic. Can this non-atomic memset()
race with concurrent atomic bit setting operations?

If KVM concurrently calls mark_page_dirty() (for example, from
kvm_s390_sync_request() for virtio indicators), which sets bits in this
same bitmap using set_bit_le(), it seems we could lose dirty bits in the
gap between the copy and this clear, or have the atomic set overwritten by
the memset().

Could this lead to lost dirty bits and memory corruption during live
migration?

[ ... ]
> +void gmap_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;
> +	int rc = 0;
> +
> +	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 is it safe for this memory allocation to
fail during the memslot commit phase?

If kvm_s390_new_mmu_cache() fails under memory pressure, the error
is only printed at the goto out label and the required gmap table updates
are skipped:

[ ... ]
> +out:
> +	if (rc)
> +		pr_warn("failed to commit memory region\n");
> +	kvm_s390_free_mmu_cache(mc);
> +}

For KVM_MR_DELETE and KVM_MR_MOVE, skipping dat_delete_slot() could
leave the guest with a stale, usable hardware mapping to host memory.

Does this lead to a guest-to-host use-after-free if userspace issues
KVM_SET_USER_MEMORY_REGION under high memory pressure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706085229.979525-1-seiden@linux.ibm.com?part=15