| Message ID | 20191209104824.17059-1-anton.ivanov@cambridgegreys.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | virtio: Work around frames incorrectly marked as gso | expand |
> 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; maybe use "else if" like in the before? yes, it's a different type of condition, but braces look a bit unnatural here to me at least johannes
On 09/12/2019 10:48, anton.ivanov@cambridgegreys.com wrote: > From: Anton Ivanov <anton.ivanov@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@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.
On 2020/2/11 上午12:55, Anton Ivanov wrote: > > > On 09/12/2019 10:48, anton.ivanov@cambridgegreys.com wrote: >> From: Anton Ivanov <anton.ivanov@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@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
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@cambridgegreys.com wrote: >>> From: Anton Ivanov <anton.ivanov@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@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.
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@cambridgegreys.com wrote: > > > > From: Anton Ivanov <anton.ivanov@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@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? > -- > Anton R. Ivanov > Cambridgegreys Limited. Registered in England. Company Number 10273661 > https://www.cambridgegreys.com/
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@cambridgegreys.com wrote: >>>>> From: Anton Ivanov <anton.ivanov@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@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? I will rebuild my rig and retest (it's been a while since I worked on this bug). In theory, it should be veth - the test is over a vEth pair and all frames are locally originated by iperf. In practice - I will retest and post the results sometimes later today. Brgds, > > > >> -- >> Anton R. Ivanov >> Cambridgegreys Limited. Registered in England. Company Number 10273661 >> https://www.cambridgegreys.com/ > > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um >
On Wed, Feb 12, 2020 at 10:03:31AM +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@cambridgegreys.com wrote: > > > > > > From: Anton Ivanov <anton.ivanov@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@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? > > I will rebuild my rig and retest (it's been a while since I worked on this bug). > > In theory, it should be veth - the test is over a vEth pair and all frames are locally originated by iperf. > > In practice - I will retest and post the results sometimes later today. > > Brgds, ok if it's veth then you need to add a similar printk patch to veth and re-run to see where does it come from originally. > > > > > > > > > -- > > > Anton R. Ivanov > > > Cambridgegreys Limited. Registered in England. Company Number 10273661 > > > https://www.cambridgegreys.com/ > > > > > > _______________________________________________ > > linux-um mailing list > > linux-um@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/
On 12/02/2020 10:19, Michael S. Tsirkin wrote: > On Wed, Feb 12, 2020 at 10:03:31AM +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@cambridgegreys.com wrote: >>>>>>> From: Anton Ivanov <anton.ivanov@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@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? >> >> I will rebuild my rig and retest (it's been a while since I worked on this bug). >> >> In theory, it should be veth - the test is over a vEth pair and all frames are locally originated by iperf. >> >> In practice - I will retest and post the results sometimes later today. >> >> Brgds, > > > ok if it's veth then you need to add a similar printk patch to veth > and re-run to see where does it come from originally. Most likely - an iperf running on localhost :) It is generating the traffic. Thanks, I will add both printks and re-test ASAP. > >>> >>> >>> >>>> -- >>>> Anton R. Ivanov >>>> Cambridgegreys Limited. Registered in England. Company Number 10273661 >>>> https://www.cambridgegreys.com/ >>> >>> >>> _______________________________________________ >>> linux-um mailing list >>> linux-um@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/ > > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um >
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@cambridgegreys.com wrote: >>>>> From: Anton Ivanov <anton.ivanov@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@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, > > >> -- >> Anton R. Ivanov >> Cambridgegreys Limited. Registered in England. Company Number 10273661 >> https://www.cambridgegreys.com/ > > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um >
On 2020/2/13 上午1:38, 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@cambridgegreys.com wrote: >>>>>> From: Anton Ivanov <anton.ivanov@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@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, Or maybe you can try add dump_stack() there. Thanks
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@cambridgegreys.com wrote: > > > > > > From: Anton Ivanov <anton.ivanov@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@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@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/
On 13/02/2020 03:31, Jason Wang wrote: > > On 2020/2/13 上午1:38, 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@cambridgegreys.com wrote: >>>>>>> From: Anton Ivanov <anton.ivanov@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@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, > > > Or maybe you can try add dump_stack() there. That unfortunately is not very informative. At that point it shows me the invocation chain from recvmmsg: [ 2334.180854] Call Trace: [ 2334.181947] dump_stack+0x5c/0x80 [ 2334.183021] packet_recvmsg.cold+0x23/0x49 [ 2334.184063] ___sys_recvmsg+0xe1/0x1f0 [ 2334.185034] ? packet_poll+0xca/0x130 [ 2334.186014] ? sock_poll+0x77/0xb0 [ 2334.186977] ? ep_item_poll.isra.0+0x3f/0xb0 [ 2334.187936] ? ep_send_events_proc+0xf1/0x240 [ 2334.188901] ? dequeue_signal+0xdb/0x180 [ 2334.189848] do_recvmmsg+0xc8/0x2d0 [ 2334.190728] ? ep_poll+0x8c/0x470 [ 2334.191581] __sys_recvmmsg+0x108/0x150 [ 2334.192441] __x64_sys_recvmmsg+0x25/0x30 [ 2334.193346] do_syscall_64+0x53/0x140 [ 2334.194262] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > Thanks > > > > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um
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@cambridgegreys.com wrote: >>>>>>> From: Anton Ivanov <anton.ivanov@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@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@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/ > >
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@cambridgegreys.com wrote: > > > > > > > > From: Anton Ivanov <anton.ivanov@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@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@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/
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@cambridgegreys.com wrote: >>>>>>> From: Anton Ivanov <anton.ivanov@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@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.
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@cambridgegreys.com wrote: > >>>>>>> From: Anton Ivanov <anton.ivanov@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@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) { ?
On 13/02/2020 15:53, Michael S. Tsirkin wrote: > 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@cambridgegreys.com wrote: >>>>>>>>> From: Anton Ivanov <anton.ivanov@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@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) { > Yes, provided that skb_is_gso(skb) and sinfo->gso_type == 0 is a valid state. I agree with Jason, there may be something wrong going on here and we need to find the source which creates these packets. A. > > ? > > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um
On Thu, Feb 13, 2020 at 04:23:24PM +0000, Anton Ivanov wrote: > > On 13/02/2020 15:53, Michael S. Tsirkin wrote: > > 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@cambridgegreys.com wrote: > > > > > > > > > > From: Anton Ivanov <anton.ivanov@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@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) { > > > Yes, provided that skb_is_gso(skb) and sinfo->gso_type == 0 is a valid state. > > I agree with Jason, there may be something wrong going on here and we need to find the source which creates these packets. > > A. Well Eric will know for sure. Eric is skb_is_gso(skb) and sinfo->gso_type == 0 a valid state? > > > > ? > > > > > > _______________________________________________ > > linux-um mailing list > > linux-um@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/
On Thu, Feb 13, 2020 at 04:23:24PM +0000, Anton Ivanov wrote: > > On 13/02/2020 15:53, Michael S. Tsirkin wrote: > > 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@cambridgegreys.com wrote: > > > > > > > > > > From: Anton Ivanov <anton.ivanov@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@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) { > > > Yes, provided that skb_is_gso(skb) and sinfo->gso_type == 0 is a valid state. > > I agree with Jason, there may be something wrong going on here and we need to find the source which creates these packets. > > A. Want to submit a patch to address this for now? > > > > ? > > > > > > _______________________________________________ > > linux-um mailing list > > linux-um@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/
On 20/02/2020 07:58, Michael S. Tsirkin wrote: > On Thu, Feb 13, 2020 at 04:23:24PM +0000, Anton Ivanov wrote: >> On 13/02/2020 15:53, Michael S. Tsirkin wrote: >>> 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@cambridgegreys.com wrote: >>>>>>>>>>> From: Anton Ivanov <anton.ivanov@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@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) { >>> >> Yes, provided that skb_is_gso(skb) and sinfo->gso_type == 0 is a valid state. >> >> I agree with Jason, there may be something wrong going on here and we need to find the source which creates these packets. >> >> A. > > Want to submit a patch to address this for now? I can do that on Monday - traveling till then. A. > >>> ? >>> >>> >>> _______________________________________________ >>> linux-um mailing list >>> linux-um@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/ > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um
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