Re: [PATCH net v5 1/1] net: gso: limit recursive IP-in-IP segmentation

[Willem de Bruijn]

Zihan Xi wrote:
> IP-in-IP GSO can re-enter inet_gso_segment() or ipv6_gso_segment()
> for each nested IP header. encap_level tracks header bytes, not callback
> depth, so a deep chain can exhaust the kernel stack. Making
> inet_gso_segment() stackable introduced unbounded IPv4 nesting; IPIP
> GSO/TSO later made the path reachable. The IPv6 stackable path was
> introduced separately and shares the guard; Fixes identifies the IPv4
> root cause.
> 
> Count IPv4 and IPv6 GSO handler entries in skb_gso_cb, initialized once
> per top-level GSO operation and preserved across GRE/UDP context changes.
> Set the cap to five, matching IP_TUNNEL_RECURSION_LIMIT. Allow five
> entries and reject the sixth with -EINVAL before it can dispatch another
> GSO callback.
> 
> On the patched x86_64 kernel, tracefs observed six
> inet_gso_segment() entries for the 199-header reproducer; the sixth
> returned -EINVAL without a stack-guard fault. This validates the tested
> configuration, not an architecture-independent stack-safety margin.
> 
> Fixes: 3347c9602955 ("ipv4: gso: make inet_gso_segment() stackable")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Vega <vega@xxxxxxxxxx>
> Assisted-by: LLM
> Co-developed-by: Luxing Yin <root@xxxxxxxxxx>
> Signed-off-by: Luxing Yin <root@xxxxxxxxxx>
> Signed-off-by: Zihan Xi <zihanx@xxxxxxxxxx>
> ---
> changes in v5:
>   - Replace the header-offset budget with a shared IP GSO handler-entry
>     counter. Entries one through five pass; entry six is rejected.
>   - Initialize the count for each top-level GSO operation and preserve it
>     across IPv4/IPv6 dispatch and GRE/UDP context changes.
>   - Use the existing IP_TUNNEL_RECURSION_LIMIT value of five as a
>     practical bound. Tracefs observed the sixth entry return -EINVAL on
>     the final x86_64 revision; rerun both IPv4 PoCs on that revision.
>   - Avoid rejecting a single-level tunnel based only on header length.
>   - v4 Link: https://lore.kernel.org/all/cover.1790041241.git.zihanx@xxxxxxxxxx/
>  include/net/gso.h      | 9 +++++++++
>  net/core/gso.c         | 1 +
>  net/ipv4/af_inet.c     | 2 ++
>  net/ipv6/ip6_offload.c | 2 ++
>  4 files changed, 14 insertions(+)
> 
> diff --git a/include/net/gso.h b/include/net/gso.h
> index 29975440c..18944ff06 100644
> --- a/include/net/gso.h
> +++ b/include/net/gso.h
> @@ -19,10 +19,19 @@ struct skb_gso_cb {
>  	int	encap_level;
>  	__wsum	csum;
>  	__u16	csum_start;
> +	/* Number of IPv4/IPv6 GSO handler entries for this packet. */
> +	u8	recursion_counter;
>  };
>  #define SKB_GSO_CB_OFFSET	32
>  #define SKB_GSO_CB(skb) ((struct skb_gso_cb *)((skb)->cb + SKB_GSO_CB_OFFSET))
>  
> +#define GSO_RECURSION_LIMIT	5

The commit-msg explains this as derived from IP_TUNNEL_RECURSION_LIMIT.
Which makes sense as a choice. Use that directly, rather than
duplicating it?

Or else at least add a comment. Right now it looks like an unexplained
magic variable.

If respinning, no need to send the long cover letter again, imho.
Reported-by: is expected to be followed by a Closes: tag. That can
point to the v5 cover letter on lore.kernel.org.