| Message ID | 1611962007-80092-1-git-send-email-dseok.yi@samsung.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c3df39ac9b0e3747bf8233ea9ce4ed5ceb3199d3 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RESEND,net,v4] udp: ipv4: manipulate network header of NATed UDP GRO fraglist | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 16 of 16 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 306 this patch: 306 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 113 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 414 this patch: 414 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
From: Dongseok Yi <dseok.yi@samsung.com> Date: Sat, 30 Jan 2021 08:13:27 +0900 > 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 port, addr and check 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.yi@samsung.com> > Acked-by: Steffen Klassert <steffen.klassert@secunet.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, moved the procedure from > udp4_ufo_fragment to __udp_gso_segment_list and support SNAT. > > v3: > Per Steffen Klassert request, applied fast return by comparing seg > and seg->next at the beginning of __udpv4_gso_segment_list_csum. > > Fixed uh->dest = *newport and iph->daddr = *newip to > *oldport = *newport and *oldip = *newip. > > v4: > Clear "Changes Requested" mark in > https://patchwork.kernel.org/project/netdevbpf > > Simplified the return statement in __udp_gso_segment_list. > > include/net/udp.h | 2 +- > net/ipv4/udp_offload.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++---- > net/ipv6/udp_offload.c | 2 +- > 3 files changed, 66 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..cfc8726 100644 > --- a/net/ipv4/udp_offload.c > +++ b/net/ipv4/udp_offload.c > @@ -187,8 +187,67 @@ 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; > + struct iphdr *iph; > + > + if (*oldip == *newip && *oldport == *newport) > + return; > + > + uh = udp_hdr(seg); > + 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; > + } > + *oldport = *newport; > + > + csum_replace4(&iph->check, *oldip, *newip); > + *oldip = *newip; > +} > + > +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. > } > > 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
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
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Sat, 30 Jan 2021 08:13:27 +0900 you 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. > > [...] Here is the summary with links: - [RESEND,net,v4] udp: ipv4: manipulate network header of NATed UDP GRO fraglist https://git.kernel.org/netdev/net/c/c3df39ac9b0e You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
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..cfc8726 100644 --- a/net/ipv4/udp_offload.c +++ b/net/ipv4/udp_offload.c @@ -187,8 +187,67 @@ 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; + struct iphdr *iph; + + if (*oldip == *newip && *oldport == *newport) + return; + + uh = udp_hdr(seg); + 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; + } + *oldport = *newport; + + csum_replace4(&iph->check, *oldip, *newip); + *oldip = *newip; +} + +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); } 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))