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

[netdev-bot+sashiko@kernel.org]

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