[ovs-dev] [brcompatd 7/8] ovs-brcompatd: Properly fix race between device destruction and insertion.

Ethan Jackson ethan at nicira.com
Wed Jun 8 00:01:43 UTC 2011


Looks Good.

Ethan

On Mon, Jun 6, 2011 at 12:41, Ben Pfaff <blp at nicira.com> wrote:
> 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 files changed, 20 insertions(+), 48 deletions(-)
>
> diff --git a/vswitchd/ovs-brcompatd.c b/vswitchd/ovs-brcompatd.c
> index 52cd93f..458aead 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);
>
> --
> 1.7.4.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>



More information about the dev mailing list