Cake - FQ_codel the next generation
 help / color / mirror / Atom feed
* [Cake] [PATCH net] gso_segment: Reset skb->mac_len after modifying network header
@ 2018-09-11 22:19 Toke Høiland-Jørgensen
  2018-09-13 14:11 ` Eric Dumazet
  2018-09-13 14:43 ` [Cake] [PATCH net v2] " Toke Høiland-Jørgensen
  0 siblings, 2 replies; 6+ messages in thread
From: Toke Høiland-Jørgensen @ 2018-09-11 22:19 UTC (permalink / raw)
  To: netdev; +Cc: cake, Eric Dumazet

When splitting a GSO segment that consists of encapsulated packets, the
skb->mac_len of the segments can end up being set wrong, causing packet
drops in particular when using act_mirred and ifb interfaces in
combination with a qdisc that splits GSO packets.

This happens because at the time skb_segment() is called, network_header
will point to the inner header, throwing off the calculation in
skb_reset_mac_len(). The network_header is subsequently adjust by the
outer IP gso_segment handlers, but they don't set the mac_len.

Fix this by adding skb_reset_mac_len() calls to both the IPv4 and IPv6
gso_segment handlers, after they modify the network_header.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 net/ipv4/af_inet.c     |    1 +
 net/ipv6/ip6_offload.c |    1 +
 2 files changed, 2 insertions(+)

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 20fda8fb8ffd..1fbe2f815474 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1377,6 +1377,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 		if (encap)
 			skb_reset_inner_headers(skb);
 		skb->network_header = (u8 *)iph - skb->head;
+		skb_reset_mac_len(skb);
 	} while ((skb = skb->next));
 
 out:
diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
index 37ff4805b20c..c7e495f12011 100644
--- a/net/ipv6/ip6_offload.c
+++ b/net/ipv6/ip6_offload.c
@@ -115,6 +115,7 @@ static struct sk_buff *ipv6_gso_segment(struct sk_buff *skb,
 			payload_len = skb->len - nhoff - sizeof(*ipv6h);
 		ipv6h->payload_len = htons(payload_len);
 		skb->network_header = (u8 *)ipv6h - skb->head;
+		skb_reset_mac_len(skb);
 
 		if (udpfrag) {
 			int err = ip6_find_1stfragopt(skb, &prevhdr);


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Cake] [PATCH net] gso_segment: Reset skb->mac_len after modifying network header
  2018-09-11 22:19 [Cake] [PATCH net] gso_segment: Reset skb->mac_len after modifying network header Toke Høiland-Jørgensen
@ 2018-09-13 14:11 ` Eric Dumazet
  2018-09-13 14:14   ` Dave Taht
  2018-09-13 14:22   ` Toke Høiland-Jørgensen
  2018-09-13 14:43 ` [Cake] [PATCH net v2] " Toke Høiland-Jørgensen
  1 sibling, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2018-09-13 14:11 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen, netdev; +Cc: cake, Eric Dumazet



On 09/11/2018 03:19 PM, Toke Høiland-Jørgensen wrote:
> When splitting a GSO segment that consists of encapsulated packets, the
> skb->mac_len of the segments can end up being set wrong, causing packet
> drops in particular when using act_mirred and ifb interfaces in
> combination with a qdisc that splits GSO packets.
> 
> This happens because at the time skb_segment() is called, network_header
> will point to the inner header, throwing off the calculation in
> skb_reset_mac_len(). The network_header is subsequently adjust by the
> outer IP gso_segment handlers, but they don't set the mac_len.
> 
> Fix this by adding skb_reset_mac_len() calls to both the IPv4 and IPv6
> gso_segment handlers, after they modify the network_header.
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>

Looks good but I would have appreciated a thanks or something
after the help I gave on this problem.

Reviewed-by: Eric Dumazet <edumazet@google.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Cake] [PATCH net] gso_segment: Reset skb->mac_len after modifying network header
  2018-09-13 14:11 ` Eric Dumazet
@ 2018-09-13 14:14   ` Dave Taht
  2018-09-13 14:22   ` Toke Høiland-Jørgensen
  1 sibling, 0 replies; 6+ messages in thread
From: Dave Taht @ 2018-09-13 14:14 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Toke Høiland-Jørgensen,
	Linux Kernel Network Developers, Cake List, Eric Dumazet

On Thu, Sep 13, 2018 at 7:11 AM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 09/11/2018 03:19 PM, Toke Høiland-Jørgensen wrote:
> > When splitting a GSO segment that consists of encapsulated packets, the
> > skb->mac_len of the segments can end up being set wrong, causing packet
> > drops in particular when using act_mirred and ifb interfaces in
> > combination with a qdisc that splits GSO packets.
> >
> > This happens because at the time skb_segment() is called, network_header
> > will point to the inner header, throwing off the calculation in
> > skb_reset_mac_len(). The network_header is subsequently adjust by the
> > outer IP gso_segment handlers, but they don't set the mac_len.
> >
> > Fix this by adding skb_reset_mac_len() calls to both the IPv4 and IPv6
> > gso_segment handlers, after they modify the network_header.
> >
> > Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>
> Looks good but I would have appreciated a thanks or something
> after the help I gave on this problem.
>
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks, eric! (post-hoc)

Acked-by: Dave Taht <dave.taht@gmail.com>

> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake



-- 

Dave Täht
CEO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-669-226-2619

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Cake] [PATCH net] gso_segment: Reset skb->mac_len after modifying network header
  2018-09-13 14:11 ` Eric Dumazet
  2018-09-13 14:14   ` Dave Taht
@ 2018-09-13 14:22   ` Toke Høiland-Jørgensen
  1 sibling, 0 replies; 6+ messages in thread
From: Toke Høiland-Jørgensen @ 2018-09-13 14:22 UTC (permalink / raw)
  To: Eric Dumazet, netdev; +Cc: cake, Eric Dumazet



On 13 September 2018 16:11:18 CEST, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>On 09/11/2018 03:19 PM, Toke Høiland-Jørgensen wrote:
>> When splitting a GSO segment that consists of encapsulated packets,
>the
>> skb->mac_len of the segments can end up being set wrong, causing
>packet
>> drops in particular when using act_mirred and ifb interfaces in
>> combination with a qdisc that splits GSO packets.
>> 
>> This happens because at the time skb_segment() is called,
>network_header
>> will point to the inner header, throwing off the calculation in
>> skb_reset_mac_len(). The network_header is subsequently adjust by the
>> outer IP gso_segment handlers, but they don't set the mac_len.
>> 
>> Fix this by adding skb_reset_mac_len() calls to both the IPv4 and
>IPv6
>> gso_segment handlers, after they modify the network_header.
>> 
>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>
>Looks good but I would have appreciated a thanks or something
>after the help I gave on this problem.

Yes, of course. Should have mentioned that in the commit message.

My apologies, won't happen again. And thanks! :)

-Toke

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Cake] [PATCH net v2] gso_segment: Reset skb->mac_len after modifying network header
  2018-09-11 22:19 [Cake] [PATCH net] gso_segment: Reset skb->mac_len after modifying network header Toke Høiland-Jørgensen
  2018-09-13 14:11 ` Eric Dumazet
@ 2018-09-13 14:43 ` Toke Høiland-Jørgensen
  2018-09-13 19:10   ` David Miller
  1 sibling, 1 reply; 6+ messages in thread
From: Toke Høiland-Jørgensen @ 2018-09-13 14:43 UTC (permalink / raw)
  To: netdev
  Cc: Toke Høiland-Jørgensen, cake, David Miller,
	Eric Dumazet, Dave Taht

When splitting a GSO segment that consists of encapsulated packets, the
skb->mac_len of the segments can end up being set wrong, causing packet
drops in particular when using act_mirred and ifb interfaces in
combination with a qdisc that splits GSO packets.

This happens because at the time skb_segment() is called, network_header
will point to the inner header, throwing off the calculation in
skb_reset_mac_len(). The network_header is subsequently adjust by the
outer IP gso_segment handlers, but they don't set the mac_len.

Fix this by adding skb_reset_mac_len() calls to both the IPv4 and IPv6
gso_segment handlers, after they modify the network_header.

Many thanks to Eric Dumazet for his help in identifying the cause of
the bug.

Acked-by: Dave Taht <dave.taht@gmail.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
v2:
  - Properly credit Eric for his help
  - Add review and ack tags

 net/ipv4/af_inet.c     | 1 +
 net/ipv6/ip6_offload.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 20fda8fb8ffd..1fbe2f815474 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1377,6 +1377,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 		if (encap)
 			skb_reset_inner_headers(skb);
 		skb->network_header = (u8 *)iph - skb->head;
+		skb_reset_mac_len(skb);
 	} while ((skb = skb->next));
 
 out:
diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
index 37ff4805b20c..c7e495f12011 100644
--- a/net/ipv6/ip6_offload.c
+++ b/net/ipv6/ip6_offload.c
@@ -115,6 +115,7 @@ static struct sk_buff *ipv6_gso_segment(struct sk_buff *skb,
 			payload_len = skb->len - nhoff - sizeof(*ipv6h);
 		ipv6h->payload_len = htons(payload_len);
 		skb->network_header = (u8 *)ipv6h - skb->head;
+		skb_reset_mac_len(skb);
 
 		if (udpfrag) {
 			int err = ip6_find_1stfragopt(skb, &prevhdr);
-- 
2.18.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Cake] [PATCH net v2] gso_segment: Reset skb->mac_len after modifying network header
  2018-09-13 14:43 ` [Cake] [PATCH net v2] " Toke Høiland-Jørgensen
@ 2018-09-13 19:10   ` David Miller
  0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2018-09-13 19:10 UTC (permalink / raw)
  To: toke; +Cc: netdev, cake, edumazet, dave.taht

From: Toke Høiland-Jørgensen <toke@toke.dk>
Date: Thu, 13 Sep 2018 16:43:07 +0200

> When splitting a GSO segment that consists of encapsulated packets, the
> skb->mac_len of the segments can end up being set wrong, causing packet
> drops in particular when using act_mirred and ifb interfaces in
> combination with a qdisc that splits GSO packets.
> 
> This happens because at the time skb_segment() is called, network_header
> will point to the inner header, throwing off the calculation in
> skb_reset_mac_len(). The network_header is subsequently adjust by the
> outer IP gso_segment handlers, but they don't set the mac_len.
> 
> Fix this by adding skb_reset_mac_len() calls to both the IPv4 and IPv6
> gso_segment handlers, after they modify the network_header.
> 
> Many thanks to Eric Dumazet for his help in identifying the cause of
> the bug.
> 
> Acked-by: Dave Taht <dave.taht@gmail.com>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> ---
> v2:
>   - Properly credit Eric for his help
>   - Add review and ack tags

Applied and queued up for -stable, thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-09-13 19:10 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-11 22:19 [Cake] [PATCH net] gso_segment: Reset skb->mac_len after modifying network header Toke Høiland-Jørgensen
2018-09-13 14:11 ` Eric Dumazet
2018-09-13 14:14   ` Dave Taht
2018-09-13 14:22   ` Toke Høiland-Jørgensen
2018-09-13 14:43 ` [Cake] [PATCH net v2] " Toke Høiland-Jørgensen
2018-09-13 19:10   ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox