From 5336b5cb342b8f81115299540f3268f734a6d009 Mon Sep 17 00:00:00 2001

From: Dumitru Ceara <dceara@redhat.com>

Date: Wed, 10 Feb 2021 12:20:17 +0100

Subject: [PATCH] northd: Skip matching on ct flags for stateless

 configurations.

If no load balancers or "allow-related" ACLs are configured on a logical

switch, no packets will be sent to conntrack in the logical switch

pipeline and ACL flows in tables ls_in/out_acl will not match on

conntrack state.  In this case there's no need to try to set ACL hints

in tables ls_in/out_acl_hint.

Furthermore, setting the hints translates to always generating flows

that match on ct.state.  Depending on the underlying hardware such flows

may not be offloadable inducing a hit in performance even when no

conntrack recirculations are required.

To avoid iterating through all configured ACLs and load balancers

multiple times, we now store two new fields in the 'ovn_datapath'

structure:

- has_stateful_acl

- has_lb_vip

Also, rename the 'has_lb_vip()' and 'has_stateful_acl()' functions,

prefixing them with 'ls_' to match other helper function names.

Fixes: 209ea46bbf9d ("ovn-northd: Reduce number of flows generated for stateful ACLs.")

Reported-by: Haresh Khandelwal <hakhande@redhat.com>

Reported-at: https://bugzilla.redhat.com/1927211

Signed-off-by: Dumitru Ceara <dceara@redhat.com>

Signed-off-by: Mark Michelson <mmichels@redhat.com>

Acked-by: Mark Michelson <mmichels@redhat.com>

---

 northd/ovn-northd.8.xml |  3 +-

 northd/ovn-northd.c     | 34 +++++++++++-------

 tests/ovn-northd.at     | 77 +++++++++++++++++++++++++++++++++++++++++

 3 files changed, 101 insertions(+), 13 deletions(-)

diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml

index 70065a36d..2f8b4e8c3 100644

--- a/northd/ovn-northd.8.xml

+++ b/northd/ovn-northd.8.xml

@@ -415,7 +415,8 @@

       This table consists of logical flows that set hints

       (reg0 bits) to be used in the next stage, in the ACL

-      processing table. Multiple hints can be set for the same packet.

+      processing table, if stateful ACLs or load balancers are configured.

+      Multiple hints can be set for the same packet.

       The possible hints are:

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c

index db6572a62..b85e6e78a 100644

--- a/northd/ovn-northd.c

+++ b/northd/ovn-northd.c

@@ -597,6 +597,8 @@ struct ovn_datapath {

     struct hmap port_tnlids;

     uint32_t port_key_hint;

+    bool has_stateful_acl;

+    bool has_lb_vip;

     bool has_unknown;

     /* IPAM data. */

@@ -635,6 +637,9 @@ struct ovn_datapath {

     struct hmap nb_pgs;

 };

+static bool ls_has_stateful_acl(struct ovn_datapath *od);

+static bool ls_has_lb_vip(struct ovn_datapath *od);

+

 /* Contains a NAT entry with the external addresses pre-parsed. */

 struct ovn_nat {

     const struct nbrec_nat *nb;

@@ -4635,7 +4640,7 @@ ovn_ls_port_group_destroy(struct hmap *nb_pgs)

 }

 static bool

-has_stateful_acl(struct ovn_datapath *od)

+ls_has_stateful_acl(struct ovn_datapath *od)

 {

     for (size_t i = 0; i < od->nbs->n_acls; i++) {

         struct nbrec_acl *acl = od->nbs->acls[i];

@@ -4814,8 +4819,6 @@ skip_port_from_conntrack(struct ovn_datapath *od, struct ovn_port *op,

 static void

 build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)

 {

-    bool has_stateful = has_stateful_acl(od);

-

     /* Ingress and Egress Pre-ACL Table (Priority 0): Packets are

      * allowed by default. */

     ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 0, "1", "next;");

@@ -4830,7 +4833,7 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)

     /* If there are any stateful ACL rules in this datapath, we must

      * send all IP packets through the conntrack action, which handles

      * defragmentation, in order to match L4 headers. */

-    if (has_stateful) {

+    if (od->has_stateful_acl) {

         for (size_t i = 0; i < od->n_router_ports; i++) {

             skip_port_from_conntrack(od, od->router_ports[i],

                                      S_SWITCH_IN_PRE_ACL, S_SWITCH_OUT_PRE_ACL,

@@ -4933,7 +4936,7 @@ build_empty_lb_event_flow(struct ovn_datapath *od, struct hmap *lflows,

 }

 static bool

-has_lb_vip(struct ovn_datapath *od)

+ls_has_lb_vip(struct ovn_datapath *od)

 {

     for (int i = 0; i < od->nbs->n_load_balancer; i++) {

         struct nbrec_load_balancer *nb_lb = od->nbs->load_balancer[i];

@@ -5076,6 +5079,13 @@ build_acl_hints(struct ovn_datapath *od, struct hmap *lflows)

     for (size_t i = 0; i < ARRAY_SIZE(stages); i++) {

         enum ovn_stage stage = stages[i];

+        /* In any case, advance to the next stage. */

+        ovn_lflow_add(lflows, od, stage, 0, "1", "next;");

+

+        if (!od->has_stateful_acl && !od->has_lb_vip) {

+            continue;

+        }

+

         /* New, not already established connections, may hit either allow

          * or drop ACLs. For allow ACLs, the connection must also be committed

          * to conntrack so we set REGBIT_ACL_HINT_ALLOW_NEW.

@@ -5136,9 +5146,6 @@ build_acl_hints(struct ovn_datapath *od, struct hmap *lflows)

         ovn_lflow_add(lflows, od, stage, 1, "ct.est && ct_label.blocked == 0",

                       REGBIT_ACL_HINT_BLOCK " = 1; "

                       "next;");

-

-        /* In any case, advance to the next stage. */

-        ovn_lflow_add(lflows, od, stage, 0, "1", "next;");

     }

 }

@@ -5470,7 +5477,7 @@ static void

 build_acls(struct ovn_datapath *od, struct hmap *lflows,

            struct hmap *port_groups, const struct shash *meter_groups)

 {

-    bool has_stateful = (has_stateful_acl(od) || has_lb_vip(od));

+    bool has_stateful = od->has_stateful_acl || od->has_lb_vip;

     /* Ingress and Egress ACL Table (Priority 0): Packets are allowed by

      * default.  A related rule at priority 1 is added below if there

@@ -5739,7 +5746,7 @@ build_lb(struct ovn_datapath *od, struct hmap *lflows)

         }

     }

-    if (has_lb_vip(od)) {

+    if (od->has_lb_vip) {

         /* Ingress and Egress LB Table (Priority 65534).

          *

          * Send established traffic through conntrack for just NAT. */

@@ -5860,7 +5867,7 @@ build_lb_hairpin(struct ovn_datapath *od, struct hmap *lflows)

     ovn_lflow_add(lflows, od, S_SWITCH_IN_NAT_HAIRPIN, 0, "1", "next;");

     ovn_lflow_add(lflows, od, S_SWITCH_IN_HAIRPIN, 0, "1", "next;");

-    if (has_lb_vip(od)) {

+    if (od->has_lb_vip) {

         /* Check if the packet needs to be hairpinned. */

         ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_PRE_HAIRPIN, 100,

                                 "ip && ct.trk && ct.dnat",

@@ -6597,7 +6604,10 @@ build_lswitch_lflows_pre_acl_and_acl(struct ovn_datapath *od,

                                      struct shash *meter_groups,

                                      struct hmap *lbs)

 {

-   if (od->nbs) {

+    if (od->nbs) {

+        od->has_stateful_acl = ls_has_stateful_acl(od);

+        od->has_lb_vip = ls_has_lb_vip(od);

+

         build_pre_acls(od, lflows);

         build_pre_lb(od, lflows, meter_groups, lbs);

         build_pre_stateful(od, lflows);

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at

index 7240e22ba..64a788067 100644

--- a/tests/ovn-northd.at

+++ b/tests/ovn-northd.at

@@ -1912,6 +1912,83 @@ check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2}

 AT_CLEANUP

+AT_SETUP([ovn -- ACL skip hints for stateless config])

+AT_KEYWORDS([acl])

+ovn_start

+

+check ovn-nbctl --wait=sb \

+    -- ls-add ls \

+    -- lsp-add ls lsp \

+    -- acl-add ls from-lport 1 "ip" allow \

+    -- acl-add ls to-lport 1 "ip" allow

+

+AS_BOX([Check no match on ct_state with stateless ACLs])

+AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e ls_in_acl -e ls_out_acl | grep 'ct\.' | sort], [0], [dnl

+])

+

+AS_BOX([Check match ct_state with stateful ACLs])

+check ovn-nbctl --wait=sb \

+    -- acl-add ls from-lport 2 "udp" allow-related \

+    -- acl-add ls to-lport 2 "udp" allow-related

+AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e ls_in_acl -e ls_out_acl | grep 'ct\.' | sort], [0], [dnl

+  table=4 (ls_out_acl_hint    ), priority=1    , match=(ct.est && ct_label.blocked == 0), action=(reg0[[10]] = 1; next;)

+  table=4 (ls_out_acl_hint    ), priority=2    , match=(ct.est && ct_label.blocked == 1), action=(reg0[[9]] = 1; next;)

+  table=4 (ls_out_acl_hint    ), priority=3    , match=(!ct.est), action=(reg0[[9]] = 1; next;)

+  table=4 (ls_out_acl_hint    ), priority=4    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] = 1; next;)

+  table=4 (ls_out_acl_hint    ), priority=5    , match=(!ct.trk), action=(reg0[[8]] = 1; reg0[[9]] = 1; next;)

+  table=4 (ls_out_acl_hint    ), priority=6    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)

+  table=4 (ls_out_acl_hint    ), priority=7    , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)

+  table=5 (ls_out_acl         ), priority=1    , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)

+  table=5 (ls_out_acl         ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)

+  table=5 (ls_out_acl         ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)

+  table=5 (ls_out_acl         ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)

+  table=6 (ls_in_acl_hint     ), priority=1    , match=(ct.est && ct_label.blocked == 0), action=(reg0[[10]] = 1; next;)

+  table=6 (ls_in_acl_hint     ), priority=2    , match=(ct.est && ct_label.blocked == 1), action=(reg0[[9]] = 1; next;)

+  table=6 (ls_in_acl_hint     ), priority=3    , match=(!ct.est), action=(reg0[[9]] = 1; next;)

+  table=6 (ls_in_acl_hint     ), priority=4    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] = 1; next;)

+  table=6 (ls_in_acl_hint     ), priority=5    , match=(!ct.trk), action=(reg0[[8]] = 1; reg0[[9]] = 1; next;)

+  table=6 (ls_in_acl_hint     ), priority=6    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)

+  table=6 (ls_in_acl_hint     ), priority=7    , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)

+  table=7 (ls_in_acl          ), priority=1    , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)

+  table=7 (ls_in_acl          ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)

+  table=7 (ls_in_acl          ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)

+  table=7 (ls_in_acl          ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)

+])

+

+AS_BOX([Check match ct_state with load balancer])

+check ovn-nbctl --wait=sb \

+    -- acl-del ls from-lport 2 "udp" \

+    -- acl-del ls to-lport 2 "udp" \

+    -- lb-add lb "10.0.0.1" "10.0.0.2" \

+    -- ls-lb-add ls lb

+

+AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e ls_in_acl -e ls_out_acl | grep 'ct\.' | sort], [0], [dnl

+  table=4 (ls_out_acl_hint    ), priority=1    , match=(ct.est && ct_label.blocked == 0), action=(reg0[[10]] = 1; next;)

+  table=4 (ls_out_acl_hint    ), priority=2    , match=(ct.est && ct_label.blocked == 1), action=(reg0[[9]] = 1; next;)

+  table=4 (ls_out_acl_hint    ), priority=3    , match=(!ct.est), action=(reg0[[9]] = 1; next;)

+  table=4 (ls_out_acl_hint    ), priority=4    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] = 1; next;)

+  table=4 (ls_out_acl_hint    ), priority=5    , match=(!ct.trk), action=(reg0[[8]] = 1; reg0[[9]] = 1; next;)

+  table=4 (ls_out_acl_hint    ), priority=6    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)

+  table=4 (ls_out_acl_hint    ), priority=7    , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)

+  table=5 (ls_out_acl         ), priority=1    , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)

+  table=5 (ls_out_acl         ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)

+  table=5 (ls_out_acl         ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)

+  table=5 (ls_out_acl         ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)

+  table=6 (ls_in_acl_hint     ), priority=1    , match=(ct.est && ct_label.blocked == 0), action=(reg0[[10]] = 1; next;)

+  table=6 (ls_in_acl_hint     ), priority=2    , match=(ct.est && ct_label.blocked == 1), action=(reg0[[9]] = 1; next;)

+  table=6 (ls_in_acl_hint     ), priority=3    , match=(!ct.est), action=(reg0[[9]] = 1; next;)

+  table=6 (ls_in_acl_hint     ), priority=4    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] = 1; next;)

+  table=6 (ls_in_acl_hint     ), priority=5    , match=(!ct.trk), action=(reg0[[8]] = 1; reg0[[9]] = 1; next;)

+  table=6 (ls_in_acl_hint     ), priority=6    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)

+  table=6 (ls_in_acl_hint     ), priority=7    , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)

+  table=7 (ls_in_acl          ), priority=1    , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)

+  table=7 (ls_in_acl          ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)

+  table=7 (ls_in_acl          ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)

+  table=7 (ls_in_acl          ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)

+])

+

+AT_CLEANUP

+

 AT_SETUP([datapath requested-tnl-key])

 AT_KEYWORDS([requested tnl tunnel key keys])

 ovn_start

-- 

2.29.2