Re: [PATCH net v2] fou: reserve headroom before pushing the encap header

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
> diff --git a/net/ipv4/fou_core.c b/net/ipv4/fou_core.c
> --- a/net/ipv4/fou_core.c
> +++ b/net/ipv4/fou_core.c
> @@ -1071,16 +1074,20 @@ static int fou_build_header(struct sk_buff *skb, struct ip_tunnel_encap *e,
>  static int gue_build_header(struct sk_buff *skb, struct ip_tunnel_encap *e,
>  			    u8 *protocol, struct flowi4 *fl4)
>  {
> -	int type = e->flags & TUNNEL_ENCAP_FLAG_CSUM ? SKB_GSO_UDP_TUNNEL_CSUM :
> -						       SKB_GSO_UDP_TUNNEL;
> +	struct ip_tunnel_encap encap = READ_ONCE(*e);
> +	int type = encap.flags & TUNNEL_ENCAP_FLAG_CSUM ?
> +		   SKB_GSO_UDP_TUNNEL_CSUM : SKB_GSO_UDP_TUNNEL;
>  	__be16 sport;
>  	int err;
>  
> -	err = __gue_build_header(skb, e, protocol, &sport, type);
> +	if (skb_cow_head(skb, gue_encap_hlen(&encap)))
> +		return -ENOMEM;
> +
> +	err = __gue_build_header(skb, &encap, protocol, &sport, type);
>  	if (err)
>  		return err;
>  
> -	fou_build_udp(skb, e, fl4, protocol, sport);
> +	fou_build_udp(skb, &encap, fl4, protocol, sport);
>  
>  	return 0;
>  }

[Severity: High]
[Finding: sashiko-gemini-db46d5c5]
[Sources: sashiko-gemini]
struct ip_tunnel_encap is four 16-bit fields. It is 8 bytes in size
but only 2-byte aligned. Inside struct ip_tunnel the encap member
follows parms and three ints (mlink, encap_hlen, hlen). That places
&tunnel->encap on a 4-byte boundary, not an 8-byte one, on 64-bit.

READ_ONCE(*e) on an 8-byte object passes
compiletime_assert_rwonce_type() and is done as a single 64-bit
access. On arm64 with CONFIG_LTO, the 8-byte case of __READ_ONCE()
emits ldar/ldapr, which requires natural alignment. Every GUE
transmit that reaches gue_build_header() with &tunnel->encap, from
ip_tunnel_xmit() or ipip6_tunnel_xmit(), can then take an alignment
fault on the hot path. CONFIG_UBSAN_ALIGNMENT will also flag the
misaligned 64-bit access elsewhere.

The whole-struct READ_ONCE() also does not give the atomicity it
suggests. ip_tunnel_encap_setup() fills tunnel->encap with separate
stores, so the copy can still be torn between fields.

What must stay consistent here is flags, between gue_encap_hlen() and
__gue_build_header(). Could the local copy be built field by field,
with READ_ONCE() on flags, sport and dport, instead of one READ_ONCE()
of the whole struct? Or is there a reason you need a single 8-byte
load?
-- 
This is an AI-generated review.