Re: [PATCH QEMU HCI-8.0 v8 06/14] usb-host: drain in-flight URBs across CPR #VSTOR-137800

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] usb-host: cpr_replay_all drops already-completed URBs, losing IN data
- [Low] usb-host: drain timeout path drops newly queued URBs without cancelling them
--

commit 5a6ed65d6723a2887c0ae5c9eb6d765f5c531174
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

usb-host: drain in-flight URBs across CPR #VSTOR-137800

This patch registers a vm_change_state handler that cancels and reaps all
pending usb-host URBs when the VM stops for a CPR switchover. It also adds
a pre_save fallback for VMs that were already paused, and cancels
isochronous URBs on their own rings.

> 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 bool usb_host_req_cpr_replay(USBHostRequest *r,
> +                                    struct libusb_transfer *xfer)
> +{
> +    if (!r->host->cpr_drained) {
> +        /* no CPR drain in progress: normal completion */
> +        return false;
> +    }
> +    if (!r->host->cpr_replay_all) {
> +        if (xfer->status != LIBUSB_TRANSFER_CANCELLED) {
> +            return false;
> +        }
> +        if (xfer->actual_length != 0) {
> +            /* deliver as a short transfer */
> +            xfer->status = LIBUSB_TRANSFER_COMPLETED;
> +            if (!r->in) {
> +                r->p->actual_length = xfer->actual_length;
> +            }
> +            return false;
> +        }
> +    }
> +    return true;
> +}

[Severity: Medium]
When cpr_replay_all is set, the status and actual_length checks are
skipped and usb_host_req_cpr_replay() returns true for every reaped URB.
Does this also discard URBs that the kernel already completed with
LIBUSB_TRANSFER_COMPLETED and actual_length > 0?

For an IN endpoint, the data the device already returned would be dropped.
The target then replays the TD from its start, so that data appears to be
lost. A completed OUT or control request could be sent a second time.

The commit message says "A transfer that already completed in the kernel is
delivered normally". That only holds when cpr_replay_all is false. The
message also says the fallback suppresses every URB, so the two statements
seem to disagree. Could the commit message be made consistent with the code,
or could completed URBs be delivered in the replay_all case too?

The path is: the VM is already paused when a CPR migration starts, so the
FINISH_MIGRATE handler never runs. usb_host_pre_save() then sets
cpr_replay_all and calls usb_host_cpr_quiesce(). A URB that completed in
the kernel but has not been reaped yet then goes through
usb_host_req_complete_data() or usb_host_req_complete_ctrl() and is
suppressed.

Later patches in the series do not change this. The final
usb_host_req_cpr_replay() still returns true for every reap when
cpr_replay_all is set. The later patch "usb-host: re-issue drained URBs if
CPR is aborted" only adds replay for an aborted migration. Its own comment
says the paused-VM fallback queues partially transferred OUT URBs and may
send them twice.

[ ... ]

> +static int usb_host_cpr_drain_xfers(USBHostDevice *s)
> +{
[ ... ]
> +    while (!QTAILQ_EMPTY(&s->requests)) {
> +        struct timeval tv = { .tv_usec = 2500 };
> +
> +        QTAILQ_FOREACH_SAFE(r, &s->requests, next, rtmp) {
> +            libusb_cancel_transfer(r->xfer);
> +        }
> +        libusb_handle_events_timeout(ctx, &tv);
> +        if (--limit == 0) {
> +            /*
> +             * The device won't release these URBs.  Error the packets out so
> +             * the resumed guest retries them, leak the requests (libusb still
> +             * owns their transfers) and abort the migration.
> +             */
> +            QTAILQ_FOREACH_SAFE(r, &s->requests, next, rtmp) {
> +                usb_host_req_complete_err(r, USB_RET_IOERROR);
> +            }
> +            usb_host_drop_requests(s);
> +            return -1;
> +        }
> +    }

[Severity: Low]
On the 100-iteration timeout, can this path leave URBs live that were never
cancelled? usb_host_req_complete_err() calls usb_packet_complete(), which
can start the next queued packet:

usb_host_cpr_drain_xfers()
  usb_host_req_complete_err()
    usb_packet_complete()
      usb_process_one()
        usb_host_handle_data()
          usb_host_req_alloc()   /* new request appended to s->requests */

usb_host_drop_requests() then unlinks every request on s->requests,
including the newly queued ones. Those were never passed to
libusb_cancel_transfer(), and their packets stay in USB_PACKET_ASYNC.

The URBs would stay live in the kernel on the preserved fd, which is what
this patch is meant to prevent. Their late completion callbacks would run
with r->host still set, so they could touch stale state if the device is
closed or unrealized first.

This is an error path on a migration that is already failing. It mirrors
the existing leak in usb_host_reap_xfers().

Later patches, including "usb-host: re-issue drained URBs if CPR is
aborted", do not change this timeout path. The final code still calls
usb_host_req_complete_err() followed by usb_host_drop_requests().

[ ... ]

> +    limit = 2 * s->iso_urb_count * s->iso_urb_frames;
> +    while (usb_host_iso_inflight(s)) {
> +        struct timeval tv = { .tv_usec = 1000 };
> +
> +        QTAILQ_FOREACH(ring, &s->isorings, next) {
> +            QTAILQ_FOREACH(xfer, &ring->inflight, next) {
> +                libusb_cancel_transfer(xfer->xfer);
> +            }
> +        }
> +        libusb_handle_events_timeout(ctx, &tv);
> +        if (limit-- == 0) {
> +            return -1;
> +        }
> +    }

[Severity: Low]
Does the iso timeout path also return with iso URBs still in flight and the
rings still allocated? On limit-- == 0, usb_host_cpr_drain_xfers() returns
-1 before usb_host_iso_free_all(). The cancelled iso URBs may remain on
ring->inflight.

[ ... ]

> +    if (usb_host_cpr_quiesce(s, mode) < 0) {
> +        migration_file_set_error(-EIO, NULL);
> +    }

[Severity: Low]
In usb_host_cpr_vm_state(), the drain failure is reported with
migration_file_set_error(-EIO, NULL). Since the Error is NULL, no
diagnostic reaches the user. Could an Error describing the usb-host drain
failure be passed instead?

[ ... ]

> +    if ((mode == MIG_MODE_CPR_EXEC || mode == MIG_MODE_CPR_TRANSFER) &&
> +        s->dh && !s->cpr_drained) {
> +        s->cpr_replay_all = true;
> +        if (usb_host_cpr_quiesce(s, mode) < 0) {
> +            return -1;
> +        }
> +    }

[ ... ]

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