Date: Mon, 23 Jul 2007 15:11:19 +1200 From: Andrew Thompson <thompsa@FreeBSD.org> To: Doug Rabson <dfr@rabson.org> Cc: Attilio Rao <attilio@freebsd.org>, current@freebsd.org Subject: Re: if_bridge crash Message-ID: <20070723031119.GA5541@heff.fud.org.nz> In-Reply-To: <200707220858.12770.dfr@rabson.org> References: <200707211925.59698.dfr@rabson.org> <20070721210759.GA84580@heff.fud.org.nz> <20070721210837.GB84580@heff.fud.org.nz> <200707220858.12770.dfr@rabson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--/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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070723031119.GA5541>