Re: [PATCH net v3] net: gro: Fix nesting of TCP GSO SKBs in skb_gro_receive_list()

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

zhaoping.shu@ wrote:
> From: HW He <hw.he@mediatek.com>
> 
> A device supports GRO_HW, and the device driver enables the
> NETIF_F_GRO_FRAGLIST feature. During a tethering test,
> skb_gro_receive_list() reaggregates the GSO packet. However,
> skb_segment_list() cannot segment this packet back into
> the original packets, which leads to IP fragmentation or packet drop.
> 
> Scenario (Tethering/Forwarding):
> 1.Driver submits a single TCP packet, P1. P1 is kept in the
> gro_list as the first packet.
> 
> 2. The driver submits a TCP GSO skb, P2. P2 has already aggregated
> multiple TCP packets by HW_GRO, and its non-linear data is stored in
> frags[].
> 
> 3. P1 and P2 match the GRO rules, and since there is no local socket,
> they are aggregated by skb_gro_receive_list(). The resulting skb,
> P3, has a frag_list entry that still contains frags[]:
> P3: [ Linear Data ] -> frag_list -> [ Linear Data ]
>                                     [ frag[1] ]
>                                     [ frag[2] ]
>                                     ...
> 4. Later, tcp4_gso_segment() or tcp6_gso_segment() calls
> skb_segment_list() to segment P3. However, skb_segment_list() only
> segments the entries in frag_list. It does not segment the frags[]
> inside P2, so P3 is not restored to the original packets, which leads
> to IP fragmentation or packet drop in the following path.
> 
> When NETIF_F_GRO_HW is enabled, do not set NAPI_GRO_CB(skb)->is_flist.
> Fall through to the regular skb_gro_receive() path instead of
> skb_gro_receive_list().
> 
> Fixes: 8d95dc474f85 ("net: add code for TCP fraglist GRO")
> Signed-off-by: HW He <hw.he@mediatek.com>
> Signed-off-by: Zhaoping Shu <zhaoping.shu@mediatek.com>
> ---
> [2]: https://patchwork.kernel.org/patch/14706032
> [1]: https://patchwork.kernel.org/patch/14702209
> ---
>  net/ipv4/tcp_offload.c   | 7 +++----
>  net/ipv6/tcpv6_offload.c | 3 ++-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> index 3b1fdcd3cb29..641c47fb1ea2 100644
> --- a/net/ipv4/tcp_offload.c
> +++ b/net/ipv4/tcp_offload.c
> @@ -395,9 +395,6 @@ static void tcp4_check_fraglist_gro(struct list_head *head, struct sk_buff *skb,
>  	struct net *net;
>  	int iif, sdif;
>  
> -	if (likely(!(skb->dev->features & NETIF_F_GRO_FRAGLIST)))
> -		return;
> -

Interesting that ipv4 and ipv6 diverge here. Nice to try to make them
more alike.

>  	p = tcp_gro_lookup(head, th);
>  	if (p) {
>  		NAPI_GRO_CB(skb)->is_flist = NAPI_GRO_CB(p)->is_flist;
> @@ -430,7 +427,9 @@ struct sk_buff *tcp4_gro_receive(struct list_head *head, struct sk_buff *skb)
>  	if (!th)
>  		goto flush;
>  
> -	tcp4_check_fraglist_gro(head, skb, th);
> +	if (unlikely((skb->dev->features & NETIF_F_GRO_FRAGLIST) &&
> +		     !(skb->dev->features & NETIF_F_GRO_HW)))

Would it be better to test skb_is_gso(skb) and only skip fraglist GRO
for HW-GRO skbs, rather than disabling it for all skbs?

If treating the features as mutually exclusive, another option would
be to replace these datapath checks with disabling one at configuration
time, in netdev_fix_features.