diff mbox

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

Message ID CA+mtBx-se+u4j8WHzZ+AXeiFN2uevrvcTb6xFpeNrhjOv=EVng@mail.gmail.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Tom Herbert Dec. 4, 2014, 4 p.m. UTC
A fix is pending for net. Please try if you can.

Tom

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
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Timo Teras Dec. 9, 2014, 6:26 a.m. UTC | #1
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
> >

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Timo Teras Dec. 9, 2014, 6:44 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tom Herbert Dec. 9, 2014, 9:23 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 }