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

[netdev-bot+sashiko@kernel.org]

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