On 7/3/26 5:57 PM, Lorenzo Bianconi via dev 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]>
>
> Acked-by: Lorenzo Bianconi <[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.
Hi Arpit, Lorenzo,
Thanks for the patch and review!
Arpit, just double checking, were some of the contents of this patch
written with AI assistance? That's no problem, we accept AI assisted
contributions, but that should be disclosed with an "Assisted-by:" tag.
It's also possible that I'm wrong and this is completely written without
AI assistance.
In any case, the change looks OK to me. I have a minor comment below
but I can address that when merging the patch.
I'll first wait for confirmation from you to see if I also need to
squash an "Assisted-by" tag into the commit message.
Regards,
Dumitru
>>
>> 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");
While at it, we should also change this to ds_put_cstr().
>> + if (ls_stateful_rec->max_acl_tier.ingress_pre_lb) {
It's annoying that this is conditional, REG_ACL_TIER == 0 should be just
fine if there's no other tiers. However, the place where we initialize
REG_ACL_TIER = 0 is also conditional (already before this patch) so we'd
have to change that too.
It's outside the scope of this patch so let's leave it as is for now.
I'll try to remember to follow up on this in the future.
>> + 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");
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..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
>>
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
