diff options
author | Ben Pfaff <blp@nicira.com> | 2012-04-23 09:16:18 -0700 |
---|---|---|
committer | Ben Pfaff <blp@nicira.com> | 2012-04-23 13:58:45 -0700 |
commit | c71db22fc0795251d9ddfca8d2789ab584261129 (patch) | |
tree | da64be45d6291c20685580f417d508f2c3792dde | |
parent | 613bded455dfed2755c4f734baf2f8ecb046c9ae (diff) |
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>
-rw-r--r-- | ofproto/ofproto.c | 18 |
1 files changed, 16 insertions, 2 deletions
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index dd34536f..766646db 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) { |