Re: [PATCH QEMU HCI-8.0 v7 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 5 potential issue(s) to consider:
- [Medium] usb-host: failed CPR post_load makes target exit notifier reset the shared device
- [Medium] usb-host: CPR reclaim claims all interfaces and sets configuration even if guest never configured
- [Medium] usb-host: CPR post_load dereferences NULL dev/dh when device was not opened
- [Low] usb-host: usb_host_ep_update() on CPR target resets endpoint halt state
- [Low] usb-host: reclaim comment claims no half-claimed state but claim loop can fail midway
--

commit 557f801d929f427a0cdb08c818b08fd2db0316df
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

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

This patch makes the CPR target call usb_host_ep_update() after vmload, so
endpoints for a non-zero altsetting are rebuilt. It also re-claims the
interfaces through libusb so libusb's view matches the preserved fd. The
new helper usb_host_cpr_reclaim_interfaces() validates the migrated
altsetting values before claiming.

> 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)
> +{
> +    USBDevice *udev = USB_DEVICE(s);
> +    struct libusb_config_descriptor *conf;
> +    int rc;
> +
> +    rc = libusb_get_active_config_descriptor(s->dev, &conf);

[Severity: Medium]
Can s->dev (and later s->dh in libusb_claim_interface()) be NULL here?

usb_host_post_load() takes the CPR branch whenever cpr_is_incoming(), and
neither usb_host_cpr_reclaim_interfaces() nor usb_host_ep_update() checks
dev->dh or dev->dev first.

On the target, only the hostdevice= path opens the device in realize().
With autoscan (hostvendor/hostproduct/hostport), usb_host_auto_check()
only calls usb_host_open() under runstate_is_running().

If the target command line differs from the source's, a NULL pointer would
be passed to libusb.  This is somewhat speculative.  Would an
"if (!dev->dh)" guard make sense?

[ ... ]

> +    /* 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: Medium]
Does this claim every interface and set udev->configuration even when the
guest never configured the device on the source?

On the source, claims are only made in usb_host_claim_interfaces(), which
is reached from usb_host_set_config().  usb_host_open() only detaches
kernel drivers.

If the guest had not issued SET_CONFIGURATION at CPR time,
libusb_get_active_config_descriptor() can still succeed for a
kernel-autoconfigured device.  The target would then create claims the
source never made.  It would also set configuration and ninterfaces from
the host's active config, so the guest would see a configured device.

If migration fails and the source resumes, its ifs[].claimed flags and
configuration would disagree with the kernel state.

The commit message says the re-claim is "a no-op at the usbfs level
anyway".  Does that hold when the source never claimed the interfaces?  I
could not confirm that this case is reachable in practice.

> +    libusb_free_config_descriptor(conf);
> +
> +    return 0;
> +}

[Severity: Low]
The comment above this function and the one in the validation loop say
validation happens first so "garbage migration data can't leave it
half-claimed".  However, the claim loop can still fail partway through
libusb_claim_interface().

In that case the function returns -1 with the earlier interfaces claimed
and marked in s->ifs[].claimed, and nothing rolls that back.  Could the
comment be adjusted, or the claims be undone on failure?

>  static int usb_host_post_load(void *opaque, int version_id)
>  {
>      USBHostDevice *dev = opaque;
[ ... ]
>      if (cpr_is_incoming()) {
> +        /*
> +         * The device stayed open, so rebuild libusb's claims and endpoints,
> +         * and start reaping now that the source has handed the fd off.
> +         */
> +        if (usb_host_cpr_reclaim_interfaces(dev) < 0) {
> +            return -1;
> +        }

[Severity: Medium]
Can this new failure return cause the target's exit notifier to reset the
shared device?

If usb_host_cpr_reclaim_interfaces() returns -1, the incoming migration
fails and the target process exits through usb_host_exit_notifier().  On
the target, s->dh is set and s->cpr_handed_off is false, because it is
only set by usb_host_cpr_quiesce() on the source:

usb_host_exit_notifier() {
    ...
    if (!s->cpr_handed_off) {
        usb_host_abort_xfers(s);
        usb_host_release_interfaces(s);
        libusb_reset_device(s->dh);
        usb_host_attach_kernel(s);
    }
    ...
}

That would reset and release a device on an fd that the still-live source
expects to keep.  Any s->ifs[].claimed flags set by a partial reclaim also
feed usb_host_release_interfaces().

The notifier still gates this only on cpr_handed_off, and the later patch
"usb-host: make CPR migration blocker conditional" does not change that.
Should the target be marked as not owning the device when post_load fails?

> +        usb_host_ep_update(dev);

[Severity: Low]
Does usb_host_ep_update() here reset per-endpoint runtime state that the
guest changed on the source?

It calls usb_ep_reset() and then usb_ep_set_halted(udev, pid, ep, 0) for
every endpoint, and the vmstate carries only the altsetting.  A halted
endpoint would be un-halted without CLEAR_FEATURE(ENDPOINT_HALT), and
max_streams would be rebuilt in default form.

>          usb_host_start_events();
>          return 0;
>      }

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