Re: [PATCH QEMU HCI-8.1 v7 24/35] 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:
- [Medium] kvm: lockless coalesced_mmio_ring cleared with plain store racing with plain-load flush
--

commit a37b91db7da68f58e8daa300e29ffcbb6cb539f1
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(), before the vCPUs are
destroyed. The aim is to stop the RCU thread from flushing the coalesced
MMIO ring after the vCPU kvm_run area has been unmapped.

> 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;
> +    drain_call_rcu();
> +

[Severity: Medium]
Can this plain store race with the lockless reader in
kvm_flush_coalesced_mmio_buffer()?  That function does:

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

The check and the assignment to ring are two separate plain loads, with
no qatomic_read() or READ_ONCE().  If the RCU thread runs
flatview_destroy() -> memory_region_finalize() ->
memory_region_transaction_begin() -> qemu_flush_coalesced_mmio_buffer()
-> kvm_flush_coalesced_mmio_buffer() while the main thread stores NULL
here, the store could land between the test and the reload.  Then ring
would be NULL and ring->first would dereference it.

The only guard in the flush function is the plain bool
coalesced_flush_in_progress, and the RCU thread does not take the BQL, so
nothing orders the two sides.

drain_call_rcu() runs only after the store.  It covers a reader that
already holds a non-NULL pointer, but it does not cover a reader that
reloads NULL.  The finding is speculative, since compilers normally merge
the two loads, but it is a data race in C terms.

The same plain-store pattern exists in do_kvm_destroy_vcpu():

    if (s->coalesced_mmio_ring == (void *)cpu->kvm_run + PAGE_SIZE) {
        s->coalesced_mmio_ring = NULL;
    }

Would it be better to use qatomic_set() for the stores in kvm_close() and
do_kvm_destroy_vcpu(), and a single qatomic_read() into a local variable
in kvm_flush_coalesced_mmio_buffer()?  Alternatively, could drain_call_rcu()
be called first and the pointer cleared afterwards?

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103-vfio_v1-v7-0-4467fdccbbe6@virtuozzo.com?part=24