On Wed, Sep 25, 2019 at 1:46 PM Ben Pfaff <b...@ovn.org> wrote: > On September 25, 2019 1:42:36 PM PDT, Darrell Ball <dlu...@gmail.com> > wrote: >> >> Thank you >> >> Pls see inline >> >> On Wed, Sep 25, 2019 at 10:26 AM Ben Pfaff <b...@ovn.org> wrote: >> >>> 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 >> >> >> I changed 'virtual' to 'virtue' >> >> >>> 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 <dlu...@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: >> >> >> How about ? >> >> /* Check TCP sequence numbers. */ >> >> >>> > + 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. >> >> >> Better >> >> >>> >> >> >>> The text in dpctl.man could be wordsmithed a little. Also appending a >>> suggestion for that. >>> >> >> Looks good >> >> >>> >>> The text in dpctl.man mentions 'be_liberal' mode but the tree doesn't >>> have any other mention of that anywhere. >>> >> >> Good point, I added some context wording. >> >> 'but not the same as 'be_liberal' mode, as in Netfilter.' >> >> >>> >>> 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 >>> >> >> I only dropped the 'inline' specifier since it's implied. >> >> >>> +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 */ >>> >> >> Better; thank you >> >> >>> >>> /* 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. >>> >> >> Looks good; thank you >> I ended up with: >> >> Enables or disables TCP sequence checking. When set to disabled, all >> sequence >> number verification is disabled, including for TCP resets. This is >> similar, but not the same as 'be_liberal' mode, as in Netfilter. >> Disabling >> sequence number verification is not an optimization in itself, but is >> needed >> for some hardware 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 required for hardware offload >> support. >> This command is only supported for the userspace datapath. >> >> If this is ok, I will send V4 with these changes ? >> >> Thanks Darrell >> >> >> >> >>> . >>> .TP >>> >> > All that makes sense, thanks. Please do send v4. >
Thanks; sent _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
