⬢ DragonFlyBSD Kernel Audit
DF-5322 / fix.diff
← back to finding ↓ download raw
--- a/sys/bus/firewire/firewire.c
+++ b/sys/bus/firewire/firewire.c
@@ -41,6 +41,9 @@
 #include <sys/kernel.h>
 #include <sys/malloc.h>
 #include <sys/conf.h>
+#include <machine/atomic.h>
+#include <sys/spinlock.h>
+#include <sys/spinlock2.h>
 #include <sys/bus.h>		/* used by smbus and newbus */
 #include <sys/sysctl.h>
 #include <sys/thread2.h>
@@ -333,20 +336,64 @@
 
 	crit_enter();
 	for (i = 0; i < 0x40; i ++) {
-		while ((tl = STAILQ_FIRST(&fc->tlabels[i])) != NULL) {
-			xfer = tl->xfer;
-			if (timevalcmp(&xfer->tv, &tv, >))
-				/* the rests are newer than this */
-				break;
-			if (xfer->state == FWXF_START)
-				/* not sent yet */
+		for (;;) {
+			struct fw_xfer *done_xfer;
+
+			done_xfer = NULL;
+			spin_lock(&fc->tlabel_lock);
+			while ((tl = STAILQ_FIRST(&fc->tlabels[i])) != NULL) {
+				xfer = tl->xfer;
+				if (timevalcmp(&xfer->tv, &tv, >))
+					/* the rests are newer than this */
+					break;
+				if (xfer->state == FWXF_START)
+					/* not sent yet */
+					break;
+				if (xfer->state == FWXF_RCVD) {
+					/*
+					 * The response has already been
+					 * delivered; the owner will
+					 * complete and free the xfer.
+					 * Drop only the tlabel, with
+					 * its memory.
+					 */
+					STAILQ_REMOVE_HEAD(&fc->tlabels[i],
+					    link);
+					kfree(tl, M_FW);
+					continue;
+				}
+				if (xfer->state == FWXF_SENT) {
+					u_char st = FWXF_SENT;
+					/*
+					 * Atomically claim the completion
+					 * so fw_rcv() on another CPU can-
+					 * not complete (and the owner can-
+					 * not free) the xfer under us.
+					 */
+					if (!atomic_fcmpset_char(
+					    &xfer->state, &st, FWXF_RCVD))
+						break; /* fw_rcv() won */
+				}
+				device_printf(fc->bdev,
+					"split transaction timeout dst=0x%x tl=0x%x state=%d\n",
+					xfer->send.hdr.mode.hdr.dst, i,
+					xfer->state);
+				xfer->resp = ETIMEDOUT;
+				STAILQ_REMOVE_HEAD(&fc->tlabels[i], link);
+				/* the tlabel is off the list: fw_tl_free()
+				 * will not find and free it anymore */
+				kfree(tl, M_FW);
+				done_xfer = xfer;
 				break;
-			device_printf(fc->bdev,
-				"split transaction timeout dst=0x%x tl=0x%x state=%d\n",
-				xfer->send.hdr.mode.hdr.dst, i, xfer->state);
-			xfer->resp = ETIMEDOUT;
-			STAILQ_REMOVE_HEAD(&fc->tlabels[i], link);
-			fw_xfer_done(xfer);
+			}
+			spin_unlock(&fc->tlabel_lock);
+			if (done_xfer != NULL) {
+				/* run the handler outside the lock: it
+				 * may allocate or free the xfer */
+				fw_xfer_done(done_xfer);
+				continue;
+			}
+			break;
 		}
 	}
 	crit_exit();
@@ -756,6 +803,7 @@
 
 /* Initialize Async handlers */
 	STAILQ_INIT(&fc->binds);
+	spin_init(&fc->tlabel_lock, "fwtlabel");
 	for( i = 0 ; i < 0x40 ; i++){
 		STAILQ_INIT(&fc->tlabels[i]);
 	}
@@ -892,7 +940,7 @@
 {
 	struct tlabel *tl;
 
-	crit_enter();
+	spin_lock(&fc->tlabel_lock);
 	for( tl = STAILQ_FIRST(&fc->tlabels[xfer->tl]); tl != NULL;
 		tl = STAILQ_NEXT(tl, link)){
 		if(tl->xfer == xfer){
@@ -901,11 +949,16 @@
 			break;
 		}
 	}
-	crit_exit();
+	spin_unlock(&fc->tlabel_lock);
 }
 
 /*
- * To obtain XFER structure by transaction label.
+ * To obtain XFER structure by transaction label.  On a FWXF_SENT xfer
+ * the completion is atomically claimed (FWXF_SENT -> FWXF_RCVD) under
+ * the tlabel lock, so firewire_xfer_timeout() on another CPU cannot
+ * complete (and the owner cannot free) the same xfer concurrently.
+ * Returns the xfer only when it was claimed, or when it is still
+ * FWXF_START (response raced transmission); NULL otherwise.
  */
 static struct fw_xfer *
 fw_tl2xfer(struct firewire_comm *fc, int node, int tlabel)
@@ -913,13 +966,24 @@
 	struct fw_xfer *xfer;
 	struct tlabel *tl;
 
-	crit_enter();
+	spin_lock(&fc->tlabel_lock);
 
 	for( tl = STAILQ_FIRST(&fc->tlabels[tlabel]); tl != NULL;
 		tl = STAILQ_NEXT(tl, link)){
 		if(tl->xfer->send.hdr.mode.hdr.dst == node){
 			xfer = tl->xfer;
-			crit_exit();
+			if (xfer->state == FWXF_SENT) {
+				u_char st = FWXF_SENT;
+				if (!atomic_fcmpset_char(&xfer->state,
+				    &st, FWXF_RCVD)) {
+					/* timeout is completing it */
+					xfer = NULL;
+				}
+			} else if (xfer->state != FWXF_START) {
+				/* duplicate response / not completable */
+				xfer = NULL;
+			}
+			spin_unlock(&fc->tlabel_lock);
 			if (firewire_debug > 2)
 				kprintf("fw_tl2xfer: found tl=%d\n", tlabel);
 			return(xfer);
@@ -927,7 +991,7 @@
 	}
 	if (firewire_debug > 1)
 		kprintf("fw_tl2xfer: not found tl=%d\n", tlabel);
-	crit_exit();
+	spin_unlock(&fc->tlabel_lock);
 	return(NULL);
 }
 
@@ -1644,7 +1708,17 @@
 	struct tlabel *tl, *tmptl;
 	static u_int32_t label = 0;
 
-	crit_enter();
+	/*
+	 * Allocate outside the lock (kmalloc(M_WAITOK) may sleep; this
+	 * also removes the sleep from the critical section this function
+	 * used to run in).
+	 */
+	tl = kmalloc(sizeof(struct tlabel), M_FW, M_WAITOK);
+	if (tl == NULL)
+		return(-1);
+	tl->xfer = xfer;
+
+	spin_lock(&fc->tlabel_lock);
 	for( i = 0 ; i < 0x40 ; i ++){
 		label = (label + 1) & 0x3f;
 		for(tmptl = STAILQ_FIRST(&fc->tlabels[label]);
@@ -1654,17 +1728,16 @@
 				break;
 		}
 		if(tmptl == NULL) {
-			tl = kmalloc(sizeof(struct tlabel), M_FW, M_WAITOK);
-			tl->xfer = xfer;
 			STAILQ_INSERT_TAIL(&fc->tlabels[label], tl, link);
-			crit_exit();
+			spin_unlock(&fc->tlabel_lock);
 			if (firewire_debug > 1)
 				kprintf("fw_get_tlabel: dst=%d tl=%d\n",
 				    xfer->send.hdr.mode.hdr.dst, label);
 			return(label);
 		}
 	}
-	crit_exit();
+	spin_unlock(&fc->tlabel_lock);
+	kfree(tl, M_FW);
 
 	kprintf("fw_get_tlabel: no free tlabel\n");
 	return(-1);
@@ -1776,27 +1849,22 @@
 			goto err;
 #endif
 		}
+		/*
+		 * fw_tl2xfer() claimed the completion (FWXF_SENT ->
+		 * FWXF_RCVD) under the tlabel lock, or the xfer is still
+		 * FWXF_START (response raced transmission).  Copying is
+		 * safe: the owner cannot free it before the completion.
+		 */
+		oldstate = rb->xfer->state;
 		fw_rcv_copy(rb);
 		if (rb->xfer->recv.hdr.mode.wres.rtcode != RESP_CMP)
 			rb->xfer->resp = EIO;
 		else
 			rb->xfer->resp = 0;
 		/* make sure the packet is drained in AT queue */
-		oldstate = rb->xfer->state;
-		rb->xfer->state = FWXF_RCVD;
-		switch (oldstate) {
-		case FWXF_SENT:
+		if (oldstate == FWXF_RCVD)
 			fw_xfer_done(rb->xfer);
-			break;
-		case FWXF_START:
-#if 0
-			if (firewire_debug)
-				kprintf("not sent yet tl=%x\n", rb->xfer->tl);
-#endif
-			break;
-		default:
-			kprintf("unexpected state %d\n", rb->xfer->state);
-		}
+		/* FWXF_START: not sent yet, completion is left to TX */
 		return;
 	case FWTCODE_WREQQ:
 	case FWTCODE_WREQB:
--- a/sys/bus/firewire/firewirereg.h
+++ b/sys/bus/firewire/firewirereg.h
@@ -128,6 +128,7 @@
 	struct fw_xferq
 		*arq, *atq, *ars, *ats, *it[FW_MAX_DMACH],*ir[FW_MAX_DMACH];
 	STAILQ_HEAD(, tlabel) tlabels[0x40];
+	struct spinlock tlabel_lock;	/* guards tlabels[] lifetime */
 	STAILQ_HEAD(, fw_bind) binds;
 	STAILQ_HEAD(, fw_device) devices;
 	u_int  sid_cnt;