Re: [PATCH] network: drop use of dummy tap device in bridges

[Laine Stump <laine@xxxxxxxxxx>]

On 9/3/20 9:34 AM, Daniel P. Berrangé wrote:
> A long time ago we introduced a dummy tap device (e.g. virbr0-nic) that
> we attached to the bridge device created for virtual networks:
>
>    commit 5754dbd56d4738112a86776c09e810e32f7c3224
>    Author: Laine Stump <laine@xxxxxxxxxx>
>    Date:   Wed Feb 9 03:28:12 2011 -0500
>
>      Give each virtual network bridge its own fixed MAC address
>
> This was a hack to workaround a Linux kernel bug where it would not
> honour any attempt to set a MAC address on a bridge. Instead the
> bridge would adopt the numerically lowest MAC address of all NICs
> attached to the bridge. This lead to the MAC addrss of the bridge
> changing over time as NICs were attached/detached.
>
> The Linux bug was actually fixed 3 years before the libvirt
> workaround was added in:
>
>    commit 92c0574f11598c8036f81e27d2e8bdd6eed7d76d
>    Author: Stephen Hemminger <shemminger@xxxxxxxxxx>
>    Date:   Tue Jun 17 16:10:06 2008 -0700
>
>      bridge: make bridge address settings sticky
>
>      Normally, the bridge just chooses the smallest mac address as the
>      bridge id and mac address of bridge device. But if the administrator
>      has explictly set the interface address then don't change it.
>
>      Signed-off-by: Stephen Hemminger <shemminger@xxxxxxxxxx>
>      Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
>
> but libvirt needed to support RHEL-5 kernels at that time, so
> none the less added the workaround.
>
> We have long since dropped support for RHEL-5 vintage distros,
> so there's no reason to keep the dummy tap device for the purpose
> of setting the bridge MAC address.
>
> Later the dummy TAP device was used for a second purpose related
> to IPv6 DAD (Duplicate Address Detection) in:
>
>    commit db488c79173b240459c7754f38c3c6af9b432970
>    Author: Benjamin Cama <benoar@xxxxxxxx>
>    Date:   Wed Sep 26 21:02:20 2012 +0200
>
>      network: fix dnsmasq/radvd binding to IPv6 on recent kernels
>
> This was again dealing with a regression in the Linux kernel, where
> if there were no devices attached to the bridge in the UP state,
> IPv6 DAD would not be performed. The virbr0-nic was attached but
> in the DOWN state, so the above libvirt fix tenporarily brought
> the NIC online. The Linux commit causing the problem was in v2.6.38
>
>    commit 1faa4356a3bd89ea11fb92752d897cff3a20ec0e
>    Author: stephen hemminger <shemminger@xxxxxxxxxx>
>    Date:   Mon Mar 7 08:34:06 2011 +0000
>
>      bridge: control carrier based on ports online
>
> A short while later Linux was tweaked so that DAD would still occur
> if the bridge had no attached devices at all in 3.1:
>
>    commit b64b73d7d0c480f75684519c6134e79d50c1b341
>    Author: stephen hemminger <shemminger@xxxxxxxxxx>
>    Date:   Mon Oct 3 18:14:45 2011 +0000
>
>      bridge: leave carrier on for empty bridge
>
> IOW, the only reason we need the DAD hack of bringing virbr0-nic
> online is because virbr0-nic exists. Once it doesn't exist, then
> we hit the "empty bridge" case which works in Linux.

Ooh, cool! Having it used for DAD was the only reason I didn't remove 
virbrX-nic after I put in the patch to set the bridge MAC directly:


commit 13ec827052fcd79a4350f499aab5f4aa20ea83fa
Author: Laine Stump <laine@xxxxxxxxxx>
Date:   Thu Oct 17 21:12:30 2019 -0400

    util: set bridge device MAC address explicitly during
    virNetDevBridgeCreate

(I even comment about this, but didn't dig down to learn that it was no 
longer required for DAD - I just thought that was the way bridges + DAD 
were designed to work).

Nice detective work! Did some problem lead you to this, or did you just 
get tired of always seeing that useless device hanging around?

Reviewed-by: Laine Stump <laine@xxxxxxxxxx>

(I give this assuming the veracity of all the info you've cited.)

One thing that comes to mind - do we still need to wait for DAD to 
finish in networkStartNetworkVirtual(). I think maybe we were waiting 
for that only so that we could set the dummy interface offline (there 
might be some other reason I'm not thinking of though).

> We can rely on distros having Linux kernel >= 3.1, so both things
> that the virbr0-nic are doing are redundant.
>
> Fixes https://gitlab.com/libvirt/libvirt/-/issues/53
> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> ---
>   src/network/bridge_driver.c | 58 +++++--------------------------------
>   1 file changed, 8 insertions(+), 50 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index b016d86b9f..5c00befc16 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2247,8 +2247,7 @@ networkAddAddrToBridge(virNetworkObjPtr obj,
>   
>   
>   static int
> -networkStartHandleMACTableManagerMode(virNetworkObjPtr obj,
> -                                      const char *macTapIfName)
> +networkStartHandleMACTableManagerMode(virNetworkObjPtr obj)
>   {
>       virNetworkDefPtr def = virNetworkObjGetDef(obj);
>       const char *brname = def->bridge;
> @@ -2257,12 +2256,6 @@ networkStartHandleMACTableManagerMode(virNetworkObjPtr obj,
>           def->macTableManager == VIR_NETWORK_BRIDGE_MAC_TABLE_MANAGER_LIBVIRT) {
>           if (virNetDevBridgeSetVlanFiltering(brname, true) < 0)
>               return -1;
> -        if (macTapIfName) {
> -            if (virNetDevBridgePortSetLearning(brname, macTapIfName, false) < 0)
> -                return -1;
> -            if (virNetDevBridgePortSetUnicastFlood(brname, macTapIfName, false) < 0)
> -                return -1;
> -        }
>       }
>       return 0;
>   }
> @@ -2330,10 +2323,8 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
>       virErrorPtr save_err = NULL;
>       virNetworkIPDefPtr ipdef;
>       virNetDevIPRoutePtr routedef;
> -    g_autofree char *macTapIfName = NULL;
>       virMacMapPtr macmap;
>       g_autofree char *macMapFile = NULL;
> -    int tapfd = -1;
>       bool dnsmasqStarted = false;
>       bool devOnline = false;
>       bool firewalRulesAdded = false;
> @@ -2360,29 +2351,6 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
>       if (virNetDevBridgeCreate(def->bridge, &def->mac) < 0)
>           return -1;
>   
> -    if (def->mac_specified) {
> -        /* To set a mac for the bridge, we need to define a dummy tap
> -         * device, set its mac, then attach it to the bridge. As long
> -         * as its mac address is lower than any other interface that
> -         * gets attached, the bridge will always maintain this mac
> -         * address.
> -         */
> -        macTapIfName = networkBridgeDummyNicName(def->bridge);
> -        if (!macTapIfName)
> -            goto error;
> -        /* Keep tun fd open and interface up to allow for IPv6 DAD to happen */
> -        if (virNetDevTapCreateInBridgePort(def->bridge,
> -                                           &macTapIfName, &def->mac,
> -                                           NULL, NULL, &tapfd, 1, NULL, NULL,
> -                                           VIR_TRISTATE_BOOL_NO,
> -                                           NULL, def->mtu, NULL,
> -                                           VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE |
> -                                           VIR_NETDEV_TAP_CREATE_IFUP |
> -                                           VIR_NETDEV_TAP_CREATE_PERSIST) < 0) {
> -            goto error;
> -        }
> -    }
> -
>       if (!(macMapFile = virMacMapFileName(driver->dnsmasqStateDir,
>                                            def->bridge)) ||
>           !(macmap = virMacMapNew(macMapFile)))
> @@ -2426,7 +2394,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
>               goto error;
>       }
>   
> -    if (networkStartHandleMACTableManagerMode(obj, macTapIfName) < 0)
> +    if (networkStartHandleMACTableManagerMode(obj) < 0)
>           goto error;
>   
>       /* Bring up the bridge interface */
> @@ -2482,15 +2450,6 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
>       if (v6present && networkWaitDadFinish(obj) < 0)
>           goto error;
>   
> -    /* DAD has finished, dnsmasq is now bound to the
> -     * bridge's IPv6 address, so we can set the dummy tun down.
> -     */
> -    if (tapfd >= 0) {
> -        if (virNetDevSetOnline(macTapIfName, false) < 0)
> -            goto error;
> -        VIR_FORCE_CLOSE(tapfd);
> -    }
> -
>       if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0)
>           goto error;
>   
> @@ -2514,16 +2473,11 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
>           def->forward.type != VIR_NETWORK_FORWARD_OPEN)
>           networkRemoveFirewallRules(def);
>   
> -    if (macTapIfName) {
> -        VIR_FORCE_CLOSE(tapfd);
> -        ignore_value(virNetDevTapDelete(macTapIfName, NULL));
> -    }
>       virNetworkObjUnrefMacMap(obj);
>   
>       ignore_value(virNetDevBridgeDelete(def->bridge));
>   
>       virErrorRestore(&save_err);
> -    /* coverity[leaked_handle] - 'tapfd' is not leaked */
>       return -1;
>   }
>   
> @@ -2555,9 +2509,13 @@ networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver,
>       if (dnsmasqPid > 0)
>           kill(dnsmasqPid, SIGTERM);
>   
> +    /* We no longer create a dummy NIC, but if we've upgraded
> +     * from old libvirt, we still need to delete any dummy NIC
> +     * that might exist. Keep this logic around for a while...
> +     */
>       if (def->mac_specified) {
>           g_autofree char *macTapIfName = networkBridgeDummyNicName(def->bridge);
> -        if (macTapIfName)
> +        if (macTapIfName && virNetDevExists(macTapIfName))
>               ignore_value(virNetDevTapDelete(macTapIfName, NULL));
>       }
>   
> @@ -2597,7 +2555,7 @@ networkStartNetworkBridge(virNetworkObjPtr obj)
>       if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0)
>           goto error;
>   
> -    if (networkStartHandleMACTableManagerMode(obj, NULL) < 0)
> +    if (networkStartHandleMACTableManagerMode(obj) < 0)
>           goto error;
>   
>       return 0;