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

Thu Oct 2 15:58:25 UTC 2014

On 10/02/2014 10:19 AM, Laine Stump wrote:
> 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).
I spent some time back when I first started looking at this trying to figure
out how to use netlink to retrieve the data, but was unable to find much
doc on how to do it, so I ultimately settled on /proc.  I think the code
can be structured in such a way that the /proc code can eventually be
replaced with netlink.
>
> 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.
That is how my code is currently structured.  I just need to change it
to use your structure.
>
> 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.
Will do.
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>