Re: [PATCH] qemu_process: Skip over non-virtio NIC models when refreshing rx-filter

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(a)redhat.com&gt; >> --- >> 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.