[ovs-dev] [port-renumbering 7/8] bridge: Reconfigure in single pass.

[Ben Pfaff]

On Wed, Dec 11, 2013 at 04:09:57PM -0800, Jarno Rajahalme wrote:
> 
> On Dec 11, 2013, at 3:14 PM, Ben Pfaff <blp at nicira.com> 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);
+            }
         }
     }