if_bridge crash

[Andrew Thompson]

On Sun, Jul 22, 2007 at 09:07:59AM +1200, Andrew Thompson wrote:
> On Sat, Jul 21, 2007 at 08:38:59PM +0200, Attilio Rao wrote:
> > Doug Rabson wrote:
> > >I've been using if_bridge and if_tap to join various qemu virtual 
> > >machines onto my local network. I use this script to set up the bridge:
> > >
> > >	ifconfig bridge0 create
> > >	ifconfig tap0 create
> > >	ifconfig bridge0 addm vr0 addm tap0 up
> > >
> > >I had forgotten what stupid mac address qemu had made up for its 
> > >interface and I needed to adjust my dhcpd config so I typed 'ifconfig 
> > >bridge addr' to list the addresses on the bridge and got an instant 
> > >panic. Qemu was not running at this point. The kernel address where it 
> > >crashed was good - it was the userland address which faulted.
> > >
> > >The crash was in generic_copyout+0x36 called from bridge_ioctl+0x1ae. I 
> > >took a look at the code and as far as I can make out, trap() got a bit 
> > >confused and managed to ignore the pcb_onfault marker left by copyout. 
> > >Its hard to tell exactly what happened since the damn compiler has 
> > >optimised the crap out of the code there.
> > >
> > >As far as I can see, the bridge code is calling copyout with a mutex 
> > >held. Is that allowed? It doesn't sound like it should be allowed but 
> > >I'm not quite up-to-date on that aspect of the current kernel api.
> > 
> > Since a copyout() can generate a page fault (which can let the thread 
> > sleep) it is not allowed to mantain neither a blockable lock (mutex, 
> > rwlock) or a spinlock over a copyout.
> 
> 
> Please test this patch.

One more time with the file attached.
-------------- next part --------------
Index: if_bridge.c
===================================================================
RCS file: /home/ncvs/src/sys/net/if_bridge.c,v
retrieving revision 1.100
diff -u -p -r1.100 if_bridge.c
--- if_bridge.c	13 Jun 2007 18:58:04 -0000	1.100
+++ if_bridge.c	21 Jul 2007 21:05:55 -0000
@@ -668,8 +668,6 @@ bridge_ioctl(struct ifnet *ifp, u_long c
 	const struct bridge_control *bc;
 	int error = 0;
 
-	BRIDGE_LOCK(sc);
-
 	switch (cmd) {
 
 	case SIOCADDMULTI:
@@ -714,7 +712,9 @@ bridge_ioctl(struct ifnet *ifp, u_long c
 				break;
 		}
 
+		BRIDGE_LOCK(sc);
 		error = (*bc->bc_func)(sc, &args);
+		BRIDGE_UNLOCK(sc);
 		if (error)
 			break;
 
@@ -730,14 +730,15 @@ bridge_ioctl(struct ifnet *ifp, u_long c
 			 * If interface is marked down and it is running,
 			 * then stop and disable it.
 			 */
+			BRIDGE_LOCK(sc);
 			bridge_stop(ifp, 1);
+			BRIDGE_UNLOCK(sc);
 		} else if ((ifp->if_flags & IFF_UP) &&
 		    !(ifp->if_drv_flags & IFF_DRV_RUNNING)) {
 			/*
 			 * If interface is marked up and it is stopped, then
 			 * start it.
 			 */
-			BRIDGE_UNLOCK(sc);
 			(*ifp->if_init)(sc);
 		}
 		break;
@@ -752,14 +753,10 @@ bridge_ioctl(struct ifnet *ifp, u_long c
 		 * drop the lock as ether_ioctl() will call bridge_start() and
 		 * cause the lock to be recursed.
 		 */
-		BRIDGE_UNLOCK(sc);
 		error = ether_ioctl(ifp, cmd, data);
 		break;
 	}
 
-	if (BRIDGE_LOCKED(sc))
-		BRIDGE_UNLOCK(sc);
-
 	return (error);
 }
 
Index: if_bridgevar.h
===================================================================
RCS file: /home/ncvs/src/sys/net/if_bridgevar.h,v
retrieving revision 1.21
diff -u -p -r1.21 if_bridgevar.h
--- if_bridgevar.h	13 Jun 2007 18:58:04 -0000	1.21
+++ if_bridgevar.h	21 Jul 2007 21:06:08 -0000
@@ -272,7 +272,6 @@ struct ifbpstpconf {
 } while (0)
 #define BRIDGE_LOCK(_sc)		mtx_lock(&(_sc)->sc_mtx)
 #define BRIDGE_UNLOCK(_sc)		mtx_unlock(&(_sc)->sc_mtx)
-#define BRIDGE_LOCKED(_sc)		mtx_owned(&(_sc)->sc_mtx)
 #define BRIDGE_LOCK_ASSERT(_sc)		mtx_assert(&(_sc)->sc_mtx, MA_OWNED)
 #define	BRIDGE_LOCK2REF(_sc, _err)	do {	\
 	mtx_assert(&(_sc)->sc_mtx, MA_OWNED);	\