On 7/10/26 6:36 PM, Arpit Jain wrote: > Hi Dumitru, > Hi Arpit, > Thanks for review. Part of this patch was indeed AI-assisted (Cursor with > Opus 4.8), so an Assisted-by: tag would be appropriate. > > This is my first OVN patch, and I wasn't aware of the disclosure requirement. > I'll make sure to include the tag in future submissions. >
No worries, thanks for the confirmation. Looking forward to more submissions in the future! > Regards, > Arpit > > From: Dumitru Ceara <[email protected]> > Date: Friday, 10 July 2026 at 1:09 PM > To: Lorenzo Bianconi <[email protected]> > Cc: [email protected] <[email protected]>; Arpit Jain > <[email protected]> > Subject: Re: [ovs-dev] [PATCH ovn v2] northd: Gate priority-1 default-allow > on REG_ACL_TIER for tiered ACLs. > > !-------------------------------------------------------------------| > CAUTION: External Email > > |-------------------------------------------------------------------! > > 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 A few things slipped through my previous review, sorry. There's no --wait=sb sync here so checking the flows below might fail making the test flaky. >>> + >>> +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/"]) >>> + While this shortens the test a bit, I think I'd prefer the explicit checks as we do in other in other tests. >>> +# 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 Nit: the first --wait=sb is not needed. >>> +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) We should use fetch_column for both of these instead. >>> +check ovn-nbctl --wait=sb set ACL $in_uuid tier=1 Nit: the first --wait=sb is not needed. >>> +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 Nit: the first --wait=sb is not needed. >>> +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 >>> To avoid a v3, I could go ahead and squash the following changes in. Just let me know if they look OK to you and I can merge the patch then. Regards, Dumitru diff --git a/northd/northd.c b/northd/northd.c index 0d87d75a43..1cc62eac52 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -8164,14 +8164,15 @@ build_acls(const struct ls_stateful_record *ls_stateful_rec, * uses "next;". * * When tiered ACLs are configured, this default-allow must only - * fire after the tier loop has reached the highest configured + * be hit 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. */ + * so the rule can only be matched once every tier has been + * evaluated. */ ds_clear(&match); - ds_put_format(&match, "ip && ct.est && ct_mark.blocked == 1"); + ds_put_cstr(&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); @@ -8182,7 +8183,7 @@ build_acls(const struct ls_stateful_record *ls_stateful_rec, lflow_ref); ds_clear(&match); - ds_put_format(&match, "ip && ct.est && ct_mark.blocked == 1"); + ds_put_cstr(&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); diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 4a0a3c5ef2..4fb9a11146 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -11632,64 +11632,59 @@ 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/"]) +check ovn-nbctl --wait=sb ls-add ls -- lsp-add ls lsp # 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], []) +AT_CHECK([ovn-sbctl lflow-list | grep 'ls_in_acl_eval' | grep -q 'ip && ct.est && ct_mark.blocked == 1'], [1]) +AT_CHECK([ovn-sbctl lflow-list | grep 'ls_out_acl_eval' | grep -q 'ip && ct.est && ct_mark.blocked == 1'], [1]) # 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 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([ovn-sbctl lflow-list | grep 'ls_in_acl_eval' | grep 'ip && ct.est && ct_mark.blocked == 1' | ovn_strip_lflows], [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;) +AT_CHECK([ovn-sbctl lflow-list | grep 'ls_out_acl_eval' | grep 'ip && ct.est && ct_mark.blocked == 1' | ovn_strip_lflows], [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 +# by REG_ACL_TIER == 1 so it only matches after the tier loop completes. +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([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([ovn-sbctl lflow-list | grep 'ls_in_acl_eval' | grep 'ip && ct.est && ct_mark.blocked == 1' | ovn_strip_lflows], [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;) +AT_CHECK([ovn-sbctl lflow-list | grep 'ls_out_acl_eval' | grep 'ip && ct.est && ct_mark.blocked == 1' | ovn_strip_lflows], [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([ovn-sbctl lflow-list | grep 'ls_in_acl_eval' | grep 'ip && ct.est && ct_mark.blocked == 1' | ovn_strip_lflows], [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;) +AT_CHECK([ovn-sbctl lflow-list | grep 'ls_out_acl_eval' | grep 'ip && ct.est && ct_mark.blocked == 1' | ovn_strip_lflows], [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 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([ovn-sbctl lflow-list | grep 'ls_in_acl_eval' | grep 'ip && ct.est && ct_mark.blocked == 1' | ovn_strip_lflows], [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;) +AT_CHECK([ovn-sbctl lflow-list | grep 'ls_out_acl_eval' | grep 'ip && ct.est && ct_mark.blocked == 1' | ovn_strip_lflows], [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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
