Re: [PATCH RFC] virtio-net: Fix VLAN filter table initialization timing

["Michael S. Tsirkin" <mst@redhat.com>]

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>