Re: [PATCH] net: fix wrong network header length

From: Paolo Abeni <pabeni@redhat.com>
To: "lina.wang" <lina.wang@mediatek.com>,
	"Maciej Żenczykowski" <maze@google.com>,
	"Steffen Klassert" <steffen.klassert@secunet.com>
Cc: "David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Linux NetDev <netdev@vger.kernel.org>,
	Kernel hackers <linux-kernel@vger.kernel.org>,
	bpf <bpf@vger.kernel.org>, Martin KaFai Lau <kafai@fb.com>,
	Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@kernel.org>, Willem Bruijn <willemb@google.com>,
	Eric Dumazet <edumazet@google.com>,
	zhuoliang@mediatek.com, chao.song@mediatek.com
Subject: Re: [PATCH] net: fix wrong network header length
Date: Thu, 10 Feb 2022 17:02:53 +0100	[thread overview]
Message-ID: <d5dd3f10c144f7150ec508fa8e6d7a78ceabfc10.camel@redhat.com> (raw)
In-Reply-To: <5ca86c46109794a627e6e2a62b140963217984a0.camel@mediatek.com>

On Wed, 2022-02-09 at 18:25 +0800, lina.wang wrote:
> We use NETIF_F_GRO_FRAGLIST not for forwarding scenary, just for
> software udp gro. 
> 
I'm wondering why don't you simply enable UDP_GRO on the relevant
socket? 

> Whatever NETIF_F_GRO_FRAGLIST or NETIF_F_GRO_FWD,
> skb_segment_list should not have bugs.

The bug is arguably in bpf_skb_proto_6_to_4(), even if fixing it in
skb_segment_list() is possibly easier.

> We modify skb_segment_list, not in epbf. One point is traversing the
> segments costly, another is what @Maciej said, *other* helper may have
> the same problem. In skb_segment_list, it calls
> skb_headers_offset_update to update different headroom, which implys
> header maybe different.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 75dfbde8d2e6..f15bbb7449ce 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3682,6 +3682,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
>  	struct sk_buff *tail = NULL;
>  	struct sk_buff *nskb, *tmp;
>  	int err;
> +	unsigned int len_diff = 0;

Mintor nit: please respect the reverse x-mas tree order.

>  
>  	skb_push(skb, -skb_network_offset(skb) + offset);

> @@ -3721,9 +3722,11 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
>  		skb_push(nskb, -skb_network_offset(nskb) + offset);
>  
>  		skb_release_head_state(nskb);
> +		len_diff = skb_network_header_len(nskb) - skb_network_header_len(skb);
>  		 __copy_skb_header(nskb, skb);
>  
>  		skb_headers_offset_update(nskb, skb_headroom(nskb) - skb_headroom(skb));
> +		nskb->transport_header += len_diff;

This does not look correct ?!? the network hdr position for nskb will
still be uncorrect?!? and even the mac hdr likely?!? possibly you need
to change the offset in skb_headers_offset_update().

Paolo