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.
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
