[PATCH] network: drop use of dummy tap device in bridges
Laine Stump
laine at redhat.com
Thu Sep 3 14:56:25 UTC 2020
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 at 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 at 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 at vyatta.com>
> Signed-off-by: David S. Miller <davem at 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 at 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 at 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 at 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 at 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 at 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 at 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;
>
More information about the libvir-list
mailing list