[libvirt] [PATCH 7/9] qemu: setup tap devices for promiscLinks='no'

[Laine Stump]

On 11/24/2014 06:41 PM, John Ferlan wrote:
>
> On 11/24/2014 12:48 PM, Laine Stump wrote:
>> In order for the kernel to be able to promiscuous mode on as many
>> ports of a bridge as possible, at most one attached device can have
>> learning and unicast_flood enabled (in practice, this one device is
>> always the physical device that connects the bridge to the real
>> world). If more than one device has those settings enabled, the kernel
>> cannot enable promiscuous mode. Since both settings default to
>> enabled, we need to turn them both off (using
>> virNetDevBridgeSetLearning() and virNetDevBridgeSetUnicastFlood()) for
>> every tap device plugged into a bridge that has promiscLinks='no'.
>>
>> If there is only one device with learning/unicast_flood enabled, then
>> that device has promiscuous mode disabled. If there are *no* devices
>> with learning/unicast_flood enabled (e.g. for a libvirt "route",
>> "nat", or isolated network that has no physical device attached), then
>> all devices will have promiscuous mode disabled.
>>
>> Once we have disabled learning and flooding, any packet that has a
>> destination MAC address not present in the forwarding database (fdb)
>> will be dropped by the bridge, and libvirt is responsible for adding
>> entries to the bridge fdb for the MAC address of each guest
>> interface. That is done with virNetDevBridgeFDBAdd().
>>
>> None of this has any effect for kernels prior to 3.15 (upstream kernel
>> commit 2796d0c648c940b4796f84384fbcfb0a2399db84 "bridge: Automatically
>> manage port promiscuous mode"). Even after that, until kernel 3.17
>> (upstream commit 5be5a2df40f005ea7fb7e280e87bbbcfcf1c2fc0 "bridge: Add
>> filtering support for default_pvid") the following workaround is
>> required in order for untagged traffic to pass (vlan tagged traffic
>> will in any case currently not pass with promiscLinks='no')
>> ---
>>  src/qemu/qemu_command.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index cbdef9c..d3d129a 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -35,6 +35,7 @@
>>  #include "virerror.h"
>>  #include "virfile.h"
>>  #include "virnetdev.h"
>> +#include "virnetdevbridge.h"
>>  #include "virstring.h"
>>  #include "virtime.h"
>>  #include "viruuid.h"
>> @@ -350,6 +351,21 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
>>              virDomainAuditNetDevice(def, net, tunpath, false);
>>              goto cleanup;
>>          }
>> +        if (virDomainNetGetActualPromiscLinks(net) == VIR_TRISTATE_BOOL_NO) {
>> +            /* In order to make as many as possible of the links to a
>> +             * bridge promiscuous/no-flood, we need to turn off
>> +             * learning and unicast_flood, and add an fdb entry to the
>> +             * bridge for this new device.
>> +             */
> In patch 6 we explicitly check vlan filtering being enabled - that's not
> done here, does it need to be?

No. vlan_filtering is a property of the bridge, while "learning" and
"unicast_flood" are properties of the ports of a bridge.

>
>> +            if (virNetDevBridgePortSetLearning(brname, net->ifname, false) < 0)
>> +                goto cleanup;
>> +            if (virNetDevBridgePortSetUnicastFlood(brname, net->ifname, false) < 0)
>> +                goto cleanup;
>> +            if (virNetDevBridgeFDBAdd(&net->mac, net->ifname,
>> +                                      VIR_NETDEVBRIDGE_FDB_FLAG_MASTER |
>> +                                      VIR_NETDEVBRIDGE_FDB_FLAG_TEMP) < 0)
>> +                goto cleanup;
> I'd probably have similar "concerns" as patch 6 with issues that could
> occur if the default for promiscLinks changed and the error path logic.
> Although I see now that restart logic doesn't mean much (hey, it's late
> in the day).
>
> ACK to what's here though. Obviously if you changed the name from my
> patch 3 suggestion, then the BOOL_NO above would change as well.
>