Attempt to fix a number of race conditions in the handling of

transfer timeouts that typically cause a transfer to be completed
twice, resulting in panics and page faults:

 o A transfer completion interrupt could arrive while an abort_task
   event was set up, so the transfer would be aborted after it had
   completed. This is very easy to reproduce. Fix this by setting
   the transfer status to USBD_TIMEOUT before scheduling the
   abort_task so that the transfer completion code will ignore it.

 o The transfer completion code could execute concurrently with the
   timeout callout, leaving the callout blocked (e.g. waiting for
   Giant) while the transfer completion code runs. In this case,
   callout_stop() does not prevent the callout from running, so
   again the timeout code would run after the transfer was complete.
   Handle this case by checking the return value from callout_stop(),
   and ignoring the transfer if the callout could not be removed.

 o Finally, protect against a timeout callout occurring while a
   transfer is being aborted by another process. Here we arrange
   for the timeout processing to ignore the transfer, and use
   callout_drain() to ensure that the callout has really gone before
   completing the transfer.

This was tested by repeatedly performing USB transfers with a timeout
set to approximately the same as the normal transfer completion
time. In the PR below, apparently this occurred by accident with a
particular printer and the default timeout.

PR:		kern/71491

4 files changed, 125 insertions, 49 deletions

diff --git a/sys/dev/usb/ehci.c b/sys/dev/usb/ehci.c
index a417828..20d0853 100644
--- a/sys/dev/usb/ehci.c
+++ b/sys/dev/usb/ehci.c
@@ -754,7 +754,13 @@ ehci_check_intr(ehci_softc_t *sc, struct ehci_xfer *ex)
 	}
  done:
 	DPRINTFN(12, ("ehci_check_intr: ex=%p done\n", ex));
-	usb_uncallout(ex->xfer.timeout_handle, ehci_timeout, ex);
+	/* The timeout may have fired already but not yet run. */
+	if (ex->xfer.timeout && !sc->sc_bus.use_polling &&
+	    usb_uncallout(ex->xfer.timeout_handle, ehci_timeout,
+	    &ex->xfer) == 0) {
+		/* Make ehci_idone() ignore this xfer. */
+		ex->xfer.status = USBD_TIMEOUT;
+	}
 	ehci_idone(ex);
 }
 
@@ -1169,6 +1175,8 @@ ehci_allocx(struct usbd_bus *bus)
 	}
 	if (xfer != NULL) {
 		memset(xfer, 0, sizeof(struct ehci_xfer));
+		usb_init_task(&EXFER(xfer)->abort_task, ehci_timeout_task,
+		    EXFER(xfer));
 #ifdef DIAGNOSTIC
 		EXFER(xfer)->isdone = 1;
 		xfer->busy_free = XFER_BUSY;
@@ -2491,11 +2499,37 @@ ehci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
 
 	DPRINTF(("ehci_abort_xfer: xfer=%p pipe=%p\n", xfer, epipe));
 
+
+	/*
+	 * Step 1: Make interrupt routine, timers and hardware ignore xfer.
+	 */
+	s = splusb();
+	xfer->status = status;	/* make software ignore it */
+	qhstatus = sqh->qh.qh_qtd.qtd_status;
+	if (!sc->sc_dying) {
+		/* Disable the xfer in hardware to prevent interrupts. */
+		sqh->qh.qh_qtd.qtd_status = qhstatus | htole32(EHCI_QTD_HALTED);
+		for (sqtd = exfer->sqtdstart; ; sqtd = sqtd->nextqtd) {
+			sqtd->qtd.qtd_status |= htole32(EHCI_QTD_HALTED);
+			if (sqtd == exfer->sqtdend)
+				break;
+		}
+	}
+	if (status == USBD_CANCELLED) {
+		/*
+		 * Stop the timeout timer, waiting if required. We can
+		 * guarantee that interrupts or timeouts will not complete
+		 * the the xfer while we wait, because both ignore transfers
+		 * with a status of USBD_CANCELLED.
+		 */
+		usb_rem_task(xfer->pipe->device, &EXFER(xfer)->abort_task);
+		usb_uncallout_drain(xfer->timeout_handle, ehci_timeout, xfer);
+	}
+	splx(s);
+	
 	if (sc->sc_dying) {
 		/* If we're dying, just do the software part. */
 		s = splusb();
-		xfer->status = status;	/* make software ignore it */
-		usb_uncallout(xfer->timeout_handle, ehci_timeout, xfer);
 		usb_transfer_complete(xfer);
 		splx(s);
 		return;
@@ -2505,21 +2539,6 @@ ehci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
 		panic("ehci_abort_xfer: not in process context");
 
 	/*
-	 * Step 1: Make interrupt routine and hardware ignore xfer.
-	 */
-	s = splusb();
-	xfer->status = status;	/* make software ignore it */
-	usb_uncallout(xfer->timeout_handle, ehci_timeout, xfer);
-	qhstatus = sqh->qh.qh_qtd.qtd_status;
-	sqh->qh.qh_qtd.qtd_status = qhstatus | htole32(EHCI_QTD_HALTED);
-	for (sqtd = exfer->sqtdstart; ; sqtd = sqtd->nextqtd) {
-		sqtd->qtd.qtd_status |= htole32(EHCI_QTD_HALTED);
-		if (sqtd == exfer->sqtdend)
-			break;
-	}
-	splx(s);
-
-	/*
 	 * Step 2: Wait until we know hardware has finished any possible
 	 * use of the xfer.  Also make sure the soft interrupt routine
 	 * has run.
@@ -2585,13 +2604,20 @@ ehci_timeout(void *addr)
 		usbd_dump_pipe(exfer->xfer.pipe);
 #endif
 
+	/*
+	 * When a timeout happens concurrently with ehci_abort_xfer(), let
+	 * the non-timeout process do the completion.
+	 */
+	if (exfer->xfer.status == USBD_CANCELLED)
+		return;
+
 	if (sc->sc_dying) {
 		ehci_abort_xfer(&exfer->xfer, USBD_TIMEOUT);
 		return;
 	}
 
 	/* Execute the abort in a process context. */
-	usb_init_task(&exfer->abort_task, ehci_timeout_task, addr);
+	exfer->xfer.status = USBD_TIMEOUT;
 	usb_add_task(exfer->xfer.pipe->device, &exfer->abort_task);
 }
 
diff --git a/sys/dev/usb/ohci.c b/sys/dev/usb/ohci.c
index 9de8479..93ad495 100644
--- a/sys/dev/usb/ohci.c
+++ b/sys/dev/usb/ohci.c
@@ -998,6 +998,8 @@ ohci_allocx(struct usbd_bus *bus)
 	}
 	if (xfer != NULL) {
 		memset(xfer, 0, sizeof (struct ohci_xfer));
+		usb_init_task(&OXFER(xfer)->abort_task, ohci_timeout_task,
+		    OXFER(xfer));
 #ifdef DIAGNOSTIC
 		xfer->busy_free = XFER_BUSY;
 #endif
@@ -1397,6 +1399,13 @@ ohci_softintr(void *v)
 			 */
 			continue;
 		}
+		/* The timeout may have fired already but not yet run. */
+		if (xfer->timeout && !sc->sc_bus.use_polling &&
+		    usb_uncallout(xfer->timeout_handle, ohci_timeout,
+		    xfer) == 0) {
+			/* Ignore and let the timer handle it. */
+			xfer->status = USBD_TIMEOUT;
+		}
 		if (xfer->status == USBD_CANCELLED ||
 		    xfer->status == USBD_TIMEOUT) {
 			DPRINTF(("ohci_process_done: cancel/timeout %p\n",
@@ -1404,7 +1413,6 @@ ohci_softintr(void *v)
 			/* Handled by abort routine. */
 			continue;
 		}
-		usb_uncallout(xfer->timeout_handle, ohci_timeout, xfer);
 
 		len = std->len;
 		if (std->td.td_cbp != 0)
@@ -1969,13 +1977,20 @@ ohci_timeout(void *addr)
 
 	DPRINTF(("ohci_timeout: oxfer=%p\n", oxfer));
 
+	/*
+	 * When a timeout happens concurrently with uhci_abort_xfer(), let
+	 * the non-timeout process do the completion.
+	 */
+	if (oxfer->xfer.status == USBD_CANCELLED)
+		return;
+
 	if (sc->sc_dying) {
 		ohci_abort_xfer(&oxfer->xfer, USBD_TIMEOUT);
 		return;
 	}
 
 	/* Execute the abort in a process context. */
-	usb_init_task(&oxfer->abort_task, ohci_timeout_task, addr);
+	oxfer->xfer.status = USBD_TIMEOUT;
 	usb_add_task(oxfer->xfer.pipe->device, &oxfer->abort_task);
 }
 
@@ -2251,11 +2266,31 @@ ohci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
 
 	DPRINTF(("ohci_abort_xfer: xfer=%p pipe=%p sed=%p\n", xfer, opipe,sed));
 
+	/*
+	 * Step 1: Make interrupt routine, timers and hardware ignore xfer.
+	 */
+	s = splusb();
+	xfer->status = status;	/* make software ignore it */
+	if (!sc->sc_dying) {
+		DPRINTFN(1,("ohci_abort_xfer: stop ed=%p\n", sed));
+		/* force hardware skip */
+		sed->ed.ed_flags |= htole32(OHCI_ED_SKIP);
+	}
+	if (status == USBD_CANCELLED) {
+		/*
+		 * Stop the timeout timer, waiting if required. We can
+		 * guarantee that interrupts or timeouts will not complete
+		 * the the xfer while we wait, because both ignore transfers
+		 * with a status of USBD_CANCELLED.
+		 */
+		usb_rem_task(xfer->pipe->device, &OXFER(xfer)->abort_task);
+		usb_uncallout_drain(xfer->timeout_handle, ohci_timeout, xfer);
+	}
+	splx(s);
+
 	if (sc->sc_dying) {
 		/* If we're dying, just do the software part. */
 		s = splusb();
-		xfer->status = status;	/* make software ignore it */
-		usb_uncallout(xfer->timeout_handle, ohci_timeout, xfer);
 		usb_transfer_complete(xfer);
 		splx(s);
 	}
@@ -2264,16 +2299,6 @@ ohci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
 		panic("ohci_abort_xfer: not in process context");
 
 	/*
-	 * Step 1: Make interrupt routine and hardware ignore xfer.
-	 */
-	s = splusb();
-	xfer->status = status;	/* make software ignore it */
-	usb_uncallout(xfer->timeout_handle, ohci_timeout, xfer);
-	splx(s);
-	DPRINTFN(1,("ohci_abort_xfer: stop ed=%p\n", sed));
-	sed->ed.ed_flags |= htole32(OHCI_ED_SKIP); /* force hardware skip */
-
-	/*
 	 * Step 2: Wait until we know hardware has finished any possible
 	 * use of the xfer.  Also make sure the soft interrupt routine
 	 * has run.
diff --git a/sys/dev/usb/uhci.c b/sys/dev/usb/uhci.c
index 411f809..71152a6 100644
--- a/sys/dev/usb/uhci.c
+++ b/sys/dev/usb/uhci.c
@@ -1369,7 +1369,12 @@ uhci_check_intr(uhci_softc_t *sc, uhci_intr_info_t *ii)
 	}
  done:
 	DPRINTFN(12, ("uhci_check_intr: ii=%p done\n", ii));
-	usb_uncallout(ii->xfer->timeout_handle, uhci_timeout, ii);
+	/* The timeout may have fired already but not yet run. */
+	if (ii->xfer->timeout && !sc->sc_bus.use_polling &&
+	    usb_uncallout(ii->xfer->timeout_handle, uhci_timeout, ii) == 0) {
+		/* Make uhci_idone() ignore this xfer. */
+		ii->xfer->status = USBD_TIMEOUT;
+	}
 	uhci_idone(ii);
 }
 
@@ -1495,6 +1500,7 @@ uhci_idone(uhci_intr_info_t *ii)
 	}
 
  end:
+	usb_rem_task(xfer->pipe->device, &UXFER(xfer)->abort_task);
 	usb_transfer_complete(xfer);
 	DPRINTFN(12, ("uhci_idone: ii=%p done\n", ii));
 }
@@ -1512,12 +1518,20 @@ uhci_timeout(void *addr)
 
 	DPRINTF(("uhci_timeout: uxfer=%p\n", uxfer));
 
+	/*
+	 * When a timeout happens concurrently with ehci_abort_xfer(), let
+	 * the non-timeout process do the completion.
+	 */
+	if (ii->xfer->status == USBD_CANCELLED)
+		return;
+
 	if (sc->sc_dying) {
 		uhci_abort_xfer(&uxfer->xfer, USBD_TIMEOUT);
 		return;
 	}
 
 	/* Execute the abort in a process context. */
+	uxfer->xfer.status = USBD_TIMEOUT;
 	usb_add_task(uxfer->xfer.pipe->device, &uxfer->abort_task);
 }
 
@@ -1940,13 +1954,35 @@ uhci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
 
 	DPRINTFN(1,("uhci_abort_xfer: xfer=%p, status=%d\n", xfer, status));
 
+	/*
+	 * Step 1: Make interrupt routine, timers and hardware ignore xfer.
+	 */
+	s = splusb();
+	xfer->status = status;	/* make software ignore it */
+	if (!sc->sc_dying) {
+		/* Disable the xfer in hardware to prevent interrupts. */
+		DPRINTFN(1,("uhci_abort_xfer: stop ii=%p\n", ii));
+		for (std = ii->stdstart; std != NULL; std = std->link.std)
+			std->td.td_status &= htole32(~(UHCI_TD_ACTIVE |
+			    UHCI_TD_IOC));
+	}
+	if (status == USBD_CANCELLED) {
+		/*
+		 * Stop the timeout timer, waiting if required. We can
+		 * guarantee that interrupts or timeouts will not complete
+		 * the the xfer while we wait, because both ignore transfers
+		 * with a status of USBD_CANCELLED.
+		 */
+		usb_rem_task(xfer->pipe->device, &UXFER(xfer)->abort_task);
+		usb_uncallout_drain(xfer->timeout_handle, uhci_timeout,
+		    &UXFER(xfer)->iinfo);
+	}
+	splx(s);
+
 	if (sc->sc_dying) {
 		/* If we're dying, just do the software part. */
 		s = splusb();
-		xfer->status = status;	/* make software ignore it */
-		usb_uncallout(xfer->timeout_handle, uhci_timeout, xfer);
 		usb_transfer_complete(xfer);
-		usb_rem_task(xfer->pipe->device, &UXFER(xfer)->abort_task);
 		splx(s);
 		return;
 	}
@@ -1955,18 +1991,6 @@ uhci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
 		panic("uhci_abort_xfer: not in process context");
 
 	/*
-	 * Step 1: Make interrupt routine and hardware ignore xfer.
-	 */
-	s = splusb();
-	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);
-	DPRINTFN(1,("uhci_abort_xfer: stop ii=%p\n", ii));
-	for (std = ii->stdstart; std != NULL; std = std->link.std)
-		std->td.td_status &= htole32(~(UHCI_TD_ACTIVE | UHCI_TD_IOC));
-	splx(s);
-
-	/*
 	 * Step 2: Wait until we know hardware has finished any possible
 	 * use of the xfer.  Also make sure the soft interrupt routine
 	 * has run.
diff --git a/sys/dev/usb/usb_port.h b/sys/dev/usb/usb_port.h
index 972c4e9..38dd65e 100644
--- a/sys/dev/usb/usb_port.h
+++ b/sys/dev/usb/usb_port.h
@@ -388,6 +388,7 @@ typedef struct callout usb_callout_t;
 #define usb_callout_init(h)     callout_init(&(h), 0)
 #define usb_callout(h, t, f, d) callout_reset(&(h), (t), (f), (d))
 #define usb_uncallout(h, f, d)  callout_stop(&(h))
+#define usb_uncallout_drain(h, f, d)  callout_drain(&(h))
 #else
 typedef struct proc *usb_proc_ptr;