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

[Jason Wang <jasowang@redhat.com>]

On Tue, Apr 19, 2022 at 11:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> 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.

Yes, I think we need to have a check and only defer the setting when
virtio device is not ready.

Thanks

>
> > +
> > +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