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@xxxxxxxxxxxxx>
> Signed-off-by: Akihiko Odaki <odaki@xxxxxxxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxxx>
