Re: virtio-net: Unpermitted usage of virtqueue before virtio driver initialization

* Re: virtio-net: Unpermitted usage of virtqueue before virtio driver initialization
       [not found] <c0c17b91-747e-ab58-83e5-b6f7dfa55e75@opensynergy.com>
@ 2022-04-19 15:03 ` Michael S. Tsirkin
  2022-04-20  3:07   ` Jason Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2022-04-19 15:03 UTC (permalink / raw)
  To: Maciej Szymański; +Cc: Willem de Bruijn, virtualization

On Tue, Apr 19, 2022 at 04:12:31PM +0200, Maciej Szymański wrote:
> Hello,
> 
> I've found a problem in virtio-net driver.
> If virtio-net backend device advertises guest offload features, there is
> an unpermitted usage of control virtqueue before driver is initialized.
> According to VIRTIO specification 2.1.2 :
> "The device MUST NOT consume buffers or send any used buffer
> notifications to the driver before DRIVER_OK."

Right.

> During an initialization, driver calls register_netdevice which invokes
> callback function virtnet_set_features from __netdev_update_features.
> If guest offload features are advertised by the device,
> virtnet_set_guest_offloads is using virtnet_send_command to write and
> read from VQ.
> That leads to initialization stuck as device is not permitted yet to use VQ.

Hmm so we have this:

        if ((dev->features ^ features) & NETIF_F_GRO_HW) {
                if (vi->xdp_enabled)
                        return -EBUSY;

                if (features & NETIF_F_GRO_HW)
                        offloads = vi->guest_offloads_capable;
                else
                        offloads = vi->guest_offloads_capable &
                                   ~GUEST_OFFLOAD_GRO_HW_MASK;

                err = virtnet_set_guest_offloads(vi, offloads);
                if (err)
                        return err;
                vi->guest_offloads = offloads;
        }

which I guess should have prevented virtnet_set_guest_offloads from ever running.

From your description it sounds like you have observed this
in practice, right?

> I have attached a patch for kernel 5.18-rc3 which fixes the problem by
> deferring feature set after virtio driver initialization.
> 
> Best Regards,
> 
> --
> Maciej Szymański
> Senior Staff Engineer
> 
> OpenSynergy GmbH
> Rotherstr. 20, 10245 Berlin
> 
> Phone:    +49 30 60 98 54 0 -86
> Fax:      +49 30 60 98 54 0 -99
> E-Mail:   maciej.szymanski@opensynergy.com
> 
> www.opensynergy.com
> 
> Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B
> Geschäftsführer/Managing Director: Regis Adjamah
> 
> Please mind our privacy notice<https://www.opensynergy.com/datenschutzerklaerung/privacy-notice-for-business-partners-pursuant-to-article-13-of-the-general-data-protection-regulation-gdpr/> pursuant to Art. 13 GDPR. // Unsere Hinweise zum Datenschutz gem. Art. 13 DSGVO finden Sie hier.<https://www.opensynergy.com/de/datenschutzerklaerung/datenschutzhinweise-fuer-geschaeftspartner-gem-art-13-dsgvo/>

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 87838cb..a44462d 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -264,6 +264,8 @@ struct virtnet_info {
>         unsigned long guest_offloads;
>         unsigned long guest_offloads_capable;
>  
> +       netdev_features_t features;
> +

I don't much like how we are forced to keep a copy of features
here :( At least pls add a comment explaining what's going on,
who owns this etc.

>         /* failover when STANDBY feature enabled */
>         struct failover *failover;
>  };
> @@ -2976,6 +2978,15 @@ static int virtnet_get_phys_port_name(struct net_device *dev, char *buf,
>  
>  static int virtnet_set_features(struct net_device *dev,
>                                 netdev_features_t features)
> +{
> +       struct virtnet_info *vi = netdev_priv(dev);
> +       vi->features = features;
> +
> +       return 0;
> +}

Looks like this breaks changing features after initialization -
these will never be propagated to hardware now.

> +
> +static int virtnet_set_features_deferred(struct net_device *dev,
> +                               netdev_features_t features)
>  {
>         struct virtnet_info *vi = netdev_priv(dev);
>         u64 offloads;
> @@ -3644,6 +3655,13 @@ static int virtnet_probe(struct virtio_device *vdev)
>  
>         virtio_device_ready(vdev);
>  
> +       /* Deferred feature set after device ready */
> +       err = virtnet_set_features_deferred(dev, vi->features);

It seems that if this is called e.g. for a device without a CVQ and
there are things that actually need to change then it will BUG_ON.

> +       if (err) {
> +               pr_debug("virtio_net: set features failed\n");
> +               goto free_unregister_netdev;
> +       }
> +
>         err = virtnet_cpu_notif_add(vi);
>         if (err) {
>                 pr_debug("virtio_net: registering cpu notifier failed\n");
> 

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 9+ messages in thread