Re: [PATCH net v3 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: 0 · Medium: 1 (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] The patch only fixes the case where
  ip_tunnel_encap_setup()/ip6_tnl_encap_setup() itself fails.

Pre-existing issues:
- [Medium] ip_tunnel_encap_setup() and ip6_tnl_encap_setup() update
  t->encap.{type,sport,dport,flags}, t->encap_hlen and t->hlen one field…

--

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

This patch removes the memset() of t->encap from ip_tunnel_encap_setup()
and ip6_tnl_encap_setup(). After this change, a request that
ip_encap_hlen() or ip6_encap_hlen() rejects no longer leaves the tunnel
set to TUNNEL_ENCAP_NONE with stale encap_hlen and hlen values. The patch
is tagged for stable.

> 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: Medium]
This covers the case where ip_tunnel_encap_setup() itself fails. Can the
symptom described in the commit message still happen when encap setup
succeeds and the caller rejects the request afterwards?

The commit message says:

  "A rejected netlink request thus permanently disables FOU/GUE on a
  working tunnel while keeping its reduced MTU and extra headroom."

Once ip_encap_hlen() succeeds, ip_tunnel_encap_setup() writes
t->encap.{type,sport,dport,flags}, t->encap_hlen and t->hlen into the
live tunnel. In ipip_changelink(), the rest of the request is only
validated after that:

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

	ipip_netlink_parms(data, &p, &collect_md, &fwmark);
	if (collect_md)
		return -EINVAL;
    ...
}

Take a changelink on an existing FOU/GUE ipip tunnel that sends
IFLA_IPTUN_ENCAP_TYPE=TUNNEL_ENCAP_NONE together with
IFLA_IPTUN_COLLECT_METADATA:

- ip_encap_hlen() returns 0, so the encap is cleared and t->hlen shrinks.
- The collect_md check then returns -EINVAL.
- Userspace gets an error, but FOU/GUE is now off.
- dev->mtu keeps its reduced value, because ip_tunnel_update() never ran.

These other rejections also happen after the encap has been committed.
None of them roll it back or refresh dev->mtu or the headroom:

- ipip on the fallback device: "ip link change tunl0 encap fou ..."
  applies FOU to tunl0. ip_tunnel_changelink() then returns -EINVAL at
  "if (dev == itn->fb_tunnel_dev)".
- ipip_changelink() and ipip6_changelink(): the IFF_POINTOPOINT/daddr
  -EINVAL check, and the -EEXIST returns from ip_tunnel_changelink() and
  ipip6_tunnel_locate().
- ip6_tnl_changelink(): the "if (p.collect_md) return -EINVAL;" check and
  the -EEXIST from ip6_tnl_locate(). Both skip ip6_tnl_update().
- ip6gre_changelink_common(): the -EEXIST from ip6gre_tunnel_locate().

Later in this series, "ip_gre: validate netlink attributes before
changing the tunnel" fixes ip_gre. It moves ipgre_netlink_parms() first
and refreshes the lengths on the ip_tunnel_changelink() error path. At
the end of the series, ipip, sit, ip6_tunnel and ip6_gre are unchanged.

The commit message says callers still call ip[6]_tunnel_encap_setup()
before validating the rest of a changelink request. The patch is tagged
Fixes/Cc: stable for this symptom. Should these callers also validate the
whole request before they commit the new encap?

> 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 isn't a bug introduced by this patch, but it concerns the same
fields. ip_tunnel_encap_setup() and ip6_tnl_encap_setup() update
t->encap.{type,sport,dport,flags}, t->encap_hlen and t->hlen one at a
time with plain stores under RTNL. The transmit paths read those fields
with no lock and no snapshot.

In ip6_tnl_xmit(), encap_hlen is read on entry and later sizes
skb_cow_head():

net/ipv6/ip6_tunnel.c:ip6_tnl_xmit() {
    ...
	unsigned int psh_hlen = sizeof(struct ipv6hdr) + t->encap_hlen;
	unsigned int max_headroom = psh_hlen;
    ...
}

Much later, ip6_tnl_encap() reads t->encap.type again to pick the
callback, and it passes the mutable t->encap:

include/net/ip6_tunnel.h:ip6_tnl_encap() {
    ...
	ops = rcu_dereference(ip6tun_encaps[t->encap.type]);
	if (likely(ops && ops->build_header))
		ret = ops->build_header(skb, &t->encap, protocol, fl6);
    ...
}

Suppose a changelink switches from NONE to GUE between these two reads.
Could headroom be reserved with encap_hlen == 0 while a GUE header is
pushed? If the skb had only the minimum headroom, this might reach
skb_under_panic(), though it is not clear that such skbs reach this
path.

Could fou6_build_udp() also read a dport/flags pair from the next
configuration after the callback has already been chosen?

The IPv4 side has the same pattern. ip_tunnel_xmit() reads
tunnel->encap once for the header and again for the headroom:

net/ipv4/ip_tunnel.c:ip_tunnel_xmit() {
    ...
	if (ip_tunnel_encap(skb, &tunnel->encap, &protocol, &fl4) < 0)
		goto tx_error;
    ...
	max_headroom = LL_RESERVED_SPACE(rt->dst.dev) + sizeof(struct iphdr)
			+ rt->dst.header_len + ip_encap_hlen(&tunnel->encap);
    ...
}

None of these accesses use READ_ONCE()/WRITE_ONCE(), so KCSAN would
likely report them. Removing the memset makes the window smaller, because
readers no longer see a temporary TUNNEL_ENCAP_NONE. According to the
commit message, the net-next commit 88b84cae6b94 adds WRITE_ONCE() only
on the IPv4 side. Should the readers, and the IPv6 writer, get matching
annotations or use a consistent snapshot of the encap?

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