[libvirt] [PATCH 0/6] handle NIC_RX_FILTER_CHANGED events from qemu

[Laine Stump]

On 10/01/2014 03:36 PM, Tony Krowiak wrote:
> On 09/30/2014 03:47 PM, Laine Stump wrote:
>> On 09/30/2014 02:28 PM, Tony Krowiak wrote:
>>> On 09/24/2014 05:50 AM, Laine Stump wrote:
>>>> These patches set up an event handler for qemu's NIC_RX_FILTER_CHANGED
>>>> event, which is sent whenever a guest makes a change to a network
>>>> device's unicast/multicast filter, vlan table, or MAC address.
>>>>
>>>> The handler checks if it is appropriate to respond to the
>>>> NIC_RX_FILTER_CHANGED event (based on device type and configuration)
>>>> and takes appropriate action. Currently it checks if the guest
>>>> interface has been configured with trustGuestRxFilters='yes' (defaults
>>>> to 'no' for security reasons), and if the host side device is
>>>> macvtap. If so, and the MAC address on the guest has changed, the MAC
>>>> address of the macvtap device is changed to match.
>>>>
>>>> The result of this is that networking from the guest will continue to
>>>> work if the mac address of a macvtap-connected network device is
>>>> changed from within the guest, as long as trustGuestRxFilters='yes'
>>>> (previously changing the MAC address in the guest would break
>>>> networking).
>>>>
>>>> I still need to add code to compare the old and new unicast and
>>>> multicast lists and program the filters in the macvtap to match the
>>>> guest, and to check for a non-empty vlan table and handle that
>>>> (currently that means just setting promiscuous mode on the macvtap),
>>>> but that can come in a followup series.
>>> I was very interested in this patch set because I developed a set of
>>> patches to respond to the NIC_RX_FILTER_CHANGED event.  I completed
>>> the patch set several weeks ago and have been awaiting completion of
>>> our internal review before submitting them to this mailing list.
>>> Apparently you beat me to the punch.  I have code that compares
>>> the old and new multicast lists and synchronizes the macvtap filters
>>> with the guest's.  I can modify my patches to integrate this function
>>> into what you have provided with this patch set.  Would that be
>>> agreeable?
>> Since I've just started working on exactly that, yes :-)
>>
>> What I'd started was a function virNetDevGetRxFilter(ifname, filter) in
>> virnetdev.c which would do for the host-side tap/macvtap device what
>> qemuMonitorQueryRxFilter() does for the guest's interface - retrieve the
>> current unicast/multicast/vlan tables & modes (using code from
>> iproute2's "bridge" command as a guide to write equivalent libnl-based
>> code) and return them in a fully-populated  virNetDevRxFilter object
>> (that's why I defined that struct in virnetdev.h even though I've so far
>> used it only in the qemu driver), which would be called from the event
>> handler; the event handler would compare the two virNetDevRxFilter
>> objects (the one from the host-side device and the one from the
>> guest-side device) and issue the necessary commands to make everything
>> match (well, actually what I've been told is that in the case of vlans,
>> if the guest has a non-empty vlan table we currently have to just set
>> the macvtap to promiscuous mode).
>>
>> It sounds like you're only interested in the multicast list, so if you
>> wanted to fill in enough to make it do that, your code could be used as
>> a model to do the unicast list (which seems to be empty most of the time
>> anyway; as I usually operate above that layer, I'm truthfully not
>> exactly sure when it's even used).
> I am interested in a complete solution, however; I chose to ignore the
> unicast
> list for the time being for reasons similar to yours.  I wrote code to
> synchronize the VLAN table, but subsequently learned that there were
> problems
> with VLAN on macvtap, so I took that code out of the patch set.
>
> I used the "ip maddr show" command in iproute2 as a model for
> acquiring the
> multicast list.  This command reads the /proc/net/dev_mcast file on
> the host to
> get the current multicast MAC address list for the macvtap device. To
> synchronize the macvtap device's multicast list, I compared the list from
> /proc/net/dev_mcast with that returned from the query_rx_filter
> command and used
> the SIOCADDMULTI and SIOCDELMULTI ioctl's add and delete multicast MAC
> addresses
> as needed.  I did not do anything with the mode (state) values as I
> have yet
> to figure out how to do that.
>
> I also wrote code to synchronize the following interface flags:
> * promiscuous
> * multicast
> * allmulti
> * broadcast
>
> I can integrate what I have with your infrastructure including:
> * Create a virNetDevGetRxFilter(ifname, filter) function in
> virnetdev.c that will
>   populate the following fields in the filter:
>     * name
>     * mac
>     * promiscuous
>     * broadcastAllowed
>     * multicast.table
>     * multicast.nTable
>     * vlan.table
>     * vlan.nTable
> * Implement the event handler code to:
>     * compare the two virNetDevRxFilter objects and issue the
> necessary commands to
>       synchronize the multicast MAC address lists
>     * set the promiscuous mode if the guest has a non-empty vlan table
>     * compare the device flags and synchronize their values
>
> If you think this would be a worthwhile endeavor, I've got your
> patches installed and can proceed.

Sure. I would prefer using netlink over /proc, but I have yet to figure
out the code to retrieve the multicast list in that manner (see my above
mention of iproute's "bridge" command - interesting that two different
parts of the same package retrieve the info in different ways).

It would be very useful if virNetDevGetRxFilter() called a function for
each one of the components (e.g. virNetDevGetMcastList(),
virNetDevGetPromiscuous(), virNetDevAddMcast(), virNetDevDelMCast(),
etc) so that the implementation of each of these functions could be
easily changed to use netlink (or whatever was appropriate for any given
platform) based on some configure-time conditionals.

I'm about to post V2 of my patches, which have minor changes, so you may
want to use those as a base instead of the originals.