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

Daniel P. Berrangé posted 1 patch 3 years, 6 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200903133423.3575562-1-berrange@redhat.com
src/network/bridge_driver.c | 58 +++++--------------------------------
1 file changed, 8 insertions(+), 50 deletions(-)
[PATCH] network: drop use of dummy tap device in bridges
Posted by Daniel P. Berrangé 3 years, 6 months ago
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@redhat.com>
  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@vyatta.com>
  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@vyatta.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

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@dolka.fr>
  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@vyatta.com>
  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@vyatta.com>
  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.

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@redhat.com>
---
 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;
-- 
2.25.4

Re: [PATCH] network: drop use of dummy tap device in bridges
Posted by Laine Stump 3 years, 6 months ago
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@redhat.com>
>    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@vyatta.com>
>    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@vyatta.com>
>      Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> 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@dolka.fr>
>    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@vyatta.com>
>    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@vyatta.com>
>    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@redhat.com>
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@redhat.com>

(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@redhat.com>
> ---
>   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;
> 

Re: [PATCH] network: drop use of dummy tap device in bridges
Posted by Daniel P. Berrangé 3 years, 6 months ago
On Thu, Sep 03, 2020 at 10:56:25AM -0400, Laine Stump wrote:
> 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@redhat.com>
> >    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@vyatta.com>
> >    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@vyatta.com>
> >      Signed-off-by: David S. Miller <davem@davemloft.net>
> > 
> > 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@dolka.fr>
> >    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@vyatta.com>
> >    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@vyatta.com>
> >    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@redhat.com>
> 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?

The bug report from a user mentioned in the commit message prompted it

> 
> Reviewed-by: Laine Stump <laine@redhat.com>
> 
> (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).

I'm not sure, and I don't currently have an IPv6 deployment to test
this, so I'm loathe to touch it myself. Perhaps someone else can do the
change and validate it though...


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] network: drop use of dummy tap device in bridges
Posted by Laine Stump 3 years, 6 months ago
On 9/3/20 10:58 AM, Daniel P. Berrangé wrote:
> On Thu, Sep 03, 2020 at 10:56:25AM -0400, Laine Stump wrote:
>> 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@redhat.com>
>>>     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@vyatta.com>
>>>     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@vyatta.com>
>>>       Signed-off-by: David S. Miller <davem@davemloft.net>
>>>
>>> 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@dolka.fr>
>>>     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@vyatta.com>
>>>     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@vyatta.com>
>>>     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@redhat.com>
>> 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?
> 
> The bug report from a user mentioned in the commit message prompted it

Sigh. Only one cup of coffee so far...

> 
>>
>> Reviewed-by: Laine Stump <laine@redhat.com>
>>
>> (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).
> 
> I'm not sure, and I don't currently have an IPv6 deployment to test
> this, so I'm loathe to touch it myself. Perhaps someone else can do the
> change and validate it though...

Well, I have IPv6 running on my internal network, and my ISP connection 
has IPv6 connectivity, and I usually have at least one libvirt network 
using IPv6. But I'm never really sure what to look for to make sure that 
something is working for everyone, and not just me :-)

Based on you saying you don't have an IPv6 deployment to test, I guess 
maybe I should install a build with this patch and match I can do some 
rudimentary things with it...