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

Darrell Ball dlu998 at gmail.com
Wed Sep 25 20:42:36 UTC 2019


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
>


More information about the dev mailing list