From owner-freebsd-current@FreeBSD.ORG Sat Jul 21 21:08:39 2007 Return-Path: Delivered-To: current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id A15CC16A417; Sat, 21 Jul 2007 21:08:39 +0000 (UTC) (envelope-from thompsa@FreeBSD.org) Received: from heff.fud.org.nz (203-109-251-39.static.bliink.ihug.co.nz [203.109.251.39]) by mx1.freebsd.org (Postfix) with ESMTP id CB75413C48E; Sat, 21 Jul 2007 21:08:38 +0000 (UTC) (envelope-from thompsa@FreeBSD.org) Received: by heff.fud.org.nz (Postfix, from userid 1001) id CC7C61CC5D; Sun, 22 Jul 2007 09:08:37 +1200 (NZST) Date: Sun, 22 Jul 2007 09:08:37 +1200 From: Andrew Thompson To: Attilio Rao Message-ID: <20070721210837.GB84580@heff.fud.org.nz> References: <200707211925.59698.dfr@rabson.org> <46A252C3.5050804@FreeBSD.org> <20070721210759.GA84580@heff.fud.org.nz> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="+HP7ph2BbKc20aGI" Content-Disposition: inline In-Reply-To: <20070721210759.GA84580@heff.fud.org.nz> User-Agent: Mutt/1.5.13 (2006-08-11) Cc: current@freebsd.org Subject: Re: if_bridge crash X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 21 Jul 2007 21:08:39 -0000 --+HP7ph2BbKc20aGI Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. --+HP7ph2BbKc20aGI Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="bridge_lock.diff" 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); \ --+HP7ph2BbKc20aGI--