It was possible to have two threads concurrently aborting the same

transfer, which lead to panics or page faults. For example if a
transfer timed out, another thread could come along and attempt to
abort the same transfer while the timeout task was sleeping in
the *_abort_xfer() function.

Add an "aborting" flag to the private transfer state in each host
controller driver and use this to ensure that the abort is only
executed once. Also prioritise normal abort requests over timeouts
so that the callback is always given a status of USB_CANCELLED even
if the timeout-initiated abort began first.

The crashes caused by this bug were mainly reported in connection
with lpd printing to a USB printer.

PR:	usb/78208, usb/78986

6 files changed, 92 insertions, 1 deletions

diff --git a/sys/dev/usb/ehci.c b/sys/dev/usb/ehci.c
index a52824a..d85a826 100644
--- a/sys/dev/usb/ehci.c
+++ b/sys/dev/usb/ehci.c
@@ -1179,6 +1179,7 @@ ehci_allocx(struct usbd_bus *bus)
 		memset(xfer, 0, sizeof(struct ehci_xfer));
 		usb_init_task(&EXFER(xfer)->abort_task, ehci_timeout_task,
 		    xfer);
+		EXFER(xfer)->ehci_xfer_flags = 0;
 #ifdef DIAGNOSTIC
 		EXFER(xfer)->isdone = 1;
 		xfer->busy_free = XFER_BUSY;
@@ -2519,9 +2520,28 @@ ehci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
 		panic("ehci_abort_xfer: not in process context");
 
 	/*
+	 * If an abort is already in progress then just wait for it to
+	 * complete and return.
+	 */
+	if (exfer->ehci_xfer_flags & EHCI_XFER_ABORTING) {
+		DPRINTFN(2, ("ehci_abort_xfer: already aborting\n"));
+		/* No need to wait if we're aborting from a timeout. */
+		if (status == USBD_TIMEOUT)
+			return;
+		/* Override the status which might be USBD_TIMEOUT. */
+		xfer->status = status;
+		DPRINTFN(2, ("ehci_abort_xfer: waiting for abort to finish\n"));
+		exfer->ehci_xfer_flags |= EHCI_XFER_ABORTWAIT;
+		while (exfer->ehci_xfer_flags & EHCI_XFER_ABORTING)
+			tsleep(&exfer->ehci_xfer_flags, PZERO, "ehciaw", 0);
+		return;
+	}
+
+	/*
 	 * Step 1: Make interrupt routine and timeouts ignore xfer.
 	 */
 	s = splusb();
+	exfer->ehci_xfer_flags |= EHCI_XFER_ABORTING;
 	xfer->status = status;	/* make software ignore it */
 	usb_uncallout(xfer->timeout_handle, ehci_timeout, xfer);
 	usb_rem_task(epipe->pipe.device, &exfer->abort_task);
@@ -2639,6 +2659,12 @@ ehci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
 #ifdef DIAGNOSTIC
 	exfer->isdone = 1;
 #endif
+	/* Do the wakeup first to avoid touching the xfer after the callback. */
+	exfer->ehci_xfer_flags &= ~EHCI_XFER_ABORTING;
+	if (exfer->ehci_xfer_flags & EHCI_XFER_ABORTWAIT) {
+		exfer->ehci_xfer_flags &= ~EHCI_XFER_ABORTWAIT;
+		wakeup(&exfer->ehci_xfer_flags);
+	}
 	usb_transfer_complete(xfer);
 
 	/* printf("%s: %d TDs aborted\n", __func__, count); */
diff --git a/sys/dev/usb/ehcivar.h b/sys/dev/usb/ehcivar.h
index 0639b70..fe791ec 100644
--- a/sys/dev/usb/ehcivar.h
+++ b/sys/dev/usb/ehcivar.h
@@ -65,10 +65,14 @@ struct ehci_xfer {
 	LIST_ENTRY(ehci_xfer) inext; /* list of active xfers */
 	ehci_soft_qtd_t *sqtdstart;
 	ehci_soft_qtd_t *sqtdend;
+	u_int32_t ehci_xfer_flags;
 #ifdef DIAGNOSTIC
 	int isdone;
 #endif
 };
+#define EHCI_XFER_ABORTING	0x0001	/* xfer is aborting. */
+#define EHCI_XFER_ABORTWAIT	0x0002	/* abort completion is being awaited. */
+
 #define EXFER(xfer) ((struct ehci_xfer *)(xfer))
 
 /*
diff --git a/sys/dev/usb/ohci.c b/sys/dev/usb/ohci.c
index 47e8b3f..e0db856 100644
--- a/sys/dev/usb/ohci.c
+++ b/sys/dev/usb/ohci.c
@@ -998,6 +998,7 @@ ohci_allocx(struct usbd_bus *bus)
 		memset(xfer, 0, sizeof (struct ohci_xfer));
 		usb_init_task(&OXFER(xfer)->abort_task, ohci_timeout_task,
 		    xfer);
+		OXFER(xfer)->ohci_xfer_flags = 0;
 #ifdef DIAGNOSTIC
 		xfer->busy_free = XFER_BUSY;
 #endif
@@ -2254,6 +2255,7 @@ ohci_close_pipe(usbd_pipe_handle pipe, ohci_soft_ed_t *head)
 void
 ohci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
 {
+	struct ohci_xfer *oxfer = OXFER(xfer);
 	struct ohci_pipe *opipe = (struct ohci_pipe *)xfer->pipe;
 	ohci_softc_t *sc = (ohci_softc_t *)opipe->pipe.device->bus;
 	ohci_soft_ed_t *sed = opipe->sed;
@@ -2277,9 +2279,28 @@ ohci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
 		panic("ohci_abort_xfer: not in process context");
 
 	/*
+	 * If an abort is already in progress then just wait for it to
+	 * complete and return.
+	 */
+	if (oxfer->ohci_xfer_flags & OHCI_XFER_ABORTING) {
+		DPRINTFN(2, ("ohci_abort_xfer: already aborting\n"));
+		/* No need to wait if we're aborting from a timeout. */
+		if (status == USBD_TIMEOUT)
+			return;
+		/* Override the status which might be USBD_TIMEOUT. */
+		xfer->status = status;
+		DPRINTFN(2, ("ohci_abort_xfer: waiting for abort to finish\n"));
+		oxfer->ohci_xfer_flags |= OHCI_XFER_ABORTWAIT;
+		while (oxfer->ohci_xfer_flags & OHCI_XFER_ABORTING)
+			tsleep(&oxfer->ohci_xfer_flags, PZERO, "ohciaw", 0);
+		return;
+	}
+
+	/*
 	 * Step 1: Make interrupt routine and hardware ignore xfer.
 	 */
 	s = splusb();
+	oxfer->ohci_xfer_flags |= OHCI_XFER_ABORTING;
 	xfer->status = status;	/* make software ignore it */
 	usb_uncallout(xfer->timeout_handle, ohci_timeout, xfer);
 	usb_rem_task(xfer->pipe->device, &OXFER(xfer)->abort_task);
@@ -2314,6 +2335,7 @@ ohci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
 	p = xfer->hcpriv;
 #ifdef DIAGNOSTIC
 	if (p == NULL) {
+		oxfer->ohci_xfer_flags &= ~OHCI_XFER_ABORTING; /* XXX */
 		splx(s);
 		printf("ohci_abort_xfer: hcpriv is NULL\n");
 		return;
@@ -2350,6 +2372,12 @@ ohci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
 	/*
 	 * Step 5: Execute callback.
 	 */
+	/* Do the wakeup first to avoid touching the xfer after the callback. */
+	oxfer->ohci_xfer_flags &= ~OHCI_XFER_ABORTING;
+	if (oxfer->ohci_xfer_flags & OHCI_XFER_ABORTWAIT) {
+		oxfer->ohci_xfer_flags &= ~OHCI_XFER_ABORTWAIT;
+		wakeup(&oxfer->ohci_xfer_flags);
+	}
 	usb_transfer_complete(xfer);
 
 	splx(s);
diff --git a/sys/dev/usb/ohcivar.h b/sys/dev/usb/ohcivar.h
index 478bcea..7cdca0a 100644
--- a/sys/dev/usb/ohcivar.h
+++ b/sys/dev/usb/ohcivar.h
@@ -159,6 +159,8 @@ struct ohci_xfer {
 	u_int32_t ohci_xfer_flags;
 };
 #define OHCI_ISOC_DIRTY  0x01
+#define OHCI_XFER_ABORTING	0x02	/* xfer is aborting. */
+#define OHCI_XFER_ABORTWAIT	0x04	/* abort completion is being awaited. */
 
 #define OXFER(xfer) ((struct ohci_xfer *)(xfer))
 
diff --git a/sys/dev/usb/uhci.c b/sys/dev/usb/uhci.c
index 213c6b3..4340a25 100644
--- a/sys/dev/usb/uhci.c
+++ b/sys/dev/usb/uhci.c
@@ -646,6 +646,7 @@ uhci_allocx(struct usbd_bus *bus)
 		UXFER(xfer)->iinfo.sc = sc;
 		usb_init_task(&UXFER(xfer)->abort_task, uhci_timeout_task,
 		    xfer);
+		UXFER(xfer)->uhci_xfer_flags = 0;
 #ifdef DIAGNOSTIC
 		UXFER(xfer)->iinfo.isdone = 1;
 		xfer->busy_free = XFER_BUSY;
@@ -1933,7 +1934,8 @@ uhci_device_bulk_abort(usbd_xfer_handle xfer)
 void
 uhci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
 {
-	uhci_intr_info_t *ii = &UXFER(xfer)->iinfo;
+	struct uhci_xfer *uxfer = UXFER(xfer);
+	uhci_intr_info_t *ii = &uxfer->iinfo;
 	struct uhci_pipe *upipe = (struct uhci_pipe *)xfer->pipe;
 	uhci_softc_t *sc = (uhci_softc_t *)upipe->pipe.device->bus;
 	uhci_soft_td_t *std;
@@ -1956,9 +1958,28 @@ uhci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
 		panic("uhci_abort_xfer: not in process context");
 
 	/*
+	 * If an abort is already in progress then just wait for it to
+	 * complete and return.
+	 */
+	if (uxfer->uhci_xfer_flags & UHCI_XFER_ABORTING) {
+		DPRINTFN(2, ("uhci_abort_xfer: already aborting\n"));
+		/* No need to wait if we're aborting from a timeout. */
+		if (status == USBD_TIMEOUT)
+			return;
+		/* Override the status which might be USBD_TIMEOUT. */
+		xfer->status = status;
+		DPRINTFN(2, ("uhci_abort_xfer: waiting for abort to finish\n"));
+		uxfer->uhci_xfer_flags |= UHCI_XFER_ABORTWAIT;
+		while (uxfer->uhci_xfer_flags & UHCI_XFER_ABORTING)
+			tsleep(&uxfer->uhci_xfer_flags, PZERO, "uhciaw", 0);
+		return;
+	}
+
+	/*
 	 * Step 1: Make interrupt routine and hardware ignore xfer.
 	 */
 	s = splusb();
+	uxfer->uhci_xfer_flags |= UHCI_XFER_ABORTING;
 	xfer->status = status;	/* make software ignore it */
 	usb_uncallout(xfer->timeout_handle, uhci_timeout, ii);
 	usb_rem_task(xfer->pipe->device, &UXFER(xfer)->abort_task);
@@ -1992,6 +2013,12 @@ uhci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
 #ifdef DIAGNOSTIC
 	ii->isdone = 1;
 #endif
+	/* Do the wakeup first to avoid touching the xfer after the callback. */
+	uxfer->uhci_xfer_flags &= ~UHCI_XFER_ABORTING;
+	if (uxfer->uhci_xfer_flags & UHCI_XFER_ABORTWAIT) {
+		uxfer->uhci_xfer_flags &= ~UHCI_XFER_ABORTWAIT;
+		wakeup(&uxfer->uhci_xfer_flags);
+	}
 	usb_transfer_complete(xfer);
 	splx(s);
 }
diff --git a/sys/dev/usb/uhcivar.h b/sys/dev/usb/uhcivar.h
index 80dba66..3100db7 100644
--- a/sys/dev/usb/uhcivar.h
+++ b/sys/dev/usb/uhcivar.h
@@ -85,8 +85,12 @@ struct uhci_xfer {
 	uhci_intr_info_t iinfo;
 	struct usb_task	abort_task;
 	int curframe;
+	u_int32_t uhci_xfer_flags;
 };
 
+#define UHCI_XFER_ABORTING	0x0001	/* xfer is aborting. */
+#define UHCI_XFER_ABORTWAIT	0x0002	/* abort completion is being awaited. */
+
 #define UXFER(xfer) ((struct uhci_xfer *)(xfer))
 
 /*