[PATCH] qemu_process: Skip over non-virtio NIC models when refreshing rx-filter
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; } -- 2.43.0
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. 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.
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
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@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
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
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa