On 7/21/26 9:47 AM, Arpit Jain wrote:
> 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]>
Hi Arpit,
Thanks for the v3!
It seems you missed the Assisted-by tag here too though. I guess it
would be:
Assisted-by: Claude Opus 4.8, Cursor
> ---
> v3: Address review comments (test-only; no change to northd.c):
> - add an explicit "ovn-nbctl --wait=sb sync" before the baseline
> flow checks to avoid racing ovn-northd (flaky test);
> - drop the local UNBLOCK_FLOW macro in favour of explicit
> lflow-list checks, as done in other tests;
> - remove the redundant --wait=sb from the first of each paired
> ovn-nbctl command;
> - use fetch_column to look up the ACL UUIDs.
> v2: Fix the expected logical flow output in the new ovn-northd test
> (drop the stage-name column padding); 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
You missed this from my v2 review, I suggested rephrasing this to
s/fire/be hit/.
> + * 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. */
s/only competes/only matches/
> 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");
You missed this from my v2 review, it should be ds_put_cstr().
> + 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..c0903e001 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -11449,6 +11449,79 @@ OVN_CLEANUP_NORTHD
> AT_CLEANUP
> ])
>
Missing OVN_FOR_EACH_NORTHD_NO_HV().
> +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
Same here, s/fire/be hit/.
> +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.
> +dnl
> +dnl The trailing "sed" strips the stage-name column padding that
> +dnl lflow-list adds, which otherwise varies with the set of stages shown.
We don't do that for other tests, let's be consistent instead.
> +
> +ovn_start
> +
> +check ovn-nbctl ls-add ls
> +check ovn-nbctl lsp-add ls lsp
> +check ovn-nbctl --wait=sb sync
> +
> +# Baseline: no ACLs => no stateful flows at all in acl_eval, so no
> +# priority-1 default-allow either.
> +AT_CHECK([ovn-sbctl lflow-list ls | grep -w ls_in_acl_eval | grep 'ip &&
> ct.est && ct_mark.blocked == 1' | ovn_strip_lflows | sed 's/ *)/)/'], [0], [])
> +AT_CHECK([ovn-sbctl lflow-list ls | grep -w ls_out_acl_eval | grep 'ip &&
> ct.est && ct_mark.blocked == 1' | ovn_strip_lflows | sed 's/ *)/)/'], [0], [])
> +
> +# Add a stateful, untiered ACL on each direction. The priority-1
> +# default-allow appears with no tier guard.
> +check ovn-nbctl 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([ovn-sbctl lflow-list ls | grep -w ls_in_acl_eval | grep 'ip &&
> ct.est && ct_mark.blocked == 1' | ovn_strip_lflows | sed 's/ *)/)/'], [0],
> [dnl
> + table=??(ls_in_acl_eval), priority=1 , match=(ip && ct.est &&
> ct_mark.blocked == 1), action=(reg8[[16]] = 1; next;)
> +])
> +AT_CHECK([ovn-sbctl lflow-list ls | grep -w ls_out_acl_eval | grep 'ip &&
> ct.est && ct_mark.blocked == 1' | ovn_strip_lflows | sed 's/ *)/)/'], [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.
s/fires/matches/
> +in_uuid=$(fetch_column nb:ACL _uuid direction=from-lport)
> +out_uuid=$(fetch_column nb:ACL _uuid direction=to-lport)
> +check ovn-nbctl set ACL $in_uuid tier=1
> +check ovn-nbctl --wait=sb set ACL $out_uuid tier=1
> +
> +AT_CHECK([ovn-sbctl lflow-list ls | grep -w ls_in_acl_eval | grep 'ip &&
> ct.est && ct_mark.blocked == 1' | ovn_strip_lflows | sed 's/ *)/)/'], [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([ovn-sbctl lflow-list ls | grep -w ls_out_acl_eval | grep 'ip &&
> ct.est && ct_mark.blocked == 1' | ovn_strip_lflows | sed 's/ *)/)/'], [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([ovn-sbctl lflow-list ls | grep -w ls_in_acl_eval | grep 'ip &&
> ct.est && ct_mark.blocked == 1' | ovn_strip_lflows | sed 's/ *)/)/'], [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([ovn-sbctl lflow-list ls | grep -w ls_out_acl_eval | grep 'ip &&
> ct.est && ct_mark.blocked == 1' | ovn_strip_lflows | sed 's/ *)/)/'], [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 set ACL $in_uuid tier=0
> +check ovn-nbctl --wait=sb set ACL $out_uuid tier=0
> +
> +AT_CHECK([ovn-sbctl lflow-list ls | grep -w ls_in_acl_eval | grep 'ip &&
> ct.est && ct_mark.blocked == 1' | ovn_strip_lflows | sed 's/ *)/)/'], [0],
> [dnl
> + table=??(ls_in_acl_eval), priority=1 , match=(ip && ct.est &&
> ct_mark.blocked == 1), action=(reg8[[16]] = 1; next;)
> +])
> +AT_CHECK([ovn-sbctl lflow-list ls | grep -w ls_out_acl_eval | grep 'ip &&
> ct.est && ct_mark.blocked == 1' | ovn_strip_lflows | sed 's/ *)/)/'], [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])
I took care of the comments I had above (I hope that's fine) and pushed
the patch to main and 26.03. I also added you to AUTHORS.rst.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
