Re: [PATCH net v4] ip_tunnel: reserve FOU/GUE headroom before encapsulation

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (2) · Medium: 2 (2) · Low: 0 (2)

- [Medium] Incomplete fix: the patch establishes the invariant "reserve
  ip_encap_hlen() bytes before ip_tunnel_encap() pushes the FOU/GUE+UDP…
- [Medium] Undisclosed on-the-wire behaviour change: moving
  ip_tunnel_encap() from before the route lookup to after it…

Pre-existing issues:
- [High] The IPv4 fix leaves the identical headroom-underflow race
  reachable in the IPv6 tunnel transmit path.
- [High] Peer call-site inconsistency with a concrete functional
  consequence: after this patch ip_tunnel_xmit() calls ip_tunnel_encap()…
- [Medium] Residual race not covered by the fix, in the GRE header build
  the commit message's timeline explicitly names.
- [Medium] Same bug class in a sibling caller: net/ipv6/sit.c
  ipip6_tunnel_xmit() sizes its headroom from a racy read of…
- [Low] Reported concern (refuted for the ICMP/-E2BIG decision): the
  concern states that `pkt_size = skb->len + encap_hlen - tunnel_hlen`…
- [Low] `ipencap = data_race(tunnel->encap)` (net/ipv4/ip_tunnel.c:772)
  only suppresses the KCSAN report; it does not make the 8-byte four-…

--

ip_tunnel: reserve FOU/GUE headroom before encapsulation

This moves ip_tunnel_encap() in ip_tunnel_xmit() to run after the route
lookup, PMTU handling and skb_cow_head(), snapshots tunnel->encap into a
local copy used for both ip_encap_hlen() and ip_tunnel_encap(), and adds
an encap_hlen argument to tnl_update_pmtu() so the inner packet size is
still computed as skb->len + encap_hlen - tunnel_hlen.

Link: https://lore.kernel.org/netdev/b58876297f7d45de008f2e94b6ecab8b2ed84d21.1786088695.git.petalzu987@xxxxxxxxx/

A few questions on the changelog first.

>   CPU 0 (ipgre_changelink)        CPU 1 (ipgre_xmit)
>   install GUE encapsulation
>                                   reserve the old needed_headroom
>   publish larger GRE flags
>   update tunnel->tun_hlen
>                                   push the larger GRE header
>                                   push the GUE and UDP headers
>   update dev->needed_headroom

[Severity: Medium]
This isn't a bug introduced by this patch, but since the table names
"publish larger GRE flags" and "update tunnel->tun_hlen" as steps of the
race, is the GRE header build itself still exposed after this change?

ipgre_xmit() snapshots the flags, but __gre_xmit() then does a separate,
later read of tunnel->tun_hlen:

net/ipv4/ip_gre.c:__gre_xmit() {
	...
	gre_build_header(skb, tunnel->tun_hlen,
			 flags, proto, tunnel->parms.o_key,
			 test_bit(IP_TUNNEL_SEQ_BIT, flags) ?
			 htonl(atomic_fetch_inc(&tunnel->o_seqno)) : 0);
	...
}

ipgre_changelink() stores the new parms.o_flags and only afterwards calls
ipgre_link_update(), which sets tunnel->tun_hlen, so hdr_len and flags can
come from different generations.

With new flags and an old tun_hlen of 4, gre_build_header() computes
ptr = greh + hdr_len - 4 == greh and writes the sequence number over
greh->flags/protocol, and the key/checksum words land before greh.

With old flags (none set) and a new tun_hlen of 16, 16 bytes are pushed but
the SEQ/KEY/CSUM branch is skipped, so 12 bytes of uninitialised headroom
go on the wire.

Neither case can underflow skb->head (the reservation always leaves at
least 20 bytes after the GRE push), so this is not the skb_under_panic()
the patch targets. Would deriving tun_hlen from the flags snapshot with
gre_calc_hlen() close it the same way the encap snapshot does here?

> The IPv6 analogue of this headroom reservation is still work in
> progress.

[Severity: High]
This is a pre-existing issue and the changelog says as much, but for the
record on what remains reachable: ip6_tnl_xmit() sizes the reservation from
one early read of t->encap_hlen:

net/ipv6/ip6_tunnel.c:ip6_tnl_xmit() {
	unsigned int psh_hlen = sizeof(struct ipv6hdr) + t->encap_hlen;
	unsigned int max_headroom = psh_hlen;
	...
	if (skb_cow_head(skb, max_headroom))
		goto tx_err_dst_release;
	...
	err = ip6_tnl_encap(skb, t, &proto, fl6);
}

ip6gre_changelink_common()/ip6_tnl_changelink() -> ip6_tnl_encap_setup()
can replace TUNNEL_ENCAP_NONE with GUE plus REMCSUM between the length read
and the encap build, and the transmit path takes no lock.

Reserved is 40 + encap_old + LL_RESERVED_SPACE(tdev); pushed after the cow
is encap_new + 40. With encap_old = 0, encap_new = 20 (udphdr 8 + guehdr 4
+ GUE_LEN_PRIV 4 + GUE_PLEN_REMCSUM 4) and LL_RESERVED_SPACE(tdev) = 16 for
an Ethernet underlay, that is a 4 byte deficit, so the skb_push() inside
__gue_build_header()/fou_build_udp() hits skb_under_panic() and BUG().

__gre6_xmit() has the same shape: it reserves a possibly stale
dev->needed_headroom before reading the current GRE parameters. Is the
IPv6 follow-up expected in the same series, given this one is tagged for
stable?

> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index e6bcf01411d0b..a8c846aab30c6 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -512,14 +512,15 @@ EXPORT_SYMBOL_GPL(ip_tunnel_encap_setup);
>  static int tnl_update_pmtu(struct net_device *dev, struct sk_buff *skb,
>  			    struct rtable *rt, __be16 df,
>  			    const struct iphdr *inner_iph,
> -			    int tunnel_hlen, __be32 dst, bool md)
> +			    int tunnel_hlen, __be32 dst, bool md,
> +			    int encap_hlen)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
>  	int pkt_size;
>  	int mtu;
>  
>  	tunnel_hlen = md ? tunnel_hlen : tunnel->hlen;
> -	pkt_size = skb->len - tunnel_hlen;
> +	pkt_size = skb->len + encap_hlen - tunnel_hlen;

[Severity: Low]
This is a pre-existing property rather than something the patch breaks, but
it is worth noting for anyone reading the new formula: encap_hlen is a
snapshot while tunnel->hlen here is a plain read, and the GRE bytes already
on the skb came from yet another read of tunnel->tun_hlen in __gre_xmit().

On the df branch the skew cancels, because the same tunnel_hlen is
subtracted from dst_mtu():

	mtu = dst_mtu(&rt->dst) - (sizeof(struct iphdr) + tunnel_hlen);

so mtu < pkt_size reduces to
sizeof(struct iphdr) + gre_len_on_skb + encap_snapshot + inner > dst_mtu,
which is correct whichever generation tunnel->hlen came from.

Only the df == 0 branch, where mtu comes from dst_mtu(skb_dst)/dev->mtu,
keeps a residual skew of up to the GRE/encap delta during a reconfiguration
window, and that skew existed before this patch too. Is that reading
correct, or is there a case where the new term makes the df == 0 branch
worse?

> @@ -629,7 +630,7 @@ void ip_md_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
>  	if (test_bit(IP_TUNNEL_DONT_FRAGMENT_BIT, key->tun_flags))
>  		df = htons(IP_DF);
>  	if (tnl_update_pmtu(dev, skb, rt, df, inner_iph, tunnel_hlen,
> -			    key->u.ipv4.dst, true)) {
> +			    key->u.ipv4.dst, true, 0)) {
>  		ip_rt_put(rt);
>  		goto tx_error;
>  	}

[Severity: Medium]
The changelog says of this function:

  "ip_md_tunnel_xmit() is left unchanged. It takes FOU/GUE parameters
   from the skb metadata dst, not from the device configuration, so it
   is not exposed to this race."

The race is indeed different here, but does the invariant the patch
establishes ("reserve ip_encap_hlen() bytes before ip_tunnel_encap()
pushes") still hold on this path?

ip_md_tunnel_xmit() pushes the encap first:

	if (ip_tunnel_encap(skb, &tun_info->encap, &proto, &fl4) < 0)
		goto tx_error;

and grows the headroom only later, without an encap term:

	headroom += LL_RESERVED_SPACE(rt->dst.dev) + rt->dst.header_len;
	if (skb_cow_head(skb, headroom)) {
		...
	}
	ip_tunnel_adj_headroom(dev, headroom);

So the UDP/GUE push has no reservation from this function, and the
dev->needed_headroom advertised by ip_tunnel_adj_headroom() under-reports
the metadata encap size for subsequent packets.

The metadata encap is live: bpf_skb_set_fou_encap() sets
info->encap.type = TUNNEL_ENCAP_FOU/GUE together with sport/dport and
TUNNEL_ENCAP_FLAG_CSUM.

The ipip collect_md path looks like the thinnest case, since it does no
skb_cow_head() at all before the call:

net/ipv4/ipip.c:ipip_tunnel_xmit() {
	...
	if (tunnel->collect_md)
		ip_md_tunnel_xmit(skb, dev, ipproto, 0);
	...
}

For GRE collect_md, ip_tunnel_bind_dev() currently leaves slack
(needed_headroom is at least 56 versus at most 16 bytes of GRE plus 12
bytes of BPF-settable GUE), so no deterministic crash could be
constructed; for ipip there is no pre-push cow, but no caller delivering
an skb with under 12 bytes of headroom was identified either. Would it be
worth either restructuring this function the same way, or noting in the
changelog that its headroom accounting is also missing ip_encap_hlen()?

[Severity: High]
This is a pre-existing issue and not introduced by this patch, but the
patch fixes the same ordering on the ip_tunnel_xmit() side, so it may
belong in the series: ip_md_tunnel_xmit() still calls ip_tunnel_encap()
before dst_cache_get_ip4()/ip_route_output_key(), so fl4.saddr is
unresolved when the outer UDP checksum is built.

net/ipv4/fou_core.c:fou_build_udp() {
	...
	udp_set_csum(!(e->flags & TUNNEL_ENCAP_FLAG_CSUM), skb,
		     fl4->saddr, fl4->daddr, skb->len);
	...
}

On this path fl4.saddr == key->u.ipv4.src, and bpf_skb_set_tunnel_key()
leaves that at 0 unless the program sets local_ipv4, while defaulting the
checksum on:

net/core/filter.c:bpf_skb_set_tunnel_key() {
	...
	__assign_bit(IP_TUNNEL_CSUM_BIT, info->key.tun_flags,
		     !(flags & BPF_F_ZERO_CSUM_TX));
	...
}

bpf_skb_set_fou_encap() turns that into TUNNEL_ENCAP_FLAG_CSUM. The
pseudo-header then uses saddr = 0 while iptunnel_xmit() writes the
route-selected fl4.saddr into the outer IP header, so does every such
packet leave with a UDP checksum the receiver rejects in
udp4_csum_init()/udp_rcv()?

Nothing else appears to fill key.u.ipv4.src on this path: the only caller
of dev_fill_metadata_dst(), which would set it via
gre_fill_metadata_dst(), is openvswitch's output_userspace().

> @@ -765,7 +768,10 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
>  			    tunnel->net, READ_ONCE(tunnel->parms.link),
>  			    tunnel->fwmark, skb_get_hash(skb), 0);
>  
> -	if (ip_tunnel_encap(skb, &tunnel->encap, &protocol, &fl4) < 0)
> +	/* Snapshot encap; ipgre_changelink() can update it concurrently. */
> +	ipencap = data_race(tunnel->encap);

[Severity: Low]
This is a pre-existing exposure rather than a new one, but the comment plus
data_race() could be read as asserting the snapshot is coherent. Does
data_race() do anything beyond silencing KCSAN for this 8 byte, four field
copy?

The writer publishes the fields separately, starting with a memset:

net/ipv4/ip_tunnel.c:ip_tunnel_encap_setup() {
	memset(&t->encap, 0, sizeof(t->encap));
	...
}

so a snapshot can observe type == TUNNEL_ENCAP_GUE with dport == 0, or
with flags == 0 (no CSUM/REMCSUM the peer expects), or a fully zeroed copy
in which case type == TUNNEL_ENCAP_NONE and the packet goes out with no
encapsulation.

For headroom safety the patch is clearly an improvement, since
ip_encap_hlen() and ip_tunnel_encap() now consume the same local copy
instead of reading tunnel->encap twice. Would publishing the encap
configuration through RCU or a seqcount be the more robust follow-up?

> +	encap_hlen = ip_encap_hlen(&ipencap);
> +	if (encap_hlen < 0)
>  		goto tx_error;
>  
>  	if (connected && md) {

[ ... ]

> @@ -845,6 +852,11 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
>  
>  	ip_tunnel_adj_headroom(dev, max_headroom);
>  
> +	if (ip_tunnel_encap(skb, &ipencap, &protocol, &fl4) < 0) {
> +		ip_rt_put(rt);
> +		goto tx_error;
> +	}
> +

[Severity: Medium]
Does moving the call to this point also change what goes on the wire?
ip_tunnel_init_flow() sets fl4.saddr = tnl_params->saddr, which is 0 for a
tunnel created without a local address, and fl4.saddr is only resolved
during the route lookup:

net/ipv4/fib_semantics.c:fib_select_path() {
	if (fl4->flowi4_oif)
		goto check_saddr;
	...
}

Before this change fou_build_udp() ran right after
ip_tunnel_init_flow(), so udp_set_csum() built the pseudo-header with
saddr == 0 while iptunnel_xmit() later wrote the route-selected saddr into
the outer IP header. After the change the pseudo-header uses the real
source address.

For a tunnel such as:

	ip link add gre1 type gre remote R encap gue encap-csum

with no local address, the emitted outer UDP checksum therefore differs
before and after. That looks like a fix rather than a regression, but the
changelog is framed purely as a headroom fix and carries Fixes: and
Cc: stable. Could the reorder and its checksum effect be described there,
so backporters and bisecting users are not surprised?

[Severity: Medium]
This isn't a bug introduced by this patch and is outside its stated scope,
but net/ipv6/sit.c has the same reserve-then-push inversion and is not
mentioned as WIP.

ipip6_tunnel_xmit() sizes the reservation from a racy read of
tunnel->hlen:

net/ipv6/sit.c:ipip6_tunnel_xmit() {
	int t_hlen = tunnel->hlen + sizeof(struct iphdr);
	...
	max_headroom = LL_RESERVED_SPACE(tdev) + t_hlen;

	if (skb_headroom(skb) < max_headroom || skb_shared(skb) ||
	...
	if (ip_tunnel_encap(skb, &tunnel->encap, &protocol, &fl4) < 0) {
	...
}

and iptunnel_xmit() then pushes another 20 bytes of IP header.
ipip6_changelink() -> ip_tunnel_encap_setup() publishes t->encap before
t->encap_hlen and t->hlen, so a concurrent transmit can reserve
LL_RESERVED_SPACE + old_hlen + 20 and then push up to 20 bytes of GUE/UDP
with REMCSUM plus 20 bytes of IP. With encap_old = 0, encap_new = 20 and
LL_RESERVED_SPACE(tdev) = 16 that is a 4 byte deficit and skb_under_panic().

No deterministic trigger could be built, since LL_RESERVED_SPACE rounding
and SKB_DATA_ALIGN slack in skb_realloc_headroom() usually leave spare
bytes, so this is only structurally reachable. Should sit get the same
snapshot-then-reserve treatment?

>  	iptunnel_xmit(NULL, rt, skb, fl4.saddr, fl4.daddr, protocol, tos, ttl,
>  		      df, !net_eq(tunnel->net, dev_net(dev)), 0);
>  	return;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904165544.1362052-1-nicoyip.dev%40gmail.com