qemu_process: Skip over non-virtio NIC models when refreshing rx-filter

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

Posted by Michal Privoznik 3 months ago

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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

Posted by Peter Krempa 3 months ago

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.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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

Posted by Michal Prívozník 3 months ago

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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

Posted by Peter Krempa 3 months ago

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org