Jason Wang
2022-Apr-20 08:07 UTC
virtio-net: Unpermitted usage of virtqueue before virtio driver initialization
? 2022/4/20 15:32, Maciej Szyma?ski ??:> On 20.04.2022 08:35, Michael S. Tsirkin wrote: >> 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? >>>> > Yes. I have proprietary virtio-net device which advertises following > guest offload features : > - VIRTIO_NET_F_GUEST_CSUM > - VIRTIO_NET_F_GUEST_TSO4 > - VIRTIO_NET_F_GUEST_TSO6 > - VIRTIO_NET_F_GUEST_UFO > > This feature set passes the condition in virtnet_set_features. > > When I disable guest offloads in my device - virtnet_set_guest_offloads > is not called and driver initialization completes successfully. >>>> >>>>> 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. > Personally I don't like this as well but I think it is only way to keep > the features for deferred set. >>>> >>>>> ???????? /* 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. > Indeed. An original function won't be used. > >>> Yes, I think we need to have a check and only defer the setting when >>> virtio device is not ready. > Right. I have updated my patch to check status to deffer feature set. > That will also solve the problem from comment above. > >>> Thanks >> I think we should first understand how does the issue trigger, >> is this a theoretical or a practical issue. > As mentioned above - practical issue. It does not occur for f.e. QEMU > built-in virtio-net device as it does not advertise guest offload > features.Qemu has that support (enabled by default): ??? DEFINE_PROP_BIT64("ctrl_guest_offloads", VirtIONet, host_features, ??????????????????? VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, true), But Qemu doesn't check DRIVER_OK when processing ctrl vq, that's why we never see any report before. 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 >>>> > > 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/>