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

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(a)redhat.com&gt; >> 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(a)vyatta.com&gt; >> 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(a)vyatta.com&gt; >> Signed-off-by: David S. Miller <davem(a)davemloft.net&gt; >> >> 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(a)dolka.fr&gt; >> 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(a)vyatta.com&gt; >> 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(a)vyatta.com&gt; >> 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(a)redhat.com&gt; > 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(a)redhat.com&gt; > > (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...