| Message ID | 1560090909-103610-1-git-send-email-dlu998@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2] conntrack: Add option to disable TCP sequence checking. | expand |
On Sun, Jun 09, 2019 at 07:35:09AM -0700, Darrell Ball wrote: > This may be needed in some special cases, such as to support some > hardware offload implementations. > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359188.html > Signed-off-by: Darrell Ball <dlu998@gmail.com> > --- > > v2: Per particular requirement, support 'no-tcp-seq-chk' rather than > 'liberal' mode. > > Add some debug counters. I'm not sure whether an ovs-appctl command is the best way for users to enable and disable this. It means that it is difficult for an OpenFlow controller to do it, since those commands aren't exposed via OpenFlow or OVSDB. The documentation says that sequence checking should only be disabled if absolutely necessary. If you have an example of such a case, it would be helpful to add it to the documentation.
On Mon, Jun 10, 2019 at 9:51 AM Ben Pfaff <blp@ovn.org> wrote: > On Sun, Jun 09, 2019 at 07:35:09AM -0700, Darrell Ball wrote: > > This may be needed in some special cases, such as to support some > > hardware offload implementations. > > > > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359188.html > > Signed-off-by: Darrell Ball <dlu998@gmail.com> > > --- > > > > v2: Per particular requirement, support 'no-tcp-seq-chk' rather than > > 'liberal' mode. > > > > Add some debug counters. > > I'm not sure whether an ovs-appctl command is the best way for users to > enable and disable this. It means that it is difficult for an OpenFlow > controller to do it, since those commands aren't exposed via OpenFlow or > OVSDB. > Thanks for your comments For local controller usage, we are using ovs-appctl today in similar cases for existing products. In the case of non-local controller usage, the remote controller would need remote access. However, in this case, I don't expect the remote controller to be involved; I was assuming that a deployment script would be used to set the value to non-default value (in needed cases) when ovs-vswitchd is (re)started only. If this assumption cannot be satisfied then we would have to have to introduce a dependency on the database for these types of commands. > > The documentation says that sequence checking should only be disabled if > absolutely necessary. If you have an example of such a case, it would > be helpful to add it to the documentation. > done.
On Wed, Jun 12, 2019 at 08:46:06AM -0700, Darrell Ball wrote: > On Mon, Jun 10, 2019 at 9:51 AM Ben Pfaff <blp@ovn.org> wrote: > > > On Sun, Jun 09, 2019 at 07:35:09AM -0700, Darrell Ball wrote: > > > This may be needed in some special cases, such as to support some > > > hardware offload implementations. > > > > > > Reported-at: > > https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359188.html > > > Signed-off-by: Darrell Ball <dlu998@gmail.com> > > > --- > > > > > > v2: Per particular requirement, support 'no-tcp-seq-chk' rather than > > > 'liberal' mode. > > > > > > Add some debug counters. > > > > I'm not sure whether an ovs-appctl command is the best way for users to > > enable and disable this. It means that it is difficult for an OpenFlow > > controller to do it, since those commands aren't exposed via OpenFlow or > > OVSDB. > > > > Thanks for your comments > > For local controller usage, we are using ovs-appctl today in similar cases > for existing products. > > In the case of non-local controller usage, the remote controller would need > remote access. > > However, in this case, I don't expect the remote controller to be > involved; I was assuming > that a deployment script would be used to set the value to non-default > value (in needed cases) > when ovs-vswitchd is (re)started only. If this assumption cannot be > satisfied then we would > have to have to introduce a dependency on the database for these types of > commands. This seems to be teetering toward the pre-SDN model of having to separately configure each switch. Do you have some rationale in mind why this should be a per-node decision rather than one made by the controller? I understand that we currently have other configuration done through ovs-appctl, but that doesn't necessarily mean we made the right decision there either.
On Wed, Jun 12, 2019 at 10:09 AM Ben Pfaff <blp@ovn.org> wrote: > On Wed, Jun 12, 2019 at 08:46:06AM -0700, Darrell Ball wrote: > > On Mon, Jun 10, 2019 at 9:51 AM Ben Pfaff <blp@ovn.org> wrote: > > > > > On Sun, Jun 09, 2019 at 07:35:09AM -0700, Darrell Ball wrote: > > > > This may be needed in some special cases, such as to support some > > > > hardware offload implementations. > > > > > > > > Reported-at: > > > https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359188.html > > > > Signed-off-by: Darrell Ball <dlu998@gmail.com> > > > > --- > > > > > > > > v2: Per particular requirement, support 'no-tcp-seq-chk' rather than > > > > 'liberal' mode. > > > > > > > > Add some debug counters. > > > > > > I'm not sure whether an ovs-appctl command is the best way for users to > > > enable and disable this. It means that it is difficult for an OpenFlow > > > controller to do it, since those commands aren't exposed via OpenFlow > or > > > OVSDB. > > > > > > > Thanks for your comments > > > > For local controller usage, we are using ovs-appctl today in similar > cases > > for existing products. > > > > In the case of non-local controller usage, the remote controller would > need > > remote access. > > > > However, in this case, I don't expect the remote controller to be > > involved; I was assuming > > that a deployment script would be used to set the value to non-default > > value (in needed cases) > > when ovs-vswitchd is (re)started only. If this assumption cannot be > > satisfied then we would > > have to have to introduce a dependency on the database for these types of > > commands. > > This seems to be teetering toward the pre-SDN model of having to > separately configure each switch. Do you have some rationale in mind > why this should be a per-node decision rather than one made by the > controller? 1/ Because of the reduced security implications vs higher performance advantage, it would be a per node (or per node role) decision of whether to use it or not. 2/ Furthermore, the general applicability is otherwise limited across implementations (offload or otherwise). This means that one node in a network could easily be using one offload implementation requiring this config, while other nodes using another offload implementation not needing this config. > I understand that we currently have other configuration > done through ovs-appctl, but that doesn't necessarily mean we made the > right decision there either. >
On Wed, Jun 12, 2019 at 10:31:17AM -0700, Darrell Ball wrote: > On Wed, Jun 12, 2019 at 10:09 AM Ben Pfaff <blp@ovn.org> wrote: > > > On Wed, Jun 12, 2019 at 08:46:06AM -0700, Darrell Ball wrote: > > > On Mon, Jun 10, 2019 at 9:51 AM Ben Pfaff <blp@ovn.org> wrote: > > > > > > > On Sun, Jun 09, 2019 at 07:35:09AM -0700, Darrell Ball wrote: > > > > > This may be needed in some special cases, such as to support some > > > > > hardware offload implementations. > > > > > > > > > > Reported-at: > > > > https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359188.html > > > > > Signed-off-by: Darrell Ball <dlu998@gmail.com> > > > > > --- > > > > > > > > > > v2: Per particular requirement, support 'no-tcp-seq-chk' rather than > > > > > 'liberal' mode. > > > > > > > > > > Add some debug counters. > > > > > > > > I'm not sure whether an ovs-appctl command is the best way for users to > > > > enable and disable this. It means that it is difficult for an OpenFlow > > > > controller to do it, since those commands aren't exposed via OpenFlow > > or > > > > OVSDB. > > > > > > > > > > Thanks for your comments > > > > > > For local controller usage, we are using ovs-appctl today in similar > > cases > > > for existing products. > > > > > > In the case of non-local controller usage, the remote controller would > > need > > > remote access. > > > > > > However, in this case, I don't expect the remote controller to be > > > involved; I was assuming > > > that a deployment script would be used to set the value to non-default > > > value (in needed cases) > > > when ovs-vswitchd is (re)started only. If this assumption cannot be > > > satisfied then we would > > > have to have to introduce a dependency on the database for these types of > > > commands. > > > > This seems to be teetering toward the pre-SDN model of having to > > separately configure each switch. Do you have some rationale in mind > > why this should be a per-node decision rather than one made by the > > controller? > > > 1/ Because of the reduced security implications vs higher performance > advantage, it would be a per node (or per node role) decision of whether > to use it or not. Are you saying that the only advantage of disabling TCP sequence checking is performance, and only in the presence of hardware for offloading that requires it? This has not been clear to me. If that is the case, then I can see that this would be a local decision, because one would want to disable TCP sequence checking only on a system with the given hardware.
On Wed, Jun 12, 2019 at 10:58 AM Ben Pfaff <blp@ovn.org> wrote: > On Wed, Jun 12, 2019 at 10:31:17AM -0700, Darrell Ball wrote: > > On Wed, Jun 12, 2019 at 10:09 AM Ben Pfaff <blp@ovn.org> wrote: > > > > > On Wed, Jun 12, 2019 at 08:46:06AM -0700, Darrell Ball wrote: > > > > On Mon, Jun 10, 2019 at 9:51 AM Ben Pfaff <blp@ovn.org> wrote: > > > > > > > > > On Sun, Jun 09, 2019 at 07:35:09AM -0700, Darrell Ball wrote: > > > > > > This may be needed in some special cases, such as to support some > > > > > > hardware offload implementations. > > > > > > > > > > > > Reported-at: > > > > > > https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359188.html > > > > > > Signed-off-by: Darrell Ball <dlu998@gmail.com> > > > > > > --- > > > > > > > > > > > > v2: Per particular requirement, support 'no-tcp-seq-chk' rather > than > > > > > > 'liberal' mode. > > > > > > > > > > > > Add some debug counters. > > > > > > > > > > I'm not sure whether an ovs-appctl command is the best way for > users to > > > > > enable and disable this. It means that it is difficult for an > OpenFlow > > > > > controller to do it, since those commands aren't exposed via > OpenFlow > > > or > > > > > OVSDB. > > > > > > > > > > > > > Thanks for your comments > > > > > > > > For local controller usage, we are using ovs-appctl today in similar > > > cases > > > > for existing products. > > > > > > > > In the case of non-local controller usage, the remote controller > would > > > need > > > > remote access. > > > > > > > > However, in this case, I don't expect the remote controller to be > > > > involved; I was assuming > > > > that a deployment script would be used to set the value to > non-default > > > > value (in needed cases) > > > > when ovs-vswitchd is (re)started only. If this assumption cannot be > > > > satisfied then we would > > > > have to have to introduce a dependency on the database for these > types of > > > > commands. > > > > > > This seems to be teetering toward the pre-SDN model of having to > > > separately configure each switch. Do you have some rationale in mind > > > why this should be a per-node decision rather than one made by the > > > controller? > > > > > > 1/ Because of the reduced security implications vs higher performance > > advantage, it would be a per node (or per node role) decision of whether > > to use it or not. > > Are you saying that the only advantage of disabling TCP sequence > checking is performance, and only in the presence of hardware for > offloading that requires it? Some HWOL implementations would be the most common 'recommended' usage. I will be adding a general statement to the documentation and will echo it in the commit message. > This has not been clear to me. If that is > the case, then I can see that this would be a local decision, because > one would want to disable TCP sequence checking only on a system with > the given hardware. >
On Wed, Jun 12, 2019 at 12:44:18PM -0700, Darrell Ball wrote: > On Wed, Jun 12, 2019 at 10:58 AM Ben Pfaff <blp@ovn.org> wrote: > > > On Wed, Jun 12, 2019 at 10:31:17AM -0700, Darrell Ball wrote: > > > On Wed, Jun 12, 2019 at 10:09 AM Ben Pfaff <blp@ovn.org> wrote: > > > > > > > On Wed, Jun 12, 2019 at 08:46:06AM -0700, Darrell Ball wrote: > > > > > On Mon, Jun 10, 2019 at 9:51 AM Ben Pfaff <blp@ovn.org> wrote: > > > > > > > > > > > On Sun, Jun 09, 2019 at 07:35:09AM -0700, Darrell Ball wrote: > > > > > > > This may be needed in some special cases, such as to support some > > > > > > > hardware offload implementations. > > > > > > > > > > > > > > Reported-at: > > > > > > > > https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359188.html > > > > > > > Signed-off-by: Darrell Ball <dlu998@gmail.com> > > > > > > > --- > > > > > > > > > > > > > > v2: Per particular requirement, support 'no-tcp-seq-chk' rather > > than > > > > > > > 'liberal' mode. > > > > > > > > > > > > > > Add some debug counters. > > > > > > > > > > > > I'm not sure whether an ovs-appctl command is the best way for > > users to > > > > > > enable and disable this. It means that it is difficult for an > > OpenFlow > > > > > > controller to do it, since those commands aren't exposed via > > OpenFlow > > > > or > > > > > > OVSDB. > > > > > > > > > > > > > > > > Thanks for your comments > > > > > > > > > > For local controller usage, we are using ovs-appctl today in similar > > > > cases > > > > > for existing products. > > > > > > > > > > In the case of non-local controller usage, the remote controller > > would > > > > need > > > > > remote access. > > > > > > > > > > However, in this case, I don't expect the remote controller to be > > > > > involved; I was assuming > > > > > that a deployment script would be used to set the value to > > non-default > > > > > value (in needed cases) > > > > > when ovs-vswitchd is (re)started only. If this assumption cannot be > > > > > satisfied then we would > > > > > have to have to introduce a dependency on the database for these > > types of > > > > > commands. > > > > > > > > This seems to be teetering toward the pre-SDN model of having to > > > > separately configure each switch. Do you have some rationale in mind > > > > why this should be a per-node decision rather than one made by the > > > > controller? > > > > > > > > > 1/ Because of the reduced security implications vs higher performance > > > advantage, it would be a per node (or per node role) decision of whether > > > to use it or not. > > > > Are you saying that the only advantage of disabling TCP sequence > > checking is performance, and only in the presence of hardware for > > offloading that requires it? > > > Some HWOL implementations would be the most common 'recommended' usage. > I will be adding a general statement to the documentation and will echo it > in the commit > message. Is there a v3 with that change? I haven't been able to find it.
Thanks Ben I just noticed I sent another version here https://patchwork.ozlabs.org/patch/1153283/ but there were a couple of issues that deserve a resend. It might need a rebase as well Darrell On Tue, Sep 24, 2019 at 3:18 PM Ben Pfaff <blp@ovn.org> wrote: > On Wed, Jun 12, 2019 at 12:44:18PM -0700, Darrell Ball wrote: > > On Wed, Jun 12, 2019 at 10:58 AM Ben Pfaff <blp@ovn.org> wrote: > > > > > On Wed, Jun 12, 2019 at 10:31:17AM -0700, Darrell Ball wrote: > > > > On Wed, Jun 12, 2019 at 10:09 AM Ben Pfaff <blp@ovn.org> wrote: > > > > > > > > > On Wed, Jun 12, 2019 at 08:46:06AM -0700, Darrell Ball wrote: > > > > > > On Mon, Jun 10, 2019 at 9:51 AM Ben Pfaff <blp@ovn.org> wrote: > > > > > > > > > > > > > On Sun, Jun 09, 2019 at 07:35:09AM -0700, Darrell Ball wrote: > > > > > > > > This may be needed in some special cases, such as to support > some > > > > > > > > hardware offload implementations. > > > > > > > > > > > > > > > > Reported-at: > > > > > > > > > > https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359188.html > > > > > > > > Signed-off-by: Darrell Ball <dlu998@gmail.com> > > > > > > > > --- > > > > > > > > > > > > > > > > v2: Per particular requirement, support 'no-tcp-seq-chk' > rather > > > than > > > > > > > > 'liberal' mode. > > > > > > > > > > > > > > > > Add some debug counters. > > > > > > > > > > > > > > I'm not sure whether an ovs-appctl command is the best way for > > > users to > > > > > > > enable and disable this. It means that it is difficult for an > > > OpenFlow > > > > > > > controller to do it, since those commands aren't exposed via > > > OpenFlow > > > > > or > > > > > > > OVSDB. > > > > > > > > > > > > > > > > > > > Thanks for your comments > > > > > > > > > > > > For local controller usage, we are using ovs-appctl today in > similar > > > > > cases > > > > > > for existing products. > > > > > > > > > > > > In the case of non-local controller usage, the remote controller > > > would > > > > > need > > > > > > remote access. > > > > > > > > > > > > However, in this case, I don't expect the remote controller to > be > > > > > > involved; I was assuming > > > > > > that a deployment script would be used to set the value to > > > non-default > > > > > > value (in needed cases) > > > > > > when ovs-vswitchd is (re)started only. If this assumption cannot > be > > > > > > satisfied then we would > > > > > > have to have to introduce a dependency on the database for these > > > types of > > > > > > commands. > > > > > > > > > > This seems to be teetering toward the pre-SDN model of having to > > > > > separately configure each switch. Do you have some rationale in > mind > > > > > why this should be a per-node decision rather than one made by the > > > > > controller? > > > > > > > > > > > > 1/ Because of the reduced security implications vs higher performance > > > > advantage, it would be a per node (or per node role) decision of > whether > > > > to use it or not. > > > > > > Are you saying that the only advantage of disabling TCP sequence > > > checking is performance, and only in the presence of hardware for > > > offloading that requires it? > > > > > > Some HWOL implementations would be the most common 'recommended' usage. > > I will be adding a general statement to the documentation and will echo > it > > in the commit > > message. > > Is there a v3 with that change? I haven't been able to find it. >
diff --git a/NEWS b/NEWS index 19cebf8..12b35be 100644 --- a/NEWS +++ b/NEWS @@ -23,6 +23,8 @@ Post-v2.11.0 * New action "check_pkt_len". * Port configuration with "other-config:priority-tags" now has a mode that retains the 802.1Q header even if VLAN and priority are both zero. + * Add option to enable, disable and query TCP sequence checking in + conntrack. - OVSDB: * OVSDB clients can now resynchronize with clustered servers much more quickly after a brief disconnection, saving bandwidth and CPU time. 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 5f60fea..0125bb4 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(); @@ -2396,6 +2397,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 9c4eb65..a54dedb 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -1760,6 +1760,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) { @@ -2428,9 +2484,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 1ff3511..b5e12e7 100644 --- a/lib/dpctl.man +++ b/lib/dpctl.man @@ -318,6 +318,23 @@ 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 in the userspace connection tracker. +If enabled, TCP sequence number verification will be enabled. This is enabled +by default to enforce better security and should only be disabled if +absolutely required. When set to disabled, all sequence number checking is +disabled, including for TCP resets and hence this is similar, but not +equivalent to 'be_liberal' mode. 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 fb2eb4b..918d554 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -7354,6 +7354,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); @@ -7456,6 +7472,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 c554666..dd1deac 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -3429,6 +3429,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 b2a4dff..fc64a31 100644 --- a/lib/dpif-provider.h +++ b/lib/dpif-provider.h @@ -462,6 +462,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 9640d1c..0f67226 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -10301,6 +10301,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. Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359188.html Signed-off-by: Darrell Ball <dlu998@gmail.com> --- v2: Per particular requirement, support 'no-tcp-seq-chk' rather than 'liberal' mode. Add some debug counters. NEWS | 2 + 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 | 17 ++++++ lib/dpif-netdev.c | 18 +++++++ lib/dpif-netlink.c | 2 + lib/dpif-provider.h | 5 ++ tests/ofproto-dpif.at | 138 ++++++++++++++++++++++++++++++++++++++++++++++++ 13 files changed, 296 insertions(+), 5 deletions(-)