From: Ben Pfaff <blp@nicira.com>
Date: Mon, 23 Apr 2012 16:16:18 +0000 (-0700)
Subject: ofproto: Fix use-after-free error when ports disappear.
X-Git-Url: http://git.onelab.eu/?a=commitdiff_plain;h=c71db22fc0795251d9ddfca8d2789ab584261129;hp=613bded455dfed2755c4f734baf2f8ecb046c9ae;p=sliver-openvswitch.git

ofproto: Fix use-after-free error when ports disappear.

update_port() can delete the port for which it is called, if the underlying
network device has been destroyed, so HMAP_FOR_EACH is unsafe in
ofproto_run().

Less obviously, update_port() can delete unrelated ports.  For example,
suppose that initially device A is port 1 and device B is port 2.  If
update_port("A") runs just after this, then it will ofport_remove() both
ports, then ofport_install() A as the new port 2.

So this commit first assembles a list of ports to update, then updates them
in a separate loop.

Without this commit, running "ovs-dpctl del-dp" while ovs-vswitchd is
running consistently causes a crash for me within a few seconds.

Signed-off-by: Ben Pfaff <blp@nicira.com>
---

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index dd34536fc..766646dba 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -913,8 +913,9 @@ process_port_change(struct ofproto *ofproto, int error, char *devname)
 int
 ofproto_run(struct ofproto *p)
 {
+    struct sset changed_netdevs;
+    const char *changed_netdev;
     struct ofport *ofport;
-    char *devname;
     int error;
 
     error = p->ofproto_class->run(p);
@@ -923,18 +924,31 @@ ofproto_run(struct ofproto *p)
     }
 
     if (p->ofproto_class->port_poll) {
+        char *devname;
+
         while ((error = p->ofproto_class->port_poll(p, &devname)) != EAGAIN) {
             process_port_change(p, error, devname);
         }
     }
 
+    /* Update OpenFlow port status for any port whose netdev has changed.
+     *
+     * Refreshing a given 'ofport' can cause an arbitrary ofport to be
+     * destroyed, so it's not safe to update ports directly from the
+     * HMAP_FOR_EACH loop, or even to use HMAP_FOR_EACH_SAFE.  Instead, we
+     * need this two-phase approach. */
+    sset_init(&changed_netdevs);
     HMAP_FOR_EACH (ofport, hmap_node, &p->ports) {
         unsigned int change_seq = netdev_change_seq(ofport->netdev);
         if (ofport->change_seq != change_seq) {
             ofport->change_seq = change_seq;
-            update_port(p, netdev_get_name(ofport->netdev));
+            sset_add(&changed_netdevs, netdev_get_name(ofport->netdev));
         }
     }
+    SSET_FOR_EACH (changed_netdev, &changed_netdevs) {
+        update_port(p, changed_netdev);
+    }
+    sset_destroy(&changed_netdevs);
 
 
     switch (p->state) {