sAdd a comment indicating why there continues to be a race condition in

the tap driver, even with Giant over the cdev operation vector, due to
a non-atomic test-and-set of the si_drv1 field in the dev_t.  This bug
exists with Giant under high memory pressure, as malloc() may sleep
in tapcreate(), but is less likely to occur.  The resolution will
probably be to cover si_drv1 using the global tapmtx since no softc is
available, but I need to think about this problem more generally
across a range of drivers using si_drv1 in combination with SI_CHEAPCLONE
to defer expensive allocation to open().

Correct what appears to be a bug in the original if_tap implementation,
in which tapopen() will panic if a tap device instance is opened more
than once due to an incorrect assertion -- only triggered if INVARIANTS
is compiled in (i.e., when built into a kernel).  Return EBUSY instead.

Expand mtx_lock() coverage using tp->tap_mtx to include tp->ether_addr.

1 files changed, 14 insertions, 5 deletions

diff --git a/sys/net/if_tap.c b/sys/net/if_tap.c
index 3daf8ec..17f371d 100644
--- a/sys/net/if_tap.c
+++ b/sys/net/if_tap.c
@@ -346,19 +346,24 @@ tapopen(dev, flag, mode, td)
 	if ((dev2unit(dev) & CLONE_UNITMASK) > TAPMAXUNIT)
 		return (ENXIO);
 
+	/*
+	 * XXXRW: Non-atomic test-and-set of si_drv1.  Currently protected
+	 * by Giant, but the race actually exists under memory pressure as
+	 * well even when running with Giant, as malloc() may sleep.
+	 */
 	tp = dev->si_drv1;
 	if (tp == NULL) {
 		tapcreate(dev);
 		tp = dev->si_drv1;
 	}
 
-	/* Unlocked read. */
-	KASSERT(!(tp->tap_flags & TAP_OPEN), 
-		("%s flags is out of sync", tp->tap_if.if_xname));
+	mtx_lock(&tp->tap_mtx);
+	if (tp->tap_flags & TAP_OPEN) {
+		mtx_unlock(&tp->tap_mtx);
+		return (EBUSY);
+	}
 
 	bcopy(tp->arpcom.ac_enaddr, tp->ether_addr, sizeof(tp->ether_addr));
-
-	mtx_lock(&tp->tap_mtx);
 	tp->tap_pid = td->td_proc->p_pid;
 	tp->tap_flags |= TAP_OPEN;
 	mtx_unlock(&tp->tap_mtx);
@@ -679,11 +684,15 @@ tapioctl(dev, cmd, data, flag, td)
 
 		case OSIOCGIFADDR:	/* get MAC address of the remote side */
 		case SIOCGIFADDR:
+			mtx_lock(&tp->tap_mtx);
 			bcopy(tp->ether_addr, data, sizeof(tp->ether_addr));
+			mtx_unlock(&tp->tap_mtx);
 			break;
 
 		case SIOCSIFADDR:	/* set MAC address of the remote side */
+			mtx_lock(&tp->tap_mtx);
 			bcopy(data, tp->ether_addr, sizeof(tp->ether_addr));
+			mtx_unlock(&tp->tap_mtx);
 			break;
 
 		default: