From: Ben Pfaff Date: Mon, 23 Apr 2012 16:16:18 +0000 (-0700) Subject: ofproto: Fix use-after-free error when ports disappear. X-Git-Tag: sliver-openvswitch-0.1-1~59 X-Git-Url: http://git.onelab.eu/?a=commitdiff_plain;h=7436ed8071a045f265ab3e06c5d7b0d5d678db1c;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 --- diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index f9343069b..cb46d26df 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -1027,8 +1027,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); @@ -1037,18 +1038,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) { case S_OPENFLOW: