usb: ehci: fix QTD list corruption in qh_completions

[PATCH] usb: ehci: fix QTD list corruption in qh_completions

Posted by Vaibhav Nagare 1 month, 1 week ago

In qh_completions(), when completing a URB at a URB boundary
(last->urb != urb), ehci_urb_done() is called which drops ehci->lock
via usb_hcd_giveback_urb() for the completion callback. While the lock
is dropped, a concurrent ehci_urb_dequeue() (e.g. from a TX timeout
recovery) can modify the QTD list, making the 'tmp' pointer saved by
list_for_each_safe() stale. Continuing iteration with a stale pointer
leads to list_del() corruption and a kernel panic:

    list_del corruption. prev->next should be ff27e4e01aefa580,
    but was ff27e4e01aefa1c0
    kernel BUG at lib/list_debug.c:51!
    Call Trace:
      qh_completions+0x28f/0x640
      ehci_work.part.0+0x1d5/0x330
      ehci_irq+0x3d2/0x490

This was observed on systems with an HPE iLO5 Virtual NIC (cdc_ncm)
where repeated NETDEV WATCHDOG TX timeouts trigger concurrent URB
unlinks that race with the qh_completions lock-drop window.

Fix this by freeing the completed QTD and restarting the list scan
via the existing rescan label after ehci_urb_done(). This is safe
because already-processed QTDs have been removed via list_del() and
the list is strictly shrinking, guaranteeing forward progress.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Vaibhav Nagare <vnagare@redhat.com>
---
 drivers/usb/host/ehci-q.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index ba37a9fcab92..c715648e97ab 100644
--- a/drivers/usb/host/ehci-q.c
+++ b/drivers/usb/host/ehci-q.c
@@ -328,7 +328,16 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)
 		if (last) {
 			if (likely (last->urb != urb)) {
 				ehci_urb_done(ehci, last->urb, last_status);
-				last_status = -EINPROGRESS;
+				/*
+				 * ehci_urb_done() drops ehci->lock for the
+				 * completion callback. The QTD list may have
+				 * been modified (e.g. by URB unlink during
+				 * TX timeout recovery). The 'tmp' saved by
+				 * list_for_each_safe() may be stale.
+				 * Free last and restart the scan.
+				 */
+				ehci_qtd_free(ehci, last);
+				goto rescan;
 			}
 			ehci_qtd_free (ehci, last);
 			last = NULL;
-- 
2.54.0

Re: [PATCH] usb: ehci: fix QTD list corruption in qh_completions

Posted by Alan Stern 1 month, 1 week ago

On Tue, Aug 18, 2026 at 02:03:23PM +0530, Vaibhav Nagare wrote:
> In qh_completions(), when completing a URB at a URB boundary
> (last->urb != urb), ehci_urb_done() is called which drops ehci->lock
> via usb_hcd_giveback_urb() for the completion callback. While the lock
> is dropped, a concurrent ehci_urb_dequeue() (e.g. from a TX timeout
> recovery) can modify the QTD list, making the 'tmp' pointer saved by
> list_for_each_safe() stale. Continuing iteration with a stale pointer
> leads to list_del() corruption and a kernel panic:
> 
>     list_del corruption. prev->next should be ff27e4e01aefa580,
>     but was ff27e4e01aefa1c0
>     kernel BUG at lib/list_debug.c:51!
>     Call Trace:
>       qh_completions+0x28f/0x640
>       ehci_work.part.0+0x1d5/0x330
>       ehci_irq+0x3d2/0x490
> 
> This was observed on systems with an HPE iLO5 Virtual NIC (cdc_ncm)
> where repeated NETDEV WATCHDOG TX timeouts trigger concurrent URB
> unlinks that race with the qh_completions lock-drop window.
> 
> Fix this by freeing the completed QTD and restarting the list scan
> via the existing rescan label after ehci_urb_done(). This is safe
> because already-processed QTDs have been removed via list_del() and
> the list is strictly shrinking, guaranteeing forward progress.

This is good thing to fix, but have you considered what should happen to 
the "end" ehci_qtd pointer when the scan is restarted?

Alan Stern

> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Vaibhav Nagare <vnagare@redhat.com>
> ---
>  drivers/usb/host/ehci-q.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> index ba37a9fcab92..c715648e97ab 100644
> --- a/drivers/usb/host/ehci-q.c
> +++ b/drivers/usb/host/ehci-q.c
> @@ -328,7 +328,16 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)
>  		if (last) {
>  			if (likely (last->urb != urb)) {
>  				ehci_urb_done(ehci, last->urb, last_status);
> -				last_status = -EINPROGRESS;
> +				/*
> +				 * ehci_urb_done() drops ehci->lock for the
> +				 * completion callback. The QTD list may have
> +				 * been modified (e.g. by URB unlink during
> +				 * TX timeout recovery). The 'tmp' saved by
> +				 * list_for_each_safe() may be stale.
> +				 * Free last and restart the scan.
> +				 */
> +				ehci_qtd_free(ehci, last);
> +				goto rescan;
>  			}
>  			ehci_qtd_free (ehci, last);
>  			last = NULL;
> -- 
> 2.54.0
>

Re: [PATCH] usb: ehci: fix QTD list corruption in qh_completions

Posted by Greg KH 1 month, 1 week ago

On Tue, Aug 18, 2026 at 02:03:23PM +0530, Vaibhav Nagare wrote:
> In qh_completions(), when completing a URB at a URB boundary
> (last->urb != urb), ehci_urb_done() is called which drops ehci->lock
> via usb_hcd_giveback_urb() for the completion callback. While the lock
> is dropped, a concurrent ehci_urb_dequeue() (e.g. from a TX timeout
> recovery) can modify the QTD list, making the 'tmp' pointer saved by
> list_for_each_safe() stale. Continuing iteration with a stale pointer
> leads to list_del() corruption and a kernel panic:
> 
>     list_del corruption. prev->next should be ff27e4e01aefa580,
>     but was ff27e4e01aefa1c0
>     kernel BUG at lib/list_debug.c:51!
>     Call Trace:
>       qh_completions+0x28f/0x640
>       ehci_work.part.0+0x1d5/0x330
>       ehci_irq+0x3d2/0x490
> 
> This was observed on systems with an HPE iLO5 Virtual NIC (cdc_ncm)
> where repeated NETDEV WATCHDOG TX timeouts trigger concurrent URB
> unlinks that race with the qh_completions lock-drop window.
> 
> Fix this by freeing the completed QTD and restarting the list scan
> via the existing rescan label after ehci_urb_done(). This is safe
> because already-processed QTDs have been removed via list_del() and
> the list is strictly shrinking, guaranteeing forward progress.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Vaibhav Nagare <vnagare@redhat.com>

Does not match your From: line.

> ---
>  drivers/usb/host/ehci-q.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> index ba37a9fcab92..c715648e97ab 100644
> --- a/drivers/usb/host/ehci-q.c
> +++ b/drivers/usb/host/ehci-q.c
> @@ -328,7 +328,16 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)
>  		if (last) {
>  			if (likely (last->urb != urb)) {
>  				ehci_urb_done(ehci, last->urb, last_status);
> -				last_status = -EINPROGRESS;
> +				/*
> +				 * ehci_urb_done() drops ehci->lock for the
> +				 * completion callback. The QTD list may have
> +				 * been modified (e.g. by URB unlink during
> +				 * TX timeout recovery). The 'tmp' saved by
> +				 * list_for_each_safe() may be stale.
> +				 * Free last and restart the scan.
> +				 */

Did a LLM write this?

thanks,

greg k-h