Re: [PATCH QEMU HCI-8.0 v7 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 4 potential issue(s) to consider:
- [Medium] usb-host: CPR drain suppresses completed URBs and reports short OUT as full
- [Medium] usb-host: iso drain cancels in-flight URBs once, so resubmitted URBs survive
- [Medium] usb-host: timeout path completes packets mid-iteration then drops live URBs
- [Medium] usb-host: usb_host_req_cpr_replay() not gated on an active CPR drain
--

commit e57108ef005a6c96c6eda4382d78b25d2524c377
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 CPR. It also adds a
.pre_save() fallback for a VM that is already paused, and cancels and
reaps isochronous URBs on their own rings. Transfers that moved no data
are suppressed so the target can replay them.

> 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

[ ... ]

> +/*
> + * Classify a reap during the CPR drain (r->p is still set).  The paused-VM
> + * fallback (cpr_replay_all) can't deliver completions, so it suppresses every
> + * reap for the target to replay; otherwise one that moved no data is suppressed
> + * and one that moved some is kept as a short transfer.  Returns true if
> + * suppressed, so the caller frees the request.
> + */
> +static bool usb_host_req_cpr_replay(USBHostRequest *r,
> +                                    struct libusb_transfer *xfer)
> +{
> +    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;
> +            return false;
> +        }
> +    }
> +    return true;
> +}

[Severity: Medium]
Does this function lose data or duplicate side effects across CPR?

In the paused-VM fallback, usb_host_pre_save() sets cpr_replay_all = true.
usb_host_req_cpr_replay() then skips its status check and returns true
for every reap, including LIBUSB_TRANSFER_COMPLETED ones.

The packet is suppressed and replayed by the target.  An IN URB that had
already received data loses it, since the device will not resend.  A bulk
OUT or control URB that already executed is sent again.

This also seems to contradict the commit message statement "A transfer
that already completed in the kernel is delivered normally".

The final series state still has this behaviour.  The later patch
"usb-host: re-issue drained URBs if CPR is aborted" only documents it in a
comment ("an OUT fragment the device consumed may be sent twice") and does
not change it.

On the normal vm_change_state path, a CANCELLED URB with actual_length != 0
is rewritten to COMPLETED:

    if (xfer->actual_length != 0) {
        /* deliver as a short transfer */
        xfer->status = LIBUSB_TRANSFER_COMPLETED;
        return false;
    }

usb_host_req_complete_data() only trims the packet length for IN:

    r->p->status = status_map[xfer->status];
    if (r->in && xfer->actual_length) {
        usb_packet_copy(r->p, r->buffer, xfer->actual_length);
    }

For an OUT packet, p->actual_length was already set to the full length
when the guest data was copied at submit time.  Can the guest be told that
the whole OUT TD succeeded when the device received only
xfer->actual_length bytes, with nothing replaying the remainder?

[Severity: Medium]
Separately, is usb_host_req_cpr_replay() meant to apply outside an active
CPR drain?

usb_host_req_complete_ctrl() and usb_host_req_complete_data() now call it
unconditionally for every completion with r->p != NULL.  With
cpr_replay_all false, nothing checks cpr_drained or any other drain-active
state.

A transfer that arrives with LIBUSB_TRANSFER_CANCELLED outside a CPR drain
would behave differently:

- actual_length == 0: the request is freed and the packet is never
  completed, which could leave it in USB_PACKET_ASYNC.
- actual_length != 0: the status is rewritten to COMPLETED.

Previously such a transfer was mapped by status_map[] to USB_RET_IOERROR.
Guest-initiated cancels clear r->p in usb_host_cancel_packet() and
usb_host_req_abort(), so they are not affected.

I could not confirm that libusb reports CANCELLED for a transfer with a
live packet outside CPR, so this one is speculative.  Should the logic be
gated on an active drain?

[ ... ]

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

[ ... ]

> +        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: Medium]
Can this timeout path leave live, uncancelled URBs behind?

The call chain is:

usb_host_cpr_drain_xfers()
  usb_host_req_complete_err()
    usb_packet_complete()
      usb_process_one()
        usb_host_handle_data()
          libusb_submit_transfer()

For a non-ep0 packet, usb_packet_complete() pops the next queued packet on
the endpoint and processes it.  That can allocate a new request, append it
to s->requests, and submit a new URB while QTAILQ_FOREACH_SAFE is running.

usb_host_drop_requests() then unlinks every request, including the newly
submitted ones, without cancelling them.  Would such a URB stay on the
preserved fd with no tracking request, and later complete into a callback
with stale r->p or r->host?

This needs the 100-iteration timeout plus a queued packet behind the failed
one on the same endpoint, which xHCI can have.

[ ... ]

> +    QTAILQ_FOREACH(ring, &s->isorings, next) {
> +        QTAILQ_FOREACH(xfer, &ring->inflight, next) {
> +            libusb_cancel_transfer(xfer->xfer);
> +        }
> +    }
> +
> +    /*
> +     * Cap the reap at 2x a full ring:
> +     * iso_urb_count URBs x iso_urb_frames packets each
> +     */
> +    limit = 2 * s->iso_urb_count * s->iso_urb_frames;
> +    while (usb_host_iso_inflight(s)) {
> +        struct timeval tv = { .tv_usec = 1000 };
> +        libusb_handle_events_timeout(ctx, &tv);
> +        if (limit-- == 0) {
> +            return -1;
> +        }
> +    }
> +    usb_host_iso_free_all(s);
> +    return 0;
> +}

[Severity: Medium]
Can iso URBs be resubmitted while this reap loop runs, and then escape
cancellation?

The iso URBs on ring->inflight are cancelled in a single pass, and the
reap loop does not cancel again.  For an IN endpoint,
usb_host_req_complete_iso() moves the xfer to ring->copy and calls
usb_wakeup():

    QTAILQ_INSERT_TAIL(&xfer->ring->copy, xfer, next);
    usb_wakeup(xfer->ring->ep, 0);

usb_wakeup() reaches xhci_wakeup_endpoint(), which calls xhci_kick_ep()
and xhci_kick_epctx() without checking the VM run state.  The kick can
reprocess an iso IN packet through usb_host_handle_data() and
usb_host_iso_data_in(), which moves ring->unused entries to inflight with
libusb_submit_transfer().

If that happens, the reap loop either runs out its limit and returns -1,
or it exits with new iso URBs from the old process still submitted on the
preserved fd.  The control/bulk loop above re-cancels on every iteration,
but this one does not.

I did not prove that a guest iso TD is always available at that point.
Should the iso phase re-cancel inflight URBs on each iteration as well?

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