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

Ben Pfaff blp at ovn.org
Wed Sep 25 20:46:51 UTC 2019


On September 25, 2019 1:42:36 PM PDT, Darrell Ball <dlu998 at gmail.com> wrote:
>Thank you
>
>Pls see inline
>
>On Wed, Sep 25, 2019 at 10:26 AM Ben Pfaff <blp at 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 <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:
>
>
>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.


More information about the dev mailing list