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 >
Michael S. Tsirkin
2022-Apr-20 06:35 UTC
virtio-net: Unpermitted usage of virtqueue before virtio driver initialization
On Wed, Apr 20, 2022 at 11:07:00AM +0800, Jason Wang wrote:> 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. > > ThanksI think we should first understand how does the issue trigger, is this a theoretical or a practical issue.> > > > > + > > > +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 > >