linux-kernel - Re: [PATCH 4.4 92/97] ip6_vti: adjust vti mtu according to mtu of lower device

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:   Thu, 5 Apr 2018 17:36:36 +0200
From:   Stefano Brivio <sbrivio@...hat.com>
To:     Ben Hutchings <ben.hutchings@...ethink.co.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Petr Vorel <pvorel@...e.cz>,
        Alexey Kodanev <alexey.kodanev@...cle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Sasha Levin <alexander.levin@...rosoft.com>,
        Steffen Klassert <steffen.klassert@...unet.com>
Subject: Re: [PATCH 4.4 92/97] ip6_vti: adjust vti mtu according to mtu of
 lower device

On Wed, 04 Apr 2018 01:09:16 +0100
Ben Hutchings <ben.hutchings@...ethink.co.uk> wrote:

> On Fri, 2018-03-23 at 10:55 +0100, Greg Kroah-Hartman wrote:
> > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Alexey Kodanev <alexey.kodanev@...cle.com>
> > 
> > 
> > [ Upstream commit 53c81e95df1793933f87748d36070a721f6cb287 ]  
> [...]
> 
> There are a couple of follow-ups to this:
> 
> c6741fbed6dc vti6: Properly adjust vti6 MTU from MTU of lower device
> 7a67e69a339a vti6: Keep set MTU on link creation or change, validate it
> 
> The second of those will fail to build on branches older than 4.10
> though.  It might be better to revert this one instead.

Thanks Ben for spotting this.

Actually,
  53c81e95df17 ("ip6_vti: adjust vti mtu according to mtu of lower device")
alone improves things already, despite being "fixed" by
  c6741fbed6dc ("vti6: Properly adjust vti6 MTU from MTU of lower device")

With just 53c81e95df17 the MTU of a vti6 interface will be somewhat
linked to the MTU of the lower layer, but will be underestimated.

With c6741fbed6dc the calculation of MTU from lower layer will be
accurate instead.

However, without
  7a67e69a339a ("vti6: Keep set MTU on link creation or change, validate it")
but with
  53c81e95df17 ("ip6_vti: adjust vti mtu according to mtu of lower device")
assignment of MTU on link change is discarded, so this would actually
introduce a bug.

Fixing
  7a67e69a339a ("vti6: Keep set MTU on link creation or change, validate it")
for 4.4 up to 4.9 is trivial, we simply need to adjust for the lack of
  b96f9afee4eb ("ipv4/6: use core net MTU range checking")
and reflect the change introduced by
  f8a554b4aa96 ("vti6: Fix dev->max_mtu setting").

So, Greg, here comes the backport of
  7a67e69a339a ("vti6: Keep set MTU on link creation or change, validate it")
based on latest linux-4.4.y branch, in case you want to keep the existing
change and add the follow-ups on top. Please let me know if I should submit
it formally.

diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
index 176e79076fb1..f388f54b4162 100644
--- a/net/ipv6/ip6_vti.c
+++ b/net/ipv6/ip6_vti.c
@@ -610,7 +610,7 @@ static int vti6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	return 0;
 }
 
-static void vti6_link_config(struct ip6_tnl *t)
+static void vti6_link_config(struct ip6_tnl *t, bool keep_mtu)
 {
 	struct net_device *dev = t->dev;
 	struct __ip6_tnl_parm *p = &t->parms;
@@ -629,6 +629,13 @@ static void vti6_link_config(struct ip6_tnl *t)
 	else
 		dev->flags &= ~IFF_POINTOPOINT;
 
+	if (keep_mtu && dev->mtu) {
+		dev->mtu = clamp(dev->mtu, (unsigned)IPV6_MIN_MTU,
+				 (unsigned)(IP_MAX_MTU -
+					    sizeof(struct ipv6hdr)));
+		return;
+	}
+
 	if (p->flags & IP6_TNL_F_CAP_XMIT) {
 		int strict = (ipv6_addr_type(&p->raddr) &
 			      (IPV6_ADDR_MULTICAST | IPV6_ADDR_LINKLOCAL));
@@ -656,12 +663,14 @@ static void vti6_link_config(struct ip6_tnl *t)
  * vti6_tnl_change - update the tunnel parameters
  *   @t: tunnel to be changed
  *   @p: tunnel configuration parameters
+ *   @keep_mtu: MTU was set from userspace, don't re-compute it
  *
  * Description:
  *   vti6_tnl_change() updates the tunnel parameters
  **/
 static int
-vti6_tnl_change(struct ip6_tnl *t, const struct __ip6_tnl_parm *p)
+vti6_tnl_change(struct ip6_tnl *t, const struct __ip6_tnl_parm *p,
+		bool keep_mtu)
 {
 	t->parms.laddr = p->laddr;
 	t->parms.raddr = p->raddr;
@@ -670,11 +679,12 @@ vti6_tnl_change(struct ip6_tnl *t, const struct __ip6_tnl_parm *p)
 	t->parms.o_key = p->o_key;
 	t->parms.proto = p->proto;
 	dst_cache_reset(&t->dst_cache);
-	vti6_link_config(t);
+	vti6_link_config(t, keep_mtu);
 	return 0;
 }
 
-static int vti6_update(struct ip6_tnl *t, struct __ip6_tnl_parm *p)
+static int vti6_update(struct ip6_tnl *t, struct __ip6_tnl_parm *p,
+		       bool keep_mtu)
 {
 	struct net *net = dev_net(t->dev);
 	struct vti6_net *ip6n = net_generic(net, vti6_net_id);
@@ -682,7 +692,7 @@ static int vti6_update(struct ip6_tnl *t, struct __ip6_tnl_parm *p)
 
 	vti6_tnl_unlink(ip6n, t);
 	synchronize_net();
-	err = vti6_tnl_change(t, p);
+	err = vti6_tnl_change(t, p, keep_mtu);
 	vti6_tnl_link(ip6n, t);
 	netdev_state_change(t->dev);
 	return err;
@@ -795,7 +805,7 @@ vti6_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 			} else
 				t = netdev_priv(dev);
 
-			err = vti6_update(t, &p1);
+			err = vti6_update(t, &p1, false);
 		}
 		if (t) {
 			err = 0;
@@ -907,7 +917,7 @@ static int vti6_dev_init(struct net_device *dev)
 
 	if (err)
 		return err;
-	vti6_link_config(t);
+	vti6_link_config(t, true);
 	return 0;
 }
 
@@ -1006,7 +1016,7 @@ static int vti6_changelink(struct net_device *dev, struct nlattr *tb[],
 	} else
 		t = netdev_priv(dev);
 
-	return vti6_update(t, &p);
+	return vti6_update(t, &p, tb && tb[IFLA_MTU]);
 }
 
 static size_t vti6_get_size(const struct net_device *dev)


-- 
Stefano