Re: [PATCH net v3] net: use skb_header_pointer() only for DODGY TCPv4 GSO skbs

[Paolo Abeni <pabeni@redhat.com>]

On 3/12/26 11:43 AM, Guoyu Su wrote:
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 14a83f2035b9..f3340d7dd87c 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3805,10 +3805,21 @@ static netdev_features_t gso_features_check(const struct sk_buff *skb,
>  	 * segmentation-offloads.rst).
>  	 */
>  	if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4) {
> -		struct iphdr *iph = skb->encapsulation ?
> -				    inner_ip_hdr(skb) : ip_hdr(skb);
> +		int nhoff = skb->encapsulation ?
> +			    skb_inner_network_offset(skb) :
> +			    skb_network_offset(skb);
> +		const struct iphdr *iph;
>  
> -		if (!(iph->frag_off & htons(IP_DF)))
> +		if (unlikely(skb_shinfo(skb)->gso_type & SKB_GSO_DODGY)) {
> +			struct iphdr _iph;
> +
> +			iph = nhoff < 0 ? NULL :
> +			      skb_header_pointer(skb, nhoff, sizeof(_iph), &_iph);
> +		} else {
> +			iph = skb->encapsulation ? inner_ip_hdr(skb) : ip_hdr(skb);
> +		}
> +
> +		if (!iph || !(iph->frag_off & htons(IP_DF)))
>  			features &= ~dev->mangleid_features;

AI review noted the following:

Does this code use `_iph` after it goes out of scope?

The stack-local variable `_iph` is declared inside the
`if (unlikely(...SKB_GSO_DODGY))` block, but `iph` (which may point to
`&_iph` when skb_header_pointer() copies the header) is dereferenced via
`iph->frag_off` after that block's closing brace, where `_iph` is out of
scope.

When skb_header_pointer() needs to copy the IP header from paged
fragments (the exact scenario this patch targets for DODGY packets from
AF_PACKET or HSR), it returns `&_iph`. The subsequent access to
`iph->frag_off` then reads from a dead stack variable.

All other skb_header_pointer() usage patterns in the kernel declare the
buffer at the same scope as the pointer usage. For example, in
qdisc_pkt_len_segs_init():

  static int qdisc_pkt_len_segs_init(struct sk_buff *skb)
  {
  	const struct skb_shared_info *shinfo = skb_shinfo(skb);
  	unsigned int hdr_len, mss = shinfo->gso_size;
  	u16 gso_segs = shinfo->gso_segs;
  	const struct iphdr *iph;
  	struct iphdr _iph;  // <-- buffer declared in same scope as usage
  	int pkt_len;
  	...
  	iph = skb_header_pointer(skb, skb_network_offset(skb),
  				 sizeof(_iph), &_iph);
  	...
  	if (iph->protocol != IPPROTO_TCP && iph->protocol != IPPROTO_UDP)
  		return -EINVAL;

Should `struct iphdr _iph;` be moved to the outer block, next to the
`const struct iphdr *iph;` declaration?