| Message ID | 1569365255-3600-1-git-send-email-dlu998@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v3] conntrack: Add option to disable TCP sequence checking. | expand |
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 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@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: > + 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. The text in dpctl.man could be wordsmithed a little. Also appending a suggestion for that. The text in dpctl.man mentions 'be_liberal' mode but the tree doesn't have any other mention of that anywhere. 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 +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 */ /* 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. . .TP
Thank you Pls see inline On Wed, Sep 25, 2019 at 10:26 AM Ben Pfaff <blp@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@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 >
On September 25, 2019 1:42:36 PM PDT, Darrell Ball <dlu998@gmail.com> wrote: >Thank you > >Pls see inline > >On Wed, Sep 25, 2019 at 10:26 AM Ben Pfaff <blp@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@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.
On Wed, Sep 25, 2019 at 1:46 PM Ben Pfaff <blp@ovn.org> wrote: > On September 25, 2019 1:42:36 PM PDT, Darrell Ball <dlu998@gmail.com> > wrote: >> >> Thank you >> >> Pls see inline >> >> On Wed, Sep 25, 2019 at 10:26 AM Ben Pfaff <blp@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@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
diff --git a/NEWS b/NEWS index aba2e4a..3b72d1b 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,9 @@ Post-v2.12.0 * OVN has been removed from this repository. It now exists as a separate project. You can find it at https://github.com/ovn-org/ovn.git + - Userspace datapath: + * Add option to enable, disable and query TCP sequence checking in + conntrack. v2.12.0 - 03 Sep 2019 --------------------- diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h index bcfbe10..7cdc2a4 100644 --- a/lib/conntrack-private.h +++ b/lib/conntrack-private.h @@ -171,8 +171,10 @@ struct conntrack { struct hindex alg_expectation_refs OVS_GUARDED; /* For lookup from * control context. */ - /* Fragmentation handling context. */ - struct ipf *ipf; + struct ipf *ipf; /* Fragmentation handling context. */ + atomic_bool tcp_seq_ckk; /* TCP sequence number verification; when + enabled, this enables sequence number + verification; enabled by default. */ }; /* Lock acquisition order: diff --git a/lib/conntrack-tcp.c b/lib/conntrack-tcp.c index 397aca1..1e843f3 100644 --- a/lib/conntrack-tcp.c +++ b/lib/conntrack-tcp.c @@ -39,10 +39,15 @@ #include <config.h> #include "conntrack-private.h" +#include "coverage.h" #include "ct-dpif.h" #include "dp-packet.h" #include "util.h" +COVERAGE_DEFINE(conntrack_tcp_seq_chk_bypass); +COVERAGE_DEFINE(conntrack_tcp_seq_chk_failed); +COVERAGE_DEFINE(conntrack_invalid_tcp_flags); + struct tcp_peer { uint32_t seqlo; /* Max sequence number sent */ uint32_t seqhi; /* Max the other end ACKd + win */ @@ -162,6 +167,7 @@ tcp_conn_update(struct conntrack *ct, struct conn *conn_, uint32_t p_len = tcp_payload_length(pkt); if (tcp_invalid_flags(tcp_flags)) { + COVERAGE_INC(conntrack_invalid_tcp_flags); return CT_UPDATE_INVALID; } @@ -272,7 +278,7 @@ tcp_conn_update(struct conntrack *ct, struct conn *conn_, int ackskew = check_ackskew ? dst->seqlo - ack : 0; #define MAXACKWINDOW (0xffff + 1500) /* 1500 is an arbitrary fudge factor */ - if (SEQ_GEQ(src->seqhi, end) + if ((SEQ_GEQ(src->seqhi, end) /* Last octet inside other's window space */ && SEQ_GEQ(seq, src->seqlo - (dst->max_win << dws)) /* Retrans: not more than one window back */ @@ -281,7 +287,9 @@ tcp_conn_update(struct conntrack *ct, struct conn *conn_, && (ackskew <= (MAXACKWINDOW << sws)) /* 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))) { + || (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)) { /* Require an exact/+1 sequence match on resets when possible */ /* update max window */ @@ -385,6 +393,7 @@ tcp_conn_update(struct conntrack *ct, struct conn *conn_, src->state = dst->state = CT_DPIF_TCPS_TIME_WAIT; } } else { + COVERAGE_INC(conntrack_tcp_seq_chk_failed); return CT_UPDATE_INVALID; } diff --git a/lib/conntrack.c b/lib/conntrack.c index b56ef06..755934a 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -310,6 +310,7 @@ conntrack_init(void) ct->hash_basis = random_uint32(); atomic_count_init(&ct->n_conn, 0); atomic_init(&ct->n_conn_limit, DEFAULT_N_CONN_LIMIT); + atomic_init(&ct->tcp_seq_ckk, true); latch_init(&ct->clean_thread_exit); ct->clean_thread = ovs_thread_create("ct_clean", clean_thread_main, ct); ct->ipf = ipf_init(); @@ -2407,6 +2408,21 @@ conntrack_get_nconns(struct conntrack *ct, uint32_t *nconns) return 0; } +int +conntrack_set_tcp_seq_chk(struct conntrack *ct, bool enabled) +{ + atomic_store_relaxed(&ct->tcp_seq_ckk, enabled); + return 0; +} + +bool +conntrack_get_tcp_seq_chk(struct conntrack *ct) +{ + bool enabled; + atomic_read_relaxed(&ct->tcp_seq_ckk, &enabled); + return enabled; +} + /* This function must be called with the ct->resources read lock taken. */ static struct alg_exp_node * expectation_lookup(struct hmap *alg_expectations, const struct conn_key *key, diff --git a/lib/conntrack.h b/lib/conntrack.h index 2012150..75409ba 100644 --- a/lib/conntrack.h +++ b/lib/conntrack.h @@ -118,6 +118,8 @@ int conntrack_flush_tuple(struct conntrack *, const struct ct_dpif_tuple *, int conntrack_set_maxconns(struct conntrack *ct, uint32_t maxconns); int conntrack_get_maxconns(struct conntrack *ct, uint32_t *maxconns); int conntrack_get_nconns(struct conntrack *ct, uint32_t *nconns); +int conntrack_set_tcp_seq_chk(struct conntrack *ct, bool enabled); +bool conntrack_get_tcp_seq_chk(struct conntrack *ct); struct ipf *conntrack_ipf_ctx(struct conntrack *ct); #endif /* conntrack.h */ diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c index 5d8a75d..542b18c 100644 --- a/lib/ct-dpif.c +++ b/lib/ct-dpif.c @@ -165,6 +165,22 @@ ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns) } int +ct_dpif_set_tcp_seq_chk(struct dpif *dpif, bool enabled) +{ + return (dpif->dpif_class->ct_set_tcp_seq_chk + ? dpif->dpif_class->ct_set_tcp_seq_chk(dpif, enabled) + : EOPNOTSUPP); +} + +int +ct_dpif_get_tcp_seq_chk(struct dpif *dpif, bool *enabled) +{ + return (dpif->dpif_class->ct_get_tcp_seq_chk + ? dpif->dpif_class->ct_get_tcp_seq_chk(dpif, enabled) + : EOPNOTSUPP); +} + +int ct_dpif_set_limits(struct dpif *dpif, const uint32_t *default_limit, const struct ovs_list *zone_limits) { diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h index 14178bb..6017933 100644 --- a/lib/ct-dpif.h +++ b/lib/ct-dpif.h @@ -234,6 +234,8 @@ int ct_dpif_flush(struct dpif *, const uint16_t *zone, int ct_dpif_set_maxconns(struct dpif *dpif, uint32_t maxconns); int ct_dpif_get_maxconns(struct dpif *dpif, uint32_t *maxconns); int ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns); +int ct_dpif_set_tcp_seq_chk(struct dpif *dpif, bool enabled); +int ct_dpif_get_tcp_seq_chk(struct dpif *dpif, bool *enabled); int ct_dpif_set_limits(struct dpif *dpif, const uint32_t *default_limit, const struct ovs_list *); int ct_dpif_get_limits(struct dpif *dpif, uint32_t *default_limit, diff --git a/lib/dpctl.c b/lib/dpctl.c index f9eb1ca..0ee6471 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -1767,6 +1767,62 @@ dpctl_ct_get_nconns(int argc, const char *argv[], } static int +dpctl_ct_set_tcp_seq_chk__(int argc, const char *argv[], + struct dpctl_params *dpctl_p, bool enabled) +{ + struct dpif *dpif; + int error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif); + if (!error) { + error = ct_dpif_set_tcp_seq_chk(dpif, enabled); + if (!error) { + dpctl_print(dpctl_p, + "%s TCP sequence checking successful", + enabled ? "enabling" : "disabling"); + } else { + dpctl_error(dpctl_p, error, + "%s TCP sequence checking failed", + enabled ? "enabling" : "disabling"); + } + dpif_close(dpif); + } + return error; +} + +static int +dpctl_ct_enable_tcp_seq_chk(int argc, const char *argv[], + struct dpctl_params *dpctl_p) +{ + return dpctl_ct_set_tcp_seq_chk__(argc, argv, dpctl_p, true); +} + +static int +dpctl_ct_disable_tcp_seq_chk(int argc, const char *argv[], + struct dpctl_params *dpctl_p) +{ + return dpctl_ct_set_tcp_seq_chk__(argc, argv, dpctl_p, false); +} + +static int +dpctl_ct_get_tcp_seq_chk(int argc, const char *argv[], + struct dpctl_params *dpctl_p) +{ + struct dpif *dpif; + int error = opt_dpif_open(argc, argv, dpctl_p, 2, &dpif); + if (!error) { + bool enabled; + error = ct_dpif_get_tcp_seq_chk(dpif, &enabled); + if (!error) { + dpctl_print(dpctl_p, "TCP sequence checking: %s\n", + enabled ? "enabled" : "disabled"); + } else { + dpctl_error(dpctl_p, error, "TCP sequence checking query failed"); + } + dpif_close(dpif); + } + return error; +} + +static int dpctl_ct_set_limits(int argc, const char *argv[], struct dpctl_params *dpctl_p) { @@ -2435,9 +2491,15 @@ static const struct dpctl_command all_commands[] = { { "ct-stats-show", "[dp] [zone=N]", 0, 3, dpctl_ct_stats_show, DP_RO }, { "ct-bkts", "[dp] [gt=N]", 0, 2, dpctl_ct_bkts, DP_RO }, - { "ct-set-maxconns", "[dp] maxconns", 1, 2, dpctl_ct_set_maxconns, DP_RW }, + { "ct-set-maxconns", "[dp] maxconns", 1, 2, dpctl_ct_set_maxconns, + DP_RW }, { "ct-get-maxconns", "[dp]", 0, 1, dpctl_ct_get_maxconns, DP_RO }, { "ct-get-nconns", "[dp]", 0, 1, dpctl_ct_get_nconns, DP_RO }, + { "ct-enable-tcp-seq-chk", "[dp]", 0, 1, dpctl_ct_enable_tcp_seq_chk, + DP_RW }, + { "ct-disable-tcp-seq-chk", "[dp]", 0, 1, dpctl_ct_disable_tcp_seq_chk, + DP_RW }, + { "ct-get-tcp-seq-chk", "[dp]", 0, 1, dpctl_ct_get_tcp_seq_chk, DP_RO }, { "ct-set-limits", "[dp] [default=L] [zone=N,limit=L]...", 1, INT_MAX, dpctl_ct_set_limits, DP_RO }, { "ct-del-limits", "[dp] zone=N1[,N2]...", 1, 2, dpctl_ct_del_limits, diff --git a/lib/dpctl.man b/lib/dpctl.man index 1eeef38..806e5d8 100644 --- a/lib/dpctl.man +++ b/lib/dpctl.man @@ -320,6 +320,24 @@ Prints the current number of connection tracker entries on \fIdp\fR. Only supported for userspace datapath. . .TP +\*(DX\fBct\-enable\-tcp\-seq\-chk\fR [\fIdp\fR] +.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 +verification is not an optimization in itself, but is needed for some hardware +offload support which might offer some performance advantage, This is enabled +by default to enforce better security and should only be disabled if +absolutely required. This command is only supported for the userspace +datapath. +. +.TP +\*(DX\fBct\-get\-tcp\-seq\-chk\fR [\fIdp\fR] +Prints whether TCP sequence checking is enabled or disabled on \fIdp\fR. Only +supported for the userspace datapath. +. +.TP \*(DX\fBct\-set\-limits\fR [\fIdp\fR] [\fBdefault=\fIdefault_limit\fR] [\fBzone=\fIzone\fR,\fBlimit=\fIlimit\fR]... Sets the maximum allowed number of connections in a connection tracking zone. A specific \fIzone\fR may be set to \fIlimit\fR, and multiple zones diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 6be6e47..e4a68a7 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -7441,6 +7441,22 @@ dpif_netdev_ct_get_nconns(struct dpif *dpif, uint32_t *nconns) } static int +dpif_netdev_ct_set_tcp_seq_chk(struct dpif *dpif, bool enabled) +{ + struct dp_netdev *dp = get_dp_netdev(dpif); + + return conntrack_set_tcp_seq_chk(dp->conntrack, enabled); +} + +static int +dpif_netdev_ct_get_tcp_seq_chk(struct dpif *dpif, bool *enabled) +{ + struct dp_netdev *dp = get_dp_netdev(dpif); + *enabled = conntrack_get_tcp_seq_chk(dp->conntrack); + return 0; +} + +static int dpif_netdev_ipf_set_enabled(struct dpif *dpif, bool v6, bool enable) { struct dp_netdev *dp = get_dp_netdev(dpif); @@ -7544,6 +7560,8 @@ const struct dpif_class dpif_netdev_class = { dpif_netdev_ct_set_maxconns, dpif_netdev_ct_get_maxconns, dpif_netdev_ct_get_nconns, + dpif_netdev_ct_set_tcp_seq_chk, + dpif_netdev_ct_get_tcp_seq_chk, NULL, /* ct_set_limits */ NULL, /* ct_get_limits */ NULL, /* ct_del_limits */ diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 7bc71d6..7db2e73 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -3426,6 +3426,8 @@ const struct dpif_class dpif_netlink_class = { NULL, /* ct_set_maxconns */ NULL, /* ct_get_maxconns */ NULL, /* ct_get_nconns */ + NULL, /* ct_set_tcp_seq_chk */ + NULL, /* ct_get_tcp_seq_chk */ dpif_netlink_ct_set_limits, dpif_netlink_ct_get_limits, dpif_netlink_ct_del_limits, diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h index 12898b9..14956f8 100644 --- a/lib/dpif-provider.h +++ b/lib/dpif-provider.h @@ -468,6 +468,11 @@ struct dpif_class { int (*ct_get_maxconns)(struct dpif *, uint32_t *maxconns); /* Get number of connections tracked. */ int (*ct_get_nconns)(struct dpif *, uint32_t *nconns); + /* Enable or disable TCP sequence checking. */ + int (*ct_set_tcp_seq_chk)(struct dpif *, bool enabled); + /* Get the TCP sequence checking configuration. */ + int (*ct_get_tcp_seq_chk)(struct dpif *, bool *enabled); + /* Connection tracking per zone limit */ diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index dc21c88..b530d57 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -10342,6 +10342,144 @@ n_packets=0 OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif - conntrack - disable tcp sequence checking]) +OVS_VSWITCHD_START + +add_of_ports br0 1 2 + +dnl Allow new connections on p1->p2. Allow only established connections p2->p1 +AT_DATA([flows.txt], [dnl +dnl Table 0 +dnl +table=0,priority=10,in_port=1,ip,action=ct(commit,table=1) +table=0,priority=10,in_port=2,ip,action=ct(table=1) +table=0,priority=1,action=drop +dnl +dnl Table 1 +dnl +dnl The following two flows are separated to explicitly count the packets +dnl that create a new connection +table=1,priority=100,cookie=0x1,in_port=1,ip,ct_state=+trk+new-inv-rpl,action=2 +table=1,priority=100,cookie=0x2,in_port=1,ip,ct_state=+trk-new-inv-rpl,action=2 +dnl +table=1,priority=100,cookie=0x3,in_port=2,ip,ct_state=+trk+est+rpl-new-inv,action=1 +table=1,cookie=0x4,ip,ct_state=+trk+inv,action=drop +]) + +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) + +dnl Send 9 packets; one packet will be marked invalid. +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000a50540000000908004500002ca4e5400040067fe20a0101010a0101020001000259b5d93f0000000060027210dd190000020405b4']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 '50540000000950540000000a08004500002c00004000400624c80a0101020a010101000200017c35468459b5d940601272101a4f0000020405b4']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000a505400000009080045000028a4e6400040067fe50a0101010a0101020001000259b5d9407c35468550107210320c0000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 '50540000000950540000000a0800450000303b5f40004006e9640a0101020a010101000200010000000000000000501872106a7300007061796c6f61640a']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000a505400000009080045000028a4e7400040067fe40a0101010a0101020001000259b5d9407c35468d5010721032040000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 '50540000000950540000000a0800450000283b6040004006e96b0a0101020a010101000200017c35468d59b5d9405011721032030000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000a505400000009080045000028a4e8400040067fe30a0101010a0101020001000259b5d9407c35468e5010721032030000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000a505400000009080045000028a4e9400040067fe20a0101010a0101020001000259b5d9407c35468e5011721032020000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 '50540000000950540000000a0800450000283b6140004006e96a0a0101020a010101000200017c35468e59b5d9415010721032020000']) + +AT_CHECK([ovs-appctl revalidator/purge]) +AT_CHECK([ovs-ofctl dump-flows br0 | grep cookie=0x1 | grep -o "n_packets=[[0-9]]*"], [0], [dnl +n_packets=1 +]) + +AT_CHECK([ovs-ofctl dump-flows br0 | grep cookie=0x2 | grep -o "n_packets=[[0-9]]*"], [0], [dnl +n_packets=4 +]) + +AT_CHECK([ovs-ofctl dump-flows br0 | grep cookie=0x3 | grep -o "n_packets=[[0-9]]*"], [0], [dnl +n_packets=3 +]) + +AT_CHECK([ovs-ofctl dump-flows br0 | grep cookie=0x4 | grep -o "n_packets=[[0-9]]*"], [0], [dnl +n_packets=1 +]) + +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) + +AT_CHECK([ovs-appctl dpctl/ct-get-tcp-seq-chk], [], [dnl +TCP sequence checking: enabled +]) + +AT_CHECK([ovs-appctl dpctl/ct-disable-tcp-seq-chk], [], [dnl +disabling TCP sequence checking successful +]) + +AT_CHECK([ovs-appctl dpctl/ct-get-tcp-seq-chk], [], [dnl +TCP sequence checking: disabled +]) + +dnl Send exactly the same 9 packets to confirm no additional packets are marked invalid. +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000a50540000000908004500002ca4e5400040067fe20a0101010a0101020001000259b5d93f0000000060027210dd190000020405b4']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 '50540000000950540000000a08004500002c00004000400624c80a0101020a010101000200017c35468459b5d940601272101a4f0000020405b4']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000a505400000009080045000028a4e6400040067fe50a0101010a0101020001000259b5d9407c35468550107210320c0000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 '50540000000950540000000a0800450000303b5f40004006e9640a0101020a010101000200010000000000000000501872106a7300007061796c6f61640a']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000a505400000009080045000028a4e7400040067fe40a0101010a0101020001000259b5d9407c35468d5010721032040000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 '50540000000950540000000a0800450000283b6040004006e96b0a0101020a010101000200017c35468d59b5d9405011721032030000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000a505400000009080045000028a4e8400040067fe30a0101010a0101020001000259b5d9407c35468e5010721032030000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000a505400000009080045000028a4e9400040067fe20a0101010a0101020001000259b5d9407c35468e5011721032020000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 '50540000000950540000000a0800450000283b6140004006e96a0a0101020a010101000200017c35468e59b5d9415010721032020000']) + +AT_CHECK([ovs-appctl revalidator/purge]) +AT_CHECK([ovs-ofctl dump-flows br0 | grep cookie=0x1 | grep -o "n_packets=[[0-9]]*"], [0], [dnl +n_packets=2 +]) + +AT_CHECK([ovs-ofctl dump-flows br0 | grep cookie=0x2 | grep -o "n_packets=[[0-9]]*"], [0], [dnl +n_packets=8 +]) + +AT_CHECK([ovs-ofctl dump-flows br0 | grep cookie=0x3 | grep -o "n_packets=[[0-9]]*"], [0], [dnl +n_packets=7 +]) + +AT_CHECK([ovs-ofctl dump-flows br0 | grep cookie=0x4 | grep -o "n_packets=[[0-9]]*"], [0], [dnl +n_packets=1 +]) + +AT_CHECK([ovs-appctl dpctl/ct-enable-tcp-seq-chk], [], [dnl +enabling TCP sequence checking successful +]) + +AT_CHECK([ovs-appctl dpctl/ct-get-tcp-seq-chk], [], [dnl +TCP sequence checking: enabled +]) + +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) + +dnl Send exactly the same 9 packets after disabling TCP sequence checking to +dnl confirm one more packet is marked invalid. +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000a50540000000908004500002ca4e5400040067fe20a0101010a0101020001000259b5d93f0000000060027210dd190000020405b4']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 '50540000000950540000000a08004500002c00004000400624c80a0101020a010101000200017c35468459b5d940601272101a4f0000020405b4']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000a505400000009080045000028a4e6400040067fe50a0101010a0101020001000259b5d9407c35468550107210320c0000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 '50540000000950540000000a0800450000303b5f40004006e9640a0101020a010101000200010000000000000000501872106a7300007061796c6f61640a']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000a505400000009080045000028a4e7400040067fe40a0101010a0101020001000259b5d9407c35468d5010721032040000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 '50540000000950540000000a0800450000283b6040004006e96b0a0101020a010101000200017c35468d59b5d9405011721032030000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000a505400000009080045000028a4e8400040067fe30a0101010a0101020001000259b5d9407c35468e5010721032030000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000a505400000009080045000028a4e9400040067fe20a0101010a0101020001000259b5d9407c35468e5011721032020000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 '50540000000950540000000a0800450000283b6140004006e96a0a0101020a010101000200017c35468e59b5d9415010721032020000']) + +AT_CHECK([ovs-appctl revalidator/purge]) +AT_CHECK([ovs-ofctl dump-flows br0 | grep cookie=0x1 | grep -o "n_packets=[[0-9]]*"], [0], [dnl +n_packets=3 +]) + +AT_CHECK([ovs-ofctl dump-flows br0 | grep cookie=0x2 | grep -o "n_packets=[[0-9]]*"], [0], [dnl +n_packets=12 +]) + +AT_CHECK([ovs-ofctl dump-flows br0 | grep cookie=0x3 | grep -o "n_packets=[[0-9]]*"], [0], [dnl +n_packets=10 +]) + +AT_CHECK([ovs-ofctl dump-flows br0 | grep cookie=0x4 | grep -o "n_packets=[[0-9]]*"], [0], [dnl +n_packets=2 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + dnl This is a truncated version of "ofproto-dpif - conntrack - controller", dnl with extra send-to-controller actions following ct_clear to show that dnl the connection tracking data has been cleared.
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 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@gmail.com> --- v3: Make manpage comments more verbose. Expand commit message comments. v2: Per particular requirement, support 'no-tcp-seq-chk' rather than 'liberal' mode. NEWS | 3 ++ lib/conntrack-private.h | 6 ++- lib/conntrack-tcp.c | 13 ++++- lib/conntrack.c | 16 ++++++ lib/conntrack.h | 2 + lib/ct-dpif.c | 16 ++++++ lib/ct-dpif.h | 2 + lib/dpctl.c | 64 +++++++++++++++++++++- lib/dpctl.man | 18 +++++++ lib/dpif-netdev.c | 18 +++++++ lib/dpif-netlink.c | 2 + lib/dpif-provider.h | 5 ++ tests/ofproto-dpif.at | 138 ++++++++++++++++++++++++++++++++++++++++++++++++ 13 files changed, 298 insertions(+), 5 deletions(-)