RE: [RESEND PATCH net v4] udp: ipv4: manipulate network header of NATed UDP GRO fraglist

[Dongseok Yi]

On 1/31/21 12:55 AM, Alexander Lobakin wrote:
> From: Dongseok Yi <dseok.yi@xxxxxxxxxxx>
> Date: Sat, 30 Jan 2021 08:13:27 +0900
> 
> > +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);
> > +
> > +	if ((udp_hdr(seg)->dest == udp_hdr(seg->next)->dest) &&
> > +	    (udp_hdr(seg)->source == udp_hdr(seg->next)->source) &&
> > +	    (ip_hdr(seg)->daddr == ip_hdr(seg->next)->daddr) &&
> > +	    (ip_hdr(seg)->saddr == ip_hdr(seg->next)->saddr))
> > +		return segs;
> > +
> > +	while ((seg = seg->next)) {
> > +		uh2 = udp_hdr(seg);
> > +		iph2 = ip_hdr(seg);
> > +
> > +		__udpv4_gso_segment_csum(seg,
> > +					 &iph2->saddr, &iph->saddr,
> > +					 &uh2->source, &uh->source);
> > +		__udpv4_gso_segment_csum(seg,
> > +					 &iph2->daddr, &iph->daddr,
> > +					 &uh2->dest, &uh->dest);
> > +	}
> > +
> > +	return segs;
> > +}
> > +
> >  static struct sk_buff *__udp_gso_segment_list(struct sk_buff *skb,
> > -					      netdev_features_t features)
> > +					      netdev_features_t features,
> > +					      bool is_ipv6)
> >  {
> >  	unsigned int mss = skb_shinfo(skb)->gso_size;
> >
> > @@ -198,11 +257,11 @@ static struct sk_buff *__udp_gso_segment_list(struct sk_buff *skb,
> >
> >  	udp_hdr(skb)->len = htons(sizeof(struct udphdr) + mss);
> >
> > -	return skb;
> > +	return is_ipv6 ? skb : __udpv4_gso_segment_list_csum(skb);
> 
> I don't think it's okay to fix checksums only for IPv4.
> IPv6 checksum mangling doesn't depend on any code from net/ipv6. Just
> use inet_proto_csum_replace16() for v6 addresses (see nf_nat_proto.c
> for reference). You can guard the path for IPv6 with
> IS_ENABLED(CONFIG_IPV6) to optimize IPv4-only systems a bit.

As you can see in __udpv4_gso_segment_list_csum, we compare
ports and addrs. We should use *struct ipv6hdr* to compare the values
for IPv6 but I am not sure the struct could be under net/ipv4.

The initial idea was to support both IPv4 and IPv6. Thanks, that's a
good point. But the supporting IPv6 would be a new feature. I want to
fix IPv4 first, so the title is restricted to ipv4.

> 
> >  }
> >
> >  struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
> > -				  netdev_features_t features)
> > +				  netdev_features_t features, bool is_ipv6)
> >  {
> >  	struct sock *sk = gso_skb->sk;
> >  	unsigned int sum_truesize = 0;
> > @@ -214,7 +273,7 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
> >  	__be16 newlen;
> >
> >  	if (skb_shinfo(gso_skb)->gso_type & SKB_GSO_FRAGLIST)
> > -		return __udp_gso_segment_list(gso_skb, features);
> > +		return __udp_gso_segment_list(gso_skb, features, is_ipv6);
> >
> >  	mss = skb_shinfo(gso_skb)->gso_size;
> >  	if (gso_skb->len <= sizeof(*uh) + mss)
> > @@ -328,7 +387,7 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
> >  		goto out;
> >
> >  	if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4)
> > -		return __udp_gso_segment(skb, features);
> > +		return __udp_gso_segment(skb, features, false);
> >
> >  	mss = skb_shinfo(skb)->gso_size;
> >  	if (unlikely(skb->len <= mss))
> > diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
> > index c7bd7b1..faa823c 100644
> > --- a/net/ipv6/udp_offload.c
> > +++ b/net/ipv6/udp_offload.c
> > @@ -42,7 +42,7 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
> >  			goto out;
> >
> >  		if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4)
> > -			return __udp_gso_segment(skb, features);
> > +			return __udp_gso_segment(skb, features, true);
> >
> >  		mss = skb_shinfo(skb)->gso_size;
> >  		if (unlikely(skb->len <= mss))
> > --
> > 2.7.4
> 
> Thanks,
> Al