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 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.
>
> Yes, I think we need to have a check and only defer the setting when
> virtio device is not ready.
>
> Thanks
I 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
> >
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
