Re: [PATCH QEMU HCI-8.0 v7 07/14] usb-host: re-issue drained URBs if CPR is aborted #VSTOR-137800

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb-host: replay_all queues completed URBs, so aborted CPR re-sends or drops data
--

commit 2947a35eaf543430e9227a58d6c2b6610a15e890
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

usb-host: re-issue drained URBs if CPR is aborted #VSTOR-137800

This patch records the packets suppressed during the CPR drain on a
cpr_inflight list. If CPR is aborted, the vm-state handler's resume branch
re-issues them on the preserved fd. Control transfers are completed with an
error instead of being re-issued.

> 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

[ ... ]

> @@ -450,6 +453,9 @@ static bool usb_host_req_cpr_replay(USBHostRequest *r,
>              return false;
>          }
>      }
> +    if (r->p->state == USB_PACKET_ASYNC) {
> +        r->host->cpr_inflight = g_slist_append(r->host->cpr_inflight, r->p);
> +    }
>      return true;
>  }

[Severity: Medium]
When cpr_replay_all is set by the paused-VM usb_host_pre_save() fallback,
the status and actual_length checks above are skipped. Does
usb_host_req_cpr_replay() then queue every reaped URB on cpr_inflight,
including URBs that completed successfully?

The request is then freed without its data being copied into the packet.

[ ... ]

> +    for (it = s->cpr_inflight; it; it = it->next) {
> +        p = it->data;
> +        /*
> +         * Replay from the start of the TD.  A transfer that already moved some
> +         * data is re-run in full, so an OUT fragment the device consumed may be
> +         * sent twice.  Only the paused-VM pre_save fallback queues such partial
> +         * transfers here; the normal drain delivers them as short transfers.
> +         */
> +        p->actual_length = 0;
> +        if (p->ep->nr == 0) {
> +            /* control endpoint: can't be reconstructed, so error it out */
> +            p->status = USB_RET_IOERROR;
> +            usb_generic_async_ctrl_complete(udev, p);
> +        } else {
> +            usb_host_handle_data(udev, p);

If CPR aborts after that fallback, usb_host_cpr_reissue() re-submits these
fully completed transfers from the start:

 - A completed OUT transfer can be sent to the physical device a second
   time.
 - A completed IN transfer has its data discarded, and a new read is
   issued.
 - A completed control transfer is failed with USB_RET_IOERROR, even
   though the device already executed it.

The commit message and the comment above only mention a partial OUT
fragment being sent twice. Could they, or the queueing in
usb_host_req_cpr_replay(), be changed to cover fully completed transfers
too?

This could matter for non-idempotent streams such as mass storage, serial
or printer traffic.

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