On Thu, Jan 25, 2024 at 11:33:26 +0100, Michal Prívozník wrote:
> 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
Even if they did, the commit message should contain all the information
needed. If anyone will be reading it later on, cross-referencing
unmentioned discussions becomes very hard.
The commit message should mention the symptom and more explicitly
mention the workaround that it implies. Additionally please also add a
NEWS entry clearly stating the symptom, and workaround.
> 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.
This should be mentioned in the commit message explicitly.
> 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.
Validation callbacks, while allowing this kind of additional checks
always come with a downside. The VM would keep running/defined, but any
subsequent start or 'define' would result in an error, thus it always
should be considered carefully.
>
> > 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.
Okay, I think the placement of the code as you've proposed is okay.
Please improve the commit message and add NEWS
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
