[libvirt] [PATCH 6/9] network: setup bridge devices for promiscLinks='no'

[Laine Stump]

On 11/24/2014 06:22 PM, John Ferlan wrote:
>
> On 11/24/2014 12:48 PM, Laine Stump wrote:
>> When the bridge device for a network has promiscLinks='no', the intent
>> is to setup 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 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).
>> ---
>>  src/network/bridge_driver.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index bc8da79..f2564c3 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -1893,6 +1893,28 @@ networkAddAddrToBridge(virNetworkObjPtr network,
>>      return 0;
>>  }
>>  
>> +
>> +static int
>> +networkStartHandlePromiscLinks(virNetworkObjPtr network, const char *macTapIfName)
>> +{
>> +    const char *brname = network->def->bridge;
>> +
>> +    /* promiscuous mode won't be turned off unless vlan filtering is enabled. */
>> +    if (brname &&
>> +        network->def->promiscLinks == VIR_TRISTATE_BOOL_NO) {
>> +        if (virNetDevBridgeSetVlanFiltering(brname, true) < 0)
> Could this return failure if we don't have the right kernel? Then do we
> really want to kill the whole startup processing?

Yes, and yes. If they've requested it and we can't comply, then we need
to fail.

> It almost seems as if
> the failure case here should be some kind of VIR_WARN or VIR_INFO,
> return 0 and do nothing.
>
> I guess what I'm most worried about is the "future" if the decision is
> to quietly change the default of 'promiscLinks' (or whatever name is
> used) and we get here from networkStartNetworkBridge which in a libvirtd
> restart has what effect on something that was running before?

If we ever do that, it will be done in a "quietly fall back" way. I
don't want to build in too much "intelligence" that ends up never
getting used (and in the meantime makes the code (more) confusing).

>
> You would be correct in pointing out that for the current design and
> assumptions this is the right course of action.  Someone set promisc=no
> and there was a failure, so we need to fail too.
>
>
>> +            return -1;
>> +        if (macTapIfName) {
>> +            if (virNetDevBridgePortSetLearning(brname, macTapIfName, false) < 0)
>> +                return -1;
>> +            if (virNetDevBridgePortSetUnicastFlood(brname, macTapIfName, false) < 0)
>> +                return -1;
> Ah - although networkStartNetworkBridge says to restore things on
> failure, since we'll be deleting the bridge if either of these fail so
> it doesn't matter.

Correct.

>
> The BOOL_NO logic could change if you took my suggestion from patch3
>
> ACK, but this is ultimately where changing the default would be affected
> and perhaps we need to somehow note that.
>