netdev - Re: [PATCH v3] IPv4: Tunnel: Fix effective path mtu calculation

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]

Message-ID: <20e25b9c-db3c-e6cd-f383-aa4ac84a2177@gmail.com>
Date:   Tue, 30 Jun 2020 12:21:14 +0200
From:   Oliver Herms <oliver.peter.herms@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuznet@....inr.ac.ru,
        yoshfuji@...ux-ipv6.org
Subject: Re: [PATCH v3] IPv4: Tunnel: Fix effective path mtu calculation

On 30.06.20 08:22, Jakub Kicinski wrote:
> On Fri, 26 Jun 2020 00:44:35 +0200 Oliver Herms wrote:
>> The calculation of the effective tunnel mtu, that is used to create
>> mtu exceptions if necessary, is currently not done correctly. This
>> leads to unnecessary entries in the IPv6 route cache for any
>> packet send through the tunnel.
>>
>> The root cause is, that "dev->hard_header_len" is subtracted from the
>> tunnel destionations path mtu. Thus subtracting too much, if
>> dev->hard_header_len is filled in. This is that case for SIT tunnels
>> where hard_header_len is the underlyings dev hard_header_len (e.g. 14
>> for ethernet) + 20 bytes IP header (see net/ipv6/sit.c:1091).
> 
> It seems like SIT possibly got missed in evolution of the ip_tunnel
> code? It seems to duplicate a lot of code, including pmtu checking.
> Doesn't call ip_tunnel_init()...

Are you open for patches cleaning this up?

> 
> My understanding is that for a while now tunnels are not supposed to use
> dev->hard_header_len to reserve skb space, and use dev->needed_headroom, 
> instead. sit uses hard_header_len and doesn't even copy needed_headroom
> of the lower device.
> 
>> However, the MTU of the path is exclusive of the ethernet header
>> and the 20 bytes for the IP header are being subtracted separately
>> already. Thus hard_header_len is removed from this calculation.
>>
>> For IPIP and GRE tunnels this doesn't change anything as
>> hard_header_len is zero in those cases anyways.
> 
> This statement is definitely not true. Please see the calls to
> ether_setup() in ip_gre.c, and the implementation of this function
Right. I have to admit I've only checked for L3 tunnels using printk
on dev->hard_header_len. Showing 0 for IPIP and GRE.

So shall I file a patch that changes hard_header_len for SIT tunnels to 0?