Skip to contentSkip to conversation

net: Handle gso packets in dev_forward_skb

PATCH In review Eric W. Biederman 4 participants 9 messages Mar 21, 2011 – Mar 30, 2011

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Eric W. Biederman

author
#0 lore raw

PATCH 1/1 net: Handle gso packets in dev_forward_skb

Today when gso packets reach dev_forward_skb through the macvlan driver we drop them on the floor because they exceed the device mtu. Ouch!

I don't undersand the subtelties of gso but I think it is sufficient to simply relax the checks and let gso packets through without an mtu check, and it works in my test case.

If needed we can split the gso packets into multiple packets here but that just seems like a wast of memory and time.

Signed-off-by Eric W. Biederman <[email protected]>
---
 net/core/dev.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)
net/core/dev.c +10 -6
index 0b88eba..2e26606 100644
@@ -1527,17 +1527,21 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
 	skb_orphan(skb);
 	nf_reset(skb);
 
-	if (unlikely(!(dev->flags & IFF_UP) ||
-		     (skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) {
-		atomic_long_inc(&dev->rx_dropped);
-		kfree_skb(skb);
-		return NET_RX_DROP;
-	}
+	if (unlikely(!(dev->flags & IFF_UP)))
+		goto kfree_skb;
+	/* Don't check mtu on gso packets... */
+	if (unlikely(!skb_is_gso(skb) &&
+		     (skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN))))
+		goto kfree_skb;
 	skb_set_dev(skb, dev);
 	skb->tstamp.tv64 = 0;
 	skb->pkt_type = PACKET_HOST;
 	skb->protocol = eth_type_trans(skb, dev);
 	return netif_rx(skb);
+kfree_skb:
+	atomic_long_inc(&dev->rx_dropped);
+	kfree_skb(skb);
+	return NET_RX_DROP;
 }
 EXPORT_SYMBOL_GPL(dev_forward_skb);
Signature · 1 line
1.7.4
#1 lore raw
Quoted 37 lines · Eric W. Biederman

Today when gso packets reach dev_forward_skb through the macvlan driver we drop them on the floor because they exceed the device mtu. Ouch!

I don't undersand the subtelties of gso but I think it is sufficient to simply relax the checks and let gso packets through without an mtu check, and it works in my test case.

If needed we can split the gso packets into multiple packets here but that just seems like a wast of memory and time.

Signed-off-by Eric W. Biederman <[email protected]>
---
 net/core/dev.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)
net/core/dev.c +6 -6
index 0b88eba..2e26606 100644
@@ -1527,17 +1527,21 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
 	skb_orphan(skb);
 	nf_reset(skb);
 
-	if (unlikely(!(dev->flags & IFF_UP) ||
-		     (skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) {
-		atomic_long_inc(&dev->rx_dropped);
-		kfree_skb(skb);
-		return NET_RX_DROP;
-	}
+	if (unlikely(!(dev->flags & IFF_UP)))
+		goto kfree_skb;
+	/* Don't check mtu on gso packets... */
+	if (unlikely(!skb_is_gso(skb) &&
+		     (skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN))))
+		goto kfree_skb;
 	skb_set_dev(skb, dev);
 	skb->tstamp.tv64 = 0;

net/core/dev.c int dev_forward_skb(struct net_device *dev, struct sk_buff *skb) · line 1547

Quoted 10 lines
net/core/dev.c +4 -0
 	skb->pkt_type = PACKET_HOST;
 	skb->protocol = eth_type_trans(skb, dev);
 	return netif_rx(skb);
+kfree_skb:
+	atomic_long_inc(&dev->rx_dropped);
+	kfree_skb(skb);
+	return NET_RX_DROP;
 }
 EXPORT_SYMBOL_GPL(dev_forward_skb);

Hmm, did you follow http://patchwork.ozlabs.org/patch/86815/
discussion ?

maintainer
#3 lore raw

From: [email protected] (Eric W. Biederman)
Date: Mon, 21 Mar 2011 15:00:56 -0700

Quoted 7 lines
Quoted 2 lines · Eric Dumazet

Hmm, did you follow http://patchwork.ozlabs.org/patch/86815/
discussion ?

No. It seems I missed it, but I do have the same problem, and essentially the same fix.

Can someone work on getting this straightened out and resubmit the final patch so I can apply something?

Thanks.

#4 lore raw
Quoted 5 lines · David Miller

From: [email protected] (Eric W. Biederman)
Date: Mon, 21 Mar 2011 15:00:56 -0700

Quoted 2 lines

Eric Dumazet <[email protected]> writes:

Quoted 9 lines
Quoted 5 lines
Quoted 2 lines

Hmm, did you follow http://patchwork.ozlabs.org/patch/86815/
discussion ?

No. It seems I missed it, but I do have the same problem, and essentially the same fix.

Can someone work on getting this straightened out and resubmit the final patch so I can apply something?

Apparently Daniel is busy.

I guess we can use Eric B. patch, then ?

author
#5 lore raw
Quoted 12 lines · Eric Dumazet
Quoted 11 lines · David Miller

From: [email protected] (Eric W. Biederman)
Date: Mon, 21 Mar 2011 15:00:56 -0700

Quoted 7 lines
Quoted 2 lines · Eric Dumazet

Hmm, did you follow http://patchwork.ozlabs.org/patch/86815/
discussion ?

No. It seems I missed it, but I do have the same problem, and essentially the same fix.

Quoted 7 lines
Quoted 3 lines

Can someone work on getting this straightened out and resubmit the final patch so I can apply something?

Apparently Daniel is busy.

I guess we can use Eric B. patch, then ?

That sounds good to me.

After reviewing the other thread to the best of my knowledge there are no issues with the patch I submitted. At worst I am 4 bytes extra permissive about the mtu because of the way that path handles vlan headers and that isn't something my patch changed.

Eric

#6 lore raw
In reply to Eric Dumazet
Quoted 6 lines · Eric Dumazet
Quoted 5 lines · David Miller

From: [email protected] (Eric W. Biederman)
Date: Mon, 21 Mar 2011 15:00:56 -0700

Quoted 2 lines

Eric Dumazet<[email protected]> writes:

Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Hmm, did you follow http://patchwork.ozlabs.org/patch/86815/
discussion ?

No. It seems I missed it, but I do have the same problem, and essentially the same fix.

Can someone work on getting this straightened out and resubmit the final patch so I can apply something?

Apparently Daniel is busy.

Yes, sorry for the delay.
I would like to fix it but I am not sure to understand the different solutions proposed.

Quoted 6 lines · Eric Dumazet
Quoted 5 lines · David Miller

From: [email protected] (Eric W. Biederman)
Date: Mon, 21 Mar 2011 15:00:56 -0700

Quoted 2 lines

Eric Dumazet<[email protected]> writes:

Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Hmm, did you follow http://patchwork.ozlabs.org/patch/86815/
discussion ?

No. It seems I missed it, but I do have the same problem, and essentially the same fix.

Can someone work on getting this straightened out and resubmit the final patch so I can apply something?

Apparently Daniel is busy.

Yes, sorry for the delay.
I would like to fix it, but my knowledge in limited on offloading.

I did the following patch, it fixes the problem. If it looks good I can resend it in patch format:

net/core/dev.c 2011-03-29 14:50:35.587646000 +0200 renamed from net/core/dev.c 2011-03-29 14:50:00.047646000 +0200 +22 -2
Index: net-2.6/net/core/dev.c
===================================================================
@@ -1454,6 +1454,27 @@ static inline void net_timestamp_check(s
                __net_timestamp(skb);
 }
 
+static inline bool is_skb_forwardable(struct net_device *dev,
+                                     struct sk_buff *skb)
+{
+       unsigned int len;
+
+       if (!(dev->flags & IFF_UP))
+               return false;
+
+       len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
+       if (skb->len <= len)
+               return true;
+
+       /* if TSO is enabled, we don't care about the length as the packet
+        * could be forwarded without being segmented before
+        */
+       if (skb_is_gso(skb))
+               return true;
+
+       return false;
+}
+
 /**
  * dev_forward_skb - loopback an skb to another netif
  *

@@ -1477,8 +1498,7 @@ int dev_forward_skb(struct net_device *d
        skb_orphan(skb);
        nf_reset(skb);
 
-       if (unlikely(!(dev->flags & IFF_UP) ||
-                    (skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) {
+       if (unlikely(!is_skb_forwardable(dev, skb))) {
                atomic_long_inc(&dev->rx_dropped);
                kfree_skb(skb);
                return NET_RX_DROP;
author Acked-by
#7 lore raw
Quoted 34 lines · Daniel Lezcano
Quoted 6 lines · Eric Dumazet
Quoted 5 lines · David Miller

From: [email protected] (Eric W. Biederman)
Date: Mon, 21 Mar 2011 15:00:56 -0700

Quoted 2 lines

Eric Dumazet<[email protected]> writes:

Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Hmm, did you follow http://patchwork.ozlabs.org/patch/86815/
discussion ?

No. It seems I missed it, but I do have the same problem, and essentially the same fix.

Can someone work on getting this straightened out and resubmit the final patch so I can apply something?

Apparently Daniel is busy.

Yes, sorry for the delay.
I would like to fix it but I am not sure to understand the different solutions proposed.

Quoted 13 lines · Eric Dumazet
Quoted 12 lines · David Miller

From: [email protected] (Eric W. Biederman)
Date: Mon, 21 Mar 2011 15:00:56 -0700

Quoted 6 lines
Quoted 2 lines · Eric Dumazet

Hmm, did you follow http://patchwork.ozlabs.org/patch/86815/
discussion ?

No. It seems I missed it, but I do have the same problem, and essentially the same fix.

Can someone work on getting this straightened out and resubmit the final patch so I can apply something?

Quoted 7 lines
Quoted 1 line

Apparently Daniel is busy.

Yes, sorry for the delay.
I would like to fix it, but my knowledge in limited on offloading.

I did the following patch, it fixes the problem. If it looks good I can resend it in patch format:

It looks fine to me. I would resubmit it so that if anyone else has complaints they will be jolted into looking at the patch again.

Acked-by "Eric W. Biederman" <[email protected]>

Eric

Quoted 44 lines

Index: net-2.6/net/core/dev.c
===================================================================

net/core/dev.c 2011-03-29 14:50:35.587646000 +0200 renamed from net/core/dev.c 2011-03-29 14:50:00.047646000 +0200 +22 -2
@@ -1454,6 +1454,27 @@ static inline void net_timestamp_check(s
                __net_timestamp(skb);
 }
 
+static inline bool is_skb_forwardable(struct net_device *dev,
+                                     struct sk_buff *skb)
+{
+       unsigned int len;
+
+       if (!(dev->flags & IFF_UP))
+               return false;
+
+       len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
+       if (skb->len <= len)
+               return true;
+
+       /* if TSO is enabled, we don't care about the length as the packet
+        * could be forwarded without being segmented before
+        */
+       if (skb_is_gso(skb))
+               return true;
+
+       return false;
+}
+
 /**
  * dev_forward_skb - loopback an skb to another netif
  *

@@ -1477,8 +1498,7 @@ int dev_forward_skb(struct net_device *d
        skb_orphan(skb);
        nf_reset(skb);
 
-       if (unlikely(!(dev->flags & IFF_UP) ||
-                    (skb->len > (dev->mtu + dev->hard_header_len +
 VLAN_HLEN)))) {
+       if (unlikely(!is_skb_forwardable(dev, skb))) {
                atomic_long_inc(&dev->rx_dropped);
                kfree_skb(skb);

                return NET_RX_DROP;