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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Apr 20, 2022 at 08:57:18PM +0200, Maciej Szymański wrote:
> On 20.04.2022 19:54, Michael S. Tsirkin wrote:
> > 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?
> The problem seems to be more complicated. In fact
> VIRTIO_NET_F_GUEST_CSUM is offered by our device, but during feature
> negotiation it is being dropped.
> This most likely does not happen when we use MMIO, but for some reason
> happens in QEMU for VHOST_USER + PCI.
> I need to investigate this more deeply...


I don't see where linux would drop it. I suspect it's dropped between
QEMU and vhost user. I'd say let's fix it in the device first.
We can next consider marking vqs broken before device is ready -
Jason what do you think?
Finally, we can add code to avoid acking dependent features
if the feature they depend on has not been negotiated - doing
so is also a spec violation after all.


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