Michael S. Tsirkin
2022-Apr-20 20:17 UTC
virtio-net: Unpermitted usage of virtqueue before virtio driver initialization
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/>
Jason Wang
2022-Apr-21 02:47 UTC
virtio-net: Unpermitted usage of virtqueue before virtio driver initialization
On Thu, Apr 21, 2022 at 4:18 AM Michael S. Tsirkin <mst at redhat.com> wrote:> > 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?Yes, we can do that. Thanks> 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/> >