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

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.