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: <20200629232235.6047a9c1@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 29 Jun 2020 23:22:35 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Oliver Herms <oliver.peter.herms@...il.com>
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 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()...

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.

> This patch also corrects the calculation of the payload's packet size.
> 
> Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> Signed-off-by: Oliver Herms <oliver.peter.herms@...il.com>

All in all, I think it's the SIT code that needs work, not ip_tunnel.