Re: [PATCH 5/5] usb: xhci: Skip only one TD on Ring Underrun/Overrun

Mathias Nyman
2025-02-11T15:40:44Z

On 10.2.2025 9.42, Michal Pecio wrote:
> If skipping is deferred to events other than Missed Service Error itsef,
> it means we are running on an xHCI 1.0 host and don't know how many TDs
> were missed until we reach some ordinary transfer completion event.
>
> And in case of ring xrun, we can't know where the xrun happened either.
>
> If we skip all pending TDs, we may prematurely give back TDs added after
> the xrun had occurred, risking data loss or buffer UAF by the xHC.
>
> If we skip none, a driver may become confused and stop working when all
> its URBs are missed and appear to be "in flight" forever.
>
> Skip exactly one TD on each xrun event - the first one that was missed,
> as we can now be sure that the HC has finished processing it. Provided
> that one more TD is queued before any subsequent doorbell ring, it will
> become safe to skip another TD by the time we get an xrun again.
>
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
> ---
> drivers/usb/host/xhci-ring.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 878abf5b745d..049206a1db76 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -2875,6 +2875,18 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>
> if (!ep_seg && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
> skip_isoc_td(xhci, td, ep, status);
> +
> + if (ring_xrun_event) {
> + /*
> + * If we are here, we are on xHCI 1.0 host with no idea how
> + * many TDs were missed and where the xrun occurred. Don't
> + * skip more TDs, they may have been queued after the xrun.
> + */
> + xhci_dbg(xhci, "Skipped one TD for slot %u ep %u",
> + slot_id, ep_index);
> + break;

This would be the same as return 0; right?

Whole series looks good, I'll add it

-Mathias