The ohci driver's processing of completed transfer descriptors (TDs)

appeared to rely on all kinds of non-guaranteed behaviours: the
transfer abort code assumed that TDs with no interrupt timeout
configured would end up on the done queue within 20ms, the done
queue processing assumed that all TDs from a transfer would appear
at the same time, and there were access-after-free bugs triggered
on failed transfers.

Attempt to fix these problems by the following changes:
 - Use a maximum (6-frame) interrupt delay instead of no interrupt
   delay to ensure that the 20ms wait in ohci_abort_xfer() is enough
   for the TDs to have been taken off the hardware done queue.
 - Defer cancellation of timeouts and freeing of TDs until we either
   hit an error or reach the final TD.
 - Remove TDs from the done queue before freeing them so that it
   is safe to continue traversing the done queue.

This appears to fix a hang that was reproducable with revision 1.67
or 1.68 of ulpt.c (earlier revisions had a different transfer
pattern). With certain HP printers, the command "true > /dev/ulpt0"
would cause ohci_add_done() to spin because the done queue had a
loop. The list corruption was caused by a 3-TD transfer where the
first TD completed but remained on the internal host controller
done queue because it had no interrupt timeout. When the transfer
timed out, the TD got freed and reused, so it caused a loop in the
done queue when it was inserted a second time from a different
transfer.

Reported by:	Alex Pivovarov
MFC after:	1 week

2 files changed, 38 insertions, 31 deletions

diff --git a/sys/dev/usb/ohci.c b/sys/dev/usb/ohci.c
index 4821cc6..d2e25d3 100644
--- a/sys/dev/usb/ohci.c
+++ b/sys/dev/usb/ohci.c
@@ -524,7 +524,7 @@ ohci_alloc_std_chain(struct ohci_pipe *opipe, ohci_softc_t *sc,
 	tdflags = htole32(
 	    (rd ? OHCI_TD_IN : OHCI_TD_OUT) |
 	    (flags & USBD_SHORT_XFER_OK ? OHCI_TD_R : 0) |
-	    OHCI_TD_NOCC | OHCI_TD_TOGGLE_CARRY | OHCI_TD_NOINTR);
+	    OHCI_TD_NOCC | OHCI_TD_TOGGLE_CARRY | OHCI_TD_SET_DI(6));
 
 	for (;;) {
 		next = ohci_alloc_std(sc);
@@ -1355,7 +1355,7 @@ ohci_softintr(void *v)
 {
 	ohci_softc_t *sc = v;
 	ohci_soft_itd_t *sitd, *sidone, *sitdnext;
-	ohci_soft_td_t  *std,  *sdone,  *stdnext;
+	ohci_soft_td_t  *std,  *sdone,  *stdnext, *p, *n;
 	usbd_xfer_handle xfer;
 	struct ohci_pipe *opipe;
 	int len, cc, s;
@@ -1386,14 +1386,11 @@ ohci_softintr(void *v)
 		stdnext = std->dnext;
 		DPRINTFN(10, ("ohci_process_done: std=%p xfer=%p hcpriv=%p\n",
 				std, xfer, (xfer ? xfer->hcpriv : NULL)));
-		if (xfer == NULL || (std->flags & OHCI_TD_HANDLED)) {
+		if (xfer == NULL) {
 			/*
 			 * xfer == NULL: There seems to be no xfer associated
 			 * with this TD. It is tailp that happened to end up on
 			 * the done queue.
-			 * flags & OHCI_TD_HANDLED: The TD has already been
-			 * handled by process_done and should not be done again.
-			 * Shouldn't happen, but some chips are broken(?).
 			 */
 			continue;
 		}
@@ -1404,9 +1401,6 @@ ohci_softintr(void *v)
 			/* Handled by abort routine. */
 			continue;
 		}
-		usb_uncallout(xfer->timeout_handle, ohci_timeout, xfer);
-		usb_rem_task(OXFER(xfer)->xfer.pipe->device,
-		    &OXFER(xfer)->abort_task);
 
 		len = std->len;
 		if (std->td.td_cbp != 0)
@@ -1418,38 +1412,32 @@ ohci_softintr(void *v)
 			xfer->actlen += len;
 
 		cc = OHCI_TD_GET_CC(le32toh(std->td.td_flags));
-		if (cc == OHCI_CC_NO_ERROR) {
-			if (std->flags & OHCI_CALL_DONE) {
-				xfer->status = USBD_NORMAL_COMPLETION;
-				s = splusb();
-				usb_transfer_complete(xfer);
-				splx(s);
-			}
-			ohci_free_std(sc, std);
-		} else {
+		if (cc != OHCI_CC_NO_ERROR) {
 			/*
 			 * Endpoint is halted.  First unlink all the TDs
 			 * belonging to the failed transfer, and then restart
 			 * the endpoint.
 			 */
-			ohci_soft_td_t *p, *n;
 			opipe = (struct ohci_pipe *)xfer->pipe;
 
 			DPRINTFN(15,("ohci_process_done: error cc=%d (%s)\n",
 			  OHCI_TD_GET_CC(le32toh(std->td.td_flags)),
 			  ohci_cc_strs[OHCI_TD_GET_CC(le32toh(std->td.td_flags))]));
-
-
-			/* Mark all the TDs in the done queue for the current
-			 * xfer as handled
-			 */
-			for (p = stdnext; p; p = p->dnext) {
-				if (p->xfer == xfer)
-					p->flags |= OHCI_TD_HANDLED;
+			usb_uncallout(xfer->timeout_handle, ohci_timeout, xfer);
+			usb_rem_task(OXFER(xfer)->xfer.pipe->device,
+			    &OXFER(xfer)->abort_task);
+
+			/* Remove all this xfer's TDs from the done queue. */
+			for (p = std; p->dnext != NULL; p = p->dnext) {
+				if (p->dnext->xfer != xfer)
+					continue;
+				p->dnext = p->dnext->dnext;
 			}
+			/* The next TD may have been removed. */
+			stdnext = std->dnext;
 
-			/* remove TDs */
-			for (p = std; p->xfer == xfer; p = n) {
+			/* Remove all TDs belonging to this xfer. */
+			for (p = xfer->hcpriv; p->xfer == xfer; p = n) {
 				n = p->nexttd;
 				ohci_free_std(sc, p);
 			}
@@ -1465,7 +1453,27 @@ ohci_softintr(void *v)
 			s = splusb();
 			usb_transfer_complete(xfer);
 			splx(s);
+			continue;
+		}
+		/*
+		 * Skip intermediate TDs. They remain linked from
+		 * xfer->hcpriv and we free them when the transfer completes.
+		 */
+		if ((std->flags & OHCI_CALL_DONE) == 0)
+			continue;
+
+		/* Normal transfer completion */
+		usb_uncallout(xfer->timeout_handle, ohci_timeout, xfer);
+		usb_rem_task(OXFER(xfer)->xfer.pipe->device,
+		    &OXFER(xfer)->abort_task);
+		for (p = xfer->hcpriv; p->xfer == xfer; p = n) {
+			n = p->nexttd;
+			ohci_free_std(sc, p);
 		}
+		xfer->status = USBD_NORMAL_COMPLETION;
+		s = splusb();
+		usb_transfer_complete(xfer);
+		splx(s);
 	}
 
 #ifdef USB_DEBUG
@@ -1779,7 +1787,7 @@ ohci_device_request(usbd_xfer_handle xfer)
 	memcpy(KERNADDR(&opipe->u.ctl.reqdma, 0), req, sizeof *req);
 
 	setup->td.td_flags = htole32(OHCI_TD_SETUP | OHCI_TD_NOCC |
-				     OHCI_TD_TOGGLE_0 | OHCI_TD_NOINTR);
+				     OHCI_TD_TOGGLE_0 | OHCI_TD_SET_DI(6));
 	setup->td.td_cbp = htole32(DMAADDR(&opipe->u.ctl.reqdma, 0));
 	setup->nexttd = next;
 	setup->td.td_nexttd = htole32(next->physaddr);
diff --git a/sys/dev/usb/ohcivar.h b/sys/dev/usb/ohcivar.h
index 7cdca0a..cc4f922 100644
--- a/sys/dev/usb/ohcivar.h
+++ b/sys/dev/usb/ohcivar.h
@@ -57,7 +57,6 @@ typedef struct ohci_soft_td {
 	u_int16_t flags;
 #define OHCI_CALL_DONE	0x0001
 #define OHCI_ADD_LEN	0x0002
-#define OHCI_TD_HANDLED	0x0004		/* signal process_done has seen it */
 } ohci_soft_td_t;
 #define OHCI_STD_SIZE ((sizeof (struct ohci_soft_td) + OHCI_TD_ALIGN - 1) / OHCI_TD_ALIGN * OHCI_TD_ALIGN)
 #define OHCI_STD_CHUNK (PAGE_SIZE / OHCI_STD_SIZE)