Re: [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:

quoted

On Thu, Feb 13, 2020 at 07:44:06AM -0800, Eric Dumazet wrote:

quoted

On 2/13/20 2:00 AM, Michael S. Tsirkin wrote:

quoted

On Wed, Feb 12, 2020 at 05:38:09PM +0000, Anton Ivanov wrote:

quoted

On 11/02/2020 10:37, Michael S. Tsirkin wrote:

quoted

On Tue, Feb 11, 2020 at 07:42:37AM +0000, Anton Ivanov wrote:

quoted

On 11/02/2020 02:51, Jason Wang wrote:

quoted

On 2020/2/11 上午12:55, Anton Ivanov wrote:

quoted

On 09/12/2019 10:48, anton.ivanov@cambridgegreys.com wrote:

quoted

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?

quoted

?


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