From 87096e18f7c1012a47799d9f23be3bfec0174600 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Mon, 23 Apr 2012 09:16:18 -0700 Subject: [PATCH] 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 --- ofproto/ofproto.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index a77063a93..9057215ae 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) { -- 2.43.0