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: [email protected]
>
> 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
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
