[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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] The patch does not close the bug class its commit message claims
  to fix: a rejected GRE/ERSPAN changelink can still leave the live…

--

ip_gre: validate netlink attributes before changing the tunnel

The patch reorders ipgre_netlink_parms() and erspan_netlink_parms() so that
attribute validation happens before the live tunnel is touched.  The ERSPAN
attributes are parsed into local variables and copied into the tunnel at the
end, and the IFLA_GRE_COLLECT_METADATA handling is moved after the
IFLA_GRE_IGNORE_DF validation.

> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 82309efd417e0..40b922362a7ca 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;
> +	}
> +

[Severity: High]
The new comment states that the attributes have been validated so @dev and
@t may be changed, but the changelink path still has a fallible step after
the parser returns:

net/ipv4/ip_gre.c:ipgre_changelink() {
	...
	err = ipgre_netlink_parms(dev, data, tb, &p, &fwmark);
	if (err < 0)
		return err;

	err = ip_tunnel_changelink(dev, tb, &p, fwmark);
	if (err < 0)
		return err;
	...
}

and ip_tunnel_changelink() has failure exits with no rollback:

net/ipv4/ip_tunnel.c:ip_tunnel_changelink() {
	if (dev == itn->fb_tunnel_dev)
		return -EINVAL;

	t = ip_tunnel_find(itn, p, dev->type);

	if (t) {
		if (t->dev != dev)
			return -EEXIST;
	} else {
		t = tunnel;

		if (dev->type != ARPHRD_ETHER) {
			unsigned int nflags = 0;
			...
			if ((dev->flags ^ nflags) &
			    (IFF_POINTOPOINT | IFF_BROADCAST))
				return -EINVAL;
		}
	}
	...
}

So does the exact symptom described in the changelog remain reachable?

"dev->type has already become ARPHRD_NONE, breaking the interface for good"

For example, "ip link set gre0 type gre external" on the per-netns fallback
device hits the "dev == itn->fb_tunnel_dev" return -EINVAL, yet the parser
has already set dev->type = ARPHRD_NONE and t->collect_md = true on gre0.
The fallback device is created by __ip_tunnel_create() with
dev->rtnl_link_ops = ipgre_link_ops, so ->changelink is dispatched to it.

The same window applies to an ordinary gre device: one created with
"remote 1.1.1.1" has IFF_POINTOPOINT set, so a request without
IFLA_GRE_REMOTE computes nflags == 0 and returns -EINVAL from the flag
mismatch check, again after collect_md/ARPHRD_NONE have been committed.

Note also that ip_tunnel_find(itn, p, dev->type) and the
"dev->type != ARPHRD_ETHER" test are evaluated against the dev->type the
parser just mutated, i.e. against state the request was never allowed to
commit.

Similarly, t->ignore_df is still committed in the hunk above while the
parms carrying the matching frag_off are discarded when
ip_tunnel_changelink() fails.

Would staging dev->type/collect_md/ignore_df in locals and applying them
only after ip_tunnel_changelink() succeeds (the way net/ipv6/ip6_gre.c
works on a local parm struct) close this, rather than moving the commit
point inside the parser?

>  	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;
> +
> +	/* 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;
>  }

The staged ERSPAN values are stored into the live tunnel here, before
erspan_changelink() calls ip_tunnel_changelink():

net/ipv4/ip_gre.c:erspan_changelink() {
	...
	err = erspan_netlink_parms(dev, data, tb, &p, &fwmark);
	if (err < 0)
		return err;

	err = ip_tunnel_changelink(dev, tb, &p, fwmark);
	if (err < 0)
		return err;
	...
}

Can a request that is reported to user space as failed still change the
wire format emitted by erspan_xmit(), since erspan_ver, index, dir and hwid
are already in t when ip_tunnel_changelink() returns -EINVAL or -EEXIST?

One more note on the changelog:

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

Given that ipgre_newlink_encap_setup() -> ip_tunnel_encap_setup() clears and
rewrites t->encap, t->encap_hlen and t->hlen before the parsers run, and
that ip_tunnel_changelink() can still fail afterwards, is "all-or-nothing"
accurate for the tunnel state as observed by user space?  A stable
backporter reading the changelog would likely conclude the ARPHRD_NONE
breakage is closed.

Checking the rest of the series, the later patches add header length
refreshes on the error paths but do not undo the dev->type, collect_md or
erspan_ver commits, so the state above appears to persist at the end of the
series as well.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260912150944.3470971-1-edumazet%40google.com

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 · Low: 3 (1)

- [High] Removing the `if (dev->header_ops) dev->hard_header_len += len;`
  branch from ipgre_link_update() (net/ipv4/ip_gre.c:802-816) leaves…
- [Medium] ipgre_changelink() (and erspan_changelink()) still return
  immediately when ipgre_newlink_encap_setup() fails, which is the one…
- [Low] In ipgre_changelink() the refresh at the new `link_update:` label
  passes set_mtu = !tb[IFLA_MTU], so the mere presence of IFLA_MTU…
- [Low] The new predicate `set_mtu && tunnel->hlen != old_hlen` in
  ipgre_link_update() (net/ipv4/ip_gre.c:813) compares the final hlen…
- [Low] The new comment on ip_tunnel_refresh_lengths()
  (net/ipv4/ip_tunnel.c:329-335) states that "ip_tunnel_bind_dev() is…

Pre-existing issues:
- [Low] erspan_changelink() publishes a new encapsulation
  (ipgre_newlink_encap_setup() -> ip_tunnel_encap_setup(), which sets…

--

ip_gre: compute tunnel lengths absolutely instead of by delta

This replaces the delta-based length adjustment in ipgre_link_update() with
a full recomputation of tunnel->hlen from tun_hlen and encap_hlen, and adds
ip_tunnel_refresh_lengths() so that ip_tunnel_bind_dev() becomes the only
writer of dev->needed_headroom and dev->mtu.  ipgre_changelink() now also
refreshes the lengths on its error paths.

A few questions about the new code below.

> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 40b922362a7ca..556ebf2c5bd0d 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;

[Severity: High]
Was dropping the dev->hard_header_len update intended here?  That branch
was the only thing keeping dev->hard_header_len in sync for ARPHRD_IPGRE
devices that install ipgre_header_ops.

ipgre_tunnel_init() sets it up as the outer IP+GRE header for those
devices:

	} else if (!tunnel->collect_md) {
		dev->header_ops = &ipgre_header_ops;
		dev->hard_header_len = tunnel->hlen + sizeof(*iph);
		dev->needed_headroom = 0;
	}

and ipgre_xmit() consumes exactly that many bytes:

	if (dev->header_ops) {
		int pull_len = tunnel->hlen + sizeof(struct iphdr);
		...
		if (!pskb_may_pull(skb, pull_len))
			goto free_skb;
		...
		skb_pull(skb, pull_len);

After this change, an o_flags change through ipgre_changelink() or
SIOCCHGTUNNEL updates tunnel->tun_hlen and tunnel->hlen and, via
ip_tunnel_refresh_lengths() -> ip_tunnel_bind_dev(), dev->needed_headroom
and dev->mtu, but nothing writes dev->hard_header_len again.
ip_tunnel_bind_dev() only reads it, and only for ARPHRD_ETHER:

	dev->needed_headroom = ip_tunnel_limit_headroom(t_hlen + hlen);
	mtu -= t_hlen + (dev->type == ARPHRD_ETHER ? dev->hard_header_len : 0);

So with

	ip link add gre1 type gre local 10.0.0.1
	ip link set gre1 type gre local 10.0.0.1 okey 1 ikey 1

tunnel->hlen becomes 8 while dev->hard_header_len stays 24, permanently.

Does that break AF_PACKET senders on such a device?  ipgre_header_ops has
no .validate callback:

	static const struct header_ops ipgre_header_ops = {
		.create	= ipgre_header,
		.parse	= ipgre_header_parse,
	};

so dev_validate_header() rejects a correctly sized raw link header once
hlen shrinks (nocsum/nokey), and when hlen grows (okey/oseq) a header
built to the advertised, now too small, hard_header_len passes validation
while ipgre_xmit() still pulls tunnel->hlen + sizeof(struct iphdr) bytes,
taking payload bytes into the header.

The commit message says:

    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.

Is that statement only true for gretap (ARPHRD_ETHER)?  For the
ipgre_header_ops devices, dev->hard_header_len is the outer IP+GRE length,
not an inner Ethernet header.

>  
> -	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);

[Severity: Low]
Can this predicate miss a stale MTU when a single request changes the
encapsulation, the GRE flags and the link at once?  @old_hlen is the hlen
sampled before the request, not the hlen that dev->mtu was actually
computed from.

ip_tunnel_encap_setup() publishes an intermediate length first:

	t->encap_hlen = hlen;
	t->hlen = t->encap_hlen + t->tun_hlen;

then ip_tunnel_changelink() -> ip_tunnel_update() writes the MTU from that
intermediate value:

	if (t->parms.link != p->link || t->fwmark != fwmark) {
		...
		mtu = ip_tunnel_bind_dev(dev);
		if (set_mtu)
			WRITE_ONCE(dev->mtu, mtu);
	}

For a device with csum|key (tun_hlen 12, encap_hlen 0, hlen 12) and

	ip link set gre1 type gre ... nocsum nokey encap fou \
		encap-sport A encap-dport B dev <other>

encap_hlen becomes 8 and the MTU is written from hlen 20.  ipgre_link_update()
then computes tun_hlen 4 and hlen 12, which equals @old_hlen, so the
corrective MTU write is suppressed and dev->mtu stays 8 bytes below the
correct value.  dev->needed_headroom is fine, since
ip_tunnel_refresh_lengths() always calls ip_tunnel_bind_dev().

The deleted delta code did correct this case, so does

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

still hold for dev->mtu here?

>  
>  	if (test_bit(IP_TUNNEL_SEQ_BIT, tunnel->parms.o_flags) ||
>  	    (test_bit(IP_TUNNEL_CSUM_BIT, tunnel->parms.o_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);

[Severity: Low]
Should set_mtu still be !tb[IFLA_MTU] when arriving here from an error
path?  On the error paths the requested MTU is never applied, because
rtnl_changelink() returns as soon as changelink fails:

	err = ops->changelink(dev, tb, data, extack);
	if (err < 0)
		return err;

so do_setlink() does not run.  The mere presence of IFLA_MTU then
suppresses the recomputation, and ip_tunnel_refresh_lengths() throws the
computed value away:

	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);
	}

With, for instance,

	ip link set gre1 type gre encap fou encap-sport 1 encap-dport 2 \
		pmtudisc ignore-df mtu 1400

the encapsulation is committed, ipgre_netlink_parms() returns -EINVAL for
the IFLA_GRE_IGNORE_DF plus DF combination, and the goto runs with
set_mtu false, leaving dev->needed_headroom derived from the new header
length and dev->mtu from the old one.

>  
> -	return 0;
> +	return err;
>  }

[Severity: Medium]
There is one error return left above the new label that also runs after
the encapsulation was mutated:

	err = ipgre_newlink_encap_setup(dev, data);
	if (err)
		return err;

ip_tunnel_encap_setup() zeroes the live encapsulation before it validates
the candidate:

	memset(&t->encap, 0, sizeof(t->encap));

	hlen = ip_encap_hlen(ipencap);
	if (hlen < 0)
		return hlen;

so on failure the device keeps encap.type == TUNNEL_ENCAP_NONE while
t->encap_hlen and t->hlen still account for the previous encapsulation, and
this return skips the new refresh entirely.

Does the tunnel then stop encapsulating while keeping the encap-derived
lengths?  ip_tunnel_xmit() derives its work from tunnel->encap:

	if (ip_tunnel_encap(skb, &tunnel->encap, &protocol, &fl4) < 0)

while dev->mtu stays reduced and dev->needed_headroom inflated for an
encapsulation that no longer exists, and the GSO decision in
ipgre_link_update() is not re-evaluated:

	if (test_bit(IP_TUNNEL_SEQ_BIT, tunnel->parms.o_flags) ||
	    (test_bit(IP_TUNNEL_CSUM_BIT, tunnel->parms.o_flags) &&
	     tunnel->encap.type != TUNNEL_ENCAP_NONE)) {

A later successful changelink recomputes tunnel->hlen = tun_hlen +
encap_hlen from the stale encap_hlen, so the wrong length is carried
forward rather than corrected.  Reproducer on a device that already has
encap fou: re-issue an encap request with the fou module unavailable, or
pass an IFLA_GRE_ENCAP_TYPE >= MAX_IPTUN_ENCAP_OPS, so ip_encap_hlen()
returns -EINVAL after the memset.  Would validating before the memset, or
clearing encap_hlen/hlen on failure, be preferable to refreshing here?

>  
>  static int erspan_changelink(struct net_device *dev, struct nlattr *tb[],

[Severity: Low]
This isn't a bug introduced by this patch, but erspan_changelink() has the
same shape and also publishes a new encapsulation through
ipgre_newlink_encap_setup() -> ip_tunnel_encap_setup(), which sets

	t->encap_hlen = hlen;
	t->hlen = t->encap_hlen + t->tun_hlen;

dropping erspan_hdr_len() from t->hlen, and never refreshes
dev->needed_headroom or dev->mtu, so the defect described in this commit
message remains reachable there in this intermediate state:

	ip link set erspanX type erspan ... encap fou ...

The follow-up patch of this series, "ip_gre: recompute erspan header
lengths after a change", adds erspan_set_hlen()/erspan_link_update() and
calls erspan_link_update(dev, !tb[IFLA_MTU], old_hlen) from
erspan_changelink(), so this is closed by the end of the series and is
mentioned only for completeness.

> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index e6bcf01411d0b..6b93c1fda4948 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.
> + */

[Severity: Low]
Is the ndo_init() part of this comment accurate?  ip_tunnel_bind_dev() is
static to net/ipv4/ip_tunnel.c, and after this patch its callers are
ip_tunnel_refresh_lengths(), ip_tunnel_create(), ip_tunnel_update(),
ip_tunnel_init_net() and ip_tunnel_newlink().  The ndo_init helper shared
by ip_gre/ipip/sit is __ip_tunnel_init(), which does not call it, and since
the function is static no driver outside this file can call it either.
Could the rationale be reworded to name the actual callers?

> +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);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260912150944.3470971-1-edumazet%40google.com

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 (1) · Low: 0

- [High] erspan_set_hlen() (net/ipv4/ip_gre.c:1371), now the single
  authority for ERSPAN tun_hlen/hlen and called from the new…
- [Medium] Commit message mischaracterizes the failure mode: the claimed
  skb_under_panic() in erspan_xmit() is not reachable, and the quoted…

Pre-existing issues:
- [Medium] erspan_changelink() (net/ipv4/ip_gre.c:1548-1550) returns
  immediately when ipgre_newlink_encap_setup() fails, and…

--

ip_gre: recompute erspan header lengths after a change

This moves the ERSPAN tun_hlen/hlen computation out of erspan_tunnel_init()
into a new erspan_set_hlen() helper, and adds erspan_link_update() so that
erspan_changelink() refreshes the header lengths, dev->needed_headroom and
the MTU after the ERSPAN version or the encapsulation changes.

A few questions below.

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

[Severity: Medium]
Is this description of the failure mode accurate?

erspan_xmit() cows to dev->needed_headroom, and ip_tunnel_bind_dev() always
computes that from tunnel->hlen plus the outer IP header:

net/ipv4/ip_tunnel.c:ip_tunnel_bind_dev() {
	...
	int t_hlen = tunnel->hlen + sizeof(struct iphdr);
	...
	dev->needed_headroom = ip_tunnel_limit_headroom(t_hlen + hlen);
}

ip_tunnel_limit_headroom() only clamps at 512, so for any ERSPAN device
needed_headroom is at least 4 + 20 = 24 bytes. With a stale v0 length the
transmit path pushes at most erspan_hdr_len(2) = 12 bytes plus tun_hlen <= 8
bytes before ip_tunnel_xmit() cows again for the outer headers, i.e. at most
20 bytes into a 24 byte reserve.

Can skb_under_panic() actually be reached here? The changelog of the
preceding patch in the series states the opposite for plain GRE ("There is
no memory safety issue: ip_tunnel_xmit() computes its own headroom ... and
calls skb_cow_head() before pushing the encapsulation").

Also, going from version 0 to version 2 takes hlen from tun_hlen 4 (plus
encap) to 8 + 12 (plus encap), which is a 16 byte increase rather than 20
(20 is the total v2 push, not the delta). Could the message instead describe
the malformed GRE/ERSPAN header and the stale MTU/needed_headroom, which are
what the patch actually fixes?

> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 556ebf2c5bd0d..4807fc6d2fd88 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);
> +}

[Severity: High]
Should this derive tun_hlen from tunnel->parms.o_flags rather than from
erspan_ver only? The sibling helper ipgre_link_update() uses
gre_calc_hlen(tunnel->parms.o_flags), and erspan_changelink() can rewrite
o_flags in the same request via ip_tunnel_flags_copy().

The writer takes its length from tun_hlen but its content from o_flags:

net/ipv4/ip_gre.c:__gre_xmit() {
	...
	gre_build_header(skb, tunnel->tun_hlen,
			 flags, proto, tunnel->parms.o_key,
	...
}

Two combinations look reachable.

First, erspan_validate() only checks the OR of the two flag attributes:

net/ipv4/ip_gre.c:erspan_validate() {
	...
	if (!data[IFLA_GRE_COLLECT_METADATA] &&
	    flags != (GRE_SEQ | GRE_KEY))
		return -EINVAL;
	...
}

so IFLA_GRE_IFLAGS = GRE_SEQ|GRE_KEY together with IFLA_GRE_OFLAGS = 0 is
accepted for v1/v2. tun_hlen then becomes 8 while o_flags is empty, and
gre_build_header() skips the option-word block entirely:

include/net/gre.h:gre_build_header() {
	...
	if (ip_tunnel_flags_intersect(flags, cond)) {
		__be32 *ptr = (__be32 *)(((u8 *)greh) + hdr_len - 4);
	...
}

Does this transmit 4 bytes of uninitialised skb headroom between the GRE
base header and the ERSPAN header, and make receivers (which size the GRE
header from the flags field) mis-parse the ERSPAN header?

Second, erspan_validate() returns early for IFLA_GRE_ERSPAN_VER == 0, so
GRE_CSUM stays permitted for version 0, and erspan_xmit() only clears
SEQ/KEY for v0:

net/ipv4/ip_gre.c:erspan_xmit() {
	...
	ip_tunnel_flags_copy(flags, tunnel->parms.o_flags);
	...
		__clear_bit(IP_TUNNEL_SEQ_BIT, flags);
	...
}

After this patch, a v1/v2 -> v0 changelink with ocsum now sets tun_hlen = 4
where it previously kept 8 (which happened to match gre_calc_hlen(CSUM)).
With hdr_len == 4 the ptr computation above lands on greh itself. Can the
checksum word overwrite greh->flags and greh->protocol in that case?

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

[ ... ]

> @@ -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);

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but since the
patch is adding a length refresh on the erspan_netlink_parms() error path,
should the ipgre_newlink_encap_setup() error path just above get similar
attention?

erspan_changelink() returns straight away there:

net/ipv4/ip_gre.c:erspan_changelink() {
	...
	err = ipgre_newlink_encap_setup(dev, data);
	if (err)
		return err;
	...
}

and ip_tunnel_encap_setup() wipes the live descriptor before validating the
requested type:

net/ipv4/ip_tunnel.c:ip_tunnel_encap_setup() {
	...
	memset(&t->encap, 0, sizeof(t->encap));

	hlen = ip_encap_hlen(ipencap);
	if (hlen < 0)
		return hlen;
	...
}

ipgre_netlink_encap_parms() takes the type from nla_get_u16() without a
range check, and ip_encap_hlen() rejects type >= MAX_IPTUN_ENCAP_OPS or an
unregistered ops pointer. Does a rejected IFLA_GRE_ENCAP_TYPE therefore
disable an active FOU/GUE encapsulation on the running tunnel while
t->encap_hlen, t->hlen, dev->needed_headroom and dev->mtu still account for
it, so the tunnel keeps the reduced MTU and starts emitting un-encapsulated
packets? Would validating before publishing in ip_tunnel_encap_setup() be
the better place to address this?

> +
> +	/* 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;
>  

Note that erspan_link_update() runs here, before ip_tunnel_changelink() and
the following ip_tunnel_flags_copy() calls install the accepted o_flags, and
no further length refresh follows, which is what makes the o_flags question
above visible on the changelink path.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260912150944.3470971-1-edumazet%40google.com