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 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.
Cheers,
M
> A better solution would to have the IP csum updated if the
> packet requires and the port doesn't support that as proposed
> in the RFC[1].
>
> [1] https://mail.openvswitch.org/pipermail/ovs-dev/2021-December/389993.html
> [See: dp_packet_ol_send_prepare()]
>
> Perhaps we can achieve something in the middle with something
> like below (not tested).
>
> What do you think?
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 33a1a9295..9e0364719 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -3403,6 +3403,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)) {
> + dp_packet_ip_checksum_reset_valid(pkt);
> + } else {
> l3_hdr->ip_csum = recalc_csum16(l3_hdr->ip_csum,
> l3_hdr->ip_tot_len,
> htons(ip_len));
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index ee0805ae6..8a880a0ce 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -25,6 +25,7 @@
> #include <rte_mbuf.h>
> #endif
>
> +#include "csum.h"
> #include "netdev-afxdp.h"
> #include "netdev-dpdk.h"
> #include "openvswitch/list.h"
> @@ -1064,6 +1065,20 @@ dp_packet_ip_checksum_valid(const struct dp_packet *p)
> DP_PACKET_OL_RX_IP_CKSUM_GOOD;
> }
>
> +/* Sets IP valid checksum flag in packet 'p'. */
> +static inline void
> +dp_packet_ip_checksum_set_valid(const struct dp_packet *p)
> +{
> + *dp_packet_ol_flags_ptr(p) |= DP_PACKET_OL_RX_IP_CKSUM_GOOD;
> +}
> +
> +/* Resets IP valid checksum flag in packet 'p'. */
> +static inline void
> +dp_packet_ip_checksum_reset_valid(const struct dp_packet *p)
> +{
> + *dp_packet_ol_flags_ptr(p) &= ~DP_PACKET_OL_RX_IP_CKSUM_GOOD;
> +}
> +
> static inline bool
> dp_packet_ip_checksum_bad(const struct dp_packet *p)
> {
> @@ -1071,6 +1086,17 @@ dp_packet_ip_checksum_bad(const struct dp_packet *p)
> DP_PACKET_OL_RX_IP_CKSUM_BAD;
> }
>
> +/* Calculate and set the IPv4 header checksum in packet 'p'. */
> +static inline void
> +dp_packet_ip_set_header_csum(struct dp_packet *p)
> +{
> + struct ip_header *ip = dp_packet_l3(p);
> +
> + ovs_assert(ip);
> + ip->ip_csum = 0;
> + ip->ip_csum = csum(ip, sizeof *ip);
> +}
> +
> static inline bool
> dp_packet_l4_checksum_valid(const struct dp_packet *p)
> {
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 620a451de..a2a87ab92 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -932,7 +932,6 @@ netdev_linux_common_construct(struct netdev *netdev_)
> netdev_->ol_flags |= NETDEV_TX_OFFLOAD_TCP_CKSUM;
> netdev_->ol_flags |= NETDEV_TX_OFFLOAD_UDP_CKSUM;
> netdev_->ol_flags |= NETDEV_TX_OFFLOAD_SCTP_CKSUM;
> - netdev_->ol_flags |= NETDEV_TX_OFFLOAD_IPV4_CKSUM;
> }
>
> return 0;
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 8305f6c42..5bbde02af 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -794,6 +794,15 @@ netdev_send_prepare_packet(const uint64_t netdev_flags,
> {
> uint64_t l4_mask;
>
> + if (dp_packet_hwol_is_ipv4(packet)
> + && !dp_packet_ip_checksum_valid(packet)
> + && !(netdev_flags & NETDEV_TX_OFFLOAD_IPV4_CKSUM)) {
> + /* The packet requested IPv4 offload and the netdev
> + * doesn't support that. */
> + dp_packet_ip_set_header_csum(packet);
> + dp_packet_ip_checksum_set_valid(packet);
> + }
> +
> if (dp_packet_hwol_is_tso(packet)
> && !(netdev_flags & NETDEV_TX_OFFLOAD_TCP_TSO)) {
> /* Fall back to GSO in software. */
>
>
>
> --
> fbl
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
