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) { ?
Anton Ivanov
2020-Feb-13 16:23 UTC
[PATCH] virtio: Work around frames incorrectly marked as gso
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 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) { >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 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/
Michael S. Tsirkin
2020-Feb-13 16:37 UTC
[PATCH] virtio: Work around frames incorrectly marked as gso
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 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) { > > > 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 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/
Michael S. Tsirkin
2020-Feb-20 07:58 UTC
[PATCH] virtio: Work around frames incorrectly marked as gso
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 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) { > > > 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 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/
Possibly Parallel 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