Re: [PATCH QEMU HCI-8.0 v8 10/14] usb-host: hand off the device across cpr-transfer #VSTOR-137800

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] usb-host: cpr-transfer target takes fd ownership in post_load() before migration commits
- [Medium] usb-host: target usb_host_open() fail path resets shared device before cpr_handed_off is set
- [Low] usb-host: cpr_handed_off left set on source after failed cpr-transfer with VM paused
- [Low] usb-host: cpr_handed_off guard in usb_host_close() also skips logical detach and cleanup
--

commit 642a3db85ee636f9e50d1ff27121f39b2c1b9026
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

usb-host: hand off the device across cpr-transfer #VSTOR-137800

This patch hands reaping of the shared usbfs fd from the cpr-transfer source to the target. The target stops events in realize() and restarts them in post_load(). The source stops events when it quiesces and restarts them on resume. On success the source's exit path leaves the device untouched.

> 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

[ ... ]

> @@ -1285,13 +1306,18 @@ static int usb_host_cpr_drain_xfers(USBHostDevice *s)
>  static int usb_host_cpr_quiesce(USBHostDevice *s, MigMode mode)
>  {
>      if (usb_host_cpr_drain_xfers(s) < 0) {
>          return -1;
>      }
> +    if (mode == MIG_MODE_CPR_TRANSFER) {
> +        usb_host_stop_events();
> +        s->cpr_handed_off = true;
> +    }
>      return 0;
>  }

[Severity: Low]
cpr_handed_off is set here before the outcome of the handoff is known. It
is cleared only on VM resume (usb_host_cpr_reissue()) or device_del
(usb_host_unrealize()). The same applies to the usb_host_pre_save()
fallback for an already-paused VM, which also calls usb_host_cpr_quiesce().

Suppose the cpr-transfer migration fails or is cancelled and the VM stays
paused. The resume branch never runs, so the flag stays set and reaping
stays stopped for the whole libusb context.

A later QEMU quit then reaches 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);
    }
    libusb_close(s->dh);

Can this path tell a failed handoff from a successful one? As written, it
skips the reset and the host driver rebind even though no handoff happened.

[ ... ]

> @@ -1310,6 +1336,11 @@ static void usb_host_cpr_reissue(USBHostDevice *s)
>          return;
>      }
>  
> +    if (s->cpr_handed_off) {
> +        usb_host_start_events();
> +        s->cpr_handed_off = false;
> +    }

[Severity: Medium]
Following the failed-migration path, the source resumes and restarts
reaping here. If the target already called usb_host_start_events() in
usb_host_post_load(), both processes now reap URBs on the shared fd. See
the comment on usb_host_post_load() below.

> @@ -1384,16 +1415,19 @@ static int usb_host_close(USBHostDevice *s)
>  
>      trace_usb_host_close(s->bus_num, s->addr);
>  
> -    usb_host_abort_xfers(s);
> -    usb_host_iso_free_all(s);
> +    if (!s->cpr_handed_off) {
> +        usb_host_abort_xfers(s);
> +        usb_host_iso_free_all(s);
>  
> -    if (udev->attached) {
> -        usb_device_detach(udev);
> +        if (udev->attached) {
> +            usb_device_detach(udev);
> +        }
> +
> +        usb_host_release_interfaces(s);
> +        libusb_reset_device(s->dh);
> +        usb_host_attach_kernel(s);
>      }

[Severity: Low]
This guard covers more than the physical reset, release and rebind. It
also skips usb_host_abort_xfers(), usb_host_iso_free_all() and
usb_device_detach(). Should only the physical operations be gated?

Two callers can reach this with the flag set:

usb_host_realize()
  usb_host_open()   -> usb_device_attach()
  migrate_add_blocker_modes() fails
  usb_host_close()  -> usb_device_detach() skipped

On the target, the attach done by usb_host_open() is never paired with a
detach. usb_qdev_realize() only calls usb_release_port() on failure, so
the port is left with a stale attach.

usb_host_nodev_bh()
  usb_host_close()

If a surprise removal occurs between the source's quiesce and resume, the
handle is closed without detaching the device from the guest. The guest
then sees NODEV errors instead of a detach, while s->dh is NULL.

[ ... ]

> @@ -1500,6 +1541,18 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
>              error_setg(errp, "failed to open host usb device %s", s->hostdevice);
>              return;
>          }
> +        if (cpr_get_incoming_mode() == MIG_MODE_CPR_TRANSFER) {
> +            [ ... ]
> +            usb_host_stop_events();
> +            s->cpr_handed_off = true;
> +        }

[Severity: Medium]
The flag and the event stop are applied only after usb_host_open() has
succeeded. Can the failure path inside usb_host_open() reset the device
under the live source?

usb_host_realize()
  cpr_open_fd()
  usb_host_open(s, NULL, fd)
    ...
  fail:
    usb_host_release_interfaces(s);
    libusb_reset_device(s->dh);
    usb_host_attach_kernel(s);
    libusb_close(s->dh);

usb_host_open() does real work on the shared fd:
libusb_wrap_sys_device(), usb_host_detach_kernel(), usb_host_ep_update()
and usb_device_attach(). If a later step fails, for example
usb_device_attach() rejecting a speed mismatch, the fail: label runs
unconditionally and is not gated by cpr_handed_off. This conflicts with
the commit message statement that the target "must not touch the device at
all" until it takes over.

[ ... ]

> @@ -1563,6 +1616,17 @@ static void usb_host_unrealize(USBDevice *udev)
>      [ ... ]
> +    if (s->cpr_handed_off && !cpr_is_incoming()) {
> +        usb_host_start_events();
> +        s->cpr_handed_off = false;
> +    }
>      usb_host_close(s);

[Severity: Medium]
migration_incoming_state_destroy() calls cpr_set_incoming_mode(MIG_MODE_NONE),
so cpr_is_incoming() becomes false once an incoming migration has failed.

Consider a target that survives the failure, for example with exit-on-error
off, and still has cpr_handed_off == true. A device_del then takes the "we
own it" branch. It restarts events and runs the full reset and release
while the source has resumed with the device. Is cpr_is_incoming() a
reliable test for "the source still drives the fd" here?

[ ... ]

> @@ -2046,6 +2110,9 @@ static int usb_host_post_load(void *opaque, int version_id)
>       */
>      if (cpr_is_incoming()) {
> +        usb_host_start_events();
> +        /* the takeover is complete: from here on this process owns the fd */
> +        dev->cpr_handed_off = false;
>          return 0;
>      }

[Severity: Medium]
Ownership is taken here when this device's vmstate section is loaded, not
when the whole migration has succeeded. Later device sections, the end of
the stream and the source-side completion all come after this point.

If the migration fails after this section loads, for example from a later
device's load error, a broken stream or a cancel, the source resumes. Then
usb_host_cpr_reissue() restarts its own reaping:

target: usb_host_post_load()
          usb_host_start_events();
          dev->cpr_handed_off = false;
source: usb_host_cpr_vm_state() -> usb_host_cpr_reissue()
          usb_host_start_events();

Both processes then reap URBs on the same usbfs fd. This is the situation
the commit message says must be avoided, since one process can reap the
other's in-flight isochronous completions.

When the target then exits, usb_host_exit_notifier() sees
cpr_handed_off == false and runs:

usb_host_abort_xfers(s);
usb_host_release_interfaces(s);
libusb_reset_device(s->dh);
usb_host_attach_kernel(s);

This resets the device and rebinds the host driver under the resumed
source, which the commit message says the target's exit must not do.

Could the takeover be deferred until the migration has committed, so a
late failure leaves the target's flag set and its events stopped?

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