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

On 2021-01-15 17:12, Steffen Klassert wrote:

On Fri, Jan 15, 2021 at 02:58:24PM +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.

We still need to find out why it works for Alexander, but not for you.
Different usecases?

This patch is not for
https://lore.kernel.org/patchwork/patch/1364544/
Alexander might want to call udp_gro_receive_segment even when
!sk and ~NETIF_F_GRO_FRAGLIST.

We copy only the MAC header in skb_segment_list(), so I think
this is a valid bug when NAT changed the UDP header.

quoted

Update dport, daddr and checksums of each skb of the segment list
after __udp_gso_segment.

Fixes: 9fd1ff5d2ac7 (udp: Support UDP fraglist GRO/GSO.)
Signed-off-by: Dongseok Yi <redacted>
---
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. Should we consider
SNAT too?

If it is broken, then it is broken for both, so yes.

Okay.

quoted

 net/ipv4/udp_offload.c | 45 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index ff39e94..7e24928 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c

@@ -309,10 +309,12 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
 					 netdev_features_t features)
 {
 	struct sk_buff *segs = ERR_PTR(-EINVAL);
+	struct sk_buff *seg;
 	unsigned int mss;
 	__wsum csum;
-	struct udphdr *uh;
-	struct iphdr *iph;
+	struct udphdr *uh, *uh2;
+	struct iphdr *iph, *iph2;
+	bool is_fraglist = false;

 	if (skb->encapsulation &&
 	    (skb_shinfo(skb)->gso_type &

@@ -327,8 +329,43 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
 	if (!pskb_may_pull(skb, sizeof(struct udphdr)))
 		goto out;

-	if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4)
-		return __udp_gso_segment(skb, features);
+	if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) {
+		if (skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST)
+			is_fraglist = true;
+
+		segs = __udp_gso_segment(skb, features);
+		if (IS_ERR_OR_NULL(segs) || !is_fraglist)
+			return segs;
+
+		seg = segs;
+		uh = udp_hdr(seg);
+		iph = ip_hdr(seg);
+
+		while ((seg = seg->next)) {
+			uh2 = udp_hdr(seg);
+			iph2 = ip_hdr(seg);
+
+			if (uh->dest == uh2->dest && iph->daddr == iph2->daddr)
+				continue;
+
+			if (uh2->check) {
+				inet_proto_csum_replace4(&uh2->check, seg,
+							 iph2->daddr,
+							 iph->daddr, true);
+				inet_proto_csum_replace2(&uh2->check, seg,
+							 uh2->dest, uh->dest,
+							 false);
+				if (!uh2->check)
+					uh2->check = CSUM_MANGLED_0;
+			}
+			uh2->dest = uh->dest;
+
+			csum_replace4(&iph2->check, iph2->daddr, iph->daddr);
+			iph2->daddr = iph->daddr;
+		}
+
+		return segs;
+	}

I would not like to add this to a generic codepath. I think we can
relatively easy copy the full headers in skb_segment_list().

I tried to copy the full headers with the similar approach, but it
copies length too. Can we keep the length of each skb of the fraglist?

Sorry, I don't know CSUM magic well. Is it used for checksum
incremental update too?