Re: [PATCH QEMU HCI-8.0 v6 05/12] usb-host: drain in-flight URBs across CPR #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: xHCI replay of suppressed URBs relies on a dequeue pointer that is already advanced
- [Medium] usb-host: pre_save fallback drain can deliver completions after xHCI state is saved
- [Medium] usb-host: iso drain cancels in-flight URBs once but completions can resubmit new ones
- [Medium] usb-host: a cancelled OUT or control URB that moved data is reported as a short success
- [Medium] usb-host: drain timeout path can leave newly submitted URBs live and untracked
--

commit e9cd7ce570abec8bcd0aa45a09ea274defa372c5
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 libusb transfers when the VM stops for CPR. It also adds a
.pre_save() fallback for VMs that are already paused, and cancels and
reaps isochronous URBs as well. Cancelled URBs 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).  One that moved no
> + * data is suppressed for the target to replay; 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 (xfer->status != LIBUSB_TRANSFER_CANCELLED) {
> +        return false;
> +    }

[Severity: Medium]
The commit message says the .pre_save() fallback suppresses "every URB for
the target to replay". Is that accurate?

In usb_host_req_cpr_replay(), any status other than
LIBUSB_TRANSFER_CANCELLED returns false, so a URB that completed in the
kernel during the fallback drain is still delivered:

    usb_host_req_complete_data()
      -> usb_packet_complete()

Can this run after the xHCI and interrupt controller state has already
been saved? usb_host_pre_save() runs as part of device-state save, not
before the snapshot as the vm-state handler does. If so, event ring,
interrupter and endpoint state changed by that completion might be missing
from the migrated image.

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

[Severity: Medium]
Can this corrupt an OUT stream? The status is rewritten to
LIBUSB_TRANSFER_COMPLETED for every direction and transfer type, with no
check of r->in or the control stage.

For a bulk or interrupt OUT, or a control write, that was cancelled after
moving some bytes, the device has consumed a fragment. The guest is then
told the transfer ended short, and the target can neither replay it nor
see an error. A CBW or a data stage could be affected this way.

Should this conversion be limited to IN transfers?

[Severity: Medium]
When usb_host_req_cpr_replay() returns true, the completion is suppressed
and the request is freed. The replay then relies on the xHCI endpoint
context in guest RAM still pointing at the start of this TD.

Is that the case when several TDs are outstanding on one endpoint?
xhci_kick_epctx() advances ring->dequeue via xhci_ring_fetch() for each TD
it fires. xhci_complete() calls xhci_kick_epctx(), which calls
xhci_set_ep_state(EP_RUNNING) at its start:

    xhci_complete()
      -> xhci_kick_epctx()
           -> xhci_set_ep_state()
                ctx[2] = ring->dequeue | ring->ccs;

That writes the advanced dequeue to guest memory while later TDs are
still queued or in flight. usb_xhci_post_load() rebuilds the ring from
that value. Would a suppressed TD2 then be skipped on the target, so that
its completion event is never generated?

[ ... ]

> +/*
> + * Quiesce the device for a CPR switchover.  Cancel and reap all URBs,
> + * so that no URB submitted by this process can outlive it.
> + */
> +static int usb_host_cpr_drain_xfers(USBHostDevice *s)
> +{

[ ... ]

> +        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]
On this timeout path, can usb_host_drop_requests() orphan live URBs?

usb_host_req_complete_err() calls usb_packet_complete(), which can process
queued packets on the endpoint:

    usb_host_req_complete_err()
      -> usb_packet_complete()
           -> usb_process_one()
                -> usb_host_handle_data() / control handler
                     -> libusb_submit_transfer()

Those paths append new entries to s->requests. usb_host_drop_requests()
then unlinks every entry without cancelling it, so the new URBs stay live
on the preserved fd and are never reaped.

> +    /*
> +     * Iso URBs are on the rings, not s->requests.  Cancel them: one in
> +     * flight when the guest stops may never complete on its own, and none
> +     * may be left on the preserved fd.  The target replays from the ring.
> +     */
> +    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;
> +        }
> +    }

[Severity: Medium]
Are the iso URBs cancelled only once here? Completing a cancelled IN iso
URB runs usb_host_req_complete_iso(), which does:

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

That can reach xhci_wakeup_endpoint(), then a kick, then
usb_host_iso_data_in(), which submits every xfer on ring->unused into
ring->inflight. The loop above never re-cancels those, and the bulk loop
earlier in usb_host_cpr_drain_xfers() has already finished, so it does not
recheck s->requests.

Can this either time out and return -1, which aborts the migration, or
leave freshly submitted iso or bulk URBs live across exec?

[ ... ]

> +static int usb_host_pre_save(void *opaque)
> +{

[ ... ]

> +    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_drain_xfers(s) < 0) {
> +            return -1;
> +        }
> +    }

[Severity: Medium]
Is it safe to run the drain from usb_host_pre_save()? Packet completions
raised here can modify xHCI state that may already have been serialized,
depending on the order of the xHCI and usb-host vmstate entries in the
save sequence. Could that ordering be confirmed, or the drain be moved
earlier?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v6-0-8688ec9576d1@virtuozzo.com?part=5