Michael S. Tsirkin
2020-Feb-13 10:00 UTC
[PATCH] virtio: Work around frames incorrectly marked as gso
On Wed, Feb 12, 2020 at 05:38:09PM +0000, Anton Ivanov wrote:> > > On 11/02/2020 10:37, Michael S. Tsirkin wrote: > > On Tue, Feb 11, 2020 at 07:42:37AM +0000, Anton Ivanov wrote: > > > On 11/02/2020 02:51, Jason Wang wrote: > > > > > > > > On 2020/2/11 ??12:55, Anton Ivanov wrote: > > > > > > > > > > > > > > > On 09/12/2019 10:48, anton.ivanov at cambridgegreys.com wrote: > > > > > > From: Anton Ivanov <anton.ivanov at cambridgegreys.com> > > > > > > > > > > > > Some of the frames marked as GSO which arrive at > > > > > > virtio_net_hdr_from_skb() have no GSO_TYPE, no > > > > > > fragments (data_len = 0) and length significantly shorter > > > > > > than the MTU (752 in my experiments). > > > > > > > > > > > > This is observed on raw sockets reading off vEth interfaces > > > > > > in all 4.x and 5.x kernels I tested. > > > > > > > > > > > > These frames are reported as invalid while they are in fact > > > > > > gso-less frames. > > > > > > > > > > > > This patch marks the vnet header as no-GSO for them instead > > > > > > of reporting it as invalid. > > > > > > > > > > > > Signed-off-by: Anton Ivanov <anton.ivanov at cambridgegreys.com> > > > > > > --- > > > > > > ? include/linux/virtio_net.h | 8 ++++++-- > > > > > > ? 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > > > > > > index 0d1fe9297ac6..d90d5cff1b9a 100644 > > > > > > --- a/include/linux/virtio_net.h > > > > > > +++ b/include/linux/virtio_net.h > > > > > > @@ -112,8 +112,12 @@ static inline int > > > > > > virtio_net_hdr_from_skb(const struct sk_buff *skb, > > > > > > ????????????? hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; > > > > > > ????????? else if (sinfo->gso_type & SKB_GSO_TCPV6) > > > > > > ????????????? hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; > > > > > > -??????? else > > > > > > -??????????? return -EINVAL; > > > > > > +??????? else { > > > > > > +??????????? if (skb->data_len == 0) > > > > > > +??????????????? hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE; > > > > > > +??????????? else > > > > > > +??????????????? return -EINVAL; > > > > > > +??????? } > > > > > > ????????? if (sinfo->gso_type & SKB_GSO_TCP_ECN) > > > > > > ????????????? hdr->gso_type |= VIRTIO_NET_HDR_GSO_ECN; > > > > > > ????? } else > > > > > > > > > > > > > > > > ping. > > > > > > > > > > > > > Do you mean gso_size is set but gso_type is not? Looks like a bug > > > > elsewhere. > > > > > > > > Thanks > > > > > > > > > > > Yes. > > > > > > I could not trace it where it is coming from. > > > > > > I see it when doing recvmmsg on raw sockets in the UML vector network > > > drivers. > > > > > > > I think we need to find the culprit and fix it there, lots of other things > > can break otherwise. > > Just printing out skb->dev->name should do the trick, no? > > The printk in virtio_net_hdr_from_skb says NULL. > > That is probably normal for a locally originated frame. > > I cannot reproduce this with network traffic by the way - it happens only if the traffic is locally originated on the host. > > A,OK so is it code in __tcp_transmit_skb that sets gso_size to non-null when gso_type is 0?> > > > > > > -- > > > Anton R. Ivanov > > > Cambridgegreys Limited. Registered in England. Company Number 10273661 > > > https://www.cambridgegreys.com/ > > > > > > _______________________________________________ > > linux-um mailing list > > linux-um at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-um > > > > -- > Anton R. Ivanov > Cambridgegreys Limited. Registered in England. Company Number 10273661 > https://www.cambridgegreys.com/
Anton Ivanov
2020-Feb-13 11:12 UTC
[PATCH] virtio: Work around frames incorrectly marked as gso
On 13/02/2020 10:00, Michael S. Tsirkin wrote:> On Wed, Feb 12, 2020 at 05:38:09PM +0000, Anton Ivanov wrote: >> >> >> On 11/02/2020 10:37, Michael S. Tsirkin wrote: >>> On Tue, Feb 11, 2020 at 07:42:37AM +0000, Anton Ivanov wrote: >>>> On 11/02/2020 02:51, Jason Wang wrote: >>>>> >>>>> On 2020/2/11 ??12:55, Anton Ivanov wrote: >>>>>> >>>>>> >>>>>> On 09/12/2019 10:48, anton.ivanov at cambridgegreys.com wrote: >>>>>>> From: Anton Ivanov <anton.ivanov at cambridgegreys.com> >>>>>>> >>>>>>> Some of the frames marked as GSO which arrive at >>>>>>> virtio_net_hdr_from_skb() have no GSO_TYPE, no >>>>>>> fragments (data_len = 0) and length significantly shorter >>>>>>> than the MTU (752 in my experiments). >>>>>>> >>>>>>> This is observed on raw sockets reading off vEth interfaces >>>>>>> in all 4.x and 5.x kernels I tested. >>>>>>> >>>>>>> These frames are reported as invalid while they are in fact >>>>>>> gso-less frames. >>>>>>> >>>>>>> This patch marks the vnet header as no-GSO for them instead >>>>>>> of reporting it as invalid. >>>>>>> >>>>>>> Signed-off-by: Anton Ivanov <anton.ivanov at cambridgegreys.com> >>>>>>> --- >>>>>>> ? include/linux/virtio_net.h | 8 ++++++-- >>>>>>> ? 1 file changed, 6 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h >>>>>>> index 0d1fe9297ac6..d90d5cff1b9a 100644 >>>>>>> --- a/include/linux/virtio_net.h >>>>>>> +++ b/include/linux/virtio_net.h >>>>>>> @@ -112,8 +112,12 @@ static inline int >>>>>>> virtio_net_hdr_from_skb(const struct sk_buff *skb, >>>>>>> ????????????? hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; >>>>>>> ????????? else if (sinfo->gso_type & SKB_GSO_TCPV6) >>>>>>> ????????????? hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; >>>>>>> -??????? else >>>>>>> -??????????? return -EINVAL; >>>>>>> +??????? else { >>>>>>> +??????????? if (skb->data_len == 0) >>>>>>> +??????????????? hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE; >>>>>>> +??????????? else >>>>>>> +??????????????? return -EINVAL; >>>>>>> +??????? } >>>>>>> ????????? if (sinfo->gso_type & SKB_GSO_TCP_ECN) >>>>>>> ????????????? hdr->gso_type |= VIRTIO_NET_HDR_GSO_ECN; >>>>>>> ????? } else >>>>>>> >>>>>> >>>>>> ping. >>>>>> >>>>> >>>>> Do you mean gso_size is set but gso_type is not? Looks like a bug >>>>> elsewhere. >>>>> >>>>> Thanks >>>>> >>>>> >>>> Yes. >>>> >>>> I could not trace it where it is coming from. >>>> >>>> I see it when doing recvmmsg on raw sockets in the UML vector network >>>> drivers. >>>> >>> >>> I think we need to find the culprit and fix it there, lots of other things >>> can break otherwise. >>> Just printing out skb->dev->name should do the trick, no? >> >> The printk in virtio_net_hdr_from_skb says NULL. >> >> That is probably normal for a locally originated frame. >> >> I cannot reproduce this with network traffic by the way - it happens only if the traffic is locally originated on the host. >> >> A, > > OK so is it code in __tcp_transmit_skb that sets gso_size to non-null > when gso_type is 0?It does look like that, but I cannot see it when reading it :(> > >>> >>> >>>> -- >>>> Anton R. Ivanov >>>> Cambridgegreys Limited. Registered in England. Company Number 10273661 >>>> https://www.cambridgegreys.com/ >>> >>> >>> _______________________________________________ >>> linux-um mailing list >>> linux-um at lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-um >>> >> >> -- >> Anton R. Ivanov >> Cambridgegreys Limited. Registered in England. Company Number 10273661 >> https://www.cambridgegreys.com/ > >-- Anton R. Ivanov Cambridgegreys Limited. Registered in England. Company Number 10273661 https://www.cambridgegreys.com/
Michael S. Tsirkin
2020-Feb-13 13:12 UTC
[PATCH] virtio: Work around frames incorrectly marked as gso
On Thu, Feb 13, 2020 at 11:12:45AM +0000, Anton Ivanov wrote:> > > On 13/02/2020 10:00, Michael S. Tsirkin wrote: > > On Wed, Feb 12, 2020 at 05:38:09PM +0000, Anton Ivanov wrote: > > > > > > > > > On 11/02/2020 10:37, Michael S. Tsirkin wrote: > > > > On Tue, Feb 11, 2020 at 07:42:37AM +0000, Anton Ivanov wrote: > > > > > On 11/02/2020 02:51, Jason Wang wrote: > > > > > > > > > > > > On 2020/2/11 ??12:55, Anton Ivanov wrote: > > > > > > > > > > > > > > > > > > > > > On 09/12/2019 10:48, anton.ivanov at cambridgegreys.com wrote: > > > > > > > > From: Anton Ivanov <anton.ivanov at cambridgegreys.com> > > > > > > > > > > > > > > > > Some of the frames marked as GSO which arrive at > > > > > > > > virtio_net_hdr_from_skb() have no GSO_TYPE, no > > > > > > > > fragments (data_len = 0) and length significantly shorter > > > > > > > > than the MTU (752 in my experiments). > > > > > > > > > > > > > > > > This is observed on raw sockets reading off vEth interfaces > > > > > > > > in all 4.x and 5.x kernels I tested. > > > > > > > > > > > > > > > > These frames are reported as invalid while they are in fact > > > > > > > > gso-less frames. > > > > > > > > > > > > > > > > This patch marks the vnet header as no-GSO for them instead > > > > > > > > of reporting it as invalid. > > > > > > > > > > > > > > > > Signed-off-by: Anton Ivanov <anton.ivanov at cambridgegreys.com> > > > > > > > > --- > > > > > > > > ? include/linux/virtio_net.h | 8 ++++++-- > > > > > > > > ? 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > > > > > > > > index 0d1fe9297ac6..d90d5cff1b9a 100644 > > > > > > > > --- a/include/linux/virtio_net.h > > > > > > > > +++ b/include/linux/virtio_net.h > > > > > > > > @@ -112,8 +112,12 @@ static inline int > > > > > > > > virtio_net_hdr_from_skb(const struct sk_buff *skb, > > > > > > > > ????????????? hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; > > > > > > > > ????????? else if (sinfo->gso_type & SKB_GSO_TCPV6) > > > > > > > > ????????????? hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; > > > > > > > > -??????? else > > > > > > > > -??????????? return -EINVAL; > > > > > > > > +??????? else { > > > > > > > > +??????????? if (skb->data_len == 0) > > > > > > > > +??????????????? hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE; > > > > > > > > +??????????? else > > > > > > > > +??????????????? return -EINVAL; > > > > > > > > +??????? } > > > > > > > > ????????? if (sinfo->gso_type & SKB_GSO_TCP_ECN) > > > > > > > > ????????????? hdr->gso_type |= VIRTIO_NET_HDR_GSO_ECN; > > > > > > > > ????? } else > > > > > > > > > > > > > > > > > > > > > > ping. > > > > > > > > > > > > > > > > > > > Do you mean gso_size is set but gso_type is not? Looks like a bug > > > > > > elsewhere. > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > Yes. > > > > > > > > > > I could not trace it where it is coming from. > > > > > > > > > > I see it when doing recvmmsg on raw sockets in the UML vector network > > > > > drivers. > > > > > > > > > > > > > I think we need to find the culprit and fix it there, lots of other things > > > > can break otherwise. > > > > Just printing out skb->dev->name should do the trick, no? > > > > > > The printk in virtio_net_hdr_from_skb says NULL. > > > > > > That is probably normal for a locally originated frame. > > > > > > I cannot reproduce this with network traffic by the way - it happens only if the traffic is locally originated on the host. > > > > > > A, > > > > OK so is it code in __tcp_transmit_skb that sets gso_size to non-null > > when gso_type is 0? > > It does look like that, but I cannot see it when reading it :(dump skb pointer at the two locations and see whether it matches :)> > > > > > > > > > > > > > > > > > -- > > > > > Anton R. Ivanov > > > > > Cambridgegreys Limited. Registered in England. Company Number 10273661 > > > > > https://www.cambridgegreys.com/ > > > > > > > > > > > > _______________________________________________ > > > > linux-um mailing list > > > > linux-um at lists.infradead.org > > > > http://lists.infradead.org/mailman/listinfo/linux-um > > > > > > > > > > -- > > > Anton R. Ivanov > > > Cambridgegreys Limited. Registered in England. Company Number 10273661 > > > https://www.cambridgegreys.com/ > > > > > > -- > Anton R. Ivanov > Cambridgegreys Limited. Registered in England. Company Number 10273661 > https://www.cambridgegreys.com/
Eric Dumazet
2020-Feb-13 15:44 UTC
[PATCH] virtio: Work around frames incorrectly marked as gso
On 2/13/20 2:00 AM, Michael S. Tsirkin wrote:> On Wed, Feb 12, 2020 at 05:38:09PM +0000, Anton Ivanov wrote: >> >> >> On 11/02/2020 10:37, Michael S. Tsirkin wrote: >>> On Tue, Feb 11, 2020 at 07:42:37AM +0000, Anton Ivanov wrote: >>>> On 11/02/2020 02:51, Jason Wang wrote: >>>>> >>>>> On 2020/2/11 ??12:55, Anton Ivanov wrote: >>>>>> >>>>>> >>>>>> On 09/12/2019 10:48, anton.ivanov at cambridgegreys.com wrote: >>>>>>> From: Anton Ivanov <anton.ivanov at cambridgegreys.com> >>>>>>> >>>>>>> Some of the frames marked as GSO which arrive at >>>>>>> virtio_net_hdr_from_skb() have no GSO_TYPE, no >>>>>>> fragments (data_len = 0) and length significantly shorter >>>>>>> than the MTU (752 in my experiments). >>>>>>> >>>>>>> This is observed on raw sockets reading off vEth interfaces >>>>>>> in all 4.x and 5.x kernels I tested. >>>>>>> >>>>>>> These frames are reported as invalid while they are in fact >>>>>>> gso-less frames. >>>>>>> >>>>>>> This patch marks the vnet header as no-GSO for them instead >>>>>>> of reporting it as invalid. >>>>>>> >>>>>>> Signed-off-by: Anton Ivanov <anton.ivanov at cambridgegreys.com> >>>>>>> --- >>>>>>> ? include/linux/virtio_net.h | 8 ++++++-- >>>>>>> ? 1 file changed, 6 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h >>>>>>> index 0d1fe9297ac6..d90d5cff1b9a 100644 >>>>>>> --- a/include/linux/virtio_net.h >>>>>>> +++ b/include/linux/virtio_net.h >>>>>>> @@ -112,8 +112,12 @@ static inline int >>>>>>> virtio_net_hdr_from_skb(const struct sk_buff *skb, >>>>>>> ????????????? hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; >>>>>>> ????????? else if (sinfo->gso_type & SKB_GSO_TCPV6) >>>>>>> ????????????? hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; >>>>>>> -??????? else >>>>>>> -??????????? return -EINVAL; >>>>>>> +??????? else { >>>>>>> +??????????? if (skb->data_len == 0) >>>>>>> +??????????????? hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE; >>>>>>> +??????????? else >>>>>>> +??????????????? return -EINVAL; >>>>>>> +??????? } >>>>>>> ????????? if (sinfo->gso_type & SKB_GSO_TCP_ECN) >>>>>>> ????????????? hdr->gso_type |= VIRTIO_NET_HDR_GSO_ECN; >>>>>>> ????? } else >>>>>>> >>>>>> >>>>>> ping. >>>>>> >>>>> >>>>> Do you mean gso_size is set but gso_type is not? Looks like a bug >>>>> elsewhere. >>>>> >>>>> Thanks >>>>> >>>>> >>>> Yes. >>>> >>>> I could not trace it where it is coming from. >>>> >>>> I see it when doing recvmmsg on raw sockets in the UML vector network >>>> drivers. >>>> >>> >>> I think we need to find the culprit and fix it there, lots of other things >>> can break otherwise. >>> Just printing out skb->dev->name should do the trick, no? >> >> The printk in virtio_net_hdr_from_skb says NULL. >> >> That is probably normal for a locally originated frame. >> >> I cannot reproduce this with network traffic by the way - it happens only if the traffic is locally originated on the host. >> >> A, > > OK so is it code in __tcp_transmit_skb that sets gso_size to non-null > when gso_type is 0? >Correct way to determine if a packet is a gso one is by looking at gso_size. Then only it is legal looking at gso_type static inline bool skb_is_gso(const struct sk_buff *skb) { return skb_shinfo(skb)->gso_size; } /* Note: Should be called only if skb_is_gso(skb) is true */ static inline bool skb_is_gso_v6(const struct sk_buff *skb) ... There is absolutely no relation between GSO and skb->data_len, skb can be linearized for various orthogonal reasons.
Michael S. Tsirkin
2020-Feb-13 15:53 UTC
[PATCH] virtio: Work around frames incorrectly marked as gso
On Thu, Feb 13, 2020 at 07:44:06AM -0800, Eric Dumazet wrote:> > > On 2/13/20 2:00 AM, Michael S. Tsirkin wrote: > > On Wed, Feb 12, 2020 at 05:38:09PM +0000, Anton Ivanov wrote: > >> > >> > >> On 11/02/2020 10:37, Michael S. Tsirkin wrote: > >>> On Tue, Feb 11, 2020 at 07:42:37AM +0000, Anton Ivanov wrote: > >>>> On 11/02/2020 02:51, Jason Wang wrote: > >>>>> > >>>>> On 2020/2/11 ??12:55, Anton Ivanov wrote: > >>>>>> > >>>>>> > >>>>>> On 09/12/2019 10:48, anton.ivanov at cambridgegreys.com wrote: > >>>>>>> From: Anton Ivanov <anton.ivanov at cambridgegreys.com> > >>>>>>> > >>>>>>> Some of the frames marked as GSO which arrive at > >>>>>>> virtio_net_hdr_from_skb() have no GSO_TYPE, no > >>>>>>> fragments (data_len = 0) and length significantly shorter > >>>>>>> than the MTU (752 in my experiments). > >>>>>>> > >>>>>>> This is observed on raw sockets reading off vEth interfaces > >>>>>>> in all 4.x and 5.x kernels I tested. > >>>>>>> > >>>>>>> These frames are reported as invalid while they are in fact > >>>>>>> gso-less frames. > >>>>>>> > >>>>>>> This patch marks the vnet header as no-GSO for them instead > >>>>>>> of reporting it as invalid. > >>>>>>> > >>>>>>> Signed-off-by: Anton Ivanov <anton.ivanov at cambridgegreys.com> > >>>>>>> --- > >>>>>>> ? include/linux/virtio_net.h | 8 ++++++-- > >>>>>>> ? 1 file changed, 6 insertions(+), 2 deletions(-) > >>>>>>> > >>>>>>> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > >>>>>>> index 0d1fe9297ac6..d90d5cff1b9a 100644 > >>>>>>> --- a/include/linux/virtio_net.h > >>>>>>> +++ b/include/linux/virtio_net.h > >>>>>>> @@ -112,8 +112,12 @@ static inline int > >>>>>>> virtio_net_hdr_from_skb(const struct sk_buff *skb, > >>>>>>> ????????????? hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; > >>>>>>> ????????? else if (sinfo->gso_type & SKB_GSO_TCPV6) > >>>>>>> ????????????? hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; > >>>>>>> -??????? else > >>>>>>> -??????????? return -EINVAL; > >>>>>>> +??????? else { > >>>>>>> +??????????? if (skb->data_len == 0) > >>>>>>> +??????????????? hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE; > >>>>>>> +??????????? else > >>>>>>> +??????????????? return -EINVAL; > >>>>>>> +??????? } > >>>>>>> ????????? if (sinfo->gso_type & SKB_GSO_TCP_ECN) > >>>>>>> ????????????? hdr->gso_type |= VIRTIO_NET_HDR_GSO_ECN; > >>>>>>> ????? } else > >>>>>>> > >>>>>> > >>>>>> ping. > >>>>>> > >>>>> > >>>>> Do you mean gso_size is set but gso_type is not? Looks like a bug > >>>>> elsewhere. > >>>>> > >>>>> Thanks > >>>>> > >>>>> > >>>> Yes. > >>>> > >>>> I could not trace it where it is coming from. > >>>> > >>>> I see it when doing recvmmsg on raw sockets in the UML vector network > >>>> drivers. > >>>> > >>> > >>> I think we need to find the culprit and fix it there, lots of other things > >>> can break otherwise. > >>> Just printing out skb->dev->name should do the trick, no? > >> > >> The printk in virtio_net_hdr_from_skb says NULL. > >> > >> That is probably normal for a locally originated frame. > >> > >> I cannot reproduce this with network traffic by the way - it happens only if the traffic is locally originated on the host. > >> > >> A, > > > > OK so is it code in __tcp_transmit_skb that sets gso_size to non-null > > when gso_type is 0? > > > > Correct way to determine if a packet is a gso one is by looking at gso_size. > Then only it is legal looking at gso_type > > > static inline bool skb_is_gso(const struct sk_buff *skb) > { > return skb_shinfo(skb)->gso_size; > } > > /* Note: Should be called only if skb_is_gso(skb) is true */ > static inline bool skb_is_gso_v6(const struct sk_buff *skb) > ... > > > There is absolutely no relation between GSO and skb->data_len, skb can be linearized > for various orthogonal reasons.The reported problem is that virtio gets a packet where gso_size is !0 but gso_type is 0. It currently drops these on the assumption that it's some type of a gso packet it does not know how to handle. So you are saying if skb_is_gso we can still have gso_type set to 0, and that's an expected configuration? So the patch should just be: - if (skb_is_gso(skb)) { + if (skb_is_gso(skb) && sinfo->gso_type) { ?
Reasonably Related Threads
- [PATCH] virtio: Work around frames incorrectly marked as gso
- [PATCH] virtio: Work around frames incorrectly marked as gso
- [PATCH] virtio: Work around frames incorrectly marked as gso
- [PATCH] virtio: Work around frames incorrectly marked as gso
- [PATCH] virtio: Work around frames incorrectly marked as gso