[PATCH net 0/3] ip_gre: fix header lengths and validation on changelink

[PATCH net 0/3] ip_gre: fix header lengths and validation on changelink

[Eric Dumazet]

Three fixes in the IPv4 GRE/ERSPAN changelink path, found while preparing
an RCU conversion of the IPv4 tunnel configuration. They all come from
the same place: ipgre_changelink() and erspan_changelink() mutate the
live device as they go, without keeping tunnel->hlen,
dev->needed_headroom and dev->mtu in sync.

Patch 1 makes the netlink parsers all-or-nothing. They write into the
live tunnel before all attributes have been validated, so a rejected
request leaves it half updated; in the worst case dev->type is left at
ARPHRD_NONE and the interface is broken for good.

Patch 2 stops maintaining the device lengths as a difference.
ipgre_link_update() adjusts them by a delta computed from tun_hlen only,
while ip_tunnel_bind_dev() assigns the same fields from tunnel->hlen.
Two writers, two models, and a delta that ignores the encapsulation, is
applied on top of the absolute assignment when the link changes too, and
is computed from a length ip_tunnel_encap_setup() may have published for
a request that then failed. tunnel->hlen is now recomputed from tun_hlen
and encap_hlen, and ip_tunnel_bind_dev() becomes the only writer of the
device lengths. Not a memory safety issue: ip_tunnel_xmit() computes its
own headroom for the encapsulation, only the advertised MTU is wrong.

Patch 3 gives ERSPAN the same treatment, where it does crash:
erspan_xmit() reserves dev->needed_headroom with skb_cow_head() and then
pushes a header sized from the current version, so going from version 0
to version 2 adds 20 bytes and can reach skb_under_panic().

Notes for reviewers, because not all bugs are fixed.

When the header length really changes, dev->mtu
is now recomputed by ip_tunnel_bind_dev() instead of being shifted by
the difference, as ip_tunnel_update() already does for a link or fwmark
change; a MTU configured by the user still survives a request that does
not change the header length.

And the changelink paths still commit into the live tunnel step by step.
A rejected request is therefore not a no-op, and since the xmit path is
lockless, a concurrent erspan_xmit() can briefly see a new erspan_ver
while dev->needed_headroom still describes the old one. Both are
pre-existing. These patches shrink the second one from permanent to the
duration of a single changelink, since erspan_changelink() does not
refresh the lengths at all today, but closing it means publishing a whole
new configuration atomically. That needs a larger rework and will come
with the ip_tunnel RCU conversion in net-next.

Eric Dumazet (3):
  ip_gre: validate netlink attributes before changing the tunnel
  ip_gre: compute tunnel lengths absolutely instead of by delta
  ip_gre: recompute erspan header lengths after a change

 include/net/ip_tunnels.h |   1 +
 net/ipv4/ip_gre.c        | 146 +++++++++++++++++++++++++++------------
 net/ipv4/ip_tunnel.c     |  16 +++++
 3 files changed, 120 insertions(+), 43 deletions(-)

-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH net 1/3] ip_gre: validate netlink attributes before changing the tunnel

[Eric Dumazet]

ipgre_netlink_parms() and erspan_netlink_parms() write into the live
tunnel before all attributes have been validated, so a rejected
changelink leaves it half updated.

A request carrying IFLA_GRE_COLLECT_METADATA and an invalid
IFLA_GRE_IGNORE_DF returns -EINVAL, but dev->type has already become
ARPHRD_NONE, breaking the interface for good.

Parse the ERSPAN attributes into local variables and commit them only
once everything is validated, hence erspan_netlink_parms() now calls
ipgre_netlink_parms() last. Same reason for moving
IFLA_GRE_COLLECT_METADATA after the IFLA_GRE_IGNORE_DF validation.

Only the parsers become all-or-nothing: ip_tunnel_encap_setup() still
runs before them, ip_tunnel_changelink() after them.

Fixes: e271c7b4420d ("gre: do not keep the GRE header around in collect medata mode")
Fixes: 84e54fe0a5ea ("gre: introduce native tunnel support for ERSPAN")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/ip_gre.c | 52 +++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 20 deletions(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 82309efd417e0f1f6554e7028be8e05d769e932d..40b922362a7ca4e8ee0c3ee6c0af8d4a9fdc42d3 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1233,12 +1233,6 @@ static int ipgre_netlink_parms(struct net_device *dev,
 		parms->iph.frag_off = htons(IP_DF);
 	}
 
-	if (data[IFLA_GRE_COLLECT_METADATA]) {
-		t->collect_md = true;
-		if (dev->type == ARPHRD_IPGRE)
-			dev->type = ARPHRD_NONE;
-	}
-
 	if (data[IFLA_GRE_IGNORE_DF]) {
 		if (nla_get_u8(data[IFLA_GRE_IGNORE_DF])
 		  && (parms->iph.frag_off & htons(IP_DF)))
@@ -1246,6 +1240,13 @@ static int ipgre_netlink_parms(struct net_device *dev,
 		t->ignore_df = !!nla_get_u8(data[IFLA_GRE_IGNORE_DF]);
 	}
 
+	/* All attributes have been validated, we can change @dev and @t. */
+	if (data[IFLA_GRE_COLLECT_METADATA]) {
+		t->collect_md = true;
+		if (dev->type == ARPHRD_IPGRE)
+			dev->type = ARPHRD_NONE;
+	}
+
 	if (data[IFLA_GRE_FWMARK])
 		*fwmark = nla_get_u32(data[IFLA_GRE_FWMARK]);
 
@@ -1259,40 +1260,51 @@ static int erspan_netlink_parms(struct net_device *dev,
 				__u32 *fwmark)
 {
 	struct ip_tunnel *t = netdev_priv(dev);
+	u8 erspan_ver = t->erspan_ver;
+	u32 index = t->index;
+	u16 hwid = t->hwid;
+	u8 dir = t->dir;
 	int err;
 
-	err = ipgre_netlink_parms(dev, data, tb, parms, fwmark);
-	if (err)
-		return err;
 	if (!data)
-		return 0;
+		return ipgre_netlink_parms(dev, data, tb, parms, fwmark);
 
 	if (data[IFLA_GRE_ERSPAN_VER]) {
-		t->erspan_ver = nla_get_u8(data[IFLA_GRE_ERSPAN_VER]);
+		erspan_ver = nla_get_u8(data[IFLA_GRE_ERSPAN_VER]);
 
-		if (t->erspan_ver > 2)
+		if (erspan_ver > 2)
 			return -EINVAL;
 	}
 
-	if (t->erspan_ver == 1) {
+	if (erspan_ver == 1) {
 		if (data[IFLA_GRE_ERSPAN_INDEX]) {
-			t->index = nla_get_u32(data[IFLA_GRE_ERSPAN_INDEX]);
-			if (t->index & ~INDEX_MASK)
+			index = nla_get_u32(data[IFLA_GRE_ERSPAN_INDEX]);
+			if (index & ~INDEX_MASK)
 				return -EINVAL;
 		}
-	} else if (t->erspan_ver == 2) {
+	} else if (erspan_ver == 2) {
 		if (data[IFLA_GRE_ERSPAN_DIR]) {
-			t->dir = nla_get_u8(data[IFLA_GRE_ERSPAN_DIR]);
-			if (t->dir & ~(DIR_MASK >> DIR_OFFSET))
+			dir = nla_get_u8(data[IFLA_GRE_ERSPAN_DIR]);
+			if (dir & ~(DIR_MASK >> DIR_OFFSET))
 				return -EINVAL;
 		}
 		if (data[IFLA_GRE_ERSPAN_HWID]) {
-			t->hwid = nla_get_u16(data[IFLA_GRE_ERSPAN_HWID]);
-			if (t->hwid & ~(HWID_MASK >> HWID_OFFSET))
+			hwid = nla_get_u16(data[IFLA_GRE_ERSPAN_HWID]);
+			if (hwid & ~(HWID_MASK >> HWID_OFFSET))
 				return -EINVAL;
 		}
 	}
 
+	err = ipgre_netlink_parms(dev, data, tb, parms, fwmark);
+	if (err)
+		return err;
+
+	/* All attributes have been validated, we can change @t. */
+	t->erspan_ver = erspan_ver;
+	t->index = index;
+	t->hwid = hwid;
+	t->dir = dir;
+
 	return 0;
 }
 
-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH net 2/3] ip_gre: compute tunnel lengths absolutely instead of by delta

[Eric Dumazet]

ipgre_link_update() adjusts the device lengths by a difference it
computes from tun_hlen alone:

	len = tunnel->tun_hlen;
	tunnel->tun_hlen = gre_calc_hlen(tunnel->parms.o_flags);
	len = tunnel->tun_hlen - len;
	tunnel->hlen = tunnel->hlen + len;

But tunnel->hlen also contains encap_hlen, which ipgre_changelink() can
change through ipgre_newlink_encap_setup(). Such a request leaves @len
at zero: adding "encap fou" to an existing gre device keeps the MTU of a
bare tunnel, while creating it with "encap fou" from the start gets the
smaller MTU from ip_tunnel_bind_dev().

A difference is the wrong tool anyway: ip_tunnel_bind_dev() already
assigns dev->needed_headroom from tunnel->hlen, so changing the link and
the encapsulation at once is accounted twice, and ip_tunnel_encap_setup()
publishes a new tunnel->hlen before the request is validated, making the
next difference bogus.

Recompute tunnel->hlen from tun_hlen and encap_hlen, as
__gre_tunnel_init() does, and add ip_tunnel_refresh_lengths() so that
ip_tunnel_bind_dev() is the only writer of the device lengths.
ipgre_changelink() must then refresh on its error paths too, since the
new encapsulation is published by then.

The dev->header_ops branch is removed rather than converted:
ip_tunnel_bind_dev() does not add to dev->hard_header_len, it subtracts
it from the MTU as the inner Ethernet header.

@old_hlen survives only as a predicate telling whether the MTU became
stale, never as a difference, so it can not make the lengths drift. When
the header length does change, the MTU is now recomputed rather than
shifted, as ip_tunnel_update() already does for a link or fwmark change.

There is no memory safety issue: ip_tunnel_xmit() computes its own
headroom from ip_encap_hlen(&tunnel->encap) and calls skb_cow_head()
before pushing the encapsulation.

Fixes: dd9d598c6657 ("ip_gre: add the support for i/o_flags update via netlink")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/ip_tunnels.h |  1 +
 net/ipv4/ip_gre.c        | 46 +++++++++++++++++++++++++---------------
 net/ipv4/ip_tunnel.c     | 16 ++++++++++++++
 3 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 7c9aadfe8fe396da10a47e93499a97141ac04f4c..fd0396aa5039538a0391a352ace6d15f554df874 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -429,6 +429,7 @@ int ip_tunnel_newlink(struct net *net, struct net_device *dev,
 		      struct nlattr *tb[], struct ip_tunnel_parm_kern *p,
 		      __u32 fwmark);
 void ip_tunnel_setup(struct net_device *dev, unsigned int net_id);
+void ip_tunnel_refresh_lengths(struct net_device *dev, bool set_mtu);
 
 bool ip_tunnel_netlink_encap_parms(struct nlattr *data[],
 				   struct ip_tunnel_encap *encap);
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 40b922362a7ca4e8ee0c3ee6c0af8d4a9fdc42d3..556ebf2c5bd0d110290f2ed82ca1221dcc19c5f2 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -789,23 +789,28 @@ static netdev_tx_t gre_tap_xmit(struct sk_buff *skb,
 	return NETDEV_TX_OK;
 }
 
-static void ipgre_link_update(struct net_device *dev, bool set_mtu)
+/* tunnel->hlen depends on tunnel->parms.o_flags and on tunnel->encap_hlen,
+ * both of which ipgre_changelink() can change. Recompute it the way
+ * __gre_tunnel_init() does, then let ip_tunnel_bind_dev() derive the device
+ * lengths from it.
+ *
+ * @old_hlen is only used to tell whether the MTU became stale, never as a
+ * difference to apply, so it can not make the lengths drift. It must be
+ * sampled before ip_tunnel_encap_setup(), which already publishes the new
+ * tunnel->hlen for us.
+ */
+static void ipgre_link_update(struct net_device *dev, bool set_mtu,
+			      int old_hlen)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
-	int len;
 
-	len = tunnel->tun_hlen;
 	tunnel->tun_hlen = gre_calc_hlen(tunnel->parms.o_flags);
-	len = tunnel->tun_hlen - len;
-	tunnel->hlen = tunnel->hlen + len;
-
-	if (dev->header_ops)
-		dev->hard_header_len += len;
-	else
-		dev->needed_headroom += len;
+	tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen;
 
-	if (set_mtu)
-		WRITE_ONCE(dev->mtu, max_t(int, dev->mtu - len, 68));
+	/* Only reset a MTU that the header length just invalidated, so that
+	 * a MTU configured by the user survives an unrelated change.
+	 */
+	ip_tunnel_refresh_lengths(dev, set_mtu && tunnel->hlen != old_hlen);
 
 	if (test_bit(IP_TUNNEL_SEQ_BIT, tunnel->parms.o_flags) ||
 	    (test_bit(IP_TUNNEL_CSUM_BIT, tunnel->parms.o_flags) &&
@@ -853,7 +858,7 @@ static int ipgre_tunnel_ctl(struct net_device *dev,
 		ip_tunnel_flags_copy(t->parms.o_flags, p->o_flags);
 
 		if (strcmp(dev->rtnl_link_ops->kind, "erspan"))
-			ipgre_link_update(dev, true);
+			ipgre_link_update(dev, true, t->hlen);
 	}
 
 	i_flags = gre_tnl_flags_to_gre_flags(p->i_flags);
@@ -1471,6 +1476,7 @@ static int ipgre_changelink(struct net_device *dev, struct nlattr *tb[],
 	struct ip_tunnel *t = netdev_priv(dev);
 	struct ip_tunnel_parm_kern p;
 	__u32 fwmark = t->fwmark;
+	int old_hlen = t->hlen;
 	int err;
 
 	if (!rtnl_dev_link_net_capable(dev, t->net))
@@ -1482,18 +1488,24 @@ static int ipgre_changelink(struct net_device *dev, struct nlattr *tb[],
 
 	err = ipgre_netlink_parms(dev, data, tb, &p, &fwmark);
 	if (err < 0)
-		return err;
+		goto link_update;
 
 	err = ip_tunnel_changelink(dev, tb, &p, fwmark);
 	if (err < 0)
-		return err;
+		goto link_update;
 
 	ip_tunnel_flags_copy(t->parms.i_flags, p.i_flags);
 	ip_tunnel_flags_copy(t->parms.o_flags, p.o_flags);
 
-	ipgre_link_update(dev, !tb[IFLA_MTU]);
+link_update:
+	/* ipgre_newlink_encap_setup() has published a new encapsulation even
+	 * if the rest of the request failed, so the lengths must be refreshed
+	 * on the error paths as well. This has to come last, because
+	 * ipgre_link_update() needs the flags copied above.
+	 */
+	ipgre_link_update(dev, !tb[IFLA_MTU], old_hlen);
 
-	return 0;
+	return err;
 }
 
 static int erspan_changelink(struct net_device *dev, struct nlattr *tb[],
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index e6bcf01411d0bcd12cc9a88e449d9283c4a83c64..6b93c1fda4948d320ec00532ebb16c700590373d 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -326,6 +326,22 @@ static int ip_tunnel_bind_dev(struct net_device *dev)
 	return mtu;
 }
 
+/* Recompute dev->needed_headroom and dev->mtu after tunnel->hlen changed.
+ *
+ * Both are derived from tunnel->hlen, so they must be recomputed from it
+ * rather than adjusted by the difference: ip_tunnel_bind_dev() is also
+ * called from ndo_init() and from ip_tunnel_update(), and a driver adding
+ * its own delta on top would double count it.
+ */
+void ip_tunnel_refresh_lengths(struct net_device *dev, bool set_mtu)
+{
+	int mtu = ip_tunnel_bind_dev(dev);
+
+	if (set_mtu)
+		WRITE_ONCE(dev->mtu, mtu);
+}
+EXPORT_SYMBOL_GPL(ip_tunnel_refresh_lengths);
+
 static struct ip_tunnel *ip_tunnel_create(struct net *net,
 					  struct ip_tunnel_net *itn,
 					  struct ip_tunnel_parm_kern *parms)
-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH net 3/3] ip_gre: recompute erspan header lengths after a change

[Eric Dumazet]

erspan_tunnel_init() is the only place computing tunnel->tun_hlen and
tunnel->hlen, but erspan_changelink() can change both: tunnel->erspan_ver
selects a 4 or 8 byte GRE header and feeds erspan_hdr_len(), while
ip_tunnel_encap_setup() recomputes tunnel->hlen without the ERSPAN part.

dev->needed_headroom is not refreshed either, since ip_tunnel_update()
only rebinds when the link or the fwmark changes.

erspan_xmit() calls skb_cow_head(skb, dev->needed_headroom) before
erspan_build_header[_v2]() pushes the header. Going from version 0 to
version 2 adds 20 bytes, so a packet with little headroom can hit
skb_under_panic().

Move the computation into erspan_set_hlen() and add
erspan_link_update(), refreshing the lengths as the previous patch does
for plain GRE. It runs before the erspan_netlink_parms() error check and
before ip_tunnel_changelink(), because the new version is committed by
then and erspan_xmit() sizes its push from it.

Fixes: f551c91de262 ("net: erspan: introduce erspan v2 for ip_gre")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/ip_gre.c | 48 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 556ebf2c5bd0d110290f2ed82ca1221dcc19c5f2..4807fc6d2fd886621480dcff20f8691ec32c67bc 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1368,18 +1368,43 @@ static const struct net_device_ops gre_tap_netdev_ops = {
 	.ndo_fill_metadata_dst	= gre_fill_metadata_dst,
 };
 
+static void erspan_set_hlen(struct ip_tunnel *tunnel)
+{
+	/* Version 0 uses a 4-byte GRE header, other versions use 8 bytes. */
+	tunnel->tun_hlen = tunnel->erspan_ver == 0 ? 4 : 8;
+
+	tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen +
+		       erspan_hdr_len(tunnel->erspan_ver);
+}
+
+/* Both tunnel->erspan_ver and tunnel->encap_hlen can be changed from
+ * erspan_changelink(), and both feed tunnel->hlen. Recompute it, then let
+ * ip_tunnel_bind_dev() derive the device lengths from it.
+ *
+ * As in ipgre_link_update(), @old_hlen only tells whether the MTU became
+ * stale and must be sampled before ip_tunnel_encap_setup(), which
+ * recomputes tunnel->hlen without the ERSPAN part.
+ */
+static void erspan_link_update(struct net_device *dev, bool set_mtu,
+			       int old_hlen)
+{
+	struct ip_tunnel *tunnel = netdev_priv(dev);
+
+	erspan_set_hlen(tunnel);
+
+	/* Only reset a MTU that the header length just invalidated, so that
+	 * a MTU configured by the user survives an unrelated change.
+	 */
+	ip_tunnel_refresh_lengths(dev, set_mtu && tunnel->hlen != old_hlen);
+}
+
 static int erspan_tunnel_init(struct net_device *dev)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
 
-	if (tunnel->erspan_ver == 0)
-		tunnel->tun_hlen = 4; /* 4-byte GRE hdr. */
-	else
-		tunnel->tun_hlen = 8; /* 8-byte GRE hdr. */
+	erspan_set_hlen(tunnel);
 
 	tunnel->parms.iph.protocol = IPPROTO_GRE;
-	tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen +
-		       erspan_hdr_len(tunnel->erspan_ver);
 
 	dev->features		|= GRE_FEATURES;
 	dev->hw_features	|= GRE_FEATURES;
@@ -1515,6 +1540,7 @@ static int erspan_changelink(struct net_device *dev, struct nlattr *tb[],
 	struct ip_tunnel *t = netdev_priv(dev);
 	struct ip_tunnel_parm_kern p;
 	__u32 fwmark = t->fwmark;
+	int old_hlen = t->hlen;
 	int err;
 
 	if (!rtnl_dev_link_net_capable(dev, t->net))
@@ -1525,6 +1551,16 @@ static int erspan_changelink(struct net_device *dev, struct nlattr *tb[],
 		return err;
 
 	err = erspan_netlink_parms(dev, data, tb, &p, &fwmark);
+
+	/* ipgre_newlink_encap_setup() has published a new encapsulation, and
+	 * erspan_netlink_parms() a new ERSPAN version, both of which change
+	 * the header length. Refresh the lengths before looking at @err:
+	 * erspan_xmit() sizes its push from tunnel->erspan_ver, and both this
+	 * error path and ip_tunnel_changelink() below leave the new
+	 * encapsulation behind.
+	 */
+	erspan_link_update(dev, !tb[IFLA_MTU], old_hlen);
+
 	if (err < 0)
 		return err;
 
-- 
2.55.0.1007.g17ff1f9808-goog