Michael S. Tsirkin
2022-Apr-19 15:03 UTC
virtio-net: Unpermitted usage of virtqueue before virtio driver initialization
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 thisin 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 at 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
Jason Wang
2022-Apr-20 03:07 UTC
virtio-net: Unpermitted usage of virtqueue before virtio driver initialization
On Tue, Apr 19, 2022 at 11:03 PM Michael S. Tsirkin <mst at 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 at 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 >