Re: [PATCH net v2] udp: ipv4: manipulate network header of NATed UDP GRO fraglist

[Steffen Klassert]

On Fri, Jan 15, 2021 at 10:20:35PM +0900, Dongseok Yi wrote:
> UDP/IP header of UDP GROed frag_skbs are not updated even after NAT
> forwarding. Only the header of head_skb from ip_finish_output_gso ->
> skb_gso_segment is updated but following frag_skbs are not updated.
> 
> A call path skb_mac_gso_segment -> inet_gso_segment ->
> udp4_ufo_fragment -> __udp_gso_segment -> __udp_gso_segment_list
> does not try to update UDP/IP header of the segment list but copy
> only the MAC header.
> 
> Update dport, daddr and checksums of each skb of the segment list
> in __udp_gso_segment_list. It covers both SNAT and DNAT.
> 
> Fixes: 9fd1ff5d2ac7 (udp: Support UDP fraglist GRO/GSO.)
> Signed-off-by: Dongseok Yi <dseok.yi@xxxxxxxxxxx>
> ---
> v1:
> Steffen Klassert said, there could be 2 options.
> https://lore.kernel.org/patchwork/patch/1362257/
> I was trying to write a quick fix, but it was not easy to forward
> segmented list. Currently, assuming DNAT only.
> 
> v2:
> Per Steffen Klassert request, move the procedure from
> udp4_ufo_fragment to __udp_gso_segment_list and support SNAT.
> 
> To Alexander Lobakin, I've checked your email late. Just use this
> patch as a reference. It support SNAT too, but does not support IPv6
> yet. I cannot make IPv6 header changes in __udp_gso_segment_list due
> to the file is in IPv4 directory.
> 
>  include/net/udp.h      |  2 +-
>  net/ipv4/udp_offload.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++----
>  net/ipv6/udp_offload.c |  2 +-
>  3 files changed, 59 insertions(+), 7 deletions(-)
> 
> diff --git a/include/net/udp.h b/include/net/udp.h
> index 877832b..01351ba 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -178,7 +178,7 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
>  int udp_gro_complete(struct sk_buff *skb, int nhoff, udp_lookup_t lookup);
>  
>  struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
> -				  netdev_features_t features);
> +				  netdev_features_t features, bool is_ipv6);
>  
>  static inline struct udphdr *udp_gro_udphdr(struct sk_buff *skb)
>  {
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index ff39e94..c532d3b 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -187,8 +187,57 @@ struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
>  }
>  EXPORT_SYMBOL(skb_udp_tunnel_segment);
>  
> +static void __udpv4_gso_segment_csum(struct sk_buff *seg,
> +				     __be32 *oldip, __be32 *newip,
> +				     __be16 *oldport, __be16 *newport)
> +{
> +	struct udphdr *uh = udp_hdr(seg);
> +	struct iphdr *iph = ip_hdr(seg);
> +
> +	if (uh->check) {
> +		inet_proto_csum_replace4(&uh->check, seg, *oldip, *newip,
> +					 true);
> +		inet_proto_csum_replace2(&uh->check, seg, *oldport, *newport,
> +					 false);
> +		if (!uh->check)
> +			uh->check = CSUM_MANGLED_0;
> +	}
> +	uh->dest = *newport;
> +
> +	csum_replace4(&iph->check, *oldip, *newip);
> +	iph->daddr = *newip;
> +}

Can't we just do the checksum recalculation for this case as it is done
with standard GRO?

> +
> +static struct sk_buff *__udpv4_gso_segment_list_csum(struct sk_buff *segs)
> +{
> +	struct sk_buff *seg;
> +	struct udphdr *uh, *uh2;
> +	struct iphdr *iph, *iph2;
> +
> +	seg = segs;
> +	uh = udp_hdr(seg);
> +	iph = ip_hdr(seg);
> +
> +	while ((seg = seg->next)) {
> +		uh2 = udp_hdr(seg);
> +		iph2 = ip_hdr(seg);
> +
> +		if (uh->source != uh2->source || iph->saddr != iph2->saddr)
> +			__udpv4_gso_segment_csum(seg,
> +						 &iph2->saddr, &iph->saddr,
> +						 &uh2->source, &uh->source);
> +
> +		if (uh->dest != uh2->dest || iph->daddr != iph2->daddr)
> +			__udpv4_gso_segment_csum(seg,
> +						 &iph2->daddr, &iph->daddr,
> +						 &uh2->dest, &uh->dest);
> +	}

You don't need to check the addresses and ports of all packets in the
segment list. If the addresses and ports are equal for the first and
second packet in the list, then this also holds for the rest of the
packets.