For stateful ACLs, ovn-northd emits a priority-1 flow in the
ls_in_acl_eval / ls_out_acl_eval stages that re-allows an established
connection whose ct_mark.blocked bit is set:

  match=(ip && ct.est && ct_mark.blocked == 1)
  action=(REGBIT_ACL_VERDICT_ALLOW = 1; next;)

ct_mark.blocked is set when an established connection is denied by an
ACL.  This flow lets the connection resume, without being reset, once a
"drop"/"reject" ACL is replaced by an "allow"/"allow-related" ACL; the
connection is re-committed with ct_mark.blocked cleared in a later
stage.

The flow is emitted with no REG_ACL_TIER match.  When ACLs are assigned
to tiers, ACL evaluation re-circulates through the same eval stage with
REG_ACL_TIER (reg8[30..31]) advancing from 0 up to the highest tier
configured on the switch: tier 0 is evaluated first and the next tier
is only reached while no verdict has been determined.  Since the
priority-1 flow carries no tier match, it is hit on the first pass at
tier 0, where it sets the allow verdict and runs "next;", ending
evaluation - so an ACL at a higher tier is never reached.

For example, on a logical switch using tiered ACLs:

  - an "allow-related" ACL establishes a TCP connection from a client;
  - that ACL is later removed and a tier-1 ACL is added that drops the
    same traffic (raising the switch's highest ingress tier to 1).

The first packet after the change hits the tier-1 drop, which sets
ct_mark.blocked=1.  On the next packet the priority-1 flow matches at
tier 0, allows it and ends evaluation, so the tier-1 drop ACL is never
reached and the packet leaks; ct_mark.blocked is then cleared, the
following packet is dropped again and re-sets ct_mark.blocked, and the
cycle repeats - the connection alternates between leaking and being
dropped instead of being cleanly blocked.

When no ACL is assigned a tier there is no eval loop, and a configured
"drop"/"reject" ACL is emitted at acl->priority + OVN_ACL_PRI_OFFSET,
far above priority 1, so it always wins over the priority-1 flow and
this case is unaffected.

Gate the priority-1 flow on REG_ACL_TIER == highest-configured-tier for
the relevant stage (ingress pre-LB / egress) so that it only competes
once every tier has been evaluated.  When no tier is configured the
match is left unchanged, so the emitted flows and behaviour are
identical for existing setups.

Signed-off-by: Arpit Jain <[email protected]>
Acked-by: Naveen Yerramneni <[email protected]>
---
v2: Fix the expected logical flow output in the new ovn-northd
    test by dropping the stage-name column padding; that padding
    is normalized away by the test's filter, so v1 failed CI.
    No change to northd.c.

 northd/northd.c     | 21 ++++++++++++-
 tests/ovn-northd.at | 73 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+), 1 deletion(-)

diff --git a/northd/northd.c b/northd/northd.c
index 7a7866d9a..0e791007b 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -8116,13 +8116,32 @@ build_acls(const struct ls_stateful_record 
*ls_stateful_rec,
          * We need to set ct_mark.blocked=0 to let the connection continue,
          * which will be done by ct_commit in the "stateful" stage.
          * Subsequent packets will hit the flow at priority 0 that just
-         * uses "next;". */
+         * uses "next;".
+         *
+         * When tiered ACLs are configured, this default-allow must only
+         * fire after the tier loop has reached the highest configured
+         * tier; otherwise a drop ACL at a higher tier would be bypassed
+         * for an already-blocked connection (it would short-circuit on
+         * the first tier-0 visit, set VERDICT_ALLOW, and skip the rest
+         * of the tier loop).  Gate the match on REG_ACL_TIER == max_tier
+         * so the rule only competes once every tier has been evaluated. */
         ds_clear(&match);
         ds_put_format(&match, "ip && ct.est && ct_mark.blocked == 1");
+        if (ls_stateful_rec->max_acl_tier.ingress_pre_lb) {
+            ds_put_format(&match, " && " REG_ACL_TIER " == %"PRIu64,
+                          ls_stateful_rec->max_acl_tier.ingress_pre_lb);
+        }
         ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL_EVAL, 1,
                       ds_cstr(&match),
                       REGBIT_ACL_VERDICT_ALLOW" = 1; next;",
                       lflow_ref);
+
+        ds_clear(&match);
+        ds_put_format(&match, "ip && ct.est && ct_mark.blocked == 1");
+        if (ls_stateful_rec->max_acl_tier.egress) {
+            ds_put_format(&match, " && " REG_ACL_TIER " == %"PRIu64,
+                          ls_stateful_rec->max_acl_tier.egress);
+        }
         ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL_EVAL, 1,
                       ds_cstr(&match),
                       REGBIT_ACL_VERDICT_ALLOW" = 1; next;",
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 6c84311ed..a56be2a03 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -11449,6 +11449,79 @@ OVN_CLEANUP_NORTHD
 AT_CLEANUP
 ])
 
+AT_SETUP([Tiered ACL default-allow fallback is gated by tier])
+AT_KEYWORDS([acl])
+
+dnl When tiered ACLs are in use, the priority-1 default-allow flow that
+dnl unblocks previously-blocked connections must not fire until the
+dnl tier loop reaches the highest configured tier; otherwise a drop ACL
+dnl at a higher tier would be bypassed for an already-blocked connection.
+
+ovn_start
+
+check ovn-nbctl ls-add ls
+check ovn-nbctl lsp-add ls lsp
+
+m4_define([UNBLOCK_FLOW], [ovn-sbctl lflow-list ls dnl
+  | grep -w $1 | grep 'ip && ct.est && ct_mark.blocked == 1' dnl
+  | ovn_strip_lflows | sed "s/\($1[[^)]]*\)/$1/"])
+
+# Baseline: no ACLs => no stateful flows at all in acl_eval, so no
+# priority-1 default-allow either.
+AT_CHECK([UNBLOCK_FLOW([ls_in_acl_eval])], [0], [])
+AT_CHECK([UNBLOCK_FLOW([ls_out_acl_eval])], [0], [])
+
+# Add a stateful, untiered ACL on each direction. The priority-1
+# default-allow appears with no tier guard.
+check ovn-nbctl --wait=sb acl-add ls from-lport 1000 "tcp" allow-related
+check ovn-nbctl --wait=sb acl-add ls to-lport   1000 "tcp" allow-related
+
+AT_CHECK([UNBLOCK_FLOW([ls_in_acl_eval])], [0], [dnl
+  table=??(ls_in_acl_eval), priority=1    , match=(ip && ct.est && 
ct_mark.blocked == 1), action=(reg8[[16]] = 1; next;)
+])
+AT_CHECK([UNBLOCK_FLOW([ls_out_acl_eval])], [0], [dnl
+  table=??(ls_out_acl_eval), priority=1    , match=(ip && ct.est && 
ct_mark.blocked == 1), action=(reg8[[16]] = 1; next;)
+])
+
+# Move both ACLs to tier 1. Each fallback flow should now be guarded
+# by REG_ACL_TIER == 1 so it only fires after the tier loop completes.
+in_uuid=$(ovn-nbctl --bare --columns _uuid find ACL direction=from-lport)
+out_uuid=$(ovn-nbctl --bare --columns _uuid find ACL direction=to-lport)
+check ovn-nbctl --wait=sb set ACL $in_uuid tier=1
+check ovn-nbctl --wait=sb set ACL $out_uuid tier=1
+
+AT_CHECK([UNBLOCK_FLOW([ls_in_acl_eval])], [0], [dnl
+  table=??(ls_in_acl_eval), priority=1    , match=(ip && ct.est && 
ct_mark.blocked == 1 && reg8[[30..31]] == 1), action=(reg8[[16]] = 1; next;)
+])
+AT_CHECK([UNBLOCK_FLOW([ls_out_acl_eval])], [0], [dnl
+  table=??(ls_out_acl_eval), priority=1    , match=(ip && ct.est && 
ct_mark.blocked == 1 && reg8[[30..31]] == 1), action=(reg8[[16]] = 1; next;)
+])
+
+# Move the ingress ACL to tier 3 while the egress ACL stays at tier 1.
+# The two stages should pick up their per-stage max independently.
+check ovn-nbctl --wait=sb set ACL $in_uuid tier=3
+
+AT_CHECK([UNBLOCK_FLOW([ls_in_acl_eval])], [0], [dnl
+  table=??(ls_in_acl_eval), priority=1    , match=(ip && ct.est && 
ct_mark.blocked == 1 && reg8[[30..31]] == 3), action=(reg8[[16]] = 1; next;)
+])
+AT_CHECK([UNBLOCK_FLOW([ls_out_acl_eval])], [0], [dnl
+  table=??(ls_out_acl_eval), priority=1    , match=(ip && ct.est && 
ct_mark.blocked == 1 && reg8[[30..31]] == 1), action=(reg8[[16]] = 1; next;)
+])
+
+# Drop both ACLs back to tier 0; the tier guard should disappear.
+check ovn-nbctl --wait=sb set ACL $in_uuid tier=0
+check ovn-nbctl --wait=sb set ACL $out_uuid tier=0
+
+AT_CHECK([UNBLOCK_FLOW([ls_in_acl_eval])], [0], [dnl
+  table=??(ls_in_acl_eval), priority=1    , match=(ip && ct.est && 
ct_mark.blocked == 1), action=(reg8[[16]] = 1; next;)
+])
+AT_CHECK([UNBLOCK_FLOW([ls_out_acl_eval])], [0], [dnl
+  table=??(ls_out_acl_eval), priority=1    , match=(ip && ct.est && 
ct_mark.blocked == 1), action=(reg8[[16]] = 1; next;)
+])
+
+OVN_CLEANUP_NORTHD
+AT_CLEANUP
+
 OVN_FOR_EACH_NORTHD_NO_HV([
 AT_SETUP([ACL "pass" logical flows])
 AT_KEYWORDS([acl])
-- 
2.39.3

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to