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>
> ---
> 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