Possible regression: "gre: Use inner mac length when computing tunnel length"

Possible regression: "gre: Use inner mac length when computing tunnel length"

[Timo Teras]

Hi,

After upgrading to latest 3.14.24 or newer, I noticed a weird TSO bug
in the "dmvpn" setup I use. And seems 3.14.23 works just fine. So the
commit 14051f0452a2c26a "gre: Use inner mac length when computing
tunnel length" would appear to be the related commit (but have not yet
tested this).

In practice what happens is that forwarding path between ethX (or vlanX)
and gre1 gets broken.

There's probably two differences to the "regular" gre tunnel case:
- it's nbma mode, meaning the gre header is inserted via slightly
  different code path
- the gre1 packets are IPsec encrypted in transport mode

As additional detail, doing "ethtool -K gre1 tso off" will workaround
the issue, so it is clearly tso issue pointing even further to the
commit in question.

Is this something the suspected patch could cause? Any suggestions
what to test more?

Thanks,
Timo
 

Re: Possible regression: "gre: Use inner mac length when computing tunnel length"

[Tom Herbert]

A fix is pending for net. Please try if you can.

Tom

diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
index bb5947b..51973dd 100644
--- a/net/ipv4/gre_offload.c
+++ b/net/ipv4/gre_offload.c
@@ -247,6 +247,9 @@ static int gre_gro_complete(struct sk_buff *skb, int nhoff)
                err = ptype->callbacks.gro_complete(skb, nhoff + grehlen);

        rcu_read_unlock();
+
+       skb_set_inner_mac_header(skb, nhoff + grehlen);
+
        return err;
 }

On Thu, Dec 4, 2014 at 4:16 AM, Timo Teras <timo.teras@iki.fi> wrote:
> Hi,
>
> After upgrading to latest 3.14.24 or newer, I noticed a weird TSO bug
> in the "dmvpn" setup I use. And seems 3.14.23 works just fine. So the
> commit 14051f0452a2c26a "gre: Use inner mac length when computing
> tunnel length" would appear to be the related commit (but have not yet
> tested this).
>
> In practice what happens is that forwarding path between ethX (or vlanX)
> and gre1 gets broken.
>
> There's probably two differences to the "regular" gre tunnel case:
> - it's nbma mode, meaning the gre header is inserted via slightly
>   different code path
> - the gre1 packets are IPsec encrypted in transport mode
>
> As additional detail, doing "ethtool -K gre1 tso off" will workaround
> the issue, so it is clearly tso issue pointing even further to the
> commit in question.
>
> Is this something the suspected patch could cause? Any suggestions
> what to test more?
>
> Thanks,
> Timo
>

Re: Possible regression: "gre: Use inner mac length when computing tunnel length"

[Timo Teras]

On Thu, 4 Dec 2014 08:00:19 -0800
Tom Herbert <therbert@google.com> wrote:

> A fix is pending for net. Please try if you can.

Finally got to testing this. Unfortunately, this does not seem to fix
the issue I am experiencing.

Any suggestions?

> 
> Tom
> 
> diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
> index bb5947b..51973dd 100644
> --- a/net/ipv4/gre_offload.c
> +++ b/net/ipv4/gre_offload.c
> @@ -247,6 +247,9 @@ static int gre_gro_complete(struct sk_buff *skb,
> int nhoff) err = ptype->callbacks.gro_complete(skb, nhoff + grehlen);
> 
>         rcu_read_unlock();
> +
> +       skb_set_inner_mac_header(skb, nhoff + grehlen);
> +
>         return err;
>  }
> 
> On Thu, Dec 4, 2014 at 4:16 AM, Timo Teras <timo.teras@iki.fi> wrote:
> > Hi,
> >
> > After upgrading to latest 3.14.24 or newer, I noticed a weird TSO
> > bug in the "dmvpn" setup I use. And seems 3.14.23 works just fine.
> > So the commit 14051f0452a2c26a "gre: Use inner mac length when
> > computing tunnel length" would appear to be the related commit (but
> > have not yet tested this).
> >
> > In practice what happens is that forwarding path between ethX (or
> > vlanX) and gre1 gets broken.
> >
> > There's probably two differences to the "regular" gre tunnel case:
> > - it's nbma mode, meaning the gre header is inserted via slightly
> >   different code path
> > - the gre1 packets are IPsec encrypted in transport mode
> >
> > As additional detail, doing "ethtool -K gre1 tso off" will
> > workaround the issue, so it is clearly tso issue pointing even
> > further to the commit in question.
> >
> > Is this something the suspected patch could cause? Any suggestions
> > what to test more?
> >
> > Thanks,
> > Timo
> >

Re: Possible regression: "gre: Use inner mac length when computing tunnel length"

[Timo Teras]

On Tue, 9 Dec 2014 08:26:45 +0200
Timo Teras <timo.teras@iki.fi> wrote:

> On Thu, 4 Dec 2014 08:00:19 -0800
> Tom Herbert <therbert@google.com> wrote:
> 
> > A fix is pending for net. Please try if you can.
> 
> Finally got to testing this. Unfortunately, this does not seem to fix
> the issue I am experiencing.
> 
> Any suggestions?

I think this fix is required, but does not fix the issue.

I suspect my problem is with the fact that I have NBMA GRE tunnel.

In ipgre_xmit() it is gre_handle_offloads() that is called first.
However, in my case, the GRE header was already pushed earlier via
header_ops. That's why the packet is skb_pull()'ed in the
"if (dev->header_ops)" path, so that __gre_xmit can push back the
header again on it.

I wonder if it is correct to just move the gre_handle_offloads() call
after the if() block, or if additional fixing is needed to make
offloads work with nbma tunnels.

/Timo

Re: Possible regression: "gre: Use inner mac length when computing tunnel length"

[Tom Herbert]

On Mon, Dec 8, 2014 at 10:44 PM, Timo Teras <timo.teras@iki.fi> wrote:
> On Tue, 9 Dec 2014 08:26:45 +0200
> Timo Teras <timo.teras@iki.fi> wrote:
>
>> On Thu, 4 Dec 2014 08:00:19 -0800
>> Tom Herbert <therbert@google.com> wrote:
>>
>> > A fix is pending for net. Please try if you can.
>>
>> Finally got to testing this. Unfortunately, this does not seem to fix
>> the issue I am experiencing.
>>
>> Any suggestions?
>
> I think this fix is required, but does not fix the issue.
>
> I suspect my problem is with the fact that I have NBMA GRE tunnel.
>
> In ipgre_xmit() it is gre_handle_offloads() that is called first.
> However, in my case, the GRE header was already pushed earlier via
> header_ops. That's why the packet is skb_pull()'ed in the
> "if (dev->header_ops)" path, so that __gre_xmit can push back the
> header again on it.
>
> I wonder if it is correct to just move the gre_handle_offloads() call
> after the if() block, or if additional fixing is needed to make
> offloads work with nbma tunnels.
>
Yes, that probably is a good idea. iptunnel_handle_offloads resets
inner headers so that inner_mac header is probably wrong when there
are dev->header_ops.

Tom

> /Timo

[net] gre: fix the inner mac header in nbma gre tunnels xmit path

[Timo Teräs]

The NBMA GRE tunnels temporarily push GRE header that contain the
per-packet NBMA destination on the skb via header ops early in xmit
path. It is the later pulled before the real GRE header is constructed.

The inner mac was thus set differently in nbma case: the GRE header
has been pushed by neighbor layer, and mac header points to beginning
of the temporary gre header (set by dev_queue_xmit).

Now that the offloads expect mac header to point to the gre payload,
fix the xmit patch to:
 - pull first the temporary gre header away
 - and reset mac header to point to gre payload

This fixes tso to work again with nbma tunnels.

Fixes: 14051f0452a2 ("gre: Use inner mac length when computing tunnel length"
Signed-off-by: Timo Teräs <timo.teras@iki.fi>
Cc: Tom Herbert <therbert@google.com>
Cc: Alexander Duyck <alexander.h.duyck@redhat.com>
---
Though, normally mac header does point to the begging of the hardware header.
E.g. in ethernet case it's pointing to the ethernet header. But now in gre
case it's instead pointing to the payload which seems counter-intuitive to me.
But I guess tunnels are a bit of special case, and there's valid reasons to
have it point to tunnel payload too.

Applying this patch on top of the Tom's previous fix of 14051f0452a2 seems to
now make my dmvpn scenario work again. So this should go to -stable too
(atleast 3.14).

 net/ipv4/ip_gre.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 94213c8..6d2f84d 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -250,10 +250,6 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
 	struct ip_tunnel *tunnel = netdev_priv(dev);
 	const struct iphdr *tnl_params;
 
-	skb = gre_handle_offloads(skb, !!(tunnel->parms.o_flags&TUNNEL_CSUM));
-	if (IS_ERR(skb))
-		goto out;
-
 	if (dev->header_ops) {
 		/* Need space for new headers */
 		if (skb_cow_head(skb, dev->needed_headroom -
@@ -266,6 +262,7 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
 		 * to gre header.
 		 */
 		skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
+		skb_reset_mac_header(mac);
 	} else {
 		if (skb_cow_head(skb, dev->needed_headroom))
 			goto free_skb;
@@ -273,6 +270,10 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
 		tnl_params = &tunnel->parms.iph;
 	}
 
+	skb = gre_handle_offloads(skb, !!(tunnel->parms.o_flags&TUNNEL_CSUM));
+	if (IS_ERR(skb))
+		goto out;
+
 	__gre_xmit(skb, dev, tnl_params, skb->protocol);
 
 	return NETDEV_TX_OK;
-- 
2.2.0

Re: [net] gre: fix the inner mac header in nbma gre tunnels xmit path

[David Miller]

From: Timo Teräs <timo.teras@iki.fi>
Date: Thu, 11 Dec 2014 09:14:39 +0200

> @@ -266,6 +262,7 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
>  		 * to gre header.
>  		 */
>  		skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
> +		skb_reset_mac_header(mac);

Please explain to me how this compiles, let alone be functionally
tested.

Re: [net] gre: fix the inner mac header in nbma gre tunnels xmit path

[Timo Teras]

On Thu, 11 Dec 2014 14:36:27 -0500 (EST)
David Miller <davem@davemloft.net> wrote:

> From: Timo Teräs <timo.teras@iki.fi>
> Date: Thu, 11 Dec 2014 09:14:39 +0200
> 
> > @@ -266,6 +262,7 @@ static netdev_tx_t ipgre_xmit(struct sk_buff
> > *skb,
> >  		 * to gre header.
> >  		 */
> >  		skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
> > +		skb_reset_mac_header(mac);
> 
> Please explain to me how this compiles, let alone be functionally
> tested.

Sorry. I made the change twice; once on the build box. And again on my
git checkout on work station. Must've been on seriously coffee deprived
state.

Should be obviously:
+		skb_reset_mac_header(skb);

Is there other comments on it?

Or shall I resend?

Re: [net] gre: fix the inner mac header in nbma gre tunnels xmit path

[David Miller]

From: Timo Teras <timo.teras@iki.fi>
Date: Thu, 11 Dec 2014 21:44:56 +0200

> On Thu, 11 Dec 2014 14:36:27 -0500 (EST)
> David Miller <davem@davemloft.net> wrote:
> 
>> From: Timo Teräs <timo.teras@iki.fi>
>> Date: Thu, 11 Dec 2014 09:14:39 +0200
>> 
>> > @@ -266,6 +262,7 @@ static netdev_tx_t ipgre_xmit(struct sk_buff
>> > *skb,
>> >  		 * to gre header.
>> >  		 */
>> >  		skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
>> > +		skb_reset_mac_header(mac);
>> 
>> Please explain to me how this compiles, let alone be functionally
>> tested.
> 
> Sorry. I made the change twice; once on the build box. And again on my
> git checkout on work station. Must've been on seriously coffee deprived
> state.
> 
> Should be obviously:
> +		skb_reset_mac_header(skb);
> 
> Is there other comments on it?

I don't care where you applied the patch, if you didn't type make after
making the change, that is extremely careless.

[PATCH net, v2] gre: fix the inner mac header in nbma tunnel xmit path

[Timo Teräs]

The NBMA GRE tunnels temporarily push GRE header that contain the
per-packet NBMA destination on the skb via header ops early in xmit
path. It is the later pulled before the real GRE header is constructed.

The inner mac was thus set differently in nbma case: the GRE header
has been pushed by neighbor layer, and mac header points to beginning
of the temporary gre header (set by dev_queue_xmit).

Now that the offloads expect mac header to point to the gre payload,
fix the xmit patch to:
 - pull first the temporary gre header away
 - and reset mac header to point to gre payload

This fixes tso to work again with nbma tunnels.

Fixes: 14051f0452a2 ("gre: Use inner mac length when computing tunnel length")
Signed-off-by: Timo Teräs <timo.teras@iki.fi>
Cc: Tom Herbert <therbert@google.com>
Cc: Alexander Duyck <alexander.h.duyck@redhat.com>
---
Fixed the stupid typo of skb_reset_mac_header()'s argument. And yes,
I did recompile, retest and verify that this works.

Comments from v1 apply:

Though, normally mac header does point to the begging of the hardware header.
E.g. in ethernet case it's pointing to the ethernet header. But now in gre
case it's instead pointing to the payload which seems counter-intuitive to me.
But I guess tunnels are a bit of special case, and there's valid reasons to
have it point to tunnel payload too.

Applying this patch on top of the Tom's previous fix of 14051f0452a2 seems to
now make my dmvpn scenario work again. So this should go to -stable too
(atleast 3.14).

 net/ipv4/ip_gre.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 94213c8..b40b90d 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -250,10 +250,6 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
 	struct ip_tunnel *tunnel = netdev_priv(dev);
 	const struct iphdr *tnl_params;
 
-	skb = gre_handle_offloads(skb, !!(tunnel->parms.o_flags&TUNNEL_CSUM));
-	if (IS_ERR(skb))
-		goto out;
-
 	if (dev->header_ops) {
 		/* Need space for new headers */
 		if (skb_cow_head(skb, dev->needed_headroom -
@@ -266,6 +262,7 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
 		 * to gre header.
 		 */
 		skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
+		skb_reset_mac_header(skb);
 	} else {
 		if (skb_cow_head(skb, dev->needed_headroom))
 			goto free_skb;
@@ -273,6 +270,10 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
 		tnl_params = &tunnel->parms.iph;
 	}
 
+	skb = gre_handle_offloads(skb, !!(tunnel->parms.o_flags&TUNNEL_CSUM));
+	if (IS_ERR(skb))
+		goto out;
+
 	__gre_xmit(skb, dev, tnl_params, skb->protocol);
 
 	return NETDEV_TX_OK;
-- 
2.2.0

Re: [PATCH net, v2] gre: fix the inner mac header in nbma tunnel xmit path

[David Miller]

From: Timo Teräs <timo.teras@iki.fi>
Date: Mon, 15 Dec 2014 09:24:13 +0200

> The NBMA GRE tunnels temporarily push GRE header that contain the
> per-packet NBMA destination on the skb via header ops early in xmit
> path. It is the later pulled before the real GRE header is constructed.
> 
> The inner mac was thus set differently in nbma case: the GRE header
> has been pushed by neighbor layer, and mac header points to beginning
> of the temporary gre header (set by dev_queue_xmit).
> 
> Now that the offloads expect mac header to point to the gre payload,
> fix the xmit patch to:
>  - pull first the temporary gre header away
>  - and reset mac header to point to gre payload
> 
> This fixes tso to work again with nbma tunnels.
> 
> Fixes: 14051f0452a2 ("gre: Use inner mac length when computing tunnel length")
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>

Applied and queued up for -stable, thanks.