Re: [PATCH net v4 1/1] ip6_tunnel: snapshot encap in xmit

[Ido Schimmel <idosch@nvidia.com>]

On Sat, Sep 05, 2026 at 06:01:36PM +0800, Ren Wei wrote:
> From: Zixuan Chai <petalzu987@gmail.com>
> 
> ip6_tnl_changelink() can update encapsulation parameters while the
> netdevice is transmitting packets. ip6_tnl_xmit() can calculate packet
> headroom with t->encap_hlen and later build an encapsulation header from
> the live t->encap. A concurrent update can change the encapsulation
> header between these accesses and make skb_push() underflow the skb head.
> 
> Take a local snapshot of t->encap before calculating the encapsulation
> header length. Use that same snapshot for headroom accounting, metadata
> validation, and build_header(). This keeps all encapsulation decisions
> for an skb consistent even if changelink updates the live configuration.
> 
> Fixes: b3a27b519b22 ("ip6_tunnel: Add support for fou/gue encapsulation")
> Cc: stable@vger.kernel.org
> Reported-by: Vega <vega@nebusec.ai>
> Assisted-by: LLM
> Signed-off-by: Zixuan Chai <petalzu987@gmail.com>
> Signed-off-by: Ren Wei <weir@nebusec.ai>

tl;dr - Some changes are needed for v5.

> ---
>  include/net/ip6_tunnel.h | 10 +++++-----
>  include/net/ip_tunnels.h | 10 ++++++++++
>  net/ipv6/ip6_tunnel.c    | 18 ++++++++++++++----
>  3 files changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/ip6_tunnel.h b/include/net/ip6_tunnel.h
> index b99805ee2fd1..6e76e50a4406 100644
> --- a/include/net/ip6_tunnel.h
> +++ b/include/net/ip6_tunnel.h
> @@ -106,22 +106,22 @@ static inline int ip6_encap_hlen(struct ip_tunnel_encap *e)
>  	return hlen;
>  }
>  
> -static inline int ip6_tnl_encap(struct sk_buff *skb, struct ip6_tnl *t,
> +static inline int ip6_tnl_encap(struct sk_buff *skb, struct ip_tunnel_encap *e,
>  				u8 *protocol, struct flowi6 *fl6)
>  {
>  	const struct ip6_tnl_encap_ops *ops;
>  	int ret = -EINVAL;
>  
> -	if (t->encap.type == TUNNEL_ENCAP_NONE)
> +	if (e->type == TUNNEL_ENCAP_NONE)
>  		return 0;
>  
> -	if (t->encap.type >= MAX_IPTUN_ENCAP_OPS)
> +	if (e->type >= MAX_IPTUN_ENCAP_OPS)
>  		return -EINVAL;
>  
>  	rcu_read_lock();
> -	ops = rcu_dereference(ip6tun_encaps[t->encap.type]);
> +	ops = rcu_dereference(ip6tun_encaps[e->type]);
>  	if (likely(ops && ops->build_header))
> -		ret = ops->build_header(skb, &t->encap, protocol, fl6);
> +		ret = ops->build_header(skb, e, protocol, fl6);
>  	rcu_read_unlock();
>  
>  	return ret;
> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index 7c9aadfe8fe3..ab217ddbeb20 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -522,6 +522,16 @@ skb_vlan_inet_prepare(struct sk_buff *skb, bool inner_proto_inherit)
>  	return SKB_NOT_DROPPED_YET;
>  }
>  
> +static inline void
> +ip_tunnel_encap_snapshot(struct ip_tunnel_encap *dst,
> +			 const struct ip_tunnel_encap *src)
> +{
> +	dst->type = READ_ONCE(src->type);
> +	dst->flags = READ_ONCE(src->flags);
> +	dst->sport = READ_ONCE(src->sport);
> +	dst->dport = READ_ONCE(src->dport);
> +}

From Clashiko:

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/2ce8f3e4bbed6060afb0aee33dc4e1d9ae021280.1788262122.git.petalzu987%40gmail.com

"
The four READ_ONCE() loads set up a lockless read protocol for t->encap,
but the writer side stays unannotated.  Is the snapshot actually atomic
across the four fields?
"

Zixuan Chai, please drop the memset() from ip6_tnl_encap_setup() and add
WRITE_ONCE() annotations there to avoid KCSAN splats. Also, make it
clear in the commit message that the snapshot is not atomic and only
meant to make sure that there's enough headroom in the skb for the
headers that ip6_tnl_encap() is going to push.

"
The helper is added to the shared IPv4 tunnel header, but grep shows
only two files reference it: include/net/ip_tunnels.h (the definition)
and net/ipv6/ip6_tunnel.c (the sole caller). Should it live in
include/net/ip6_tunnel.h instead, next to its only user?
"

Mention in the commit message that the intention is to later use this
helper in the IPv4 code.

> +
>  static inline int ip_encap_hlen(struct ip_tunnel_encap *e)
>  {
>  	const struct ip_tunnel_encap_ops *ops;
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index d5ff50a2ac01..6ca373d6205a 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1102,6 +1102,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
>  		 __u8 proto)
>  {
>  	struct ip6_tnl *t = netdev_priv(dev);
> +	struct ip_tunnel_encap ipencap;
>  	struct net *net = t->net;
>  	struct ipv6hdr *ipv6h;
>  	struct ipv6_tel_txoption opt;
> @@ -1109,10 +1110,11 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
>  	struct net_device *tdev;
>  	int err_count, mtu;
>  	unsigned int eth_hlen = t->dev->type == ARPHRD_ETHER ? ETH_HLEN : 0;
> -	unsigned int psh_hlen = sizeof(struct ipv6hdr) + t->encap_hlen;
> -	unsigned int max_headroom = psh_hlen;
> +	unsigned int max_headroom;
>  	__be16 payload_protocol;
>  	bool use_cache = false;
> +	unsigned int psh_hlen;
> +	int encap_hlen;
>  	u8 hop_limit;
>  	int err = -1;
>  
> @@ -1202,6 +1204,14 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
>  				     t->parms.name);
>  		goto tx_err_dst_release;
>  	}
> +
> +	ip_tunnel_encap_snapshot(&ipencap, &t->encap);
> +	encap_hlen = ip6_encap_hlen(&ipencap);
> +	if (unlikely(encap_hlen < 0))
> +		goto tx_err_dst_release;
> +	psh_hlen = sizeof(struct ipv6hdr) + encap_hlen;
> +	max_headroom = psh_hlen;
> +
>  	mtu = dst6_mtu(dst) - eth_hlen - psh_hlen - t->tun_hlen;
>  	if (encap_limit >= 0) {
>  		max_headroom += 8;
> @@ -1240,7 +1250,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
>  		goto tx_err_dst_release;
>  
>  	if (t->parms.collect_md) {
> -		if (t->encap.type != TUNNEL_ENCAP_NONE)
> +		if (ipencap.type != TUNNEL_ENCAP_NONE)
>  			goto tx_err_dst_release;
>  	} else {
>  		if (use_cache && ndst)
> @@ -1264,7 +1274,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
>  			+ dst->header_len + t->hlen;
>  	ip_tunnel_adj_headroom(dev, max_headroom);

"
t->hlen is still read live here, and it is written with a plain store in
ip6_tnl_encap_setup() as t->hlen = t->encap_hlen + t->tun_hlen.  So
dev->needed_headroom is adjusted from a possibly half-updated value
while the encap length that sized skb_cow_head() came from the snapshot.
Should this read be covered by the same snapshot protocol the patch
introduces?
"

No. The snapshot is meant to make sure that we have enough headroom for
the headers that we are about to push and the patch achieves it.