Re: [PATCH net] net: fix segmentation after TCP/UDP fraglist GRO

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Felix Fietkau wrote:
> Since "net: gro: use cb instead of skb->network_header", the skb network
> header is no longer set in the GRO path.
> This breaks fraglist segmentation, which relies on ip_hdr()/tcp_hdr()

Only ip_hdr is in scope.

Reviewing TCP and UDP GSO, tcp_hdr/transport header is used also
outside segment list. Non segment list GSO also uses ip_hdr in case
pseudo checksum needs to be set.

The GSO code is called with skb->data at the relevant header, so L4
helpers are not strictly needed. The main issue is that data will be
at the L4 header, and some GSO code also needs to see the IP header
(e.g., for aforementioned pseudo checksum calculation).

> to check for address/port changes.

If in GSO, then the headers are probably more correctly set at the end
of GRO, in gro_complete.

The blamed commit was added to support tunneling. It's not obvious
that unconditionally setting network header again, instead of inner
network header, will break that.

Side-note: the number of times this feature has been broken indicates
we should aim for getting test coverage.

> Fix this regression by selectively setting the network header for merged
> segment skbs.

 
> Fixes: 186b1ea73ad8 ("net: gro: use cb instead of skb->network_header")
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  net/ipv4/tcp_offload.c | 1 +
>  net/ipv4/udp_offload.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> index d293087b426d..be5c2294610e 100644
> --- a/net/ipv4/tcp_offload.c
> +++ b/net/ipv4/tcp_offload.c
> @@ -359,6 +359,7 @@ struct sk_buff *tcp_gro_receive(struct list_head *head, struct sk_buff *skb,
>  		flush |= skb->ip_summed != p->ip_summed;
>  		flush |= skb->csum_level != p->csum_level;
>  		flush |= NAPI_GRO_CB(p)->count >= 64;
> +		skb_set_network_header(skb, skb_gro_receive_network_offset(skb));
>  
>  		if (flush || skb_gro_receive_list(p, skb))
>  			mss = 1;
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 85b5aa82d7d7..e0a6bfa95118 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -767,6 +767,7 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
>  					NAPI_GRO_CB(skb)->flush = 1;
>  					return NULL;
>  				}
> +				skb_set_network_header(skb, skb_gro_receive_network_offset(skb));
>  				ret = skb_gro_receive_list(p, skb);
>  			} else {
>  				skb_gro_postpull_rcsum(skb, uh,
> -- 
> 2.49.0
>