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 <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);
>
> Why not
>
> 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;
>
> Then you don't need to test inside the loop. Just update all
> packets if there is a header mismatch.
The test inside the loop would be needed to distinguish SNAT
and DNAT. I will try your approach on v3. Thanks.
>
> >
> > - 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;
> > }
