| Message ID | 1611235479-39399-1-git-send-email-dseok.yi@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net,v3] udp: ipv4: manipulate network header of NATed UDP GRO fraglist | expand |
On Thu, Jan 21, 2021 at 10:24:39PM +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 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> > --- > 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. > > include/net/udp.h | 2 +- > net/ipv4/udp_offload.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++---- > net/ipv6/udp_offload.c | 2 +- > 3 files changed, 69 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..43660cf 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; This check is redundant as you check this already in __udpv4_gso_segment_list_csum. Looks ok otherwise. > + > + 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,14 @@ static struct sk_buff *__udp_gso_segment_list(struct sk_buff *skb, > > udp_hdr(skb)->len = htons(sizeof(struct udphdr) + mss); > > - return skb; > + if (is_ipv6) > + return skb; > + else > + return __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 +276,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 +390,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
On 1/25/21 9:45 PM, Steffen Klassert wrote: > On Thu, Jan 21, 2021 at 10:24:39PM +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 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> > > --- > > 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. > > > > include/net/udp.h | 2 +- > > net/ipv4/udp_offload.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++---- > > net/ipv6/udp_offload.c | 2 +- > > 3 files changed, 69 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..43660cf 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; > > This check is redundant as you check this already in > __udpv4_gso_segment_list_csum. When comes in __udpv4_gso_segment_csum, the condition would be SNAT or DNAT. I think we don't need to do the function if the condition is not met. I want to skip the function for SNAT checksum when DNAT only case. Is it better to remove the check? > > Looks ok otherwise. > > > + > > + 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,14 @@ static struct sk_buff *__udp_gso_segment_list(struct sk_buff *skb, > > > > udp_hdr(skb)->len = htons(sizeof(struct udphdr) + mss); > > > > - return skb; > > + if (is_ipv6) > > + return skb; > > + else > > + return __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 +276,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 +390,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
On Tue, Jan 26, 2021 at 09:31:29AM +0900, Dongseok Yi wrote: > On 1/25/21 9:45 PM, Steffen Klassert wrote: > > On Thu, Jan 21, 2021 at 10:24:39PM +0900, Dongseok Yi wrote: > > > > > > +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; > > > > This check is redundant as you check this already in > > __udpv4_gso_segment_list_csum. > > When comes in __udpv4_gso_segment_csum, the condition would be > SNAT or DNAT. I think we don't need to do the function if the > condition is not met. I want to skip the function for SNAT checksum > when DNAT only case. Is it better to remove the check? Ok, so it can be seen as an optimization. It is ok as it is. Acked-by: Steffen Klassert <steffen.klassert@secunet.com> Thanks!
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..43660cf 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,14 @@ static struct sk_buff *__udp_gso_segment_list(struct sk_buff *skb, udp_hdr(skb)->len = htons(sizeof(struct udphdr) + mss); - return skb; + if (is_ipv6) + return skb; + else + return __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 +276,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 +390,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))
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> --- 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. include/net/udp.h | 2 +- net/ipv4/udp_offload.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++---- net/ipv6/udp_offload.c | 2 +- 3 files changed, 69 insertions(+), 7 deletions(-)