On Wed, Dec 11, 2013 at 05:55:35PM -0800, Ben Pfaff wrote:
> On Wed, Dec 11, 2013 at 04:09:57PM -0800, Jarno Rajahalme wrote:
> >
> > On Dec 11, 2013, at 3:14 PM, Ben Pfaff <[email protected]> wrote:
> >
> > > On Wed, Dec 11, 2013 at 02:17:13PM -0800, Jarno Rajahalme wrote:
> > >> While testing the code I accidentally requested a change in the
> > >> interface type (to dummy), like this:
> > >>
> > >> # ovs-vsctl add-br br0
> > >> # ovs-vsctl add-port br0 p1 -- set Interface p1 type=internal
> > >> # ovs-vsctl -- set Interface p1 type=dummy
> > >>
> > >> Running OVS without dummy enabled seg faults:
> > >
> > > I couldn't reproduce this, can you try to get me a backtrace or run
> > > under valgrind?
> >
>
> [...]
>
> > ==33664== Invalid read of size 2
> > ==33664== at 0x40A1C2: bridge_reconfigure (bridge.c:1568)
> > ==33664== by 0x406BAC: bridge_run (bridge.c:2289)
> > ==33664== by 0x40E498: main (ovs-vswitchd.c:118)
> > ==33664== Address 0x48 is not stack'd, malloc'd or (recently) free'd
>
> [...]
>
> > The offending line is:
> >
> > if (iface->ofp_port == OFPP_LOCAL) {
> >
> > Seems like iface is NULL (if offset of odp_port == 0x48) in this case.
>
> Thanks.
>
> I figured out how to reproduce it.
>
> I folded this in and it fixes the problem for me. For you too?
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index ed16860..5dd7752 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -653,6 +653,7 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
>
> OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) {
> struct iface *iface;
> + struct port *port;
>
> iface = iface_lookup(br, ofproto_port.name);
> if (!iface) {
> @@ -679,7 +680,11 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
> continue;
>
> delete:
> + port = iface->port;
> iface_destroy(iface);
> + if (list_is_empty(&port->ifaces)) {
> + port_destroy(port);
> + }
> del = add_ofp_port(ofproto_port.ofp_port, del, &n, &allocated);
> }
>
> @@ -3102,7 +3107,12 @@ port_del_ifaces(struct port *port)
> /* Get rid of deleted interfaces. */
> LIST_FOR_EACH_SAFE (iface, next, port_elem, &port->ifaces) {
> if (!sset_contains(&new_ifaces, iface->name)) {
> + struct port *port = iface->port;
> +
> iface_destroy(iface);
> + if (list_is_empty(&port->ifaces)) {
> + port_destroy(port);
> + }
> }
> }
>
But introduced a new segfault because 'iface' could be null in the first
case. I folded in the following additional patch as a combined fix and
cleanup.
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 8267636..37f5812 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -657,7 +657,6 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) {
ofp_port_t requested_ofp_port;
struct iface *iface;
- struct port *port;
iface = iface_lookup(br, ofproto_port.name);
if (!iface) {
@@ -721,11 +720,7 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
continue;
delete:
- port = iface->port;
iface_destroy(iface);
- if (list_is_empty(&port->ifaces)) {
- port_destroy(port);
- }
del = add_ofp_port(ofproto_port.ofp_port, del, &n, &allocated);
}
@@ -3131,6 +3126,8 @@ bridge_configure_dp_desc(struct bridge *br)
/* Port functions. */
+static void iface_destroy__(struct iface *);
+
static struct port *
port_create(struct bridge *br, const struct ovsrec_port *cfg)
{
@@ -3167,12 +3164,7 @@ port_del_ifaces(struct port *port)
/* Get rid of deleted interfaces. */
LIST_FOR_EACH_SAFE (iface, next, port_elem, &port->ifaces) {
if (!sset_contains(&new_ifaces, iface->name)) {
- struct port *port = iface->port;
-
iface_destroy(iface);
- if (list_is_empty(&port->ifaces)) {
- port_destroy(port);
- }
}
}
@@ -3191,7 +3183,7 @@ port_destroy(struct port *port)
}
LIST_FOR_EACH_SAFE (iface, next, port_elem, &port->ifaces) {
- iface_destroy(iface);
+ iface_destroy__(iface);
}
hmap_remove(&br->ports, &port->hmap_node);
@@ -3413,7 +3405,7 @@ iface_get_type(const struct ovsrec_interface *iface,
}
static void
-iface_destroy(struct iface *iface)
+iface_destroy__(struct iface *iface)
{
if (iface) {
struct port *port = iface->port;
@@ -3437,6 +3429,19 @@ iface_destroy(struct iface *iface)
}
}
+static void
+iface_destroy(struct iface *iface)
+{
+ if (iface) {
+ struct port *port = iface->port;
+
+ iface_destroy__(iface);
+ if (list_is_empty(&port->ifaces)) {
+ port_destroy(port);
+ }
+ }
+}
+
static struct iface *
iface_lookup(const struct bridge *br, const char *name)
{
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
