[ovs-dev] [patch v3] conntrack: Add option to disable TCP sequence checking.

Ben Pfaff blp at ovn.org
Wed Sep 25 15:38:30 UTC 2019


On Tue, Sep 24, 2019 at 03:47:35PM -0700, Darrell Ball wrote:
> This may be needed in some special cases, such as to support some hardware
> offload implementations.  Note that disabling TCP sequence number
> verification is not an optimization in itself, but supporting some
> hardware offload implementations may offer better performance.  TCP
> sequence number verification is enabled by default.  This option is only
> available for the userspace datapath.  Access to this option is presently
> provided via 'dpctl' commands as the need for this option is quite node
> specific, by virtual of which nics are in use on a given node.  A test is
> added to verify this option.
> 
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359188.html
> Signed-off-by: Darrell Ball <dlu998 at gmail.com>

Thanks a lot.

It sounds like there are a couple of things you're planning to update
here in any case, so I'll expect to see v4 sometime soon.

This comment seems rather verbose, I'd probably just write "Check
sequence numbers?" or similar:
> +    atomic_bool tcp_seq_ckk; /* TCP sequence number verification; when
> +                                enabled, this enables sequence number
> +                                verification; enabled by default. */

The change to tcp_conn_update() is a bit obscure with side effects in
?:.  It might be clarified somewhat.  I'm appending a suggestion.

The text in dpctl.man could be wordsmithed a little.  Also appending a
suggestion for that.

The text in dpctl.man mentions 'be_liberal' mode but the tree doesn't
have any other mention of that anywhere.

Thanks again,

Ben.

diff --git a/lib/conntrack-tcp.c b/lib/conntrack-tcp.c
index 1e843f337f8a..1dc7ead3b233 100644
--- a/lib/conntrack-tcp.c
+++ b/lib/conntrack-tcp.c
@@ -149,6 +149,16 @@ tcp_get_wscale(const struct tcp_header *tcp)
     return wscale;
 }
 
+static inline bool
+tcp_bypass_seq_chk(struct conntrack *ct)
+{
+    if (!conntrack_get_tcp_seq_chk(ct)) {
+        COVERAGE_INC(conntrack_tcp_seq_chk_bypass);
+        return true;
+    }
+    return false;
+}
+
 static enum ct_update_res
 tcp_conn_update(struct conntrack *ct, struct conn *conn_,
                 struct dp_packet *pkt, bool reply, long long now)
@@ -288,8 +298,7 @@ tcp_conn_update(struct conntrack *ct, struct conn *conn_,
         /* Acking not more than one window forward */
         && ((tcp_flags & TCP_RST) == 0 || orig_seq == src->seqlo
             || (orig_seq == src->seqlo + 1) || (orig_seq + 1 == src->seqlo)))
-        || (!conntrack_get_tcp_seq_chk(ct)
-            ? COVERAGE_INC(conntrack_tcp_seq_chk_bypass), 1 : 0)) {
+        || tcp_bypass_seq_chk(ct)) {
         /* Require an exact/+1 sequence match on resets when possible */
 
         /* update max window */

diff --git a/lib/dpctl.man b/lib/dpctl.man
index 806e5d8e840d..53b7368e3b77 100644
--- a/lib/dpctl.man
+++ b/lib/dpctl.man
@@ -324,12 +324,13 @@ Only supported for userspace datapath.
 .TQ
 \*(DX\fBct\-disable\-tcp\-seq\-chk\fR [\fIdp\fR]
 Enables or disables TCP sequence checking.  When set to disabled, all sequence
-number verification is disabled, including for TCP resets and hence this is
-similar, but not equivalent to 'be_liberal' mode.  Disabling sequence number
+number verification is disabled, including for TCP resets.  This is
+similar, but not the same as, 'be_liberal' mode.  Disabling sequence number
 verification is not an optimization in itself, but is needed for some hardware
-offload support which might offer some performance advantage,  This is enabled
+offload support which might offer some performance advantage.  Sequence
+number checking is enabled
 by default to enforce better security and should only be disabled if
-absolutely required. This command is only supported for the userspace
+required for hardware offload support. This command is only supported for the userspace
 datapath.
 .
 .TP


More information about the dev mailing list