在 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 <[email protected]> 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 invokescallback 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 bydeferring 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: [email protected] www.opensynergy.comHandelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616BGeschäftsführer/Managing Director: Regis AdjamahPlease 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.ThanksI 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. QEMUbuilt-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");-- MSTPlease 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/>
_______________________________________________ Virtualization mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/virtualization
