Re: [PATCH QEMU HCI-8.1 v5 22/36] kvm: tolerate ioctls after kvm_close()

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] kvm: ioctl wrappers return 0 after kvm_close(), faking fd 0 for fd-returning ioctls
- [Medium] kvm: fd==-1 early return makes vfio_kvm_device_add_fd() store bogus fd 0
--

commit 78cd49a51c86e49590c679c12aaf6d28efd634ee
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

kvm: tolerate ioctls after kvm_close()

This patch keeps kvm_state allocated after kvm_close() and makes kvm_ioctl(), kvm_vm_ioctl() and kvm_vcpu_ioctl() return 0 once the descriptors are closed. The goal is to stop a retired cpr-transfer source from crashing on a late interrupt, such as kvm_irqchip_send_msi() with kvm_state set to NULL.

> 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 +3393,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->vmfd == -1) {
> +        return 0;
> +    }
> +

[Severity: Medium]
Can this early return of 0 be mistaken for a valid file descriptor? Several kvm_vm_ioctl() requests return a new fd on success, and callers treat 0 as success.

query_stats_cb() and query_stats_schemas_cb() only treat -1 as an error:

    stats_fd = kvm_vm_ioctl(s, KVM_GET_STATS_FD, NULL);
    if (stats_fd == -1) {
        ...
    }
    query_stats(result, target, names, stats_fd, NULL, errp);
    close(stats_fd);

After kvm_close(), kvm_state is non-NULL and vmfd is -1. The fake 0 then passes the check, the stats are read through fd 0, and close(stats_fd) closes stdin.

kvm_create_guest_memfd() checks only "if (fd < 0)", so it would return 0 as a valid guest_memfd. kvm_create_vcpu() checks only "if (kvm_fd < 0)", so it would store 0 in cpu->kvm_fd. Later vCPU ioctls would then go to stdin.

The vCPU stats path is not affected, because kvm_close() sets kvm_vcpu_stats_fd to -1 and query_stats_vcpu() checks for it. I did not verify that the QMP stats commands are reachable on a retired source. I also did not find a kvm_enabled() or closed-state guard that would block these callers.

Would it be better to have these fd-returning requests fail explicitly after kvm_close(), or to have the callers check for the closed state?

[Severity: Medium]
Can the same early return cause vfio_kvm_device_add_fd() to store a bogus fd? After vfio_kvm_device_close() calls kvm_close(), it resets vfio_kvm_device_fd to -1, and kvm_enabled() stays true. A later VFIO add on the retired source takes this path in hw/vfio/helpers.c:

    if (kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd)) {
        error_setg_errno(errp, errno, "Failed to create KVM VFIO device");
        return -errno;
    }

    vfio_kvm_device_fd = cd.fd;

kvm_vm_ioctl() now returns 0 without running the ioctl. cd.fd is still 0 because cd is zero-initialised apart from .type, so vfio_kvm_device_fd becomes 0, which is stdin.

The following KVM_SET_DEVICE_ATTR ioctl on fd 0 fails with ENOTTY, so the add reports an error with a stale errno. vfio_kvm_device_fd is left at 0. Later add and del calls then skip creation and issue ioctls on fd 0, instead of reporting that no KVM VFIO device exists.

Is this the intended behaviour for VFIO hotplug on a retired source?

A related case is kvm_vm_check_extension(). It skips its "ret < 0" fallback to the global check after close. That looks benign, because kvm_check_extension() also returns 0 after close. It does show that callers cannot tell the closed state from success.

The commit message describes only the interrupt and ioeventfd callers. It does not mention callers that use the return value as an fd or that depend on the ioctl filling an output buffer. Examples are KVM_CREATE_DEVICE, KVM_GET_STATS_FD and KVM_GET_DIRTY_LOG, which would report success with an unfilled bitmap.

The final series state (f62ee5bb) has the same code in query_stats_cb(), query_stats_schemas_cb(), kvm_create_vcpu() and kvm_create_guest_memfd(). The later patch "kvm: fix coalesced MMIO flush racing with kvm_close()" and the other follow-ups do not change this. hw/vfio/helpers.c is also unchanged by the series.

> @@ -3409,6 +3418,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 0;
> +    }
> +

[Severity: Medium]
This is the same fake-success behaviour as in kvm_vm_ioctl(). Any vCPU ioctl that fills a buffer, such as KVM_GET_REGS, now returns 0 with the output left unfilled once cpu->kvm_fd is -1. Can callers that consume that output be affected?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=22