Re: virtio-net: Unpermitted usage of virtqueue before virtio driver initialization

[Jason Wang <jasowang@redhat.com>]

在 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@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@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/>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization