From: "Dongseok Yi" <dseok.yi@samsung.com>
To: "'Steffen Klassert'" <steffen.klassert@secunet.com>
Cc: "'David S. Miller'" <davem@davemloft.net>,
"'Alexander Lobakin'" <alobakin@pm.me>,
<namkyu78.kim@samsung.com>, "'Jakub Kicinski'" <kuba@kernel.org>,
"'Hideaki YOSHIFUJI'" <yoshfuji@linux-ipv6.org>,
"'Willem de Bruijn'" <willemb@google.com>,
<netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH net v2] udp: ipv4: manipulate network header of NATed UDP GRO fraglist
Date: Wed, 20 Jan 2021 15:55:42 +0900 [thread overview]
Message-ID: <012d01d6eef9$45516d40$cff447c0$@samsung.com> (raw)
In-Reply-To: <20210118132736.GM3576117@gauss3.secunet.de>
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.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, 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);
- 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;
}
next prev parent reply other threads:[~2021-01-20 6:56 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20210115133200epcas2p1f52efe7bbc2826ed12da2fde4e03e3b2@epcas2p1.samsung.com>
2021-01-15 13:20 ` [PATCH net v2] udp: ipv4: manipulate network header of NATed UDP GRO fraglist Dongseok Yi
2021-01-15 17:12 ` Alexander Lobakin
2021-01-17 23:55 ` Dongseok Yi
2021-01-18 6:37 ` Steffen Klassert
2021-01-18 7:23 ` Dongseok Yi
2021-01-18 12:17 ` Alexander Lobakin
2021-01-18 12:58 ` Steffen Klassert
2021-01-18 13:27 ` Steffen Klassert
2021-01-20 6:55 ` Dongseok Yi [this message]
2021-01-21 12:28 ` Steffen Klassert
2021-01-21 12:47 ` Dongseok Yi
2021-01-21 12:13 ` Dongseok Yi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='012d01d6eef9$45516d40$cff447c0$@samsung.com' \
--to=dseok.yi@samsung.com \
--cc=alobakin@pm.me \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=namkyu78.kim@samsung.com \
--cc=netdev@vger.kernel.org \
--cc=steffen.klassert@secunet.com \
--cc=willemb@google.com \
--cc=yoshfuji@linux-ipv6.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).