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@redhat.com> --- 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