Re: [PATCH QEMU HCI-8.1 v3 18/28] kvm: tolerate ioctls after kvm_close()

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] kvm: late INTx via kvm_set_irq() still crashes after kvm_close()
- [Medium] kvm: kvm_set_ioeventfd_mmio() returns stale -errno on new -ENODEV path
- [Low] kvm: unsynchronized closed-state check in ioctl wrappers races with kvm_close()
--

commit 6d5d5f6faa5b401fc25d81071e7ee69ce2c8465b
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

kvm: tolerate ioctls after kvm_close()

This patch makes kvm_ioctl(), kvm_vm_ioctl() and kvm_vcpu_ioctl() return
-ENODEV once kvm_close() has run. It also stops kvm_send_msi() from
printing the "MSI lost" warning for that case. The goal is to stop the
retired cpr-transfer source from crashing on late interrupts.

> kvm: tolerate ioctls after kvm_close()

> Make the ioctl wrappers honour the closed state instead of assuming a
> live KVMState: kvm_ioctl(), kvm_vm_ioctl() and kvm_vcpu_ioctl() return
> -ENODEV when kvm_close() has already been called, so any late interrupt
> or query on the retired source fails gracefully.

The commit message says any late interrupt fails gracefully. Is that
accurate? Only the MSI path through kvm_send_msi() is fixed. The
findings below describe other late-interrupt paths that still crash.

> 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

[ ... ]

> @@ -3389,6 +3394,11 @@ int kvm_vm_ioctl(KVMState *s, unsigned long type, ...)
>      arg = va_arg(ap, void *);
>      va_end(ap);
>  
> +    /* kvm_close() has been called: there is no VM to talk to any more */
> +    if (!s || s->vmfd == -1) {
> +        return -ENODEV;
> +    }
> +
>      trace_kvm_vm_ioctl(type, arg);
>      accel_ioctl_begin();
>      ret = ioctl(s->vmfd, type, arg);

[Severity: Medium]
Can a late INTx or GSI line interrupt still crash after kvm_close()?

kvm_close() sets kvm_state to NULL, but kvm_kernel_irqchip and
kvm_async_interrupts_allowed stay true. A late line interrupt then takes
this path:

kvm_ioapic_set_irq() / kvm_pic_set_irq()
  kvm_set_irq(kvm_state = NULL, ...)
    kvm_vm_ioctl(s, s->irq_set_ioctl, &event)

The argument s->irq_set_ioctl is evaluated in kvm_set_irq() before
kvm_vm_ioctl() runs. With s == NULL it faults there, so the new "!s"
check is never reached.

If s were non-NULL but closed, the new -ENODEV would make this code take
the error branch:

    ret = kvm_vm_ioctl(s, s->irq_set_ioctl, &event);
    if (ret < 0) {
        perror("kvm_set_irq");
        abort();
    }

That would turn the late interrupt into an abort() instead.

kvm_irqchip_commit_routes() has the same problem:

    ret = kvm_vm_ioctl(s, KVM_SET_GSI_ROUTING, s->irq_routes);
    assert(ret == 0);

The new -ENODEV would trip the assert.

I compared the end of the series against this patch for accel/kvm and
hw/i386/kvm. The only change there is the coalesced MMIO ring handling in
kvm_close(). Nothing guards kvm_set_irq(), the IOAPIC and PIC callbacks,
or the irqchip route helpers. Should those callers be taught about the
closed state as well?

[Severity: Low]
Is the new closed-state check safe against a concurrent kvm_close()?

The check is a plain, unlocked load of s->vmfd:

    if (!s || s->vmfd == -1) {
        return -ENODEV;
    }
    ...
    accel_ioctl_begin();
    ret = ioctl(s->vmfd, type, arg);

It is not atomic with the later use of s->vmfd. accel_ioctl_begin()
returns immediately when the BQL is held and only takes the lockcnt for
non-BQL callers. kvm_close() does not call kvm_ioctl_inhibit_begin().

kvm_close() also closes the fd before it stores -1:

    close(kvm_state->vmfd);
    kvm_state->vmfd = -1;

Can a non-BQL thread pass the -1 check and then call ioctl() on a closed
or recycled descriptor? The likely result is a spurious -EBADF. A
recycled fd number could refer to another file.

I did not find a specific non-BQL caller that runs during kvm_close().
The wrappers just don't exclude one. The BQL-held callers and the vCPU
threads (stopped by cpu_remove_sync()) are covered.

The same applies to kvm_vcpu_ioctl() and cpu->kvm_fd:

> @@ -3409,6 +3419,11 @@ int kvm_vcpu_ioctl(CPUState *cpu, unsigned long type, ...)
>      arg = va_arg(ap, void *);
>      va_end(ap);
>  
> +    /* kvm_close() has been called: the vCPU fds are gone */
> +    if (cpu->kvm_fd == -1) {
> +        return -ENODEV;
> +    }
> +
>      trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg);
>      accel_cpu_ioctl_begin(cpu);
>      ret = ioctl(cpu->kvm_fd, type, arg);

Similarly, kvm_send_msi() reads the global kvm_state without any
synchronization against kvm_close() setting it to NULL. The KVMState
object itself is never freed, so only the fd value is racy.

[Severity: Medium]
Can the new early return in kvm_vm_ioctl() give kvm_set_ioeventfd_mmio()
a stale errno?

kvm_vm_ioctl() already returns a negative errno value. The new -ENODEV
return never calls ioctl(), so errno is not set. kvm_set_ioeventfd_mmio()
ignores the returned value and re-reads errno:

    ret = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD, &iofd);

    if (ret < 0) {
        return -errno;
    }

After kvm_close() this returns whatever errno an earlier failed syscall
left behind. If errno is 0, the failure is reported as success. If errno
is non-zero, the caller reports a misleading error.

The callers kvm_mem_ioeventfd_add() and kvm_mem_ioeventfd_del() abort()
on any negative return:

    r = kvm_set_ioeventfd_mmio(fd, section->offset_within_address_space,
                               data, true, int128_get64(section->size),
                               match_data);
    if (r < 0) {
        fprintf(stderr, "%s: error adding ioeventfd: %s (%d)\n",
                __func__, strerror(-r), -r);
        abort();
    }

Should kvm_set_ioeventfd_mmio() return ret instead of -errno? The
sibling kvm_set_ioeventfd_pio() already returns the kvm_vm_ioctl() value
directly.

I did not confirm that kvm_enabled() stays true after kvm_close(). I also
did not find a path that deletes an ioeventfd after kvm_close(), such as
device unrealize during qemu_cleanup. So I'm not sure this can be
triggered today.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261016-vfio_v1-v3-0-47487cb5a8e5@virtuozzo.com?part=18