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

[John Ferlan]

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?

> +            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.

John
> +        }
>      } else {
>          if (qemuCreateInBridgePortWithHelper(cfg, brname,
>                                               &net->ifname,
>