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

Darrell Ball dlu998 at gmail.com
Wed Sep 25 21:10:49 UTC 2019


On Wed, Sep 25, 2019 at 1:46 PM Ben Pfaff <blp at ovn.org> wrote:

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

Thanks; sent


More information about the dev mailing list