netdev - Re: [PATCH net] sit: Set skb->protocol properly in ipip6_tunnel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 28 Nov 2016 13:01:42 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     eric.dumazet@...il.com
Cc:     alexander.duyck@...il.com, sfr@...b.auug.org.au, elicooper@....com,
        netdev@...r.kernel.org
Subject: Re: [PATCH net] sit: Set skb->protocol properly in
 ipip6_tunnel_xmit()

From: Eric Dumazet <eric.dumazet@...il.com>
Date: Mon, 28 Nov 2016 09:34:27 -0800

> On Mon, 2016-11-28 at 08:53 -0800, Eric Dumazet wrote:
>> On Mon, 2016-11-28 at 11:47 -0500, David Miller wrote:
>> > From: Stephen Rothwell <sfr@...b.auug.org.au>
>> > Date: Sun, 27 Nov 2016 13:04:00 +1100
>> > 
>> > > [Just for Dave's information]
>> > > 
>> > > On Fri, 25 Nov 2016 13:50:17 +0800 Eli Cooper <elicooper@....com> wrote:
>> > >>
>> > >> Similar to commit ae148b085876
>> > >> ("ip6_tunnel: Update skb->protocol to ETH_P_IPV6 in ip6_tnl_xmit()"),
>> > >> sit tunnels also need to update skb->protocol; otherwise, TSO/GSO packets
>> > >> might not be properly segmented, which causes the packets being dropped.
>> > >> 
>> > >> Reported-by: Stephen Rothwell <sfr@...b.auug.org.au>
>> > >> Tested-by: Eli Cooper <elicooper@....com>
>> > >> Cc: stable@...r.kernel.org
>> > >> Signed-off-by: Eli Cooper <elicooper@....com>
>> > > 
>> > > I tested this patch and it does *not* solve my problem.
>> > 
>> > I'm torn on this patch, because it looked exactly like it would solve the
>> > kind of problem Stephen is running into.
>> > 
>> > Even though it doesn't fix his case, it seems correct to me.
>> > 
>> > I was wondering if it was also important to set the skb->protocol
>> > before the call to ip_tunnel_encap() but I couldn't find a dependency.
>> > 
>> > In any event I'd like to see some other people review this change
>> > before I apply it.
>> > 
>> > My only other guess for Stephen's problem is somehow the SKB headers
>> > aren't set up properly for what the GSO engine expects.
>> 
>> Well, mlx4 just works, and uses GSO engine just fine.
>> 
>> So my guess is this is a bug in Intel IGB driver.
> 
> About Eli patch : I do not believe it is needed.
> 
> Here is the path followed by SIT packet being GSO at the device layer :
> 
> We can see that ip_output() was called, and ip_output() already does :
> 
> skb->protocol = htons(ETH_P_IP);

Hmmm, ip6_finish_output2() also does a proper assignment of skb->protocol,
therefore why was commit ae148b085876fa771d9ef2c05f85d4b4bf09ce0d
("ip6_tunnel: Update skb->protocol to ETH_P_IPV6 in ip6_tnl_xmit()")
necessary at all?