[PATCH net 0/3] vti4, ip_tunnel: Fixes for MTU assignment and validation

[PATCH net 0/3] vti4, ip_tunnel: Fixes for MTU assignment and validation

[Stefano Brivio]

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

[PATCH net 1/3] vti4: Don't count header length twice on tunnel setup

[Stefano Brivio]

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.

Fixes: b9959fd3b0fa ("vti: switch to new ip tunnel code")
Fixes: f895f0cfbb77 ("Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Acked-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/ipv4/ip_vti.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
index 51b1669334fe..502e5222eaa9 100644
--- a/net/ipv4/ip_vti.c
+++ b/net/ipv4/ip_vti.c
@@ -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

[PATCH net 2/3] ip_tunnel: Clamp MTU to bounds on new link

[Stefano Brivio]

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").

Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Acked-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/ipv4/ip_tunnel.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 6d21068f9b55..7c76dd17b6b9 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -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

[PATCH net 3/3] vti4: Don't override MTU passed on link creation via IFLA_MTU

[Stefano Brivio]

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").

Fixes: 1181412c1a67 ("net/ipv4: VTI support new module for ip_vti.")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Acked-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/ipv4/ip_vti.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
index 502e5222eaa9..3f091ccad9af 100644
--- a/net/ipv4/ip_vti.c
+++ b/net/ipv4/ip_vti.c
@@ -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

Re: [PATCH net 0/3] vti4, ip_tunnel: Fixes for MTU assignment and validation

[David Miller]

From: Stefano Brivio <sbrivio@redhat.com>
Date: Thu, 15 Mar 2018 17:16:26 +0100

> 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.

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

Thank you.

Re: [PATCH net 0/3] vti4, ip_tunnel: Fixes for MTU assignment and validation

[Steffen Klassert]

On Sat, Mar 17, 2018 at 07:46:13PM -0400, David Miller wrote:
> From: Stefano Brivio <sbrivio@redhat.com>
> Date: Thu, 15 Mar 2018 17:16:26 +0100
> 
> > 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.
> 
> Steffen, I am assuming that you will pick up this series and the
> subsequent one dealing with vti6.

Yes, series is now applied to the ipsec tree.

Thanks a lot!