Re: [PATCH QEMU HCI-8.1 v4 21/31] kvm: fix coalesced MMIO flush racing with kvm_close()

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] kvm: coalesced_mmio_ring cleared with plain store, read twice without READ_ONCE
--

commit 3b43e13d8cef028cfae52bf9fd0d5ec297cf51f8
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

kvm: fix coalesced MMIO flush racing with kvm_close()

This patch clears kvm_state->coalesced_mmio_ring and calls drain_call_rcu() at the start of kvm_close(). The aim is to stop the RCU thread from flushing the coalesced MMIO ring after cpu_remove_sync() has unmapped it.

> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c

[ ... ]

> @@ -630,6 +630,14 @@ void kvm_close(void)
>          return;
>      }
>  
> +    /*
> +     * Destroying vCPUs unmaps the coalesced MMIO ring, while the RCU thread
> +     * may be flushing it from a memory region finalizer.  Stop new flushes
> +     * and wait for the pending RCU callbacks first.
> +     */
> +    kvm_state->coalesced_mmio_ring = NULL;

[Severity: Low]
Is this plain store safe against the lockless reader in the RCU thread? The reader, kvm_flush_coalesced_mmio_buffer(), loads the same pointer twice with plain loads:

    if (s->coalesced_mmio_ring) {
        struct kvm_coalesced_mmio_ring *ring = s->coalesced_mmio_ring;
        while (ring->first != ring->last) {

The RCU thread runs flatview_destroy() -> memory_region_finalize() -> memory_region_transaction_begin() -> kvm_flush_coalesced_mmio_buffer() with no lock shared with kvm_close().

If the NULL store lands between the check and the reload, or if the compiler reloads the field, ring becomes NULL and ring->first faults. That is the same SIGSEGV this patch is meant to fix.

In practice no call or barrier sits between the two loads, so the compiler will most likely merge them. That makes the window small, but it is still a data race under the C memory model.

Would it be better to use qatomic_set() here in kvm_close(), and a single qatomic_read() into a local variable in kvm_flush_coalesced_mmio_buffer()?

drain_call_rcu() below closes the larger window, where a flush is already past the load. It queues its callback behind any in-flight flatview_destroy(), so that flush finishes before cpu_remove_sync() unmaps the ring. It does not close the double-load window described above.

> +    drain_call_rcu();
> +
>      CPU_FOREACH(cpu) {
>          cpu_remove_sync(cpu);
>          close(cpu->kvm_fd);

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=21