On Fri, Jan 14, 2022 at 03:50:52PM -0500, Mike Pattrick wrote:
> On Fri, Jan 14, 2022 at 3:33 PM Flavio Leitner <f...@sysclose.org> wrote:
> >
> >
> > Hi Mike,
> >
> > Thanks for working on this issue.
> >
> > On Fri, Jan 14, 2022 at 10:45:35AM -0500, Mike Pattrick wrote:
> > > Formerly when userspace TSO was enabled but with a non-DKDK interface
> > > without support IP checksum offloading, FTP NAT connections would fail
> > > if the packet length changed. This can happen if the packets length
> > > changes during L7 NAT translation, predominantly with FTP.
> > >
> > > Now we correct the IP header checksum if hwol is disabled or if DPDK
> > > will not handle the IP checksum. This fixes the conntrack - IPv4 FTP
> > > Passive with DNAT" test when run with check-system-tso.
> > >
> > > Reported-by: Flavio Leitner <f...@sysclose.org>
> >
> > Actually, this was initially reported by Ilya.
> >
> > > Signed-off-by: Mike Pattrick <m...@redhat.com>
> > > ---
> > > lib/conntrack.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > > index 33a1a9295..1b8a26ac2 100644
> > > --- a/lib/conntrack.c
> > > +++ b/lib/conntrack.c
> > > @@ -3402,7 +3402,8 @@ handle_ftp_ctl(struct conntrack *ct, const struct
> > > conn_lookup_ctx *ctx,
> > > }
> > > if (seq_skew) {
> > > ip_len = ntohs(l3_hdr->ip_tot_len) + seq_skew;
> > > - if (!dp_packet_hwol_is_ipv4(pkt)) {
> > > + if (!dp_packet_hwol_is_ipv4(pkt) ||
> > > + !dp_packet_ip_checksum_valid(pkt)) {
> > > l3_hdr->ip_csum = recalc_csum16(l3_hdr->ip_csum,
> > >
> > > l3_hdr->ip_tot_len,
> > > htons(ip_len));
> >
> > The problem is that the current code doesn't include IPv4 csum
> > handling as required by the Linux software ports.
> >
> > The patch above resolves the unit test issue because non-DPDK
> > interfaces will not flag the packet with good IP csum, and then
> > the csum is updated accordingly. However, a packet coming from
> > a physical DPDK port can have that flag set by the PMD, then if
> > it goes through that part the IP csum is not updated, which
> > will cause a problem if that packet is sent out over a Linux
> > software port later.
>
> This is a good point, I was trying to get to a happy medium without
> repurposing the patchset that you had previously submitted. That way
> this patch could be available immediately, and your more thorough TSO
> patchset could be applied after.
I see what you did, and I appreciate that. My concern is that it's
not unusual to have packets moving between dpdk and linux ports, so
we might have to visit this issue again, though the unit test is not
failing.
> I had also prepared a larger patch as part of this work, but scrapped
> it because it was duplicating a lot of the work you had previously
> done. But if you prefer the more substantial solution, we can scrap
> this patch and I can submit the larger one.
Feel free to duplicate any parts of that RFC if that makes sense.
I think it's not duplicating anything. It's more as doing the
pieces that are more pressing first.
Let's see the larger one and decide. If it is not suitable, then
maybe as a potential work around we can ignore offloading at that
point and always calculate full IP csum there.
fbl
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
