Re: [PATCH QEMU HCI-8.1 v2 15/24] 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: kvm_set_irq() and listeners still deref NULL kvm_state after kvm_close()
- [Medium] kvm: -ENODEV from closed-state guards still aborts in fail-fast callers; stale errno in ioeventfd
- [Low] kvm: closed-state sentinel reuses -ENODEV, which kvm_send_msi() cannot tell from a kernel -ENODEV
--

commit 32c3566efef848fa0bd62b2fabb9b95d82114443
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. kvm_send_msi() no longer warns on
-ENODEV. The aim is to stop a retired cpr-transfer source from crashing
on late interrupts.

> 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;
> +    }
> +

[Severity: Medium]
Can callers that treat any negative return as fatal still bring down the
retired source? After kvm_close() the memory listeners are still
registered and kvm_enabled() is still true, so these callers now receive
-ENODEV:

kvm_mem_ioeventfd_del()
    if (r < 0) {
        fprintf(stderr, "%s: error deleting ioeventfd: %s (%d)\n",
                __func__, strerror(-r), -r);
        abort();
    }

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

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

kvm_update_clock() in hw/i386/kvm/clock.c
    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
    if (ret < 0) {
        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(-ret));
        abort();

The same applies to kvm_mem_ioeventfd_add(), the PIO ioeventfd variants and
kvm_log_clear().

Would a late legacy IRQ, a memory region update, a routing commit or a
clock query on the retired source still end in abort() or assert()? The
commit message says these cases now "fail gracefully".

Also, the new early return skips ioctl(), so errno is not updated. In
kvm_set_ioeventfd_mmio():

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

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

Doesn't this return a stale errno, which may be 0 or an unrelated value,
when kvm_vm_ioctl() returns -ENODEV from the new guard?

[Severity: Medium]
This isn't fully fixed for legacy interrupt callers either. The guard
helps only if the call reaches kvm_vm_ioctl(), but kvm_set_irq()
evaluates s->irq_set_ioctl as an argument before the call:

kvm_set_irq(s, irq, level)
    ret = kvm_vm_ioctl(s, s->irq_set_ioctl, &event);

With s == NULL this is a NULL dereference before the !s check can run. It
is reached with kvm_state == NULL from:

hw/i386/kvm/ioapic.c:kvm_ioapic_set_irq()
    delivered = kvm_set_irq(kvm_state, s->kvm_gsi_base + irq, level);

hw/i386/kvm/i8259.c:kvm_pic_set_irq()
    delivered = kvm_set_irq(kvm_state, irq, level);

The final return in kvm_set_irq() also reads s->irq_set_ioctl. Memory
listener callbacks such as kvm_coalesce_pio_del() also use kvm_state
directly:

    KVMState *s = kvm_state;

    if (s->coalesced_pio) {

kvm_close() sets kvm_state = NULL but does not unregister the memory
listeners or the irqchip users.

Can an emulated INTx device raising a legacy interrupt, or a memory
region removed during qemu_cleanup(), still hit the SIGSEGV this patch
is meant to remove?

The later patches in the series change only coalesced-MMIO ring handling
in kvm_close() and the CPR-exec and VFIO code. They do not address these
callers.

[Severity: Low]
Is -ENODEV distinct enough to mean "closed"? The check in kvm_vm_ioctl()
tests s->vmfd == -1, and kvm_accel_instance_init() also sets:

    s->fd = -1;
    s->vmfd = -1;

so callers that run before the VM is opened now get -ENODEV instead of
-EBADF. I did not find any caller in accel/kvm, target/i386 or hw/ that
depends on -EBADF.

[ ... ]

> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> --- a/hw/i386/kvm/apic.c
> +++ b/hw/i386/kvm/apic.c
> @@ -196,7 +196,11 @@ static void kvm_send_msi(MSIMessage *msg)
>      msg->address = kvm_swizzle_msi_ext_dest_id(msg->address);
>  
>      ret = kvm_irqchip_send_msi(kvm_state, *msg);
> -    if (ret < 0) {
> +    /*
> +     * -ENODEV means kvm_close() has already run (the VM was handed over
> +     * via CPR); the interrupt is meaningless here and not worth a warning.
> +     */
> +    if (ret < 0 && ret != -ENODEV) {

[Severity: Low]
This isn't a bug I could confirm, but the check suppresses the "MSI lost"
warning for any -ENODEV, not only the post-kvm_close() case. Since the
same value is also an errno the kernel could return from KVM_SIGNAL_MSI,
the two cases cannot be told apart here. I could not confirm that the
kernel returns -ENODEV for that ioctl.

Would testing kvm_state == NULL in kvm_send_msi() before the call be a
more precise way to detect the closed state?

>          fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
>                  strerror(-ret));
>      }

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=15