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

On 2021-01-21 21:28, Steffen Klassert wrote:

On Wed, Jan 20, 2021 at 03:55:42PM +0900, Dongseok Yi wrote:

quoted

On 2021-01-18 22:27, Steffen Klassert wrote:

quoted

On Fri, Jan 15, 2021 at 10:20:35PM +0900, Dongseok Yi wrote:

quoted

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 <redacted>
---
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?

If I understand standard GRO correctly, it calculates full checksum.
Should we adopt the same method to UDP GRO fraglist? I did not find
a simple method for the incremental checksum update.

quoted

quoted

+
+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);
+	}

quoted

quoted

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.

I think we can try this with an additional flag (seg_csum).

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 36b7e30..3f892df 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c

@@ -213,25 +213,36 @@ 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;
+       bool seg_csum = false;

        seg = segs;
        uh = udp_hdr(seg);
        iph = ip_hdr(seg);

Why not

       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;

Then you don't need to test inside the loop. Just update all
packets if there is a header mismatch.

The test inside the loop would be needed to distinguish SNAT
and DNAT. I will try your approach on v3. Thanks.

quoted

-       while ((seg = seg->next)) {
+       seg = seg->next;
+       do {
+               if (!seg)
+                       break;
+
                uh2 = udp_hdr(seg);
                iph2 = ip_hdr(seg);

-               if (uh->source != uh2->source || iph->saddr != iph2->saddr)
+               if (uh->source != uh2->source || iph->saddr != iph2->saddr) {
                        __udpv4_gso_segment_csum(seg,
                                                 &iph2->saddr, &iph->saddr,
                                                 &uh2->source, &uh->source);
+                       seg_csum = true;
+               }

-               if (uh->dest != uh2->dest || iph->daddr != iph2->daddr)
+               if (uh->dest != uh2->dest || iph->daddr != iph2->daddr) {
                        __udpv4_gso_segment_csum(seg,
                                                 &iph2->daddr, &iph->daddr,
                                                 &uh2->dest, &uh->dest);
-       }
+                       seg_csum = true;
+               }
+
+               seg = seg->next;
+       } while (seg_csum);

        return segs;
 }