From owner-freebsd-current@FreeBSD.ORG Mon Jul 23 03:11:25 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 844F716A418; Mon, 23 Jul 2007 03:11:25 +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 1770413C467; Mon, 23 Jul 2007 03:11:23 +0000 (UTC) (envelope-from thompsa@FreeBSD.org) Received: by heff.fud.org.nz (Postfix, from userid 1001) id 70B661CC58; Mon, 23 Jul 2007 15:11:19 +1200 (NZST) Date: Mon, 23 Jul 2007 15:11:19 +1200 From: Andrew Thompson To: Doug Rabson Message-ID: <20070723031119.GA5541@heff.fud.org.nz> References: <200707211925.59698.dfr@rabson.org> <20070721210759.GA84580@heff.fud.org.nz> <20070721210837.GB84580@heff.fud.org.nz> <200707220858.12770.dfr@rabson.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="/04w6evG8XlLl3ft" Content-Disposition: inline In-Reply-To: <200707220858.12770.dfr@rabson.org> User-Agent: Mutt/1.5.13 (2006-08-11) Cc: Attilio Rao , 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: Mon, 23 Jul 2007 03:11:25 -0000 --/04w6evG8XlLl3ft Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sun, Jul 22, 2007 at 08:58:12AM +0100, Doug Rabson wrote: > On Saturday 21 July 2007, Andrew Thompson wrote: > > 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: > > > > > > > > > >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. > > I still get a panic but I managed to get more information this time. The > original panic was a WITNESS complaint (not sure why that put it into > the debugger rather than just logging the LOR). It seems I didnt check all the places copyout was used, please test this larger patch. cheers, Andrew --/04w6evG8XlLl3ft 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 23 Jul 2007 02:53:22 -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); } @@ -1106,7 +1103,8 @@ bridge_ioctl_gifs(struct bridge_softc *s struct ifbifconf *bifc = arg; struct bridge_iflist *bif; struct ifbreq breq; - int count, len, error = 0; + char *buf, *outbuf; + int count, buflen, len, error = 0; count = 0; LIST_FOREACH(bif, &sc->sc_iflist, bif_next) @@ -1114,13 +1112,18 @@ bridge_ioctl_gifs(struct bridge_softc *s LIST_FOREACH(bif, &sc->sc_spanlist, bif_next) count++; + buflen = sizeof(breq) * count; if (bifc->ifbic_len == 0) { - bifc->ifbic_len = sizeof(breq) * count; + bifc->ifbic_len = buflen; return (0); } + BRIDGE_UNLOCK(sc); + outbuf = malloc(buflen, M_TEMP, M_WAITOK | M_ZERO); + BRIDGE_LOCK(sc); count = 0; - len = bifc->ifbic_len; + buf = outbuf; + len = min(bifc->ifbic_len, buflen); bzero(&breq, sizeof(breq)); LIST_FOREACH(bif, &sc->sc_iflist, bif_next) { if (len < sizeof(breq)) @@ -1132,10 +1135,9 @@ bridge_ioctl_gifs(struct bridge_softc *s error = bridge_ioctl_gifflags(sc, &breq); if (error) break; - error = copyout(&breq, bifc->ifbic_req + count, sizeof(breq)); - if (error) - break; + memcpy(buf, &breq, sizeof(breq)); count++; + buf += sizeof(breq); len -= sizeof(breq); } LIST_FOREACH(bif, &sc->sc_spanlist, bif_next) { @@ -1146,14 +1148,17 @@ bridge_ioctl_gifs(struct bridge_softc *s sizeof(breq.ifbr_ifsname)); breq.ifbr_ifsflags = bif->bif_flags; breq.ifbr_portno = bif->bif_ifp->if_index & 0xfff; - error = copyout(&breq, bifc->ifbic_req + count, sizeof(breq)); - if (error) - break; + memcpy(buf, &breq, sizeof(breq)); count++; + buf += sizeof(breq); len -= sizeof(breq); } + BRIDGE_UNLOCK(sc); bifc->ifbic_len = sizeof(breq) * count; + error = copyout(outbuf, bifc->ifbic_req, bifc->ifbic_len); + BRIDGE_LOCK(sc); + free(outbuf, M_TEMP); return (error); } @@ -1163,12 +1168,24 @@ bridge_ioctl_rts(struct bridge_softc *sc struct ifbaconf *bac = arg; struct bridge_rtnode *brt; struct ifbareq bareq; - int count = 0, error = 0, len; + char *buf, *outbuf; + int count, error = 0, len, buflen; if (bac->ifbac_len == 0) return (0); - len = bac->ifbac_len; + count = 0; + LIST_FOREACH(brt, &sc->sc_rtlist, brt_list) + count++; + buflen = sizeof(bareq) * count; + + BRIDGE_UNLOCK(sc); + outbuf = malloc(buflen, M_TEMP, M_WAITOK | M_ZERO); + BRIDGE_LOCK(sc); + + count = 0; + buf = outbuf; + len = min(bac->ifbac_len, buflen); bzero(&bareq, sizeof(bareq)); LIST_FOREACH(brt, &sc->sc_rtlist, brt_list) { if (len < sizeof(bareq)) @@ -1184,14 +1201,17 @@ bridge_ioctl_rts(struct bridge_softc *sc bareq.ifba_expire = 0; bareq.ifba_flags = brt->brt_flags; - error = copyout(&bareq, bac->ifbac_req + count, sizeof(bareq)); - if (error) - goto out; + memcpy(buf, &bareq, sizeof(bareq)); count++; + buf += sizeof(bareq); len -= sizeof(bareq); } out: + BRIDGE_UNLOCK(sc); bac->ifbac_len = sizeof(bareq) * count; + error = copyout(outbuf, bac->ifbac_req, bac->ifbac_len); + BRIDGE_LOCK(sc); + free(outbuf, M_TEMP); return (error); } @@ -1453,7 +1473,8 @@ bridge_ioctl_gifsstp(struct bridge_softc struct bridge_iflist *bif; struct bstp_port *bp; struct ifbpstpreq bpreq; - int count, len, error = 0; + char *buf, *outbuf; + int count, buflen, len, error = 0; count = 0; LIST_FOREACH(bif, &sc->sc_iflist, bif_next) { @@ -1461,13 +1482,19 @@ bridge_ioctl_gifsstp(struct bridge_softc count++; } + buflen = sizeof(bpreq) * count; if (bifstp->ifbpstp_len == 0) { - bifstp->ifbpstp_len = sizeof(bpreq) * count; + bifstp->ifbpstp_len = buflen; return (0); } + BRIDGE_UNLOCK(sc); + outbuf = malloc(buflen, M_TEMP, M_WAITOK | M_ZERO); + BRIDGE_LOCK(sc); + count = 0; - len = bifstp->ifbpstp_len; + buf = outbuf; + len = min(bifstp->ifbpstp_len, buflen); bzero(&bpreq, sizeof(bpreq)); LIST_FOREACH(bif, &sc->sc_iflist, bif_next) { if (len < sizeof(bpreq)) @@ -1484,16 +1511,17 @@ bridge_ioctl_gifsstp(struct bridge_softc bpreq.ifbp_design_bridge = bp->bp_desg_pv.pv_dbridge_id; bpreq.ifbp_design_root = bp->bp_desg_pv.pv_root_id; - error = copyout(&bpreq, bifstp->ifbpstp_req + count, - sizeof(bpreq)); - if (error != 0) - break; - + memcpy(buf, &bpreq, sizeof(bpreq)); count++; + buf += sizeof(bpreq); len -= sizeof(bpreq); } + BRIDGE_UNLOCK(sc); bifstp->ifbpstp_len = sizeof(bpreq) * count; + error = copyout(outbuf, bifstp->ifbpstp_req, bifstp->ifbpstp_len); + BRIDGE_LOCK(sc); + free(outbuf, M_TEMP); 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); \ --/04w6evG8XlLl3ft--