Virtualization - Apr 2022 - virtio-net: Unpermitted usage of virtqueue before virtio driver initialization

On Wed, Apr 20, 2022 at 04:58:51PM +0200, Maciej Szyma?ski
wrote:
> On 20.04.2022 13:10, Michael S. Tsirkin wrote:
> > On Wed, Apr 20, 2022 at 10:17:27AM +0200, Maciej Szyma?ski wrote:
> > > > > > > > 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.
> > So why isn't dev->features equal to features?
> > 
> I just double verified and found that my device advertises
> VIRTIO_NET_GUEST_TSO4 and VIRTIO_NET_F_GUEST_TSO6 but not
> VIRTIO_NET_F_GUEST_CSUM as mentioned before.
So, your device is out of spec:

VIRTIO_NET_F_GUEST_TSO4 Requires VIRTIO_NET_F_GUEST_CSUM.

And

The device MUST NOT offer a feature which requires another feature which was not
offered.


Is this a production device? Can it be fixed?

> That leads to the following situation :
> 
> in virtio_probe :
> 
>   if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
>     dev->features |= NETIF_F_RXCSUM;
>   if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
>       virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
>     dev->features |= NETIF_F_GRO_HW;
>   if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
>     dev->hw_features |= NETIF_F_GRO_HW;
>
>
> while in netdev_fix_features :
> 
>   if (!(features & NETIF_F_RXCSUM)) {
>     /* NETIF_F_GRO_HW implies doing RXCSUM since every packet
>      * successfully merged by hardware must also have the
>      * checksum verified by hardware.  If the user does not
>      * want to enable RXCSUM, logically, we should disable GRO_HW.
>      */
>     if (features & NETIF_F_GRO_HW) {
>       netdev_dbg(dev, "Dropping NETIF_F_GRO_HW since no RXCSUM
> feature.\n");
>       features &= ~NETIF_F_GRO_HW;
>     }
>   }
> 
> As result dev->features and features passed from
> __netdev_update_features differs exactly in NETIF_F_GRO_HW bit.
> 
> 
> 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/>