On 1/25/24 10:52, Peter Krempa wrote:
> On Thu, Jan 25, 2024 at 10:42:13 +0100, Michal Privoznik wrote:
>> After guest is started, or we are reconnecting to already running
>> one (after daemon restart), qemuProcessRefreshRxFilters() is
>> called to refresh rx-filters (basically MAC addresses of guest
>> NICs) as they might have changed while we were not running (for
>> the case when reconnecting to an already running guest), or we
>> need to enable them by running a command (for freshly started
>> guest - see processNicRxFilterChangedEvent()).
>>
>> Now, our XML parser allowed trustGuestRxFilters attribute for all
>> types and models of <interface/> while in reality, only virtio
>> model can see MAC address changes.
>>
>> Fixes: 060d4c83ef436cf56abfad51a4d64c39448e199d
>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>> ---
>> src/qemu/qemu_process.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 3563ad215c..a736846588 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -7958,6 +7958,12 @@ qemuProcessRefreshRxFilters(virDomainObj *vm,
>> if (!virDomainNetGetActualTrustGuestRxFilters(def))
>> continue;
>>
>> + /* rx-filters are supported only for virtio macvtaps */
>> + if (def->model != VIR_DOMAIN_NET_MODEL_VIRTIO ||
>> + virDomainNetGetActualType(def) != VIR_DOMAIN_NET_TYPE_DIRECT) {
>> + continue;
>> + }
>> +
>> if (qemuDomainSyncRxFilter(vm, def, asyncJob) < 0)
>> return -1;
>> }
>
> So how did this failure manifest itself? The commit message doesn't
> mention that.
>
Ah, I incorrectly assumed people followed my discussion on the users
list. Basically, if you have a vNIC with trustGuestRxFilters="yes", then
after domain is started, or daemon is restarted
qemuProcessRefreshRxFilters() is called and since QEMU returns error for
'query-rx-filters' for everything else than a virtio macvtap, users are
unable to either start a domain OR the daemon is unable to reconnect to
a running domain.
Now, trustGuestRxFilters makes sense only for virtio macvtap and nothing
else, but we can't reject such configurations, can we? I vaguely recall
something about validate callbacks and throwing an error there.
> I'm trying to understand the reasoning to see if this check should be
> inside qemuDomainSyncRxFilter, so that it doesn't get forgotten the next
> time it will be used.
>
Yeah, I've struggled with this too. Basically, qemuDomainSyncRxFilter()
is called from just two places:
1) from qemuProcessRefreshRxFilters() - aforementioned case on domain
startup/reconnect,
2) from processNicRxFilterChangedEvent() - when responding to
NIC_RX_FILTER_CHANGED event emitted by QEMU. I doubt QEMU would emit
this event and then failed to reply to query-rx-filters monitor command.
There's a foot note though - this event and command are a bit different
to the usual events/commands. To avoid guest spoofing us with events,
the event is emitted once and no more. It's only after we issue
query-rx-filters monitor command that delivery of this event (for given
vNIC) is enabled again. That's the reason why we have to do it in
reconnect phase - an event might have been emitted while the daemon was
not running and since the daemon wasn't running it couldn't execute the
monitor command; thus no event (for that specific vNIC) would be emitted
ever again.
Michal
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
