Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Dongseok Yi" <dseok.yi@samsung.com>
To:     "'Alexander Lobakin'" <alobakin@pm.me>
Cc:     "'David S. Miller'" <davem@davemloft.net>,
        "'Steffen Klassert'" <steffen.klassert@secunet.com>,
        <namkyu78.kim@samsung.com>, "'Jakub Kicinski'" <kuba@kernel.org>,
        "'Hideaki YOSHIFUJI'" <yoshfuji@linux-ipv6.org>,
        "'David Ahern'" <dsahern@kernel.org>,
        "'Alexei Starovoitov'" <ast@kernel.org>,
        "'Daniel Borkmann'" <daniel@iogearbox.net>,
        "'Andrii Nakryiko'" <andrii@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 de Bruijn'" <willemb@google.com>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <bpf@vger.kernel.org>
In-Reply-To: <20210130155458.8523-1-alobakin@pm.me>
Subject: RE: [RESEND PATCH net v4] udp: ipv4: manipulate network header of
 NATed UDP GRO fraglist
Date:   Tue, 2 Feb 2021 10:17:46 +0900
Message-ID: <021c01d6f901$36da2d80$a48e8880$@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Thread-Index: AQH3UkD7ra4ajhnLvSE7bwMwZ51/XAG/rNSqAaWf2U6p6DEcMA==
Content-Language: ko
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 102P
DLP-Filter: Pass
References: <CGME20210129232630epcas2p1071e141ef8059c4d5c0e4b28c181a171@epcas2p1.samsung.com>
        <1611962007-80092-1-git-send-email-dseok.yi@samsung.com>
        <20210130155458.8523-1-alobakin@pm.me>
Precedence: bulk

On 1/31/21 12:55 AM, Alexander Lobakin wrote:
> From: Dongseok Yi <dseok.yi@samsung.com>
> 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