[libvirt] [PATCHv2 6/9] network: setup bridge devices for fdb='managed'

[Laine Stump]

On 12/02/2014 04:48 PM, John Ferlan wrote:
>
> On 12/02/2014 12:08 PM, Laine Stump wrote:
>> When the bridge device for a network has fdb='managed' the intent is
>> to configure the bridge to opportunistically turn off promiscuous mode
>> and/or flood mode for as many ports/links on the bridge as possible,
>> thus improving performance and security. The setup required for the
>> bridge itself is:
>>
>> 1) set the "vlan_filtering" property of the bridge device to 1.  2) If
>> the bridge has a "Dummy" tap device used to set a fixed MAC address on
>> the bridge, turn off learning and unicast_flood on this tap (this is
>> needed even though this tap is never IFF_UP, because the kernel
>> ignores the IFF_UP flag of other devices when using their settings to
>> automatically decide whether or not to turn off promiscuous mode for
>> any attached device).
>>
>> This is done both for libvirt-created/managed bridges, and for bridges
>> that are created by the host system config.
>>
>> There is no attempt to turn vlan_filtering off when destroying the
>> network because in the case of a libvirt-created bridge, the bridge is
>> about to be destroyed anyway, and in the case of a system bridge, if
>> the other devices attached to the bridge could operate properly before
>> destroying libvirt's network object, they will continue to operate
>> properly (this is similar to the way that libvirt will enable
>> ip_forwarding whenever a routed/natted network is started, but will
>> never attempt to disable it if they are stopped).
>> ---
>>
>> Changes from V1: only in attribute/value names
>>
>>  src/network/bridge_driver.c | 52 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 52 insertions(+)
>>
> ACK still applies given naming
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index 3299cd6..9b3dc58 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -1912,6 +1912,27 @@ networkAddAddrToBridge(virNetworkObjPtr network,
>>      return 0;
>>  }
>>  
>> +
>> +static int
>> +networkStartHandleFDBMode(virNetworkObjPtr network, const char *macTapIfName)
>> +{
>> +    const char *brname = network->def->bridge;
>> +
>> +    if (brname &&
>> +        network->def->fdb == VIR_NETWORK_BRIDGE_FDB_MANAGED) {
>> +        if (virNetDevBridgeSetVlanFiltering(brname, true) < 0)
>> +            return -1;
> Given this particular path where we're being managed, we set
> vlan_filtering, but macTapIfName == NULL (for who knows what reason),
> then we have the condition where we've set vlan_filtering, but not
> learning/flood.

It may seem that way on casual perusal, but no. This function is called
from two different places: 1) for networks using a bridge created by
libvirt (aka "virtual/managed network"), which also creates a special
"dummy tap device" that is created purely for the purpose of setting a
stable MAC address for the bridge[*], and 2) for networks that are just
referencing a bridge already created by the host system config, usually
attached to a physical ethernet, but having no dummy tap device (because
the bridge will always get the MAC address of the physical ethernet). In
order to preserve the necessary condition of "all ports except the
physdev have learning and flood disabled", when a dummy tap device
exists (macTapIfname), we must also set learning and flood for that
device (just as we do for every other tap device when it is created just
prior to passing it to a guest).

Even in the case where macTapIfname == NULL (because there *isn't* any
"macTap") all the guest tap devices will still have their learning/flood
disabled, so there isn't any condition where the bridge's vlan_filtering
is set but the ports don't get learning/flood disabled.

>  Thus, it seems plausible that a filterVLAN='yes|no'
> (from my comments in .0) might be possible.

Some time in the future someone may come up with a valid reason to do
that, but I haven't seen it yet, and I don't want to add any unnecessary
knobs.

> So, if that's not a desirable state, then perhaps macTapIfName needs to
> be in the outer if, leaving:
>
> if (virNetDevBridgeSetVlanFiltering(brname, true) < 0 ||
>     virNetDevBridgePortSetLearning(brname, macTapIfName, false) < 0 ||
>     virNetDevBridgePortSetUnicastFlood(brname, macTapIfName, false) < 0)
>     return -1;
>
> If you were to use the .0 suggestion to have filterVLAN, then you'd have
> to decide whether forcing filterVLAN on even though it was perhaps set
> to 'no' in the XML would be something that 'should' be done...

*If* there is ever a valid use case for setting vlan_filtering on the
bridge while leaving learning/flood on, then at that time we can
consider which setting trumps which. In the meantime I don't think the
current setup will preclude making a later decision in either direction.


[*] there is a long history behind the necessity for the dummy tap
device, including at least one BZ filed and resolved a long time ago:

  https://bugzilla.redhat.com/show_bug.cgi?id=609463

(fixed with commit 5754dbd5). A short description is that each host
bridge has one "builtin" port that is automatically presented to the
host as a netdev, but doesn't have any MAC address; instead, the
bridge's netdev will always acquire the numerically lowest MAC address
of all other netdevs attached to the bridge; if the currently lowest
numbered netdev is deleted, the MAC address of the bridge (which will
also be the MAC address of the default route for all the guests) will
change; this in turn will trigger systems like MS Windows to believe
that the network has changed, and bring up a "new network detected"
wizard. To prevent this, we create a tap device (that is never IFF_UP)
with a MAC address guaranteed to be lower than the MAC address of any
guest tap device.