* Re: [PATCH net] ip_tunnel: reserve headroom for the encap header before pushing it
2026-10-10 22:36 [PATCH net] ip_tunnel: reserve headroom for the encap header before pushing it Theodor Arsenij Larionov Trichkine
@ 2026-10-11 22:40 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-10-11 22:40 UTC (permalink / raw)
To: theodorlarionov
Cc: davem, edumazet, kuba, pabeni, horms, dsahern, idosch, cehrig,
ast, tom, netdev, linux-kernel
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 0 · Low: 1 (1)
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] The patch adds skb_cow_head(skb, encap_hlen) at the top of
ip_tunnel_xmit() (net/ipv4/ip_tunnel.c:700), before the first…
- [High] In ip_md_tunnel_xmit(), the headroom is sized from one live read
of the shared tun_info->encap: `encap_hlen =…
- [Low] The new early skb_cow_head(skb, encap_hlen) in ip_tunnel_xmit()
(net/ipv4/ip_tunnel.c:700) runs on every packet, including tunnels…
Pre-existing issues:
- [Low] ip_tunnel_xmit() now does `encap = READ_ONCE(tunnel->encap);`
(net/ipv4/ip_tunnel.c:696) under the comment 'Size and push the encap…
--
ip_tunnel: reserve headroom for the encap header before pushing it
This patch adds an early skb_cow_head() for the FOU/GUE encap header length
at the start of ip_tunnel_xmit() and ip_md_tunnel_xmit(). The goal is that
ip_tunnel_encap() no longer pushes the encap header into headroom that was
never reserved. ip_tunnel_xmit() also takes one READ_ONCE() snapshot of
tunnel->encap and uses it for both the reservation and the push.
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index e6bcf01411d0b..38ab3c3bf5c9a 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
[ ... ]
> @@ -587,6 +588,11 @@ void ip_md_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
> ip_tunnel_info_af(tun_info) != AF_INET))
> goto tx_error;
> key = &tun_info->key;
> + encap_hlen = ip_encap_hlen(&tun_info->encap);
> + if (encap_hlen < 0)
> + goto tx_error;
> + if (skb_cow_head(skb, encap_hlen))
> + goto tx_dropped;
[Severity: High]
Is tun_info->encap stable between this reservation and the push?
The headroom is sized here from one read of tun_info->encap. The header is
then pushed later in ip_md_tunnel_xmit() using separate reads:
if (!tunnel_hlen)
tunnel_hlen = ip_encap_hlen(&tun_info->encap);
if (ip_tunnel_encap(skb, &tun_info->encap, &proto, &fl4) < 0)
goto tx_error;
The fou/gue build_header callbacks also read e->flags again.
tun_info is not necessarily private to this skb. bpf_skb_set_tunnel_key()
attaches the per-CPU md_dst to every skb it handles on that CPU, and then
rewrites it in place:
net/core/filter.c:bpf_skb_set_tunnel_key() {
struct metadata_dst *md = this_cpu_ptr(md_dst);
...
skb_dst_drop(skb);
dst_hold((struct dst_entry *) md);
skb_dst_set(skb, (struct dst_entry *) md);
info = &md->u.tun_info;
memset(info, 0, sizeof(*info));
...
}
bpf_skb_set_fou_encap() also writes info->encap.type/flags/sport/dport in
place.
Suppose an skb is tagged on CPU0, queued on a qdisc of a collect_md ipip
device, and sent from CPU1. Meanwhile CPU0's tc BPF program rewrites the
same md_dst (memset to TUNNEL_ENCAP_NONE, then GUE). Could CPU1 reserve 0
or 8 bytes here and then push a 12-20 byte GUE header?
That would hit the same skb_under_panic() in fou_build_udp() that this
patch fixes. In the mirred mirror case it would overwrite the bytes in
front of skb->data instead.
ip_tunnel_xmit() takes a local copy of the encap config for this reason.
Would a local snapshot here close the same gap on the md path? For example:
struct ip_tunnel_encap encap = tun_info->encap;
used for the reservation, tunnel_hlen and ip_tunnel_encap().
> memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
> inner_iph = (const struct iphdr *)skb_inner_network_header(skb);
> tos = key->tos;
[ ... ]
> @@ -679,11 +686,23 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
[ ... ]
> + /* Size and push the encap header from one copy of the config. */
> + encap = READ_ONCE(tunnel->encap);
[Severity: Low]
This is a pre-existing issue, not one introduced by this patch, but the
writer side has no matching marked store for this READ_ONCE().
ip_tunnel_encap_setup() zeroes t->encap and then fills it in with plain
stores:
net/ipv4/ip_tunnel.c:ip_tunnel_encap_setup() {
...
memset(&t->encap, 0, sizeof(t->encap));
hlen = ip_encap_hlen(ipencap);
if (hlen < 0)
return hlen;
t->encap.type = ipencap->type;
t->encap.sport = ipencap->sport;
t->encap.dport = ipencap->dport;
t->encap.flags = ipencap->flags;
...
}
So a snapshot taken during changelink can still be half-written. It might
see type NONE right after the memset, or a new type with sport/dport still
0.
The length and the push now both come from the local copy. Because of
that, the worst case is a few packets sent without encap or with UDP port
0, plus a possible KCSAN report. The old plain reads could already see
these same states.
Would it make sense to build the new config in a local struct and store it
once on the writer side, as a follow-up?
> + encap_hlen = ip_encap_hlen(&encap);
> + if (encap_hlen < 0)
> + goto tx_error;
> + if (skb_cow_head(skb, encap_hlen)) {
> + DEV_STATS_INC(dev, tx_dropped);
> + kfree_skb(skb);
> + return;
> + }
[Severity: High]
Can this new skb_cow_head() free the memory that tnl_params points to?
For GRE devices with header_ops (NBMA or a multicast remote), ipgre_xmit()
passes ip_tunnel_xmit() a tnl_params pointer that lives inside the skb
head:
net/ipv4/ip_gre.c:ipgre_xmit() {
...
if (dev->header_ops) {
int pull_len = tunnel->hlen + sizeof(struct iphdr);
if (skb_cow_head(skb, 0))
goto free_skb;
...
tnl_params = (const struct iphdr *)skb->data;
/* ip_tunnel_xmit() needs skb->data pointing to gre header. */
skb_pull(skb, pull_len);
...
}
That pointer reaches ip_tunnel_xmit() through __gre_xmit():
ip_tunnel_xmit(skb, dev, tnl_params, tnl_params->protocol);
If skb_cow_head(skb, encap_hlen) ends up in pskb_expand_head(), the old head
is freed. Every later read of tnl_params (->daddr, ->tos, ->saddr,
->frag_off, ->ttl) then reads freed memory. Those values feed the route
lookup and the outer IP header.
Before this patch, the only skb_cow_head() in ip_tunnel_xmit() ran after the
last tnl_params read.
For the reallocation to happen, the headroom at entry has to be smaller than
encap_hlen. In the GRE header_ops path the tunnel config is read three times
without locking:
- tunnel->hlen in ipgre_xmit(), for pull_len
- tunnel->tun_hlen in __gre_xmit()
- tunnel->encap here
ipgre_changelink() writes t->encap and t->hlen in
ipgre_newlink_encap_setup(). It later writes tun_hlen and hlen in
ipgre_link_update(). There is no synchronize_net() in between.
For example, take a single "ip link set" that changes encap from none to GUE
with remcsum (0 -> 20 bytes) and o_flags to csum|key|seq (tun_hlen 4 -> 16).
If a packet is between the hlen read and the tun_hlen read, it has about
H0 + 8 bytes of headroom, where H0 is the headroom it arrived with. For
small H0 that is below 20.
Before the patch, that race ended in skb_under_panic(). With the patch it
looks like it becomes a silent use-after-free read.
Doesn't this contradict this part of the commit message?
ip_tunnel_xmit() reads tunnel->encap once, so a concurrent
changelink cannot change the length between the reservation and the
push.
Would either of these avoid it: copying *tnl_params to a local struct before
the early skb_cow_head(), or moving the reservation after the last
tnl_params read?
[Severity: Low]
Should this skb_cow_head() be skipped when encap_hlen is 0?
__skb_cow() calls pskb_expand_head() whenever skb_header_cloned() is true,
even when delta is 0. ipip_tunnel_xmit() and sit pass skbs through without
uncloning them.
Take a header-cloned skb going into a plain ipip tunnel with no encap, such
as the mirred mirror case from the commit message. It is now uncloned here.
If its headroom is still too small, the later skb_cow_head(skb,
max_headroom) reallocates it a second time.
Before the patch, one pskb_expand_head() in the max_headroom call handled
both. Either guarding this call with if (encap_hlen), or reserving the full
estimate in one call, would avoid the extra allocation and copy per packet.
> +
> inner_iph = (const struct iphdr *)skb_inner_network_header(skb);
> connected = (tunnel->parms.iph.daddr != 0);
> payload_protocol = skb_protocol(skb, true);
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010223601.1193655-1-theodorlarionov%40gmail.com
^ permalink raw reply [flat|nested] 2+ messages in thread