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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 5760 bytes --]

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

Hi Ren and Zixuan,

I agree this is a real issue, but I guess this patch is fixing just a
small part of more extended problem. In particular, there are multiple
parameters that are updated in ip6_tnl_update()/ip6_tnl_change() that are
accessed concurrently in ip6_tnl_xmit() or in ip6_tnl_fill_forward_path().
I guess we should try to find a general fix for the extended issue.
What do you think? We have probably the same issue in the IPv4 counterpart.

Regards,
Lorenzo

> ---
>  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);
> +}
> +
>  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);
>  
> -	err = ip6_tnl_encap(skb, t, &proto, fl6);
> +	err = ip6_tnl_encap(skb, &ipencap, &proto, fl6);
>  	if (err)
>  		return err;
>  
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]