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

On Fri, Jan 15, 2021 at 05:55:22PM +0900, Dongseok Yi wrote:

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

quoted

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.

Yes, I know. But he said that fraglist GRO + NAT works for him.
I want to find out why it works for him, but not for you.

quoted

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?

Ah yes, good point.

Then maybe you can move your approach into __udp_gso_segment_list()
so that we dont touch generic code.

quoted

I think about something like the (completely untested) patch below:

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f62cae3f75d8..63ae7f79fad7 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -3651,13 +3651,14 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
 				 unsigned int offset)
 {
 	struct sk_buff *list_skb = skb_shinfo(skb)->frag_list;
+	unsigned int doffset = skb->data - skb_mac_header(skb);
 	unsigned int tnl_hlen = skb_tnl_header_len(skb);
 	unsigned int delta_truesize = 0;
 	unsigned int delta_len = 0;
 	struct sk_buff *tail = NULL;
 	struct sk_buff *nskb;

-	skb_push(skb, -skb_network_offset(skb) + offset);
+	skb_push(skb, doffset);

 	skb_shinfo(skb)->frag_list = NULL;

@@ -3675,7 +3676,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
 		delta_len += nskb->len;
 		delta_truesize += nskb->truesize;

-		skb_push(nskb, -skb_network_offset(nskb) + offset);
+		skb_push(nskb, doffset);

 		skb_release_head_state(nskb);
 		 __copy_skb_header(nskb, skb);

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index ff39e94781bf..1181398378b8 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c

@@ -190,9 +190,22 @@ EXPORT_SYMBOL(skb_udp_tunnel_segment);
 static struct sk_buff *__udp_gso_segment_list(struct sk_buff *skb,
 					      netdev_features_t features)
 {
+	struct sk_buff *list_skb = skb_shinfo(skb)->frag_list;
 	unsigned int mss = skb_shinfo(skb)->gso_size;
+	unsigned int offset;

-	skb = skb_segment_list(skb, features, skb_mac_header_len(skb));
+	skb_headers_offset_update(list_skb, skb_headroom(list_skb) - skb_headroom(skb));
+
+	/* Check for header changes and copy the full header in that case. */
+	if ((udp_hdr(skb)->dest == udp_hdr(list_skb)->dest) &&
+	    (udp_hdr(skb)->source == udp_hdr(list_skb)->source) &&
+	    (ip_hdr(skb)->daddr == ip_hdr(list_skb)->daddr) &&
+	    (ip_hdr(skb)->saddr == ip_hdr(list_skb)->saddr))
+		offset = skb_mac_header_len(skb);
+	else
+		offset = skb->data - skb_mac_header(skb);
+
+	skb = skb_segment_list(skb, features, offset);
 	if (IS_ERR(skb))
 		return skb;

After that you can apply the CSUM magic in __udp_gso_segment_list().

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

With that I meant the checksum updating you did in your patch.