On 2021-01-20 15:56, Dongseok Yi wrote: > On 2021-01-18 22:27, Steffen Klassert wrote: > > On Fri, Jan 15, 2021 at 10:20:35PM +0900, Dongseok Yi wrote: > > > 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 <dseok...@samsung.com> > > > --- > > > 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. > > > > > > + > > > +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); > > > + } > > > > 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); > > - 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; > }
Hi, Steffen Do you have any further comments for this? I think the bug fix has higher priority than optimization. Can we defer the optimization patch?
