[ovs-dev] [PATCH ovn 2/2] northd: Provide the option to not use ct.inv in lflows.

numans at ovn.org numans at ovn.org
Mon Mar 22 10:59:11 UTC 2021


From: Numan Siddique <numans at ovn.org>

Presently we add 65535 priority lflows in the stages -
'ls_in_acl' and 'ls_out_acl' to drop packets which
match on 'ct.inv'.

This patch provides an option to not use this field in the
logical flow matches for the following
reasons:

 • Some of the smart NICs which support offloading datapath
   flows may not support this field.  In such cases, almost
   all the datapath flows cannot be offloaded if ACLs/load balancers
   are configured on the logical switch datapath.

 • A recent commit in kernel ovs datapath sets the committed
   connection tracking entry to be liberal for out-of-window
   tcp packets (nf_ct_set_tcp_be_liberal()).  Such TCP
   packets will not be marked as invalid.

There are still certain scenarios where a packet can be marked
invalid.  Like
 • If the tcp flags are not correct.

 • If there are checksum errors.

In such cases, the packet will be delivered to the destination
port.  So CMS should evaluate their usecases before enabling
this option.

Signed-off-by: Numan Siddique <numans at ovn.org>
---
 NEWS                 |  5 +++
 northd/lswitch.dl    | 13 +++++++-
 northd/ovn-northd.c  | 41 ++++++++++++++---------
 northd/ovn_northd.dl | 51 +++++++++++++++++++++++------
 ovn-nb.xml           | 11 +++++++
 tests/ovn-northd.at  | 77 ++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 171 insertions(+), 27 deletions(-)

diff --git a/NEWS b/NEWS
index 530c5d42fe..3181649ba8 100644
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,11 @@ Post-v21.03.0
     (This may take testing and tuning to be effective.)  This version of OVN
     requires DDLog 0.36.
   - Introduce ovn-controller incremetal processing engine statistics
+  - Add a new NB Global option - us_ct_inv_match with the default value of true.
+    If this is set to false, then the logical field - "ct.inv" will not be
+    used in the logical flow matches.  CMS can consider setting this to false,
+    if they want to use smart NICs which don't support offloading datapath flows
+    with this field used.
 
 OVN v21.03.0 - 12 Mar 2021
 -------------------------
diff --git a/northd/lswitch.dl b/northd/lswitch.dl
index 4bf8a5b907..1daf249382 100644
--- a/northd/lswitch.dl
+++ b/northd/lswitch.dl
@@ -197,6 +197,7 @@ relation &Switch(
     ipv6_prefix:       Option<in6_addr>,
     mcast_cfg:         Ref<McastSwitchCfg>,
     is_vlan_transparent: bool,
+    use_ct_inv_match:  bool,
 
     /* Does this switch have at least one port with type != "router"? */
     has_non_router_port: bool
@@ -223,7 +224,8 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> {
         .ipv6_prefix       = ipv6_prefix,
         .mcast_cfg         = mcast_cfg,
         .has_non_router_port = has_non_router_port,
-        .is_vlan_transparent = is_vlan_transparent) :-
+        .is_vlan_transparent = is_vlan_transparent,
+        .use_ct_inv_match = use_ct_inv_match) :-
     nb::Logical_Switch[ls],
     LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl),
     LogicalSwitchHasLBVIP(ls._uuid, has_lb_vip),
@@ -232,6 +234,7 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> {
     LogicalSwitchLocalnetPorts(ls._uuid, localnet_ports),
     LogicalSwitchHasNonRouterPort(ls._uuid, has_non_router_port),
     mcast_cfg in &McastSwitchCfg(.datapath = ls._uuid),
+    UseCtInvMatch(use_ct_inv_match),
     var subnet =
         match (ls.other_config.get("subnet")) {
             None -> None,
@@ -744,3 +747,11 @@ function put_svc_monitor_mac(options: Map<string,string>,
 relation SvcMonitorMac(mac: eth_addr)
 SvcMonitorMac(get_svc_monitor_mac(options, uuid)) :-
     nb::NB_Global(._uuid = uuid, .options = options).
+
+function can_use_ct_inv_match(options: Map<string,string>): bool {
+    map_get_bool_def(options, "use_ct_inv_match", true)
+}
+
+relation UseCtInvMatch(use_ct_inv_match: bool)
+UseCtInvMatch(can_use_ct_inv_match(options)) :-
+    nb::NB_Global(._uuid = uuid, .options = options).
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 3221fb9ff7..6baed2a418 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -4066,6 +4066,10 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od,
  * logical datapath only by creating a datapath group. */
 static bool use_logical_dp_groups = false;
 
+/* If this option is 'true' northd will make use of ct.inv match fields.
+ * Otherwise, it will avoid using it.  The default is true. */
+static bool use_ct_inv_match = true;
+
 /* Adds a row with the specified contents to the Logical_Flow table. */
 static void
 ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od,
@@ -5681,12 +5685,13 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
          * for deletion (bit 0 of ct_label is set).
          *
          * This is enforced at a higher priority than ACLs can be defined. */
+        char *match = xasprintf("%s(ct.est && ct.rpl && ct_label.blocked == 1)",
+                                use_ct_inv_match ? "ct.inv || " : "");
         ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
-                      "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)",
-                      "drop;");
+                      match, "drop;");
         ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
-                      "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)",
-                      "drop;");
+                      match, "drop;");
+        free(match);
 
         /* Ingress and Egress ACL Table (Priority 65535).
          *
@@ -5697,14 +5702,15 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
          * direction to hit the currently defined policy from ACLs.
          *
          * This is enforced at a higher priority than ACLs can be defined. */
+        match = xasprintf("ct.est && !ct.rel && !ct.new%s && "
+                          "ct.rpl && ct_label.blocked == 0",
+                          use_ct_inv_match ? " && !ct.inv" : "");
+
         ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
-                      "ct.est && !ct.rel && !ct.new && !ct.inv "
-                      "&& ct.rpl && ct_label.blocked == 0",
-                      "next;");
+                      match, "next;");
         ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
-                      "ct.est && !ct.rel && !ct.new && !ct.inv "
-                      "&& ct.rpl && ct_label.blocked == 0",
-                      "next;");
+                      match, "next;");
+        free(match);
 
         /* Ingress and Egress ACL Table (Priority 65535).
          *
@@ -5717,14 +5723,14 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
          * a dynamically negotiated FTP data channel), but will allow
          * related traffic such as an ICMP Port Unreachable through
          * that's generated from a non-listening UDP port.  */
+        match = xasprintf("!ct.est && ct.rel && !ct.new%s && "
+                          "ct_label.blocked == 0",
+                          use_ct_inv_match ? " && !ct.inv" : "");
         ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
-                      "!ct.est && ct.rel && !ct.new && !ct.inv "
-                      "&& ct_label.blocked == 0",
-                      "next;");
+                      match, "next;");
         ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
-                      "!ct.est && ct.rel && !ct.new && !ct.inv "
-                      "&& ct_label.blocked == 0",
-                      "next;");
+                      match, "next;");
+        free(match);
 
         /* Ingress and Egress ACL Table (Priority 65535).
          *
@@ -12897,6 +12903,9 @@ ovnnb_db_run(struct northd_context *ctx,
 
     use_logical_dp_groups = smap_get_bool(&nb->options,
                                           "use_logical_dp_groups", false);
+    use_ct_inv_match = smap_get_bool(&nb->options,
+                                     "use_ct_inv_match", true);
+
     /* deprecated, use --event instead */
     controller_event_en = smap_get_bool(&nb->options,
                                         "controller_event", false);
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 35e6ab76cb..9e1919cd80 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -2396,16 +2396,25 @@ var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in
          * for deletion (bit 0 of ct_label is set).
          *
          * This is enforced at a higher priority than ACLs can be defined. */
+        var __match = match (sw.use_ct_inv_match) {
+             true -> "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)",
+             false -> "(ct.est && ct.rpl && ct_label.blocked == 1)"
+        } in
         Flow(.logical_datapath = ls._uuid,
              .stage            = switch_stage(IN, ACL),
              .priority         = 65535,
-             .__match          = "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)",
+             .__match          = __match,
              .actions          = "drop;",
              .external_ids     = map_empty());
+
+        var __match = match (sw.use_ct_inv_match) {
+             true -> "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)",
+             false -> "(ct.est && ct.rpl && ct_label.blocked == 1)"
+        } in
         Flow(.logical_datapath = ls._uuid,
              .stage            = switch_stage(OUT, ACL),
              .priority         = 65535,
-             .__match          = "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)",
+             .__match          = __match,
              .actions          = "drop;",
              .external_ids     = map_empty());
 
@@ -2418,18 +2427,29 @@ var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in
          * direction to hit the currently defined policy from ACLs.
          *
          * This is enforced at a higher priority than ACLs can be defined. */
+        var __match = match (sw.use_ct_inv_match) {
+             true -> "ct.est && !ct.rel && !ct.new && !ct.inv "
+                     "&& ct.rpl && ct_label.blocked == 0",
+             false -> "ct.est && !ct.rel && !ct.new "
+                      "&& ct.rpl && ct_label.blocked == 0"
+        } in
         Flow(.logical_datapath = ls._uuid,
              .stage            = switch_stage(IN, ACL),
              .priority         = 65535,
-             .__match          = "ct.est && !ct.rel && !ct.new && !ct.inv "
-                                 "&& ct.rpl && ct_label.blocked == 0",
+             .__match          = __match,
              .actions          = "next;",
              .external_ids     = map_empty());
+
+        var __match = match (sw.use_ct_inv_match) {
+             true -> "ct.est && !ct.rel && !ct.new && !ct.inv "
+                     "&& ct.rpl && ct_label.blocked == 0",
+             false -> "ct.est && !ct.rel && !ct.new "
+                      "&& ct.rpl && ct_label.blocked == 0"
+        } in
         Flow(.logical_datapath = ls._uuid,
              .stage            = switch_stage(OUT, ACL),
              .priority         = 65535,
-             .__match          = "ct.est && !ct.rel && !ct.new && !ct.inv "
-                                 "&& ct.rpl && ct_label.blocked == 0",
+             .__match          = __match,
              .actions          = "next;",
              .external_ids     = map_empty());
 
@@ -2444,18 +2464,29 @@ var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in
          * a dynamically negotiated FTP data channel), but will allow
          * related traffic such as an ICMP Port Unreachable through
          * that's generated from a non-listening UDP port.  */
+        var __match = match (sw.use_ct_inv_match) {
+             true -> "!ct.est && ct.rel && !ct.new && !ct.inv "
+                     "&& ct_label.blocked == 0",
+             false -> "!ct.est && ct.rel && !ct.new "
+                      "&& ct_label.blocked == 0",
+        } in
         Flow(.logical_datapath = ls._uuid,
              .stage            = switch_stage(IN, ACL),
              .priority         = 65535,
-             .__match          = "!ct.est && ct.rel && !ct.new && !ct.inv "
-                                 "&& ct_label.blocked == 0",
+             .__match          = __match,
              .actions          = "next;",
              .external_ids     = map_empty());
+
+        var __match = match (sw.use_ct_inv_match) {
+             true -> "!ct.est && ct.rel && !ct.new && !ct.inv "
+                     "&& ct_label.blocked == 0",
+             false -> "!ct.est && ct.rel && !ct.new "
+                      "&& ct_label.blocked == 0",
+        } in
         Flow(.logical_datapath = ls._uuid,
              .stage            = switch_stage(OUT, ACL),
              .priority         = 65535,
-             .__match          = "!ct.est && ct.rel && !ct.new && !ct.inv "
-                                 "&& ct_label.blocked == 0",
+             .__match          = __match,
              .actions          = "next;",
              .external_ids     = map_empty());
 
diff --git a/ovn-nb.xml b/ovn-nb.xml
index b0a4adffe5..c3ff3b586a 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -227,6 +227,17 @@
         </p>
       </column>
 
+      <column name="options" key="use_ct_inv_match">
+        <p>
+          If set to false, <code>ovn-northd</code> will not use the
+          <code>ct.inv</code> field in any of the logical flow matches.
+          The default value is true.  CMS can consider setting this to
+          false, in case the NIC doesn't support offloading OVS datapath
+          flows having matches <code>ct_state(..+inv..)</code> or
+          <code>ct_state(..-inv..)</code>.
+        </p>
+      </column>
+
       <group title="Options for configuring interconnection route advertisement">
         <p>
           These options control how routes are advertised between OVN
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 32f956a797..3a3a443a4e 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -3066,3 +3066,80 @@ AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
 
 AT_CLEANUP 
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn -- ct.inv usage])
+ovn_start
+
+check ovn-nbctl ls-add sw0
+check ovn-nbctl lsp-add sw0 sw0p1
+
+check ovn-nbctl --wait=sb acl-add sw0 to-lport 1002 ip allow-related
+
+ovn-sbctl dump-flows sw0 > sw0flows
+AT_CAPTURE_FILE([sw0flows])
+
+AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 65535 | sort], [0], [dnl
+  table=9 (ls_in_acl          ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+  table=9 (ls_in_acl          ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
+  table=9 (ls_in_acl          ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+  table=9 (ls_in_acl          ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
+])
+
+AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 65535 | sort], [0], [dnl
+  table=4 (ls_out_acl         ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+  table=4 (ls_out_acl         ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
+  table=4 (ls_out_acl         ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+  table=4 (ls_out_acl         ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
+])
+
+# Disable ct.inv usage.
+check ovn-nbctl --wait=sb set NB_Global . options:use_ct_inv_match=false
+
+ovn-sbctl dump-flows sw0 > sw0flows
+AT_CAPTURE_FILE([sw0flows])
+
+AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 65535 | sort], [0], [dnl
+  table=9 (ls_in_acl          ), priority=65535, match=(!ct.est && ct.rel && !ct.new && ct_label.blocked == 0), action=(next;)
+  table=9 (ls_in_acl          ), priority=65535, match=((ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+  table=9 (ls_in_acl          ), priority=65535, match=(ct.est && !ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;)
+  table=9 (ls_in_acl          ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
+])
+
+AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 65535 | sort], [0], [dnl
+  table=4 (ls_out_acl         ), priority=65535, match=(!ct.est && ct.rel && !ct.new && ct_label.blocked == 0), action=(next;)
+  table=4 (ls_out_acl         ), priority=65535, match=((ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+  table=4 (ls_out_acl         ), priority=65535, match=(ct.est && !ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;)
+  table=4 (ls_out_acl         ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
+])
+
+AT_CHECK([grep -c "ct.inv" sw0flows], [1], [dnl
+0
+])
+
+# Enable ct.inv usage.
+check ovn-nbctl --wait=sb set NB_Global . options:use_ct_inv_match=true
+
+ovn-sbctl dump-flows sw0 > sw0flows
+AT_CAPTURE_FILE([sw0flows])
+
+AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 65535 | sort], [0], [dnl
+  table=9 (ls_in_acl          ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+  table=9 (ls_in_acl          ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
+  table=9 (ls_in_acl          ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+  table=9 (ls_in_acl          ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
+])
+
+AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 65535 | sort], [0], [dnl
+  table=4 (ls_out_acl         ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+  table=4 (ls_out_acl         ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
+  table=4 (ls_out_acl         ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+  table=4 (ls_out_acl         ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
+])
+
+AT_CHECK([grep -c "ct.inv" sw0flows], [0], [dnl
+6
+])
+
+AT_CLEANUP
+])
-- 
2.29.2



More information about the dev mailing list