hw/net/virtio-net.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
The VLAN filter table was not being properly initialized when the driver
sets the DRIVER_OK status bit, causing incorrect VLAN filtering behavior
that could allow unintended VLAN packets to pass through. Correct the
table initialization timing to fix the issue.
Problem
-------
The expected initial state of the table depends on feature negotiation:
With VIRTIO_NET_F_CTRL_VLAN:
The table must be empty in accordance with the specification.
Without VIRTIO_NET_F_CTRL_VLAN:
The table should be filled to permit all VLAN traffic.
Prior to commit ("virtio-net: do not reset vlan filtering at
set_features"), virtio_net_set_features() always initialized the VLAN
table. That commit changed the behavior to skip initialization when
VIRTIO_NET_F_CTRL_VLAN was negotiated, assuming the table would be
properly cleared during reset and remain stable.
However, this assumption breaks when a driver renegotiates features:
1. Initial negotiation without VIRTIO_NET_F_CTRL_VLAN (table filled)
2. Renegotiation with VIRTIO_NET_F_CTRL_VLAN (table will not be cleared)
The problem was exacerbated by commit 0caed25cd171 ("virtio: Call
set_features during reset"), which triggered virtio_net_set_features()
during reset, exposing the bug whenever VIRTIO_NET_F_CTRL_VLAN was
negotiated after a reset.
Solution
--------
Fix by moving VLAN table initialization to virtio_net_set_status(),
which sets the DRIVER_OK status bit. This ensures proper table
initialization regardless of feature negotiation sequence.
Fixes: 06b636a1e2ad ("virtio-net: do not reset vlan filtering at set_features")
Reported-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
Not tested.
Konstantin, please see if this patch fixes your workload.
---
hw/net/virtio-net.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 221252e00a50a46033d7ec8d18936e7c8196a6ca..6d2a67471e570e1e9b4b4fb5338d87c30e23ae36 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -389,6 +389,12 @@ static int virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
int i;
uint8_t queue_status;
+ if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
+ !(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+ bool vlan = virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VLAN);
+ memset(n->vlans, vlan ? 0 : 0xff, MAX_VLAN >> 3);
+ }
+
virtio_net_vnet_endian_status(n, status);
virtio_net_vhost_status(n, status);
@@ -998,10 +1004,6 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
vhost_net_save_acked_features(nc->peer);
}
- if (!virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN)) {
- memset(n->vlans, 0xff, MAX_VLAN >> 3);
- }
-
if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY)) {
qapi_event_send_failover_negotiated(n->netclient_name);
qatomic_set(&n->failover_primary_hidden, false);
@@ -3990,7 +3992,6 @@ static void virtio_net_reset(VirtIODevice *vdev)
memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
- memset(n->vlans, 0, MAX_VLAN >> 3);
/* Flush any async TX */
for (i = 0; i < n->max_queue_pairs; i++) {
---
base-commit: f0737158b483e7ec2b2512145aeab888b85cc1f7
change-id: 20250713-vlan-8c107a65ad91
Best regards,
--
Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>On 13-Jul-25 06:52, Akihiko Odaki wrote: > Konstantin, please see if this patch fixes your workload. Yes, it does. It delays vlans[] initialization until virtio_net_set_status(0xf) which later also calls vhost_net_start() which is where the NIC is actually programmed.
Tested this patch with virtio-net regression tests, everything works fine. Tested-by: Lei Yang <leiyang@redhat.com> On Mon, Jul 14, 2025 at 5:34 AM Konstantin Shkolnyy <kshk@linux.ibm.com> wrote: > > On 13-Jul-25 06:52, Akihiko Odaki wrote: > > Konstantin, please see if this patch fixes your workload. > Yes, it does. It delays vlans[] initialization until > virtio_net_set_status(0xf) which later also calls vhost_net_start() > which is where the NIC is actually programmed. > >
On Sun, Jul 13, 2025 at 08:52:43PM +0900, Akihiko Odaki wrote:
> The VLAN filter table was not being properly initialized when the driver
> sets the DRIVER_OK status bit, causing incorrect VLAN filtering behavior
> that could allow unintended VLAN packets to pass through. Correct the
> table initialization timing to fix the issue.
>
> Problem
> -------
>
> The expected initial state of the table depends on feature negotiation:
>
> With VIRTIO_NET_F_CTRL_VLAN:
> The table must be empty in accordance with the specification.
> Without VIRTIO_NET_F_CTRL_VLAN:
> The table should be filled to permit all VLAN traffic.
>
> Prior to commit ("virtio-net: do not reset vlan filtering at
> set_features"), virtio_net_set_features() always initialized the VLAN
> table. That commit changed the behavior to skip initialization when
> VIRTIO_NET_F_CTRL_VLAN was negotiated, assuming the table would be
> properly cleared during reset and remain stable.
>
> However, this assumption breaks when a driver renegotiates features:
> 1. Initial negotiation without VIRTIO_NET_F_CTRL_VLAN (table filled)
> 2. Renegotiation with VIRTIO_NET_F_CTRL_VLAN (table will not be cleared)
>
> The problem was exacerbated by commit 0caed25cd171 ("virtio: Call
> set_features during reset"), which triggered virtio_net_set_features()
> during reset, exposing the bug whenever VIRTIO_NET_F_CTRL_VLAN was
> negotiated after a reset.
>
> Solution
> --------
>
> Fix by moving VLAN table initialization to virtio_net_set_status(),
> which sets the DRIVER_OK status bit. This ensures proper table
> initialization regardless of feature negotiation sequence.
>
> Fixes: 06b636a1e2ad ("virtio-net: do not reset vlan filtering at set_features")
> Reported-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
A bit worried about legacy drivers that might use device before
DRIVER_OK, is that a problem?
> ---
> Not tested.
>
> Konstantin, please see if this patch fixes your workload.
> ---
> hw/net/virtio-net.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 221252e00a50a46033d7ec8d18936e7c8196a6ca..6d2a67471e570e1e9b4b4fb5338d87c30e23ae36 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -389,6 +389,12 @@ static int virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> int i;
> uint8_t queue_status;
>
> + if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> + !(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> + bool vlan = virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VLAN);
> + memset(n->vlans, vlan ? 0 : 0xff, MAX_VLAN >> 3);
> + }
> +
> virtio_net_vnet_endian_status(n, status);
> virtio_net_vhost_status(n, status);
>
> @@ -998,10 +1004,6 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
> vhost_net_save_acked_features(nc->peer);
> }
>
> - if (!virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN)) {
> - memset(n->vlans, 0xff, MAX_VLAN >> 3);
> - }
> -
> if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY)) {
> qapi_event_send_failover_negotiated(n->netclient_name);
> qatomic_set(&n->failover_primary_hidden, false);
> @@ -3990,7 +3992,6 @@ static void virtio_net_reset(VirtIODevice *vdev)
> memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
> memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
> qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> - memset(n->vlans, 0, MAX_VLAN >> 3);
>
> /* Flush any async TX */
> for (i = 0; i < n->max_queue_pairs; i++) {
>
> ---
> base-commit: f0737158b483e7ec2b2512145aeab888b85cc1f7
> change-id: 20250713-vlan-8c107a65ad91
>
> Best regards,
> --
> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
© 2016 - 2025 Red Hat, Inc.