Re: [PATCH QEMU HCI-8.1 14/21] 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() still crashes or aborts after kvm_close() on legacy IRQ paths
- [Medium] kvm: irq-route and accessor helpers still deref NULL kvm_state after kvm_close()
- [Medium] kvm: early -ENODEV in kvm_vm_ioctl() leaves errno stale for callers that use errno
--

commit a16b8da8a608436a2f83533cd5dc4b33bbf1644c
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() is also changed so that
-ENODEV does not print the "MSI lost" warning. 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;
> +    }
> +
>      trace_kvm_vm_ioctl(type, arg);

[Severity: Medium]
The early return here, and the matching ones in kvm_ioctl() and
kvm_vcpu_ioctl(), never call ioctl(), so errno is not set. Some callers
look at errno after a negative return instead of using the returned value.

kvm_set_ioeventfd_mmio() is one of them:

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

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

    return 0;

After kvm_close(), can this return 0 (silent success) if errno happens to
be 0, or an unrelated stale errno otherwise? Its callers
kvm_mem_ioeventfd_add() and kvm_mem_ioeventfd_del() call abort() on any
negative result:

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

So a late ioeventfd removal on the retired source, for example from a
memory region commit during device teardown, could still end in abort()
instead of failing gracefully.

kvm_create_guest_memfd() has the same pattern:

    fd = kvm_vm_ioctl(kvm_state, KVM_CREATE_GUEST_MEMFD, &guest_memfd);
    if (fd < 0) {
        error_setg_errno(errp, errno, "Error creating KVM guest_memfd");

This reports errno rather than -ENODEV. kvm_set_memory_attributes() uses
strerror(errno) in the same way.

The KVM_GET_STATS_FD callers test "== -1", which -ENODEV does not match, so
they would carry on with stats_fd set to -19.

Would setting errno = ENODEV before each early return, or changing these
callers to use the returned value, be more robust?

[Severity: Medium]
Separately, the commit message says "any late interrupt or query on the
retired source fails gracefully". Does that hold for the helpers that
dereference kvm_state before any ioctl wrapper runs? kvm_close() sets
kvm_state to NULL, but kvm_enabled() stays true:

accel/accel-irq.c:accel_irqchip_release_virq() {
    if (kvm_enabled()) {
        kvm_irqchip_release_virq(kvm_state, virq);
    }
    ...
}

accel/kvm/kvm-all.c:kvm_irqchip_release_virq() {
    ...
    for (i = 0; i < s->irq_routes->nr; i++) {
    ...
}

With s == NULL, this faults on s->irq_routes before the new "!s" check can
be reached. The only early return is kvm_gsi_direct_mapping().

The same applies to kvm_irqchip_commit_routes(), which writes
s->irq_routes->flags. It is reached via accel_irqchip_commit_routes() from
the virtio-pci unmask path and from the ioapic code.

kvm_irqchip_update_msi_route() ends in kvm_update_routing_entry(s, ...),
which reads s->irq_routes->nr.

The plain accessors kvm_kernel_irqchip_split(), kvm_kernel_irqchip_allowed(),
kvm_kernel_irqchip_required(), kvm_dirty_ring_size(), kvm_has_sync_mmu() and
kvm_has_vcpu_events() read kvm_state->field with no NULL check. Only
kvm_dirty_ring_enabled() has a "kvm_state &&" guard.

One reachable caller is the virtio-pci teardown path:

hw/virtio/virtio-pci.c:kvm_virtio_pci_vq_vector_release() {
    ...
    accel_irqchip_release_virq(irqfd->virq);
    ...
}

This could be reached through set_guest_notifiers(false), device reset or
device_del on the retired source, since QMP is still served. I did not trace
one specific exit or QMP sequence that reaches an unguarded helper.

Nothing later in the series appears to guard these helpers. The follow-up
patch "kvm: fix coalesced MMIO flush racing with kvm_close()" only adds
"kvm_state->coalesced_mmio_ring = NULL; drain_call_rcu();" to kvm_close().

Should these helpers check for a closed kvm_state as well?

[Severity: Medium]
The commit message claim that any late interrupt fails gracefully also looks
too broad for the legacy IRQ path. kvm_set_irq() evaluates s->irq_set_ioctl
before kvm_vm_ioctl() is entered:

accel/kvm/kvm-all.c:kvm_set_irq() {
    ...
    ret = kvm_vm_ioctl(s, s->irq_set_ioctl, &event);
    if (ret < 0) {
        perror("kvm_set_irq");
        abort();
    }

    return (s->irq_set_ioctl == KVM_IRQ_LINE) ? 1 : event.status;
}

After kvm_close(), can a late level-IRQ change reach this through the
callers below, all of which pass kvm_state?

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);

hw/intc/ioapic.c:ioapic_update_kvm_routes()
    kvm_set_irq(kvm_state, i, 1);

With s == NULL, the s->irq_set_ioctl read faults before the new guard runs.
If s were non-NULL, the new -ENODEV return would reach perror() followed by
abort(). Either way the retired source would still die with SIGSEGV or
SIGABRT and leave a core dump, as the commit message describes for the MSI
path.

I did not trace a concrete device that drives these legacy lines on a
retired source. Could kvm_set_irq() and its callers be made to tolerate the
closed state too?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=14