[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

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

[Eric Dumazet]

On Sat, Sep 12, 2026 at 8:09 AM Eric Dumazet <edumazet@google.com> wrote:
>
> 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

I will send a V2, integrating AI review feedback

pw-bot: cr

[PATCH net v2 0/5] ip_tunnel, ip_gre: fix changelink lengths and ERSPAN receive

[Eric Dumazet]

This series fixes bugs in how IPv4/IPv6 tunnels and IPv4/IPv6 GRE/ERSPAN
tunnels apply a netlink changelink, and an out-of-bounds read in the ERSPAN
receive path. They all come from the same place: the changelink paths mutate
the live device as they go, without keeping tunnel->hlen,
dev->needed_headroom and dev->mtu in sync.

Patch 1 stops ip_tunnel_encap_setup() and ip6_tnl_encap_setup() from clearing
the active encapsulation before the requested one has been validated. They
memset() t->encap before calling ip[6]_encap_hlen(), so a request naming an
unknown encapsulation type, or one whose module is not loaded, returns -EINVAL
with FOU/GUE already switched off on a working tunnel, while t->encap_hlen and
t->hlen keep their old values. The memset() is redundant -- all four fields of
struct ip_tunnel_encap are assigned unconditionally once the length check has
passed, and a tunnel being created starts from the zeroed private area of
alloc_netdev() -- so it is simply removed. net-next already removed the IPv4
one in commit 88b84cae6b94 ("ip_tunnel: use WRITE_ONCE in
ip_tunnel_encap_setup"), which is not in net; removing the same lines here
keeps the merge trivial, and extends the fix to IPv6, which that commit did
not touch.

Patch 2 makes ipgre_netlink_parms() and erspan_netlink_parms() parse into
local variables and commit only once every attribute has been validated.
Today a request carrying IFLA_GRE_COLLECT_METADATA together with an invalid
IFLA_GRE_IGNORE_DF is rejected after dev->type has already become
ARPHRD_NONE, which breaks the interface for good.

Patch 3 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. The patch
also restricts the dev->hard_header_len update to devices installing
ipgre_header_ops: ip_tunnel_bind_dev() subtracts hard_header_len from the MTU
only for ARPHRD_ETHER, so the old "if (dev->header_ops)" was inflating
gretap's 14-byte Ethernet header and having it subtracted a second time. Not
a memory safety issue: ip_tunnel_xmit() computes its own headroom for the
encapsulation, only the advertised MTU is wrong.

Patch 4 gives ERSPAN the same treatment, where erspan_tunnel_init() was the
only place computing tun_hlen and hlen even though erspan_changelink() can
change both. After a version 0 -> 2 change tun_hlen is still 4, so
gre_build_header() writes the sequence number at greh + tun_hlen - 4, that is
over greh->flags and greh->protocol, corrupting every transmitted packet.

Patch 5 fixes an out-of-bounds read of the ERSPAN metadata in collect_md
mode. erspan_rcv() and ip6erspan_rcv() copy 8 bytes from 12 bytes into the
GRE header, but only pull erspan_hdr_len(ver) bytes beyond it, which is 0
for version 0. A type I packet (4-byte GRE header, no ERSPAN header) reaches
a collect_md tunnel through itn->collect_md_tun and takes the
ERSPAN_V2_MDSIZE branch of the ternary, and a malformed packet with an 8-byte
GRE header and ershdr->ver == 0 gets there too, on both the IPv4 and the IPv6
side.

Apply order
-----------
Patch 1 must land before patch 3. ipgre_changelink() returns early, without
reaching the new link_update: label, when ipgre_newlink_encap_setup() fails,
which is only correct because patch 1 guarantees that a failing
ip_tunnel_encap_setup() has published nothing.

Behaviour change
----------------
Patches 3 and 4 recompute the MTU from ip_tunnel_bind_dev() instead of
shifting it by a delta, as ip_tunnel_update() already does for a link or
fwmark change. It is guarded by tunnel->hlen != old_hlen, so a MTU configured
by the user still survives a request that does not change the header length,
but one that does now discards it.

Patch 5 rejects an ERSPAN base header whose version is neither 1 nor 2,
where such packets were previously decapsulated. For version 0 the current
code leaves the 4-byte base header inside the payload, corrupting the inner
frame, and the transmit side (erspan_fb_xmit(), ip6erspan_tunnel_xmit())
already rejects anything that is not 1 or 2. Native ERSPAN type I tunnels
are not affected: they send a 4-byte GRE header and take the
is_erspan_type1() path.

Notes for reviewers, because not all bugs are fixed
---------------------------------------------------
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. Patch 4 shrinks that window
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.

The same is true of __gre_xmit(), which takes a snapshot of the flags from
its caller but reads tunnel->tun_hlen live, while ipgre_changelink()
publishes t->parms.o_flags before ipgre_link_update() recomputes tun_hlen.
gre_build_header() walks backwards from greh + hdr_len - 4, so the two must
agree: new flags with an old tun_hlen writes the sequence number over
greh->flags and greh->protocol, and old flags with a new tun_hlen leaves the
four extra skb_push()ed bytes uninitialised on the wire.

Both are pre-existing and both need the larger rework, which will come with
the ip_tunnel RCU conversion in net-next.

v2:
  - Patch 1 is new: do not clear t->encap before the requested encapsulation
    has been validated, in the IPv4 and IPv6 tunnels. It must precede the
    former patch 2, now patch 3.
  - Patch 5 is new: fix the out-of-bounds read of the ERSPAN metadata in
    collect_md mode, on the IPv4 and IPv6 receive paths.
  - The three patches of v1 are unchanged in intent and become patches 2, 3
    and 4.
  - Patch 2 (was 1/3): clarify in the comments that only the attribute
    parsers become all-or-nothing, ip_tunnel_changelink() can still fail
    afterwards.
  - Patch 3 (was 2/3): test for ipgre_header_ops rather than for any
    header_ops, so gretap keeps its 14-byte hard_header_len; add a second
    Fixes tag for fdafed459998; drop the incorrect claim that
    ip_tunnel_xmit() cows before ip_tunnel_encap() pushes the FOU/GUE header,
    and describe instead the headroom ipgre_xmit() already reserved at tunnel
    entry.
  - Patch 4 (was 3/3): also refresh the lengths on the
    ip_tunnel_changelink() error paths; replace the skb_under_panic() claim,
    which is not reachable, with the actual GRE header corruption.
  - All five patches now carry Cc: stable@vger.kernel.org. Patch 4 does not
    build without patch 3.

Assisted-by: LLM
Link: https://lore.kernel.org/netdev/20260912150944.3470971-1-edumazet@google.com/ [v1]

Eric Dumazet (5):
  ip_tunnel: do not clear the active encap before validating the new one
  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
  gre: fix out-of-bounds read of erspan metadata in collect_md mode

 include/net/ip_tunnels.h |   1 +
 net/ipv4/ip_gre.c        | 201 ++++++++++++++++++++++++++++-----------
 net/ipv4/ip_tunnel.c     |  19 +++-
 net/ipv6/ip6_gre.c       |   2 +
 net/ipv6/ip6_tunnel.c    |   2 -
 5 files changed, 164 insertions(+), 61 deletions(-)

-- 
2.55.0.1032.g73a4cd73de-goog

[PATCH net v2 1/5] ip_tunnel: do not clear the active encap before validating the new one

[Eric Dumazet]

ip_tunnel_encap_setup() and ip6_tnl_encap_setup() zero out t->encap with
memset() before calling ip_encap_hlen() / ip6_encap_hlen() to validate
the requested encapsulation type and module availability.

When a changelink request supplies an invalid encapsulation type or an
encapsulation whose module is not loaded, ip[6]_encap_hlen() returns
-EINVAL after t->encap has already been cleared to TUNNEL_ENCAP_NONE,
while t->encap_hlen and t->hlen remain at their previous values. A
rejected netlink request thus permanently disables FOU/GUE on a working
tunnel while keeping its reduced MTU and extra headroom.

The memset() is redundant: struct ip_tunnel_encap has exactly four
fields, and all of them are assigned unconditionally once the length
check has passed. A tunnel being created starts from the zeroed private
area of alloc_netdev(). Simply remove it, so that a failed changelink
leaves the active encapsulation untouched.

net-next already does this for the IPv4 side in commit 88b84cae6b94
("ip_tunnel: use WRITE_ONCE in ip_tunnel_encap_setup"), which is not in
net. Removing the same lines here keeps the merge trivial, and extends
the fix to IPv6, which that commit did not touch.

Fixes: 56328486539d ("net: Changes to ip_tunnel to support foo-over-udp encapsulation")
Fixes: 058214a4d1df ("ip6_tun: Add infrastructure for doing encapsulation")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/ip_tunnel.c  | 2 --
 net/ipv6/ip6_tunnel.c | 2 --
 2 files changed, 4 deletions(-)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index e6bcf01411d0bcd12cc9a88e449d9283c4a83c64..2a313b18134e2f0bafd52d59d8e173fa1a084f03 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -491,8 +491,6 @@ int ip_tunnel_encap_setup(struct ip_tunnel *t,
 {
 	int hlen;
 
-	memset(&t->encap, 0, sizeof(t->encap));
-
 	hlen = ip_encap_hlen(ipencap);
 	if (hlen < 0)
 		return hlen;
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index d5ff50a2ac01760acd1cb176c952f7113f733288..c918c2b0ad81b0161a2a98e4bd861436497c551c 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1818,8 +1818,6 @@ int ip6_tnl_encap_setup(struct ip6_tnl *t,
 {
 	int hlen;
 
-	memset(&t->encap, 0, sizeof(t->encap));
-
 	hlen = ip6_encap_hlen(ipencap);
 	if (hlen < 0)
 		return hlen;
-- 
2.55.0.1032.g73a4cd73de-goog

[PATCH net v2 2/5] 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")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/ip_gre.c | 55 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 82309efd417e0f1f6554e7028be8e05d769e932d..dad3d054bd15612a3ebe1cf27e6e8c5d9e6896e9 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,16 @@ static int ipgre_netlink_parms(struct net_device *dev,
 		t->ignore_df = !!nla_get_u8(data[IFLA_GRE_IGNORE_DF]);
 	}
 
+	/* All attributes parsed here have been validated, we can change @dev
+	 * and @t. This only makes this parser all-or-nothing, the caller can
+	 * still fail in ip_tunnel_changelink().
+	 */
+	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 +1263,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 parsed here 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.1032.g73a4cd73de-goog

[PATCH net v2 3/5] 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 dev->needed_headroom and
dev->mtu. ipgre_changelink() must then refresh on its error paths too,
since the new encapsulation is published by then.

dev->hard_header_len is recomputed the same way, but it stays in
ipgre_link_update(): for the ARPHRD_IPGRE devices installing
ipgre_header_ops it is the outer IP + GRE header, which
ip_tunnel_bind_dev() does not know about. Check specifically for
ipgre_header_ops: gretap devices also have header_ops (eth_header_ops),
where hard_header_len is the 14-byte inner Ethernet header that
ip_tunnel_bind_dev() subtracts from the MTU, and commit fdafed459998
("ip_gre: set dev->hard_header_len and dev->needed_headroom properly")
wrongly adjusted hard_header_len instead of needed_headroom for them.

@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: ipgre_xmit() cows dev->needed_headroom
at tunnel entry, which always reserves sizeof(struct iphdr) + lower
device headroom (>= 34 bytes after the GRE header), enough for
ip_tunnel_encap() to push the FOU/GUE header before ip_tunnel_xmit()
cows again for the outer IP header.

Fixes: dd9d598c6657 ("ip_gre: add the support for i/o_flags update via netlink")
Fixes: fdafed459998 ("ip_gre: set dev->hard_header_len and dev->needed_headroom properly")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/ip_tunnels.h |  1 +
 net/ipv4/ip_gre.c        | 55 ++++++++++++++++++++++++++++------------
 net/ipv4/ip_tunnel.c     | 17 +++++++++++++
 3 files changed, 57 insertions(+), 16 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 dad3d054bd15612a3ebe1cf27e6e8c5d9e6896e9..ced57cbeaad4991487e9ddb29fa18ae6a1f134fb 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -789,23 +789,36 @@ 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;
+	tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen;
 
-	if (dev->header_ops)
-		dev->hard_header_len += len;
-	else
-		dev->needed_headroom += len;
+	/* For the ARPHRD_IPGRE devices installing ipgre_header_ops,
+	 * dev->hard_header_len is the outer IP + GRE header, as set by
+	 * ipgre_tunnel_init(). ip_tunnel_bind_dev() does not maintain it:
+	 * it only subtracts it from the MTU, and only for ARPHRD_ETHER.
+	 */
+	if (dev->header_ops == &ipgre_header_ops)
+		dev->hard_header_len = tunnel->hlen + sizeof(struct iphdr);
 
-	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 +866,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);
@@ -1474,6 +1487,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))
@@ -1485,18 +1499,27 @@ 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.
+	 *
+	 * IFLA_MTU only defers the MTU to do_setlink(), which rtnl_changelink()
+	 * does not reach if we return an error, so it must not hold it back.
+	 */
+	ipgre_link_update(dev, err || !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 2a313b18134e2f0bafd52d59d8e173fa1a084f03..dd1b2f719f21670b02bfc1cafa62689b4a28a580 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -326,6 +326,23 @@ 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 ip_tunnel_create(), ip_tunnel_newlink(), ip_tunnel_init_net()
+ * and ip_tunnel_update(), and a caller 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.1032.g73a4cd73de-goog

[PATCH net v2 4/5] 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() then pushes an ERSPAN header sized from the new
tunnel->erspan_ver, while __gre_xmit() lays the GRE header out from the
stale tunnel->tun_hlen. After a version 0 -> 2 change, tun_hlen is still
4 and gre_build_header() writes the sequence number at
greh + tun_hlen - 4, that is over greh->flags and greh->protocol. The
MTU keeps the value derived from the old header length.

There is no memory safety issue: dev->needed_headroom is at least
tunnel->hlen + sizeof(struct iphdr), and erspan_xmit() pushes at most
12 + 8 bytes before ip_tunnel_xmit() takes over and cows again.

Move the computation into erspan_set_hlen() and add
erspan_link_update(), refreshing the lengths as the previous patch does
for plain GRE. Call erspan_set_hlen() before ip_tunnel_changelink() so
that ip_tunnel_update() sees the updated tunnel->hlen and erspan_xmit()
sees a matching tun_hlen, and run erspan_link_update() at the end of
erspan_changelink(), including on error paths, as ipgre_changelink()
does.

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

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index ced57cbeaad4991487e9ddb29fa18ae6a1f134fb..696884f53cdcc65fe87cf04f357f1cc45a7e0736 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1379,18 +1379,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;
@@ -1529,6 +1554,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))
@@ -1540,16 +1566,29 @@ static int erspan_changelink(struct net_device *dev, struct nlattr *tb[],
 
 	err = erspan_netlink_parms(dev, data, tb, &p, &fwmark);
 	if (err < 0)
-		return err;
+		goto link_update;
+
+	erspan_set_hlen(t);
 
 	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);
 
-	return 0;
+link_update:
+	/* 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 on the error paths as well,
+	 * since both leave the new encapsulation or version behind.
+	 *
+	 * As in ipgre_changelink(), IFLA_MTU must not hold the MTU back if we
+	 * return an error, because do_setlink() will not apply it then.
+	 */
+	erspan_link_update(dev, err || !tb[IFLA_MTU], old_hlen);
+
+	return err;
 }
 
 static size_t ipgre_get_size(const struct net_device *dev)
-- 
2.55.0.1032.g73a4cd73de-goog

[PATCH net v2 5/5] gre: fix out-of-bounds read of erspan metadata in collect_md mode

[Eric Dumazet]

erspan_rcv() and ip6erspan_rcv() copy the ERSPAN metadata out of the
packet for collect_md tunnels:

	pkt_md = (struct erspan_metadata *)(gh + gre_hdr_len +
					    sizeof(*ershdr));
	...
	memcpy(md2, pkt_md, ver == 1 ? ERSPAN_V1_MDSIZE :
				       ERSPAN_V2_MDSIZE);

Both read 8 bytes at 12 bytes from the start of the GRE header, but only
ask pskb_may_pull() for erspan_hdr_len(ver) bytes beyond it, which type I
support made 0 for version 0. Two ways to get there:

- An ERSPAN type I packet has a 4 byte GRE header and no ERSPAN header
  at all, so erspan_rcv() sets ver = 0 and only pulls the GRE header. It
  still falls back to a collect_md tunnel through itn->collect_md_tun,
  and there the ternary above picks ERSPAN_V2_MDSIZE.

- A packet with an 8 byte GRE header and ershdr->ver == 0 is malformed,
  yet neither erspan_rcv() nor ip6erspan_rcv() validates the version
  before using it, so erspan_hdr_len(0) pulls nothing either. A version
  above 2 is not an out-of-bounds read, but it does store a version that
  the transmit side (erspan_fb_xmit(), ip6erspan_tunnel_xmit()) rejects.

Reject a base header whose version is neither 1 nor 2, and skip the
metadata extraction for type I, which has none: ip_tun_rx_dst() hands
out a zeroed option area, so md->version = 0 alone describes it.

Fixes: f989d546a2d5 ("erspan: Add type I version 0 support.")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/ip_gre.c  | 34 ++++++++++++++++++++++------------
 net/ipv6/ip6_gre.c |  2 ++
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 696884f53cdcc65fe87cf04f357f1cc45a7e0736..92f3a52d20d38186c3ce87824a8e15c9e00a925f 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -274,7 +274,6 @@ static int erspan_rcv(struct sk_buff *skb, struct tnl_ptk_info *tpi,
 	struct ip_tunnel_net *itn;
 	struct ip_tunnel *tunnel;
 	const struct iphdr *iph;
-	struct erspan_md2 *md2;
 	int ver;
 	int len;
 
@@ -294,6 +293,9 @@ static int erspan_rcv(struct sk_buff *skb, struct tnl_ptk_info *tpi,
 
 		ershdr = (struct erspan_base_hdr *)(skb->data + gre_hdr_len);
 		ver = ershdr->ver;
+		if (unlikely(ver != 1 && ver != 2))
+			return PACKET_REJECT;
+
 		iph = ip_hdr(skb);
 		__set_bit(IP_TUNNEL_KEY_BIT, flags);
 		tunnel = ip_tunnel_lookup(itn, skb->dev->ifindex, flags,
@@ -318,6 +320,7 @@ static int erspan_rcv(struct sk_buff *skb, struct tnl_ptk_info *tpi,
 		if (tunnel->collect_md) {
 			struct erspan_metadata *pkt_md, *md;
 			struct ip_tunnel_info *info;
+			struct erspan_md2 *md2;
 			unsigned char *gh;
 			__be64 tun_id;
 
@@ -334,19 +337,26 @@ static int erspan_rcv(struct sk_buff *skb, struct tnl_ptk_info *tpi,
 			info = &tun_dst->u.tun_info;
 			info->options_len = sizeof(*md);
 
-			/* skb can be uncloned in __iptunnel_pull_header, so
-			 * old pkt_md is no longer valid and we need to reset
-			 * it
-			 */
-			gh = skb_network_header(skb) +
-			     skb_network_header_len(skb);
-			pkt_md = (struct erspan_metadata *)(gh + gre_hdr_len +
-							    sizeof(*ershdr));
 			md = ip_tunnel_info_opts(&tun_dst->u.tun_info);
 			md->version = ver;
-			md2 = &md->u.md2;
-			memcpy(md2, pkt_md, ver == 1 ? ERSPAN_V1_MDSIZE :
-						       ERSPAN_V2_MDSIZE);
+
+			/* Type I has no ERSPAN header, thus no metadata to
+			 * extract: reading it would go past the @len bytes
+			 * pulled above. ip_tun_rx_dst() zeroed @md for us.
+			 */
+			if (!is_erspan_type1(gre_hdr_len)) {
+				/* skb can be uncloned in __iptunnel_pull_header, so
+				 * old pkt_md is no longer valid and we need to reset
+				 * it
+				 */
+				gh = skb_network_header(skb) +
+				     skb_network_header_len(skb);
+				pkt_md = (struct erspan_metadata *)(gh + gre_hdr_len +
+								    sizeof(*ershdr));
+				md2 = &md->u.md2;
+				memcpy(md2, pkt_md, ver == 1 ? ERSPAN_V1_MDSIZE :
+							       ERSPAN_V2_MDSIZE);
+			}
 
 			__set_bit(IP_TUNNEL_ERSPAN_OPT_BIT,
 				  info->key.tun_flags);
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 8ebda0b6a78b2236b439f5499d84f34f652fcbe2..a59fb82c74dad7f0c1d1128338e7bed1e3c7116f 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -503,6 +503,8 @@ static int ip6erspan_rcv(struct sk_buff *skb,
 	ipv6h = ipv6_hdr(skb);
 	ershdr = (struct erspan_base_hdr *)skb->data;
 	ver = ershdr->ver;
+	if (unlikely(ver != 1 && ver != 2))
+		return PACKET_REJECT;
 
 	tunnel = ip6gre_tunnel_lookup(skb->dev,
 				      &ipv6h->saddr, &ipv6h->daddr, tpi->key,
-- 
2.55.0.1032.g73a4cd73de-goog

Re: [PATCH net v2 0/5] ip_tunnel, ip_gre: fix changelink lengths and ERSPAN receive

[Jakub Kicinski]

On Wed, 16 Sep 2026 10:01:50 +0000 Eric Dumazet wrote:
> X-Mailer: git-send-email 2.55.0.1032.g73a4cd73de-goog

Did you ask an agent to send this? :/
No postings in reply to old postings.
It breaks sashiko from time to time.

Re: [PATCH net v2 1/5] ip_tunnel: do not clear the active encap before validating the new one

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Incomplete fix: the commit message claims that removing the
  memset() makes it so that "a failed changelink leaves the active…

Pre-existing issues:
- [Medium] ip_tunnel_encap_setup() (net/ipv4/ip_tunnel.c:498-504) and
  ip6_tnl_encap_setup() (net/ipv6/ip6_tunnel.c:1825-1831) publish…

--

ip_tunnel: do not clear the active encap before validating the new one

This removes the memset() of t->encap from ip_tunnel_encap_setup() and
ip6_tnl_encap_setup() so that a failing ip_encap_hlen() / ip6_encap_hlen()
no longer leaves the tunnel with TUNNEL_ENCAP_NONE while encap_hlen and
hlen keep their old values.

> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index e6bcf01411d0b..2a313b18134e2 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -491,8 +491,6 @@ int ip_tunnel_encap_setup(struct ip_tunnel *t,
>  {
>  	int hlen;
>  
> -	memset(&t->encap, 0, sizeof(t->encap));
> -
>  	hlen = ip_encap_hlen(ipencap);
>  	if (hlen < 0)
>  		return hlen;

[Severity: High]
The changelog says the result is that "a failed changelink leaves the
active encapsulation untouched".  Is that true for the callers, though?

ip_tunnel_encap_setup() and ip6_tnl_encap_setup() still write directly
into the live tunnel (t->encap.{type,sport,dport,flags}, t->encap_hlen
and t->hlen), and every changelink caller runs them before the rest of
the netlink request is validated.  For example ip6_tnl_changelink():

	if (ip_tunnel_netlink_encap_parms(data, &ipencap)) {
		int err = ip6_tnl_encap_setup(t, &ipencap);

		if (err < 0)
			return err;
	}
	ip6_tnl_netlink_parms(data, &p);
	if (p.collect_md)
		return -EINVAL;

	t = ip6_tnl_locate(net, &p, 0);
	if (!IS_ERR(t)) {
		if (t->dev != dev)
			return -EEXIST;

The same ordering exists in ipip_changelink() and ipip6_changelink()
(both call ip_tunnel_encap_setup() and then reject the request with
-EINVAL for collect_md / the IFF_POINTOPOINT vs p.iph.daddr mismatch,
or -EEXIST from ip_tunnel_changelink() / ipip6_tunnel_locate(), or an
error from ipip6_tunnel_update_6rd()), in ip6gre_changelink_common()
(commits encap, then can return ERR_PTR(-EEXIST)), and in
ipgre_changelink() / erspan_changelink() via
ipgre_newlink_encap_setup() before the parsers run.

Can that still disable FOU/GUE on a working tunnel with a rejected
request?  ip_tunnel_netlink_encap_parms() returns true for any of
IFLA_IPTUN_ENCAP_TYPE/FLAGS/SPORT/DPORT and memsets the parsed struct
first:

	memset(encap, 0, sizeof(*encap));
	...
	if (data[IFLA_IPTUN_ENCAP_SPORT]) {
		ret = true;
		encap->sport = nla_get_be16(data[IFLA_IPTUN_ENCAP_SPORT]);
	}

so a message carrying only ENCAP_SPORT (or ENCAP_TYPE=0) reaches
encap_setup with type == TUNNEL_ENCAP_NONE, where ip[6]_encap_hlen()
returns 0 and the store always succeeds.  Combine that with an
attribute set that is rejected afterwards (collect_md, or a
local/remote pair colliding with another tunnel giving -EEXIST) and the
tunnel loses its encapsulation and gets t->hlen recomputed while
userspace is told the request failed.

For ipip, sit, ip6_tunnel and ip6_gre no length refresh runs on those
error paths either, so dev->mtu and dev->needed_headroom keep the values
derived from the old encapsulated header length, which is the mismatched
state described in the changelog.

Would it make more sense to validate the requested encapsulation into a
temporary struct and apply it to the tunnel only after all the other
checks have passed, rather than only dropping the memset()?

For reference, at the end of this series the ordering is unchanged for
ipip, sit, ip6_tunnel and ip6_gre, and the later patch "ip_gre: validate
netlink attributes before changing the tunnel" states that
ip_tunnel_encap_setup() still runs before the parsers.

> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index d5ff50a2ac017..c918c2b0ad81b 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1818,8 +1818,6 @@ int ip6_tnl_encap_setup(struct ip6_tnl *t,
>  {
>  	int hlen;
>  
> -	memset(&t->encap, 0, sizeof(t->encap));
> -
>  	hlen = ip6_encap_hlen(ipencap);
>  	if (hlen < 0)
>  		return hlen;

[Severity: Medium]
This is a pre-existing issue and not something introduced here, but while
these two functions are being touched: is the publication of the new
encapsulation to the lockless transmit path worth annotating in the same
patch?

ip_tunnel_encap_setup() and ip6_tnl_encap_setup() store the four
t->encap fields plus t->encap_hlen and t->hlen with plain writes under
RTNL:

	t->encap.type = ipencap->type;
	t->encap.sport = ipencap->sport;
	t->encap.dport = ipencap->dport;
	t->encap.flags = ipencap->flags;

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

while ip_encap_hlen() / ip_tunnel_encap() in include/net/ip_tunnels.h and
ip6_encap_hlen() / ip6_tnl_encap() in include/net/ip6_tunnel.h read them
in softirq context with no lock held, e.g.:

	rcu_read_lock();
	ops = rcu_dereference(ip6tun_encaps[t->encap.type]);
	if (likely(ops && ops->build_header))
		ret = ops->build_header(skb, &t->encap, protocol, fl6);

Can a packet already in ip_tunnel_xmit() / ip6_tnl_xmit() see the new
encap.type together with the stale sport/dport/flags, or the new type
together with the stale encap_hlen/hlen used for headroom and MTU
accounting?

The changelog notes that net-next commit 88b84cae6b94 ("ip_tunnel: use
WRITE_ONCE in ip_tunnel_encap_setup") covers the IPv4 side there; the
IPv6 side has no equivalent, and no reader uses READ_ONCE(), so these
stay plain data races that KCSAN can report.  The array index itself is
fine, since ip[6]_encap_hlen() rejects any type >= MAX_IPTUN_ENCAP_OPS
before the store, so iptun_encaps[] / ip6tun_encaps[] cannot be indexed
out of bounds.

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

Re: [PATCH net v2 2/5] 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 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Incomplete fix: the collect_md / dev->type mutation is still
  committed to the live device before the last fallible step of the…

Pre-existing issues:
- [Medium] The batched commit of the four ERSPAN configuration fields at
  the end of erspan_netlink_parms() (net/ipv4/ip_gre.c:1306-1309) uses…

--

ip_gre: validate netlink attributes before changing the tunnel

The patch moves the IFLA_GRE_COLLECT_METADATA handling after the
IFLA_GRE_IGNORE_DF validation in ipgre_netlink_parms(), and parses the
ERSPAN attributes into local variables in erspan_netlink_parms() so they
are only committed to the live tunnel once all of them validate.

A couple of questions below.

> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 82309efd417e0..dad3d054bd156 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,16 @@ static int ipgre_netlink_parms(struct net_device *dev,
>  		t->ignore_df = !!nla_get_u8(data[IFLA_GRE_IGNORE_DF]);
>  	}
>  
> +	/* All attributes parsed here have been validated, we can change @dev
> +	 * and @t. This only makes this parser all-or-nothing, the caller can
> +	 * still fail in ip_tunnel_changelink().
> +	 */
> +	if (data[IFLA_GRE_COLLECT_METADATA]) {
> +		t->collect_md = true;
> +		if (dev->type == ARPHRD_IPGRE)
> +			dev->type = ARPHRD_NONE;
> +	}
> +

[Severity: High]
Does this still leave the exact failure mode described in the changelog
reachable?  The changelog says:

    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.

The collect_md / dev->type mutation is still committed inside the parser,
and ip_tunnel_changelink() runs after it with three reachable rejections:

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 {
		...
			if ((dev->flags ^ nflags) &
			    (IFF_POINTOPOINT | IFF_BROADCAST))
				return -EINVAL;
	...
}

ipgre_changelink() and erspan_changelink() both propagate that error
directly with no rollback:

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

One deterministic trigger is:

    ip link set gre0 type gre external

The fallback device carries ipgre_link_ops, so the parser flips gre0 to
collect_md and ARPHRD_NONE, then ip_tunnel_changelink() returns -EINVAL on
the dev == itn->fb_tunnel_dev test.

A second trigger is a point-to-point gre tunnel plus a changelink carrying
only IFLA_GRE_COLLECT_METADATA: parms is memset in the parser, so
p->iph.daddr stays 0, nflags becomes 0 and the IFF_POINTOPOINT mismatch
returns -EINVAL after dev->type was already changed.

Since IFLA_GRE_COLLECT_METADATA is an NLA_FLAG and nothing in the tree ever
clears t->collect_md or restores ARPHRD_IPGRE, is the device left
permanently inconsistent while user space was told the request failed?
ipgre_fill_info() then also reports the rejected state back on a dump.

There is a further effect from the latched flag: a later deletion of that
device reaches

net/ipv4/ip_tunnel.c:ip_tunnel_del() {
	if (t->collect_md)
		rcu_assign_pointer(itn->collect_md_tun, NULL);
	...
}

even though this tunnel was never registered as the netns metadata tunnel,
so can this de-register the legitimate external GRE tunnel of the netns and
blackhole its ingress via ip_tunnel_lookup()?

Would it work to stage collect_md, ignore_df and dev->type (and the ERSPAN
fields below) and commit them only after ip_tunnel_changelink() succeeds,
the way net/ipv6/ip6_gre.c carries them through struct __ip6_tnl_parm?

>  	if (data[IFLA_GRE_FWMARK])
>  		*fwmark = nla_get_u32(data[IFLA_GRE_FWMARK]);
>  
> @@ -1259,40 +1263,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 parsed here have been validated, we can change @t. */
> +	t->erspan_ver = erspan_ver;
> +	t->index = index;
> +	t->hwid = hwid;
> +	t->dir = dir;
> +
>  	return 0;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but the publication of these
four fields is unsynchronized against the transmit path, and the version is
still stored first.

erspan_xmit() reads all four with plain loads from softirq context with no
lock, RCU or seqcount:

net/ipv4/ip_gre.c:erspan_xmit() {
	if (tunnel->erspan_ver == 0) {
		...
	} else if (tunnel->erspan_ver == 1) {
		erspan_build_header(skb, ntohl(tunnel->parms.o_key), tunnel->index, truncate, true);
	} else if (tunnel->erspan_ver == 2) {
		erspan_build_header_v2(skb, ntohl(tunnel->parms.o_key), tunnel->dir, tunnel->hwid, truncate, true);
	...
}

So if a transmit lands between the t->erspan_ver = erspan_ver store and the
hwid/dir stores, can it emit a version 2 header built from the stale dir and
hwid?  The missing WRITE_ONCE()/READ_ONCE() annotations also leave these
accesses as a KCSAN-reportable data race.

The same ordering existed before the patch, so this is only mentioned in
passing while the code is being touched.

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

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Compensating GRE-flag and encapsulation changes submitted
  together with a link/fwmark change leave dev->mtu computed from an…

Pre-existing issues:
- [Low] Claimed incomplete fix: erspan_changelink() (and ipip.c/sit.c
  changelink) publish a new encapsulation through…
- [Low] Claimed stale hlen-derived limit: dev->max_mtu is fixed from the
  creation-time tunnel->hlen by ip_tunnel_create()/ip_tunnel_newlink()…
- [Low] Claimed error-path state inconsistency: when IFLA_MTU is present
  and ipgre_changelink() succeeds, set_mtu is false so the MTU refresh…

--

ip_gre: compute tunnel lengths absolutely instead of by delta

This replaces the delta arithmetic in ipgre_link_update() with an absolute
recomputation of tunnel->hlen from tun_hlen and encap_hlen, and adds
ip_tunnel_refresh_lengths() so 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, since the new encapsulation is already
published at that point.

> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index dad3d054bd156..ced57cbeaad49 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -789,23 +789,36 @@ 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;
> +	tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen;
>  
> -	if (dev->header_ops)
> -		dev->hard_header_len += len;
> -	else
> -		dev->needed_headroom += len;
> +	/* For the ARPHRD_IPGRE devices installing ipgre_header_ops,
> +	 * dev->hard_header_len is the outer IP + GRE header, as set by
> +	 * ipgre_tunnel_init(). ip_tunnel_bind_dev() does not maintain it:
> +	 * it only subtracts it from the MTU, and only for ARPHRD_ETHER.
> +	 */
> +	if (dev->header_ops == &ipgre_header_ops)
> +		dev->hard_header_len = tunnel->hlen + sizeof(struct iphdr);
>  
> -	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: Medium]

Can this predicate leave dev->mtu at a value computed from an intermediate
tunnel->hlen?

In ipgre_changelink() the encapsulation is published before the flags are
applied:

  ipgre_newlink_encap_setup()
    ip_tunnel_encap_setup()
      t->encap_hlen = hlen;
      t->hlen = t->encap_hlen + t->tun_hlen;   /* still the old tun_hlen */

ip_tunnel_changelink() then calls ip_tunnel_update(itn, t, dev, p,
!tb[IFLA_MTU], fwmark), which writes the MTU from that intermediate hlen when
the link or the fwmark changed:

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

Only afterwards does ipgre_link_update() compute the final tunnel->hlen and
compare it with old_hlen, which was sampled at the entry of
ipgre_changelink().

For a gre device with tun_hlen 4 and encap fou (encap_hlen 8, hlen 12) over a
1500 byte lower device, the correct MTU is 1468. A single request asking for
"encap none" plus csum plus key plus a new fwmark gives an intermediate hlen
of 4, so ip_tunnel_update() writes dev->mtu = 1500 - 24 = 1476. The final hlen
is 12 + 0, equal to old_hlen, so the refresh above is suppressed and dev->mtu
stays 1476, eight bytes too large. The reverse combination (adding fou while
removing csum and key) leaves it eight bytes too small.

The previous delta code corrected exactly this case, since it applied
dev->mtu -= len on top of what ip_tunnel_update() had written (1476 - 8 =
1468).

Would refreshing unconditionally when set_mtu is true, rather than gating on
tunnel->hlen != old_hlen, avoid this?

>  
>  	if (test_bit(IP_TUNNEL_SEQ_BIT, tunnel->parms.o_flags) ||
>  	    (test_bit(IP_TUNNEL_CSUM_BIT, tunnel->parms.o_flags) &&

[ ... ]

> @@ -1485,18 +1499,27 @@ 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.
> +	 *
> +	 * IFLA_MTU only defers the MTU to do_setlink(), which rtnl_changelink()
> +	 * does not reach if we return an error, so it must not hold it back.
> +	 */
> +	ipgre_link_update(dev, err || !tb[IFLA_MTU], old_hlen);

[Severity: Low]

This isn't a bug introduced by this patch, but the IFLA_MTU deferral here has
a window where the header length change is committed with a stale MTU.

When IFLA_MTU is present and ipgre_changelink() succeeds, set_mtu is false and
the MTU is left to do_setlink(). If the rest of the rtnetlink request then
fails (IFLA_INFO_SLAVE_DATA handling, dev_validate_mtu() rejecting the
requested value, or a NETDEV_CHANGEMTU notifier), nothing rolls back the
header length change:

net/core/rtnetlink.c:rtnl_changelink() {
	...
	err = ops->changelink(dev, tb, data, extack);
	if (err < 0)
		return err;
	...
}

The baseline already passed !tb[IFLA_MTU] and rtnl_changelink() has always
called ops->changelink() first without rollback, so this is unchanged by the
patch. Is it worth noting as a known limitation?

>  
> -	return 0;
> +	return err;
>  }
>  
>  static int erspan_changelink(struct net_device *dev, struct nlattr *tb[],

[Severity: Low]

This is a pre-existing issue and not introduced by this patch, but the same
symptom the changelog describes ("adding encap fou to an existing gre device
keeps the MTU of a bare tunnel") is still reachable through other changelink
handlers that publish a new encapsulation without refreshing the lengths:

  erspan_changelink()  -> ipgre_newlink_encap_setup() -> ip_tunnel_encap_setup()
  ipip_changelink()    -> ip_tunnel_encap_setup()
  ipip6_changelink()   -> ip_tunnel_encap_setup()

ip_tunnel_update() only rebinds the device when the link or the fwmark
changed:

net/ipv4/ip_tunnel.c:ip_tunnel_update() {
	...
	if (t->parms.link != p->link || t->fwmark != fwmark) {
		...
		mtu = ip_tunnel_bind_dev(dev);
	...
}

For erspan this is handled by the following patch in the series, "ip_gre:
recompute erspan header lengths after a change", which adds erspan_set_hlen()
and erspan_link_update() with the same link_update label and error path
refresh. Are ipip.c and sit.c intentionally left for a separate change?

> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index 2a313b18134e2..dd1b2f719f216 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -326,6 +326,23 @@ 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 ip_tunnel_create(), ip_tunnel_newlink(), ip_tunnel_init_net()
> + * and ip_tunnel_update(), and a caller 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);

[Severity: Low]

This is a pre-existing issue rather than something this patch introduces, but
the new helper writes dev->mtu while dev->max_mtu keeps the value derived from
the creation-time tunnel->hlen:

net/ipv4/ip_tunnel.c:ip_tunnel_create() {
	...
	t_hlen = nt->hlen + sizeof(struct iphdr);
	dev->min_mtu = ETH_MIN_MTU;
	dev->max_mtu = IP_MAX_MTU - t_hlen;
	...
}

Nothing in ipgre_link_update() or ip_tunnel_refresh_lengths() updates
dev->max_mtu, while ip_tunnel_change_mtu() validates against the live
tunnel->hlen:

net/ipv4/ip_tunnel.c:ip_tunnel_change_mtu() {
	int t_hlen = tunnel->hlen + sizeof(struct iphdr);
	int max_mtu = IP_MAX_MTU - t_hlen;
	...
}

So dev_validate_mtu() can reject an MTU that ip_tunnel_change_mtu() would
accept, and the refreshed dev->mtu can exceed the advertised dev->max_mtu (for
example a device created with hlen 8 whose csum flag is later removed on a
65535 byte lower device ends up with dev->mtu 65511 against dev->max_mtu
65507). The old delta code produced the same value, so this is not a change in
behaviour. Should dev->max_mtu be refreshed alongside dev->mtu here?

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

Re: [PATCH net v2 4/5] ip_gre: recompute erspan header lengths after a change

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] erspan_set_hlen() (net/ipv4/ip_gre.c:1382) derives
  tunnel->tun_hlen from tunnel->erspan_ver alone (4 for version 0, 8…
- [High] erspan_changelink() now publishes the new ERSPAN version (inside
  erspan_netlink_parms(), 'All attributes parsed here have been…
- [Medium] erspan_changelink() publishes tunnel->erspan_ver (in
  erspan_netlink_parms()) and tunnel->tun_hlen / tunnel->hlen (in the…

Pre-existing issues:
- [Low] Reported by earlier stages, judged a non-issue during
  verification (baseline decision: false).

--

ip_gre: recompute erspan header lengths after a change

The patch factors the ERSPAN GRE/tunnel header length computation out of
erspan_tunnel_init() into erspan_set_hlen(), and adds erspan_link_update()
so that erspan_changelink() refreshes tunnel->tun_hlen, tunnel->hlen,
dev->needed_headroom and the MTU after a version or encapsulation change.
erspan_set_hlen() is also called before ip_tunnel_changelink() so that
ip_tunnel_update() and erspan_xmit() observe the new lengths.

A few questions about the new length computation and about the ordering on
the change path.

> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index ced57cbeaad49..696884f53cdcc 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -1379,18 +1379,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 length also account for tunnel->parms.o_flags, the way
ipgre_link_update() does via gre_calc_hlen(o_flags)?

erspan_validate() returns early as soon as the request carries
IFLA_GRE_ERSPAN_VER == 0:

	if (data[IFLA_GRE_ERSPAN_VER] &&
	    nla_get_u8(data[IFLA_GRE_ERSPAN_VER]) == 0)
		return 0;

so the "only GRE sequence and key flag" restriction is skipped and GRE_CSUM
can end up in tunnel->parms.o_flags for a version-0 device.

erspan_xmit() then copies o_flags and clears only SEQ (version 0) and KEY:

	ip_tunnel_flags_copy(flags, tunnel->parms.o_flags);
	if (tunnel->erspan_ver == 0) {
		proto = htons(ETH_P_ERSPAN);
		__clear_bit(IP_TUNNEL_SEQ_BIT, flags);
	...
	__clear_bit(IP_TUNNEL_KEY_BIT, flags);
	__gre_xmit(skb, dev, &tunnel->parms.iph, proto, flags);

With IP_TUNNEL_CSUM_BIT still set and tun_hlen == 4, gre_build_header()
computes ptr == greh:

include/net/gre.h:gre_build_header() {
	...
	if (ip_tunnel_flags_intersect(flags, cond)) {
		__be32 *ptr = (__be32 *)(((u8 *)greh) + hdr_len - 4);
	...
		if (test_bit(IP_TUNNEL_CSUM_BIT, flags) && ...) {
			*ptr = 0;
			if (skb->ip_summed == CHECKSUM_PARTIAL) {
				*(__sum16 *)ptr = csum_fold(lco_csum(skb));
			} else {
				skb->ip_summed = CHECKSUM_PARTIAL;
				skb->csum_start = skb_transport_header(skb) - skb->head;
				skb->csum_offset = sizeof(*greh);
			}
		}
	}
}

Can this overwrite greh->flags and greh->protocol that were just written,
and in the CHECKSUM_NONE case place the GRE checksum 4 bytes past the
4-byte GRE header, i.e. inside the mirrored frame?

For an "erspan_ver 0 ocsum" device this configuration was previously
consistent after a version 1/2 -> 0 change, because tun_hlen stayed at 8,
which happens to match the CSUM layout. Does the new erspan_set_hlen(t)
call on the change path turn that into a 4-byte tun_hlen with CSUM still
set?

[ ... ]

> @@ -1529,6 +1554,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))
> @@ -1540,16 +1566,29 @@ static int erspan_changelink(struct net_device *dev, struct nlattr *tb[],
>  
>  	err = erspan_netlink_parms(dev, data, tb, &p, &fwmark);
>  	if (err < 0)
> -		return err;
> +		goto link_update;
> +
> +	erspan_set_hlen(t);
>  
>  	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);

[Severity: High]
Here the new version (published inside erspan_netlink_parms() by
"All attributes parsed here have been validated, we can change @t") and the
new tun_hlen/hlen are committed before ip_tunnel_changelink(), while
o_flags is copied only on success. Can the failure path leave the two out of
sync?

ip_tunnel_changelink() can fail after both are already published:

net/ipv4/ip_tunnel.c:ip_tunnel_changelink() {
	t = ip_tunnel_find(itn, p, dev->type);

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

For example, with ers0 created as "type erspan erspan_ver 0 key 100 local A
remote B" and ersX as "type erspan erspan_ver 2 seq key 1000 local A remote
C", then:

	ip link set ers0 type erspan erspan_ver 2 seq key 1000 local A remote C

ip_tunnel_find() locates ersX, t->dev != dev, so -EEXIST is returned after
t->erspan_ver = 2 and tun_hlen = 8 have been stored, and o_flags keeps the
old value.

On the next transmit erspan_xmit() pushes the 12-byte v2 header and calls
__gre_xmit() -> gre_build_header(skb, 8, old_flags). If the old flags
contain neither SEQ nor CSUM (and KEY is cleared by erspan_xmit()), then
ip_tunnel_flags_intersect() is false and gre_build_header() only initialises
the 4-byte base header after having pushed 8 bytes.

Does that put 4 bytes of uninitialised skb headroom on the wire, with the
ERSPAN header offset by 4 bytes for the receiver? Before this patch the same
failed change left tun_hlen == 4, so exactly the pushed bytes were written.
Would it be better for the error path to reconcile version, GRE length and
flags together, or to roll the version back, rather than only refreshing
lengths?

[Severity: Medium]
A second question about the same two stores: t->erspan_ver is published
inside erspan_netlink_parms() and tun_hlen/hlen only on the following
statement, while erspan_xmit() and __gre_xmit() read all three locklessly
from ndo_start_xmit under RTNL-free conditions.

	err = erspan_netlink_parms(dev, data, tb, &p, &fwmark);   /* t->erspan_ver = 2 */
	if (err < 0)
		goto link_update;

	erspan_set_hlen(t);                                       /* tun_hlen = 8 */

A transmit landing between the two would read:

net/ipv4/ip_gre.c:erspan_xmit() {
	} else if (tunnel->erspan_ver == 2) {
		erspan_build_header_v2(...);          /* pushes 12 bytes */
	...
	__gre_xmit(skb, dev, &tunnel->parms.iph, proto, flags);
}

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

which is the same greh + 4 - 4 write over greh->flags/greh->protocol the
commit message describes, and for a 2 -> 0 change the mirror image (8 bytes
pushed, 4 initialised). Is a consistent snapshot of the (version, tun_hlen)
pair, or at least WRITE_ONCE() plus ordering, worth adding here? The same
publish-then-recompute window exists in ipgre_changelink() for
o_flags/tun_hlen, so this may be a known residual rather than something new,
but the commit message states erspan_xmit() "sees a matching tun_hlen".

>  
> -	return 0;
> +link_update:
> +	/* 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 on the error paths as well,
> +	 * since both leave the new encapsulation or version behind.
> +	 *
> +	 * As in ipgre_changelink(), IFLA_MTU must not hold the MTU back if we
> +	 * return an error, because do_setlink() will not apply it then.
> +	 */
> +	erspan_link_update(dev, err || !tb[IFLA_MTU], old_hlen);
> +
> +	return err;
>  }

[Severity: Low]
This is a pre-existing behaviour and not introduced by the patch, but on a
successful change carrying IFLA_MTU the automatic refresh is suppressed and
the MTU is left to do_setlink(), which can still fail:

net/core/rtnetlink.c:do_setlink() {
	if (tb[IFLA_MTU]) {
		err = netif_set_mtu_ext(dev, nla_get_u32(tb[IFLA_MTU]), extack);
		if (err < 0)
			goto errout;
	...
}

In that case the device keeps an MTU derived from the old, shorter header
(for instance 65497 from a version-0 device while the bound computed by
ip_tunnel_change_mtu() drops to 65481 for version 2). The
"set_mtu = !tb[IFLA_MTU]" convention is what ip_tunnel_changelink() already
passes to ip_tunnel_update(), and tnl_update_pmtu() re-derives the limit on
every transmit, so nothing malformed goes out. Mentioning it only in case
the deferred-MTU failure is worth handling here.

Also, the new comment above erspan_link_update() says both
tunnel->erspan_ver and tunnel->encap_hlen "feed tunnel->hlen"; given the
first question above, would it be accurate to mention tunnel->parms.o_flags
as an input to the GRE header length as well?

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

Re: [PATCH net v2 5/5] gre: fix out-of-bounds read of erspan metadata in collect_md mode

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Commit message / in-code comment mismatch: the patch is titled
  "gre: fix out-of-bounds read of erspan metadata in collect_md mode"…

Pre-existing issues:
- [Medium] erspan_rcv() (net/ipv4/ip_gre.c:334-361) hands collect_md
  consumers an ERSPAN option with md->version == 0 for ERSPAN type I…
- [Medium] is_erspan_type1() (net/ipv4/ip_gre.c:258-265) classifies a
  packet as ERSPAN type I purely from gre_hdr_len == 4, without…

--

gre: fix out-of-bounds read of erspan metadata in collect_md mode

This adds a check rejecting an ERSPAN base header whose version field is
neither 1 nor 2 in erspan_rcv() and ip6erspan_rcv(), and skips the md2
metadata copy in erspan_rcv() for ERSPAN type I, which carries no ERSPAN
header.  The collect_md option area is left as the zeroed area handed out
by ip_tun_rx_dst(), so md->version = 0 alone describes type I.

No Link: tags were present in the commit header.

First, a question about the changelog itself:

> gre: fix out-of-bounds read of erspan metadata in collect_md mode

and

>     Both read 8 bytes at 12 bytes from the start of the GRE header, but only
>     ask pskb_may_pull() for erspan_hdr_len(ver) bytes beyond it, which type I
>     support made 0 for version 0.

[Severity: Medium]
Is the "out-of-bounds read" description accurate here?  Both erspan_rcv()
and ip6erspan_rcv() only reach the memcpy() after
__iptunnel_pull_header(skb, len, htons(ETH_P_TEB), false, false) returned
success, and that helper linearizes an extra ETH_HLEN for ETH_P_TEB with
raw_proto == false:

net/ipv4/ip_tunnel_core.c:__iptunnel_pull_header() {
	...
	if (!raw_proto && inner_proto == htons(ETH_P_TEB)) {
		struct ethhdr *eh;

		if (unlikely(!pskb_may_pull(skb, ETH_HLEN)))
			return -ENOMEM;
	...
}

erspan_rcv() does "goto drop" and ip6erspan_rcv() does
"return PACKET_REJECT" when that fails, so at the copy point there are at
least 14 linear bytes from skb->data.  Walking the three cases in the
changelog:

- IPv4 type I: gre_hdr_len == 4, len == 4, pkt_md == skb->data + 4, an
  ERSPAN_V2_MDSIZE copy ends at skb->data + 12.
- IPv4 8 byte GRE with ershdr->ver == 0: len == 8, pkt_md == skb->data + 4,
  copy ends at skb->data + 12.
- IPv6 with ver == 0: len == 0, pkt_md == skb->data + 4, copy ends at
  skb->data + 12.

All three stay inside the linearized region, so nothing outside the skb is
touched and KASAN would not fire.  What does happen is that the first 8
bytes of the inner Ethernet frame get copied into md->u.md2 and an
unvalidated 4 bit wire value is stored in md->version, which is a
data-correctness problem rather than a memory-safety one.

Could the subject, the changelog and the stable justification describe that
instead, or is there a path that defeats the ETH_HLEN guarantee?

> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 696884f53cdcc..92f3a52d20d38 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c

[ ... ]

> @@ -294,6 +293,9 @@ static int erspan_rcv(struct sk_buff *skb, struct tnl_ptk_info *tpi,
>  
>  		ershdr = (struct erspan_base_hdr *)(skb->data + gre_hdr_len);
>  		ver = ershdr->ver;
> +		if (unlikely(ver != 1 && ver != 2))
> +			return PACKET_REJECT;
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but can this new check be
bypassed for a packet class that does carry a base header?

is_erspan_type1() decides purely on the GRE header length and never looks
at the GRE protocol:

net/ipv4/ip_gre.c:is_erspan_type1() {
	/* Both ERSPAN type I (version 0) and type II (version 1) use
	 * protocol 0x88BE, but the type I has only 4-byte GRE header,
	 * while type II has 8-byte.
	 */
	return gre_hdr_len == 4;
}

gre_parse_header() is more careful and treats the two protocols
differently:

net/ipv4/gre_demux.c:gre_parse_header() {
	if ((greh->protocol == htons(ETH_P_ERSPAN) && hdr_len != 4) ||
	    greh->protocol == htons(ETH_P_ERSPAN2)) {
		struct erspan_base_hdr *ershdr;

		if (!pskb_may_pull(skb, nhs + hdr_len + sizeof(*ershdr)))
	...
}

So a frame with greh->flags == 0 and greh->protocol == htons(ETH_P_ERSPAN2)
yields hdr_len == 4, and gre_rcv() still routes it to erspan_rcv():

net/ipv4/ip_gre.c:gre_rcv() {
	if (unlikely(tpi.proto == htons(ETH_P_ERSPAN) ||
		     tpi.proto == htons(ETH_P_ERSPAN2))) {
		if (erspan_rcv(skb, &tpi, hdr_len) == PACKET_RCVD)
			return 0;
	...
}

In erspan_rcv() is_erspan_type1(4) then returns true, ver is forced to 0 and
the new "if (unlikely(ver != 1 && ver != 2))" above is never reached for
that packet.  len also stays at 4, so the 4 byte base header plus the 8 byte
md2 are handed to ip_tunnel_rcv() as the start of the inner Ethernet frame.

Would making the type I predicate also require
tpi->proto == htons(ETH_P_ERSPAN) close that, given ETH_P_ERSPAN2 can never
describe type I?

> @@ -334,19 +337,26 @@ static int erspan_rcv(struct sk_buff *skb, struct tnl_ptk_info *tpi,
>  			info = &tun_dst->u.tun_info;
>  			info->options_len = sizeof(*md);
>  
> -			/* skb can be uncloned in __iptunnel_pull_header, so
> -			 * old pkt_md is no longer valid and we need to reset
> -			 * it
> -			 */
> -			gh = skb_network_header(skb) +
> -			     skb_network_header_len(skb);
> -			pkt_md = (struct erspan_metadata *)(gh + gre_hdr_len +
> -							    sizeof(*ershdr));
>  			md = ip_tunnel_info_opts(&tun_dst->u.tun_info);
>  			md->version = ver;

[Severity: Medium]
This is a pre-existing issue, but is the version 0 encoding usable by any
consumer of this option?  For type I this publishes md->version == 0 with
info->options_len = sizeof(*md) and IP_TUNNEL_ERSPAN_OPT_BIT set, while
every peer of that option only accepts 1 or 2:

net/ipv4/ip_gre.c:erspan_fb_xmit() {
	if (version == 1) {
		...
	} else if (version == 2) {
		...
	} else {
		goto err_free_skb;
	}
}

net/ipv6/ip6_gre.c:ip6erspan_tunnel_xmit() {
		if (md->version == 1) {
			...
		} else if (md->version == 2) {
			...
		} else {
			goto tx_err;
		}
}

net/ipv4/ip_tunnel_core.c:ip_tun_parse_opts_erspan() {
	ver = nla_get_u8(tb[LWTUNNEL_IP_OPT_ERSPAN_VER]);
	if (ver == 1) {
	...
	} else if (ver == 2) {
	...
	} else {
		return -EINVAL;
	}
}

So a BPF or OVS mirror that re-emits a received ERSPAN type I frame with the
received tun_info is dropped in erspan_fb_xmit() (kfree_skb plus
tx_dropped), even though the non collect_md path does implement type I:

net/ipv4/ip_gre.c:erspan_xmit() {
	if (tunnel->erspan_ver == 0) {
		proto = htons(ETH_P_ERSPAN);
		__clear_bit(IP_TUNNEL_SEQ_BIT, flags);
	} else if (tunnel->erspan_ver == 1) {
	...
}

The patch does not create that asymmetry, but the changelog wording
"md->version = 0 alone describes it" makes version 0 a permanent, blessed
encoding.  Should erspan_fb_xmit(), ip6erspan_tunnel_xmit() and the netlink
parser learn about type I, or should IP_TUNNEL_ERSPAN_OPT_BIT not be
advertised for type I at all?

> +
> +			/* Type I has no ERSPAN header, thus no metadata to
> +			 * extract: reading it would go past the @len bytes
> +			 * pulled above. ip_tun_rx_dst() zeroed @md for us.
> +			 */

[Severity: Medium]
Same question as on the changelog above: "reading it would go past the @len
bytes pulled above" leaves out that __iptunnel_pull_header() already
required ETH_HLEN more linear bytes, so the read landed on the inner
Ethernet frame rather than past the linear area.  Could the comment say that
type I has no metadata to copy and the bytes there belong to the inner
frame?

> +			if (!is_erspan_type1(gre_hdr_len)) {
> +				/* skb can be uncloned in __iptunnel_pull_header, so
> +				 * old pkt_md is no longer valid and we need to reset
> +				 * it
> +				 */
> +				gh = skb_network_header(skb) +
> +				     skb_network_header_len(skb);
> +				pkt_md = (struct erspan_metadata *)(gh + gre_hdr_len +
> +								    sizeof(*ershdr));
> +				md2 = &md->u.md2;
> +				memcpy(md2, pkt_md, ver == 1 ? ERSPAN_V1_MDSIZE :
> +							       ERSPAN_V2_MDSIZE);
> +			}

[Severity: Medium]
This isn't a bug introduced by this patch, but tied to the is_erspan_type1()
question above: for an ETH_P_ERSPAN2 frame with a 4 byte GRE header this
guard now skips the copy for a packet that does carry an ERSPAN v2 metadata
block, so collect_md consumers see version 0 with an all zero md2.  Before
this change the copy at gh + gre_hdr_len + sizeof(*ershdr) happened to land
on the real md2 for that packet.

>  
>  			__set_bit(IP_TUNNEL_ERSPAN_OPT_BIT,
>  				  info->key.tun_flags);
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index 8ebda0b6a78b2..a59fb82c74dad 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -503,6 +503,8 @@ static int ip6erspan_rcv(struct sk_buff *skb,
>  	ipv6h = ipv6_hdr(skb);
>  	ershdr = (struct erspan_base_hdr *)skb->data;
>  	ver = ershdr->ver;
> +	if (unlikely(ver != 1 && ver != 2))
> +		return PACKET_REJECT;

The version rejection itself looks right for this path, since ip6erspan_rcv()
always has a base header at skb->data.  The only question here is the one
raised on the changelog about the ver == 0 case being described as an out of
bounds read.

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