From e1f406a32b785670c72cca0c8b8acd95ea474416 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 3 Jun 2011 10:46:45 -0700 Subject: [PATCH] ovs-brcompatd: Properly fix race between device destruction and insertion. I believe that this actually fixes the race described in the comments, whereas I'm pretty sure that the old way still left a race window. --- vswitchd/ovs-brcompatd.c | 68 ++++++++++++---------------------------- 1 file changed, 20 insertions(+), 48 deletions(-) diff --git a/vswitchd/ovs-brcompatd.c b/vswitchd/ovs-brcompatd.c index 52cd93fc..458aeadf 100644 --- a/vswitchd/ovs-brcompatd.c +++ b/vswitchd/ovs-brcompatd.c @@ -1096,6 +1096,26 @@ brc_recv_update(struct ovsdb_idl *idl) goto error; } + /* Service all pending network device notifications before executing the + * command. This is very important to avoid a race in a scenario like the + * following, which is what happens with XenServer Tools version 5.0.0 + * during boot of a Windows VM: + * + * 1. Create tap1.0 and vif1.0. + * 2. Delete tap1.0. + * 3. Delete vif1.0. + * 4. Re-create vif1.0. + * + * We must process the network device notification from step 3 before we + * process the brctl command from step 4. If we process them in the + * reverse order, then step 4 completes as a no-op but step 3 then deletes + * the port that was just added. + * + * (XenServer Tools 5.5.0 does not exhibit this behavior, and neither does + * a VM without Tools installed at all.) + */ + rtnetlink_link_notifier_run(); + switch (genlmsghdr->cmd) { case BRC_GENL_C_DP_ADD: handle_bridge_cmd(idl, ovs, buffer, true); @@ -1167,54 +1187,6 @@ netdev_changed_cb(const struct rtnetlink_link_change *change, void *idl_) return; } - if (netdev_exists(port_name)) { - /* A network device by that name exists even though the kernel - * told us it had disappeared. Probably, what happened was - * this: - * - * 1. Device destroyed. - * 2. Notification sent to us. - * 3. New device created with same name as old one. - * 4. ovs-brcompatd notified, removes device from bridge. - * - * There's no a priori reason that in this situation that the - * new device with the same name should remain in the bridge; - * on the contrary, that would be unexpected. *But* there is - * one important situation where, if we do this, bad things - * happen. This is the case of XenServer Tools version 5.0.0, - * which on boot of a Windows VM cause something like this to - * happen on the Xen host: - * - * i. Create tap1.0 and vif1.0. - * ii. Delete tap1.0. - * iii. Delete vif1.0. - * iv. Re-create vif1.0. - * - * (XenServer Tools 5.5.0 does not exhibit this behavior, and - * neither does a VM without Tools installed at all.) - * - * Steps iii and iv happen within a few seconds of each other. - * Step iv causes /etc/xensource/scripts/vif to run, which in - * turn calls ovs-cfg-mod to add the new device to the bridge. - * If step iv happens after step 4 (in our first list of - * steps), then all is well, but if it happens between 3 and 4 - * (which can easily happen if ovs-brcompatd has to wait to - * lock the configuration file), then we will remove the new - * incarnation from the bridge instead of the old one! - * - * So, to avoid this problem, we do nothing here. This is - * strictly incorrect except for this one particular case, and - * perhaps that will bite us someday. If that happens, then we - * will have to somehow track network devices by ifindex, since - * a new device will have a new ifindex even if it has the same - * name as an old device. - */ - VLOG_INFO("kernel reported network device %s removed but " - "a device by that name exists (XS Tools 5.0.0?)", - port_name); - return; - } - VLOG_INFO("network device %s destroyed, removing from bridge %s", port_name, br_name); -- 2.30.2