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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 (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).

- [Medium] When the GRE header length changes, ipgre_link_update() now
  replaces the user-configured dev->mtu with the default computed by…
- [Medium] A changelink that publishes a new header length but carries an
  IFLA_MTU that do_setlink() later rejects leaves the tunnel with the…

Pre-existing issues:
- [Medium] Sibling changelink handlers publish a new encap_hlen through
  ip_tunnel_encap_setup() but never recompute dev->mtu or…

--

ip_gre: compute tunnel lengths absolutely instead of by delta

This commit changes ipgre_link_update() to recompute tunnel->hlen from
tun_hlen and encap_hlen instead of applying a delta. It adds
ip_tunnel_refresh_lengths() so that ip_tunnel_bind_dev() is the only
writer of dev->needed_headroom and dev->mtu. ipgre_changelink() now also
refreshes the lengths on the ip_tunnel_changelink() error path.

> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index df4d2f1f1d60c..27b3b4c584b1e 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c

[ ... ]

> +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 (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 overwrite an MTU that the user configured? Before this patch, a
change to tun_hlen reduced the current dev->mtu by the delta:

    WRITE_ONCE(dev->mtu, max_t(int, dev->mtu - len, 68));

Now, when hlen differs from old_hlen, ip_tunnel_refresh_lengths() writes
the value that ip_tunnel_bind_dev() computes from the underlay. The
current dev->mtu is not taken into account.

The ioctl path always passes set_mtu = true:

ipgre_tunnel_ctl()
    ipgre_link_update(dev, true, t->hlen)
        ip_tunnel_refresh_lengths(dev, true)
            WRITE_ONCE(dev->mtu, ip_tunnel_bind_dev(dev))

ipgre_changelink() does the same whenever IFLA_MTU is absent.

Take gre1 over a 1500 byte eth0 with MTU 1300 set, and run
"ip link set gre1 type gre okey 1" or "ip tunnel change gre1 key 1". The
MTU becomes 1472 instead of 1296, which is larger than the value the
user chose.

If ip_tunnel_bind_dev() finds no underlay device (daddr == 0 with no
link, or a failed route lookup), the MTU falls back to
ETH_DATA_LEN - t_hlen. A jumbo MTU of 8976 would then drop to 1472.

The comment says the header length change invalidated the MTU. A user
MTU that is still below the new maximum still looks valid, though. The
later patches in the series do not appear to change this.

This is tagged for stable. Should the commit message describe this
behaviour change? Or should the user's MTU be kept when it still fits?

[ ... ]

> @@ -1509,17 +1524,35 @@ static int ipgre_changelink(struct net_device *dev, struct nlattr *tb[],
>  	if (err)
>  		return err;
>  
> +	link_changed = t->parms.link != p.link || t->fwmark != gparms.fwmark;
> +
>  	err = ip_tunnel_changelink(dev, tb, &p, gparms.fwmark);
>  	if (err < 0)
> -		return err;
> +		goto link_update;

[ ... ]

> -	ipgre_link_update(dev, !tb[IFLA_MTU]);
> +link_update:
> +	/* ipgre_newlink_encap_setup() has published a new encapsulation even
> +	 * if ip_tunnel_changelink() failed, so the lengths must be refreshed
> +	 * on that error path as well.
> +	 *
> +	 * 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: Medium]
What happens if IFLA_MTU is present but do_setlink() then fails to apply
it?

When ip_tunnel_changelink() succeeds and tb[IFLA_MTU] is set, set_mtu is
false here. dev->mtu is left alone on the assumption that do_setlink()
will set it. However, rtnl_changelink() only calls do_setlink() after
ops->changelink() has committed the new encap and hlen, and do_setlink()
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;
    ...
}

For example, IFLA_MTU = 0 passes the NLA_U32 policy and
validate_linkmsg(), and netif_set_mtu_ext() then rejects it. Other
do_setlink() steps that run before the MTU step can also fail.

Take a bare GRE tunnel over a 1500 byte device and add "encap fou" in
such a request. The MTU would stay at 1476 instead of 1468, which is the
stale MTU this patch sets out to fix.

A later changelink without a header change would not repair it either,
because old_hlen would already equal the new hlen.

The comment covers rtnl_changelink() not reaching do_setlink() on error.
Does it also need to handle the case where do_setlink() is reached and
fails? The same pattern also seems to be copied into erspan_changelink()
later in the series.

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

[ ... ]

> 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

[ ... ]

> +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: Medium]
This is a pre-existing issue and was not introduced by this patch. Other
changelink handlers that call ip_tunnel_encap_setup() seem to have the
same stale length problem. At this commit that includes
erspan_changelink() in net/ipv4/ip_gre.c, ipip_changelink() in
net/ipv4/ipip.c and ipip6_changelink() in net/ipv6/sit.c.

For example, ipip_changelink():

net/ipv4/ipip.c:ipip_changelink() {
    ...
	if (ip_tunnel_netlink_encap_parms(data, &ipencap)) {
		int err = ip_tunnel_encap_setup(t, &ipencap);

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

ip_tunnel_update() only calls ip_tunnel_bind_dev() when the link or
fwmark changed:

	if (t->parms.link != p->link || t->fwmark != fwmark) {

So "ip link set ipip0 type ipip encap fou ..." would leave dev->mtu and
dev->needed_headroom stale.

The later commit "ip_gre: recompute erspan header lengths after a change"
fixes the erspan part. It adds erspan_link_update() and calls it on both
the success and error paths.

ipip_changelink() and ipip6_changelink() still do not call
ip_tunnel_refresh_lengths() at the end of the series. Would it make
sense to use the new helper there as well?

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