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 <[email protected]>
Acked-by: Steffen Klassert <[email protected]>
---
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);
}
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
From: Dongseok Yi <[email protected]>
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 <[email protected]>
> Acked-by: Steffen Klassert <[email protected]>
> ---
> 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 <[email protected]>
> 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