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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
