On Thu, Dec 04, 2014 at 03:56:53AM -0500, Laine Stump wrote: > On 12/03/2014 12:02 PM, Daniel P. Berrange wrote: > > On Tue, Dec 02, 2014 at 12:08:30PM -0500, Laine Stump wrote: > >> (Everyone - see the request for opinions/ideas towards the bottom) > >> > >> The idea behind these patches is the following: > >> > >> 1) most virtual machines only have a single MAC address behind each > >> interface, and that MAC address is known by libvirt. > >> > >> 2) If we (i.e. libvirt) manually add an entry to the bridge's > >> forwarding database (fdb) for the MAC address associated with a port > >> on the bridge, we can turn off learning and unicast_flooding for that > >> port. > >> > >> 3) kernels starting with 3.15 (and actually working correctly starting > >> in kernel 3.17) will notice that all of a bridge's ports have flood > >> and learning turned off, and in that case will turn off promiscuous > >> mode on all ports. If all but one of the ports have flood/learning > >> turned off, then promiscuous will be turned off on that port (and left > >> on for all the other ports) > >> > >> 4) When (4) can be done, there is a measurable performance > >> advantage. It can also *kind of* help security, as it will prevent a > >> guest from doing anything useful if it changes its MAC address (but > >> won't prevent the guest from *sending* packets with a spoofed MAC > >> address). > >> > >> NB: These only work with a fixed MAC address, and no vlan tags set in > >> the guest. Support for both of those will be coming. > > So just to be clear on the functional differences between the > > impls > > > > With the current impl > > > > 1. The bridge has a table mapping MAC <-> TAP devices > > which is initially empty > > 2. Whenever a TAP device sends a packet, it causes an > > entry to be added to the MAC <-> TAP table > > 3. When the bridge needs to forward a packet with a MAC > > - If MAC is unknown, it is sent to all TAP devices > > - Else it is send to the specific TAP device > > 4. The physical device is always promiscuous to see all > > traffic on the LAN > > > > An result of point 2, is that if the guest changes its > > MAC address on the fly, it merely needs to send a packet > > and the bridge will learn its new MAC address. That said > > our firewall rules can potentially block this dynamic > > change. > > > > > > With the new impl > > > > 1. The bridge has a table mapping MAC <-> TAP devices > > which is initially empty > > 2. Libvirt tells the bridge what MAC address is associated > > with each TAP device added > > 3. The bridge always forwards the packets to the correct > > TAP device > > 4. The physical device is never promiscuous, it is > > programmed with MACs of all TAP devices > > > > In both cases the guest can send packets with spoofed > > MAC addresses. In the old code these packets would make > > it onto the LAN and it can receive replies back. In the > > new code these packets would make it onto the LAN but > > it will never receive replies back. > > > > > > At a high level the conceptual difference is whether the table > > of MAC addresses & TAP devices is statically defined by libvirt > > or dynamically defined on the fly. > > > >> HERE IS THE REQUEST FOR OPINIONS/IDEAS: > >> > >> This V2 of the patchset addresses several issues brought up by jferlan > >> on the original series, and changes the name of the attribute from: > >> > >> promiscLinks='yes|no' > >> > >> to > >> > >> fdb='learningWithFlood|managed' > >> > >> I'm somewhat more happy with this new naming than the previous but > >> still looking for better ideas. It is closer to describing what the > >> new code really does, but "learningWithFlood" seems a bit long and > >> awkward, while I have been told that "fdb" is too short and > >> unrecognizeable (I will point out that 1) "fdb" is the same name used > >> by iproute2's "bridge" command, and 2) another bridge option, "stp" is > >> also a three letter acronym that will only be recognized by those > >> familiar with configuring an L2 bridge device or watching NASCAR on > >> Saturday afternoons (or whenever it's on - not a fan myself :-)) > > One thing to remember is that libvirt is supposed to be providing a > > higher level configuration language that is independant of specific > > implementations. 'fdb' is terminology specific to the Linux > > bridge implementation, so I don't think that's appropriate to use > > in the XML configuration. > > Good point. I'd been seeing this as a generic term, but I guess it isn't. > > > There is a user visible change in behaviour because this new solution, > > as implemented in this patch series, will no longer allow a guest to > > change its MAC address on the fly. > > > > Of course if QEMU can provide a notification to libvirt when the guest > > changes its MAC address, then libvirt can update the MAC table and so > > re-gain functional equivalance with the old solution. I think we would > > want to be able to turn that on or off though. > > For macvtap interfaces, we can already do that using > trustGuestRxFilters='yes' (a name I don't really like in a location I > don't really like (attribute of toplevel <interface> element), but > couldn't come up with anything better and didn't get any other > suggestions at the time). This has been in libvirt since last month's > release. My thought was that if trustGuestRxFilters is yes and the > interface is on a bridge, we can then honor any changes to mac address. > > (This is where my dislike of my own choice for name of that option comes > in - I can now see a need to allow the guest to change its vlan table or > multicast table and honor those changes, while not allowing it to change > its MAC address, or vice versa; it probably would have been better to > add a new element called <rxFilter> or something, and have a separate > attribute for trusting each piece. It has now been in one release, > though, so I guess we're stuck with it.) > > > > > This all leads me to suggest that we should use the following syntax > > that is indepedant of the particular implementation choice, and instead > > represents the logical behaviour of the feature. > > > > macTable=static|dynamic > > > > The mode of 'static' means that the MAC address table will always > > match MACs recorded in libvirt guest XML. This is the new mode > > that your patches implement > > > > The mode of 'dynamic' means that the MAC address table will be > > able to dynamically update itself as the guest changes MAC addresses. > > In the near term, this will use the traditional bridge learning > > mode. In the long term, if we can get MAC change notifications from > > QEMU, we can switch this over to use the new bridge features. > > I do think that eventually libvirt should make self-management of the > mac table the default, but even then I think we need to retain the > ability to disable it and let the kernel do as it may, just in case > there are compatibility problems if for no other reason. > > Also, I think that enabling/disabling guest-initiated MAC address > changes should be possible at the per-interface level, not (just) > per-network. This can be done with the above-mentioned > trustGuestRxFilters attribute (which can be set in a single interface, > or for a network or network portgroup). > > So I like naming the attribute "macTable", as that is more generic, but > I'm not sure about the values "static" and "dynamic", because "dynamic" > could describe two different things (dynamically changed by the kernel > via learning and flood, or dynamically changed via libvirt being an > omniscient hypervisor god that knows what to plug into the table), and > "static" is something that can already be described via setting > trustGuestRxFilters='no' combined with a "let libvirt control the mac > table" option. > > Maybe the problem is that you're providing a good name based entirely on > the differences in behavior the guests can see, but what I'm looking for > is a way to control how that behavior is provided, so more of a backend > thing. (Note that if the "backend" is the bridge module, "static" isn't > possible now and never will be, while if the backend is explicit > management by libvirt, then "dynamic" isn't currently available, but > will be in the future).
So perhaps more along the lines of macTableManager="kernel|libvirt" Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
