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

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.

[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) &&

[ ... ]

[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?

[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