RE: [PATCH net v2] udp: ipv4: manipulate network header of NATed UDP GRO fraglist
On 2021-01-21 21:28, Steffen Klassert wrote:
> On Wed, Jan 20, 2021 at 03:55:42PM +0900, 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
> > > > ---
> > > > 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(>check, seg, *oldip,
> > > > *newip,
> > > > +true);
> > > > + inet_proto_csum_replace2(>check, seg, *oldport,
> > > > *newport,
> > > > +false);
> > > > + if (!uh->check)
> > > > + uh->check = CSUM_MANGLED_0;
> > > > + }
> > > > + uh->dest = *newport;
> > > > +
> > > > + csum_replace4(>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,
> > > > +>saddr,
> > > > >saddr,
> > > > +
Re: [PATCH net v2] udp: ipv4: manipulate network header of NATed UDP GRO fraglist
On Wed, Jan 20, 2021 at 03:55:42PM +0900, 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
> > > ---
> > > 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(>check, seg, *oldip, *newip,
> > > + true);
> > > + inet_proto_csum_replace2(>check, seg, *oldport, *newport,
> > > + false);
> > > + if (!uh->check)
> > > + uh->check = CSUM_MANGLED_0;
> > > + }
> > > + uh->dest = *newport;
> > > +
> > > + csum_replace4(>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,
> > > + >saddr, >saddr,
> > > + >source, >source);
> > > +
> > > + if (uh->dest != uh2->dest || iph->daddr != iph2->daddr)
> > > + __udpv4_gso_segment_csum(seg,
> > > + >daddr, >daddr,
> > > + >dest, >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
RE: [PATCH net v2] udp: ipv4: manipulate network header of NATed UDP GRO fraglist
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
> > > ---
> > > 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(>check, seg, *oldip, *newip,
> > > + true);
> > > + inet_proto_csum_replace2(>check, seg, *oldport, *newport,
> > > + false);
> > > + if (!uh->check)
> > > + uh->check = CSUM_MANGLED_0;
> > > + }
> > > + uh->dest = *newport;
> > > +
> > > + csum_replace4(>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,
> > > + >saddr, >saddr,
> > > + >source, >source);
> > > +
> > > + if (uh->dest != uh2->dest || iph->daddr != iph2->daddr)
> > > + __udpv4_gso_segment_csum(seg,
> > > + >daddr, >daddr,
> > > + >dest, >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
RE: [PATCH net v2] udp: ipv4: manipulate network header of NATed UDP GRO fraglist
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
> > ---
> > 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(>check, seg, *oldip, *newip,
> > +true);
> > + inet_proto_csum_replace2(>check, seg, *oldport, *newport,
> > +false);
> > + if (!uh->check)
> > + uh->check = CSUM_MANGLED_0;
> > + }
> > + uh->dest = *newport;
> > +
> > + csum_replace4(>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,
> > +>saddr, >saddr,
> > +>source, >source);
> > +
> > + if (uh->dest != uh2->dest || iph->daddr != iph2->daddr)
> > + __udpv4_gso_segment_csum(seg,
> > +>daddr, >daddr,
> > +>dest, >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
+++
Re: [PATCH net v2] udp: ipv4: manipulate network header of NATed UDP GRO fraglist
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
> ---
> 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(>check, seg, *oldip, *newip,
> + true);
> + inet_proto_csum_replace2(>check, seg, *oldport, *newport,
> + false);
> + if (!uh->check)
> + uh->check = CSUM_MANGLED_0;
> + }
> + uh->dest = *newport;
> +
> + csum_replace4(>check, *oldip, *newip);
> + iph->daddr = *newip;
> +}
Can't we just do the checksum recalculation for this case as it is done
with standard GRO?
> +
> +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,
> + >saddr, >saddr,
> + >source, >source);
> +
> + if (uh->dest != uh2->dest || iph->daddr != iph2->daddr)
> + __udpv4_gso_segment_csum(seg,
> + >daddr, >daddr,
> + >dest, >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.
Re: [PATCH net v2] udp: ipv4: manipulate network header of NATed UDP GRO fraglist
On Mon, Jan 18, 2021 at 12:17:34PM +, Alexander Lobakin wrote: > > From: Steffen Klassert > > Date: Mon, 18 Jan 2021 07:37:59 +0100 > > On Fri, Jan 15, 2021 at 05:12:33PM +, Alexander Lobakin wrote: > >> > >> I used another approach, tried to make fraglist GRO closer to plain > >> in terms of checksummming, as it is confusing to me why GSO packet > >> should have CHECKSUM_UNNECESSARY. > > > > This is intentional. With fraglist GRO, we don't mangle packets > > in the standard (non NAT) case. So the checksum is still correct > > after segmentation. That is one reason why it has good forwarding > > performance when software segmentation is needed. Checksuming > > touches the whole packet and has a lot of overhead, so it is > > heplfull to avoid it whenever possible. > > > > We should find a way to do the checksum only when we really > > need it. I.e. only if the headers of the head skb changed. > > I suggest to do memcmp() between skb_network_header(skb) and > skb_network_header(skb->frag_list) with the len of > skb->data - skb_network_header(skb). This way we will detect changes > in IPv4/IPv6 and UDP headers. I thought about that too. Bbut with fraglist GRO, the length of the packets can vary. Unlike standard GRO, there is no requirement that the packets in the fraglist must be equal in length here. So we can't compare the full headers. I think we need to test for addresses and ports. > If so, copy the full headers and fall back to the standard checksum, > recalculation, else use the current path. I agree that we should fallback to standard checksum recalculation if the addresses or ports changed.
Re: [PATCH net v2] udp: ipv4: manipulate network header of NATed UDP GRO fraglist
> From: Steffen Klassert > Date: Mon, 18 Jan 2021 07:37:59 +0100 > > On Fri, Jan 15, 2021 at 05:12:33PM +, Alexander Lobakin wrote: >> From: Dongseok Yi >> Date: Fri, 15 Jan 2021 22:20:35 +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 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 >>> --- >>> 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. >> >> I used another approach, tried to make fraglist GRO closer to plain >> in terms of checksummming, as it is confusing to me why GSO packet >> should have CHECKSUM_UNNECESSARY. > > This is intentional. With fraglist GRO, we don't mangle packets > in the standard (non NAT) case. So the checksum is still correct > after segmentation. That is one reason why it has good forwarding > performance when software segmentation is needed. Checksuming > touches the whole packet and has a lot of overhead, so it is > heplfull to avoid it whenever possible. > > We should find a way to do the checksum only when we really > need it. I.e. only if the headers of the head skb changed. I suggest to do memcmp() between skb_network_header(skb) and skb_network_header(skb->frag_list) with the len of skb->data - skb_network_header(skb). This way we will detect changes in IPv4/IPv6 and UDP headers. If so, copy the full headers and fall back to the standard checksum, recalculation, else use the current path. Al
RE: [PATCH net v2] udp: ipv4: manipulate network header of NATed UDP GRO fraglist
On 2021-01-18 15:37, Steffen Klassert wrote: > On Fri, Jan 15, 2021 at 05:12:33PM +, Alexander Lobakin wrote: > > From: Dongseok Yi > > Date: Fri, 15 Jan 2021 22:20:35 +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 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 > > > --- > > > 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. > > > > I used another approach, tried to make fraglist GRO closer to plain > > in terms of checksummming, as it is confusing to me why GSO packet > > should have CHECKSUM_UNNECESSARY. > > This is intentional. With fraglist GRO, we don't mangle packets > in the standard (non NAT) case. So the checksum is still correct > after segmentation. That is one reason why it has good forwarding > performance when software segmentation is needed. Checksuming > touches the whole packet and has a lot of overhead, so it is > heplfull to avoid it whenever possible. > > We should find a way to do the checksum only when we really > need it. I.e. only if the headers of the head skb changed. It would be not easy to detect if the skb is mangled by netfilter. I think v2 patch has little impact on the performance. Can you suggest an another version? If not, I can make v3 including 80 columns warning fix.
Re: [PATCH net v2] udp: ipv4: manipulate network header of NATed UDP GRO fraglist
On Fri, Jan 15, 2021 at 05:12:33PM +, Alexander Lobakin wrote: > From: Dongseok Yi > Date: Fri, 15 Jan 2021 22:20:35 +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 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 > > --- > > 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. > > I used another approach, tried to make fraglist GRO closer to plain > in terms of checksummming, as it is confusing to me why GSO packet > should have CHECKSUM_UNNECESSARY. This is intentional. With fraglist GRO, we don't mangle packets in the standard (non NAT) case. So the checksum is still correct after segmentation. That is one reason why it has good forwarding performance when software segmentation is needed. Checksuming touches the whole packet and has a lot of overhead, so it is heplfull to avoid it whenever possible. We should find a way to do the checksum only when we really need it. I.e. only if the headers of the head skb changed.
RE: [PATCH net v2] udp: ipv4: manipulate network header of NATed UDP GRO fraglist
On 2021-01-16 02:12, Alexander Lobakin wrote:
> From: Dongseok Yi
> Date: Fri, 15 Jan 2021 22:20:35 +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 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
> > ---
> > 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.
>
> I used another approach, tried to make fraglist GRO closer to plain
> in terms of checksummming, as it is confusing to me why GSO packet
> should have CHECKSUM_UNNECESSARY. Just let Netfilter do its mangling,
> and then use classic UDP GSO magic at the end of segmentation.
> I also see the idea of explicit comparing and editing of IP and UDP
> headers right in __udp_gso_segment_list() rather unacceptable.
If I understand UDP GRO fraglist correctly, it keeps the length of
each skb of the fraglist. But your approach might change the lengths
by gso_size. What if each skb of the fraglist had different lengths?
For CHECKSUM_UNNECESSARY, GROed head_skb might have an invalid
checksum. But finally, the fraglist will be segmented to queue to
sk_receive_queue with head_skb. We could pass the GROed head_skb with
CHECKSUM_UNNECESSARY.
>
> Dongseok, Steffen, please test this WIP diff and tell if this one
> works for you, so I could clean up the code and make a patch.
> For me, it works now in any configurations, with and without
> checksum/GSO/fraglist offload.
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index c1a6f262636a..646a42e88e83 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3674,6 +3674,7 @@ 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;
> @@ -3681,7 +3682,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
> struct sk_buff *nskb, *tmp;
> int err;
>
> - skb_push(skb, -skb_network_offset(skb) + offset);
> + skb_push(skb, doffset);
>
> skb_shinfo(skb)->frag_list = NULL;
>
> @@ -3716,12 +3717,11 @@ 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, skb_headroom(nskb) - skb_headroom(skb));
>
> skb_release_head_state(nskb);
> - __copy_skb_header(nskb, skb);
> + __copy_skb_header(nskb, skb);
>
> - skb_headers_offset_update(nskb, skb_headroom(nskb) -
> skb_headroom(skb));
> skb_copy_from_linear_data_offset(skb, -tnl_hlen,
>nskb->data - tnl_hlen,
>offset + tnl_hlen);
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index ff39e94781bf..61665fcd8c85 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -190,13 +190,58 @@ EXPORT_SYMBOL(skb_udp_tunnel_segment);
> static struct sk_buff *__udp_gso_segment_list(struct sk_buff *skb,
> netdev_features_t features)
> {
> - unsigned int mss = skb_shinfo(skb)->gso_size;
> + struct sk_buff *seg;
> + struct udphdr *uh;
> + unsigned int mss;
> + __be16 newlen;
> + __sum16 check;
> +
> + mss = skb_shinfo(skb)->gso_size;
> + if (skb->len <= sizeof(*uh) + mss)
> + return ERR_PTR(-EINVAL);
>
> - skb = skb_segment_list(skb, features, skb_mac_header_len(skb));
> + skb_pull(skb, sizeof(*uh));
> +
> + skb = skb_segment_list(skb, features, skb->data - skb_mac_header(skb));
> if (IS_ERR(skb))
>
Re: [PATCH net v2] udp: ipv4: manipulate network header of NATed UDP GRO fraglist
From: Dongseok Yi
Date: Fri, 15 Jan 2021 22:20:35 +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 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
> ---
> 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.
I used another approach, tried to make fraglist GRO closer to plain
in terms of checksummming, as it is confusing to me why GSO packet
should have CHECKSUM_UNNECESSARY. Just let Netfilter do its mangling,
and then use classic UDP GSO magic at the end of segmentation.
I also see the idea of explicit comparing and editing of IP and UDP
headers right in __udp_gso_segment_list() rather unacceptable.
Dongseok, Steffen, please test this WIP diff and tell if this one
works for you, so I could clean up the code and make a patch.
For me, it works now in any configurations, with and without
checksum/GSO/fraglist offload.
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index c1a6f262636a..646a42e88e83 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3674,6 +3674,7 @@ 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;
@@ -3681,7 +3682,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
struct sk_buff *nskb, *tmp;
int err;
- skb_push(skb, -skb_network_offset(skb) + offset);
+ skb_push(skb, doffset);
skb_shinfo(skb)->frag_list = NULL;
@@ -3716,12 +3717,11 @@ 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, skb_headroom(nskb) - skb_headroom(skb));
skb_release_head_state(nskb);
-__copy_skb_header(nskb, skb);
+ __copy_skb_header(nskb, skb);
- skb_headers_offset_update(nskb, skb_headroom(nskb) -
skb_headroom(skb));
skb_copy_from_linear_data_offset(skb, -tnl_hlen,
nskb->data - tnl_hlen,
offset + tnl_hlen);
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index ff39e94781bf..61665fcd8c85 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -190,13 +190,58 @@ EXPORT_SYMBOL(skb_udp_tunnel_segment);
static struct sk_buff *__udp_gso_segment_list(struct sk_buff *skb,
netdev_features_t features)
{
- unsigned int mss = skb_shinfo(skb)->gso_size;
+ struct sk_buff *seg;
+ struct udphdr *uh;
+ unsigned int mss;
+ __be16 newlen;
+ __sum16 check;
+
+ mss = skb_shinfo(skb)->gso_size;
+ if (skb->len <= sizeof(*uh) + mss)
+ return ERR_PTR(-EINVAL);
- skb = skb_segment_list(skb, features, skb_mac_header_len(skb));
+ skb_pull(skb, sizeof(*uh));
+
+ skb = skb_segment_list(skb, features, skb->data - skb_mac_header(skb));
if (IS_ERR(skb))
return skb;
- udp_hdr(skb)->len = htons(sizeof(struct udphdr) + mss);
+ seg = skb;
+ uh = udp_hdr(seg);
+
+ /* compute checksum adjustment based on old length versus new */
+ newlen = htons(sizeof(*uh) + mss);
+ check = csum16_add(csum16_sub(uh->check, uh->len), newlen);
+
+ for (;;) {
+ if (!seg->next)
+ break;
+
+ uh->len = newlen;
+ uh->check = check;
+
+ if (seg->ip_summed == CHECKSUM_PARTIAL)
+ gso_reset_checksum(seg, ~check);
+ else
