vti4, ip_tunnel: Fixes for MTU assignment and validation

Patch 1/3 re-introduces a fix to ensure that default MTU on new link is not lowered unnecessarily because of double counting of headers. This fix was originally introduced in 2014 and got lost in a merge commit shortly afterwards.

Patches 2/3 and 3/3 ensure that MTU passed from userspace on link creation is taken into account and also properly validated.

Stefano Brivio (3):
vti4: Don't count header length twice on tunnel setup
ip_tunnel: Clamp MTU to bounds on new link
vti4: Don't override MTU passed on link creation via IFLA_MTU

 net/ipv4/ip_tunnel.c | 8 +++++++-
 net/ipv4/ip_vti.c    | 2 --
 2 files changed, 7 insertions(+), 3 deletions(-)

2.15.1

This re-introduces the effect of commit a32452366b72 ("vti4:
Don't count header length twice.") which was accidentally reverted by merge commit f895f0cfbb77 ("Merge branch 'master' of
git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec").

The commit message from Steffen Klassert said:

We currently count the size of LL_MAX_HEADER and struct iphdr twice for vti4 devices, this leads to a wrong device mtu. The size of LL_MAX_HEADER and struct iphdr is already counted in ip_tunnel_bind_dev(), so don't do it again in vti_tunnel_init().

And this is still the case now: ip_tunnel_bind_dev() already accounts for the header length of the link layer (not necessarily LL_MAX_HEADER, if the output device is found), plus one IP header.

For example, with a vti device on top of veth, with MTU of 1500, the existing implementation would set the initial vti MTU to 1332, accounting once for LL_MAX_HEADER (128, included in hard_header_len by vti) and twice for the same IP header (once from hard_header_len, once from ip_tunnel_bind_dev()).

It should instead be 1480, because ip_tunnel_bind_dev() is able to figure out that the output device is veth, so no additional link layer header is attached, and will properly count one single IP header.

The existing issue had the side effect of avoiding PMTUD for most xfrm policies, by arbitrarily lowering the initial MTU. However, the only way to get a consistent PMTU value is to let the xfrm PMTU discovery do its course, and commit d6af1a31cc72 ("vti: Add pmtu handling to vti_xmit.") now takes care of local delivery cases where the application ignores local socket notifications.

---
 net/ipv4/ip_vti.c | 1 -
 1 file changed, 1 deletion(-)

index 51b1669334fe..502e5222eaa9 100644
@@ -387,7 +387,6 @@ static int vti_tunnel_init(struct net_device *dev)
 	memcpy(dev->dev_addr, &iph->saddr, 4);
 	memcpy(dev->broadcast, &iph->daddr, 4);
 
-	dev->hard_header_len	= LL_MAX_HEADER + sizeof(struct iphdr);
 	dev->mtu		= ETH_DATA_LEN;
 	dev->flags		= IFF_NOARP;
 	dev->addr_len		= 4;

2.15.1

Otherwise, it's possible to specify invalid MTU values directly on creation of a link (via 'ip link add'). This is already prevented on subsequent MTU changes by commit b96f9afee4eb ("ipv4/6: use core net MTU range checking").

---
 net/ipv4/ip_tunnel.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

index 6d21068f9b55..7c76dd17b6b9 100644
@@ -1108,8 +1108,14 @@ int ip_tunnel_newlink(struct net_device *dev, struct nlattr *tb[],
 		eth_hw_addr_random(dev);
 
 	mtu = ip_tunnel_bind_dev(dev);
-	if (!tb[IFLA_MTU])
+	if (tb[IFLA_MTU]) {
+		unsigned int max = 0xfff8 - dev->hard_header_len - nt->hlen;
+
+		dev->mtu = clamp(dev->mtu, (unsigned int)ETH_MIN_MTU,
+				 (unsigned int)(max - sizeof(struct iphdr)));
+	} else {
 		dev->mtu = mtu;
+	}
 
 	ip_tunnel_add(itn, nt);
 out:

2.15.1

Don't hardcode a MTU value on vti tunnel initialization, ip_tunnel_newlink() is able to deal with this already. See also commit ffc2b6ee4174 ("ip_gre: fix IFLA_MTU ignored on NEWLINK").

---
 net/ipv4/ip_vti.c | 1 -
 1 file changed, 1 deletion(-)

index 502e5222eaa9..3f091ccad9af 100644
@@ -387,7 +387,6 @@ static int vti_tunnel_init(struct net_device *dev)
 	memcpy(dev->dev_addr, &iph->saddr, 4);
 	memcpy(dev->broadcast, &iph->daddr, 4);
 
-	dev->mtu		= ETH_DATA_LEN;
 	dev->flags		= IFF_NOARP;
 	dev->addr_len		= 4;
 	dev->features		|= NETIF_F_LLTX;

2.15.1

From: Stefano Brivio <[email protected]>
Date: Thu, 15 Mar 2018 17:16:26 +0100

Steffen, I am assuming that you will pick up this series and the subsequent one dealing with vti6.

Thank you.

Yes, series is now applied to the ipsec tree.

Thanks a lot!