Re: [PATCH QEMU HCI-8.0 v8 12/14] usb-host: reconfigure endpoints on CPR incoming #VSTOR-137800

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] usb-host: CPR reclaim claims interfaces and sets configuration for guests that never configured the device
--

commit 4d875277381be99a72691b7786690c4fc8a9ce77
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

usb-host: reconfigure endpoints on CPR incoming #VSTOR-137800

On CPR incoming, this patch rebuilds the libusb endpoints with
usb_host_ep_update(). It also adds usb_host_cpr_reclaim_interfaces(), which
re-claims interfaces so that libusb's handle matches the preserved fd.

> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c

[ ... ]

> +static int usb_host_cpr_reclaim_interfaces(USBHostDevice *s)
> +{

[ ... ]

> +    /* Data is sane: claim each interface on the fd and record it. */
> +    for (int i = 0; i < conf->bNumInterfaces; i++) {
> +        int ifnum = conf->interface[i].altsetting[0].bInterfaceNumber;
> +
> +        rc = libusb_claim_interface(s->dh, ifnum);
> +        if (rc != 0) {
> +            usb_host_libusb_error("libusb_claim_interface", rc);
> +            libusb_free_config_descriptor(conf);
> +            return -1;
> +        }
> +        s->ifs[ifnum].claimed = true;
> +    }
> +
> +    udev->ninterfaces = conf->bNumInterfaces;
> +    udev->configuration = conf->bConfigurationValue;

[Severity: Low]
Can this claim interfaces and set udev->configuration for a guest that never
configured the device?

In usb_host_open(), interfaces are not claimed, and udev->ninterfaces and
udev->configuration are not set. They are only set through
usb_host_set_config()->usb_host_claim_interfaces() when the guest issues
SET_CONFIGURATION.

usb_host_cpr_reclaim_interfaces() instead derives its work from
libusb_get_active_config_descriptor(), which reflects the host kernel's
active configuration. It then stores:

    udev->ninterfaces = conf->bNumInterfaces;
    udev->configuration = conf->bConfigurationValue;

Nothing from the migrated state is consulted, such as the source's
s->ifs[].claimed. If the guest left the device in the address state while the
kernel's active configuration is non-zero, the target could claim interfaces
the source never claimed. It could also report a non-zero configuration that
the guest never selected.

So the commit message statement "on the preserved FD this is a no-op at the
usbfs level anyway" seems to hold only when the guest had already configured
the device. I did not trace the guest-side GET_CONFIGURATION handling.

Separately, if libusb_claim_interface() fails partway through the loop, for
example with EBUSY, the function returns -1 after earlier interfaces have
already been claimed and marked in s->ifs[ifnum].claimed:

    rc = libusb_claim_interface(s->dh, ifnum);
    if (rc != 0) {
        ...
        return -1;
    }
    s->ifs[ifnum].claimed = true;

There is no rollback. This seems to contradict the comment above the
validation loop, "so garbage migration data can't leave it half-claimed".
Would it make sense to release the earlier claims on this error path?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v8-0-72ae4fb8977b@virtuozzo.com?part=12