[vpp][acl] match on ingress interface to support SAI ACL IN_PORTS by lolyu · Pull Request #278 · sonic-net/sonic-platform-vpp

lolyu · 2026-08-29T08:14:41Z

Why I did it

SAI models the dual-ToR mux drop as one ACL table bound to every port, with SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS naming only the standby ports.
VPP ACL rules had no ingress-interface qualifier, so the VPP SAI layer had to drop IN_PORTS silently. The deny then applied to every bound port instead of just the ones the entry named, so traffic was dropped on active ports too.

How I did it

Add patch 0018-acl-match-on-ingress-interface.patch, which adds an in_sw_if_index field to the VPP ACL rule so a rule can be scoped to one receive interface. The SAI layer can then expand one ACE into one rule per named port:

0: ipv4 deny src 0.0.0.0/0 dst 0.0.0.0/0 proto 0 ... in-port bobm1
1: ipv4 deny src 0.0.0.0/0 dst 0.0.0.0/0 proto 0 ... in-port bobm10

The match is close to free. fa_5tuple_t already carries the receive interface in l4.lsb_of_sw_if_index, inside the hashed part of the key, so a scoped rule only has to stop masking that field:

in_sw_if_index == 0 means "any interface", the same convention proto = 0 already uses for L4. sw_if_index 0 is local0 and never a data-plane port, so no separate valid flag is needed. Only the low 16 bits are matched, since that is what the packet-side field carries.

All three match paths check the interface, not just the hash mask:

single_acl_match_5tuple() — non-first fragments bypass the hash and fall back to linear matching, so without this a scoped rule would match fragments received on any interface.
single_rule_match_5tuple() — the hash collision check, which must not assume the interface is in the key. tuplemerge reuses any existing less-specific mask type, so an ACL carrying a wildcard rule applied to the same lookup context ahead of a scoped one can fold the scoped rule into a mask that leaves lsb_of_sw_if_index masked off. Checking here keeps correctness independent of ACL bind order.

Also wires the field through the API type, the API/rule conversion, the in-port CLI parser and the ACL display.

Signed-off-by: Longxiang Lyu lolv@microsoft.com

SAI models the dual-ToR mux drop as one ACL table bound to every port, with SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS naming only the standby ports. VPP ACL rules had no ingress-interface qualifier, so the VPP SAI layer had to drop IN_PORTS silently and the deny ended up applying to every bound port instead of just the ones the entry named. Add an in_sw_if_index field to the ACL rule so a single rule can be scoped to one receive interface, letting the SAI layer expand one ACE into one rule per named port: 0: ipv4 deny src 0.0.0.0/0 dst 0.0.0.0/0 proto 0 ... in-port bobm1 1: ipv4 deny src 0.0.0.0/0 dst 0.0.0.0/0 proto 0 ... in-port bobm10 The match itself is close to free. fa_5tuple_t already carries the receive interface in l4.lsb_of_sw_if_index, inside the hashed part of the key, so a scoped rule only has to stop masking that field: mask->l4.lsb_of_sw_if_index = 0xffff; hi->match.l4.lsb_of_sw_if_index = r->in_sw_if_index & 0xffff; in_sw_if_index 0 means "any interface", the same convention proto = 0 already uses for L4. sw_if_index 0 is local0 and never a data-plane port, so no separate valid flag is needed. Only the low 16 bits are matched, since that is what the packet-side field carries. All three match paths check the interface, not just the hash mask: - single_acl_match_5tuple(): non-first fragments bypass the hash and fall back to linear matching, so without this a scoped rule would match fragments received on any interface. - single_rule_match_5tuple(): this is the hash collision check, and it must not assume the interface is in the key. tuplemerge reuses any existing less-specific mask type, so an ACL carrying a wildcard rule applied to the same lookup context ahead of a scoped one can fold the scoped rule into a mask that leaves lsb_of_sw_if_index masked off. Checking here keeps correctness independent of ACL bind order. Also wire the field through the API type, the API/rule conversion, the "in-port" CLI parser and the ACL display. Note the API wire type changes, so the acl_add_replace CRC changes and VPP and sonic-sairedis must be rebuilt together. rules/vpp.mk: VPP_VERSION 2606-0.5 -> 2606-0.6, since the patch series changed and the version is the buildkite deb cache key. Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

azure-pipelines · 2026-08-29T08:14:49Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

mssonicbld · 2026-08-29T08:14:59Z

/azp run

azure-pipelines · 2026-08-29T08:15:09Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

Pull request overview

This PR extends the VPP ACL plugin to allow ACL rules to match on the ingress interface (in_sw_if_index), enabling the SONiC SAI layer to correctly implement SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS by scoping denies to specific standby ports rather than all ports an ACL is bound to.

Changes:

Add a new VPP patch that wires in_sw_if_index through ACL rule structures, API types, CLI parsing, rule display, and both hash + linear match paths.
Register the new patch in the VPP patch series used by this platform build.
Bump the VPP package version suffix to avoid downstream consumers pulling stale prebuilt debs.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
`vppbld/patches/series`	Adds the new ACL ingress-interface matching patch to the VPP patch series.
`vppbld/patches/0018-acl-match-on-ingress-interface.patch`	Implements `in_sw_if_index` support in VPP ACL rule API/types, CLI, printing, and match logic.
`rules/vpp.mk`	Bumps `VPP_VERSION` minor suffix to ensure updated deb artifacts are fetched.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

mssonicbld · 2026-08-29T08:55:37Z

/azp run

azure-pipelines · 2026-08-29T08:55:46Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

mssonicbld · 2026-08-29T09:03:28Z

/azp run

azure-pipelines · 2026-08-29T09:03:37Z

Azure Pipelines: Successfully started running 1 pipeline(s).

The 5-tuple carries the ingress interface in lsb_of_sw_if_index, a u16 packed into the L4 key, so only the low 16 bits of in_sw_if_index take part in the match. An index above 0xffff would therefore be programmed as a different interface than the one named, and both the CLI output and the API dump would keep reporting the interface the user asked for, giving no indication that the rule matches somewhere else. Reject such an index in acl_add_list() instead, next to the existing prefix and port range checks, so it fails at configuration time on both the binary API and the CLI path rather than silently mismatching. This is a limit of the key layout rather than of this validation: widening the field would mean growing fa_session_l4_key_t beyond the u64 it packs into. The interface indices SONiC creates stay far below 0xffff, so the check is not expected to fire in practice. Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

mssonicbld · 2026-08-29T09:05:11Z

/azp run

azure-pipelines · 2026-08-29T09:05:23Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

…RTS branches Build a test image carrying the SAI ACL IN_PORTS support so it can be deployed to a dual-ToR VPP testbed. The two changes are a matched pair and cannot be tested apart. Adding in_sw_if_index to the VPP acl_rule type changes the layout of the acl_add_replace message and therefore its CRC, so a syncd built against one side cannot program ACLs on the other. platform/vpp -> lolyu/acl_match_in_port sonic-net/sonic-platform-vpp#278 src/sonic-sairedis -> lolyu/vpp_acl_in_port_match sonic-net/sonic-sairedis#2064 vpp.mk carries VPP_VERSION 2606-0.6 for the new patch series. That deb is not published, so the download step fails and vppbld/Makefile falls back to build_locally, which applies the patch series and builds VPP from source. The image build therefore also verifies that the patch applies and compiles under the real build flags. Signed-off-by: Longxiang Lyu <lolv@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

mssonicbld · 2026-09-01T12:38:38Z

/azp run

azure-pipelines · 2026-09-01T12:38:48Z

Azure Pipelines: Successfully started running 1 pipeline(s).

…RTS branches Build a test image carrying the SAI ACL IN_PORTS support so it can be deployed to a dual-ToR VPP testbed. The two changes are a matched pair and cannot be tested apart. Adding in_sw_if_index to the VPP acl_rule type changes the layout of the acl_add_replace message and therefore its CRC, so a syncd built against one side cannot program ACLs on the other. platform/vpp -> lolyu/acl_match_in_port sonic-net/sonic-platform-vpp#278 src/sonic-sairedis -> lolyu/vpp_acl_in_port_match sonic-net/sonic-sairedis#2064 vpp.mk carries VPP_VERSION 2606-0.6 for the new patch series. That deb is not published, so the download step fails and vppbld/Makefile falls back to build_locally, which applies the patch series and builds VPP from source. The image build therefore also verifies that the patch applies and compiles under the real build flags. Signed-off-by: Longxiang Lyu <lolv@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

lolyu · 2026-09-01T14:12:56Z

Split the RX_DRP / policy-drop change out of this PR into #280, since the two are independent (they touch disjoint files) and the counter fix shouldn't have to wait on the IN_PORTS review.

This branch is now just the IN_PORTS work, and it keeps patch number 0018 — the next free slot after master's 0017. #280 has been renumbered to 0019, so the two occupy distinct slots and neither leaves a gap. The patch contents are unchanged in both.

Verified with git apply against pinned VPP commit 3f9e978d that the series applies cleanly for this branch alone, for #280 alone, and with both patches applied together.

Note for whoever merges second: both this PR and #280 set VPP_VERSION to 2606-0.6. Because the edit is identical on both sides, git auto-resolves rules/vpp.mk without reporting a conflict — only vppbld/patches/series will conflict. Please bump VPP_VERSION to 2606-0.7 by hand as part of resolving that conflict, so the published deb version reflects both patches (per the cache-key warning in rules/vpp.mk).

mssonicbld · 2026-09-01T14:13:52Z

/azp run

azure-pipelines · 2026-09-01T14:14:02Z

Azure Pipelines: Successfully started running 1 pipeline(s).

mssonicbld · 2026-09-01T14:35:10Z

/azp run

azure-pipelines · 2026-09-01T14:35:21Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

🔵 Needs a closer look

It changes core VPP ACL matching behavior and API surface, which is high-impact and should be validated with targeted functional/regression testing in the full SONiC-VPP datapath.

Review details

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Lite

mssonicbld · 2026-09-03T04:18:31Z

/azp run

azure-pipelines · 2026-09-03T04:18:41Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

🔵 Needs a closer look

It changes VPP dataplane ACL matching semantics and the associated binary API surface, which is difficult to fully validate for correctness/regression risk without targeted runtime testing.

Review details

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Lite

An ACL deny is an intentional forwarding decision, not an interface discard, but today the deny path sends the buffer to error-drop, which charges it to the ingress interface's drop counter (/if/drops). SONiC exports that counter as SAI_PORT_STAT_IF_IN_DISCARDS and displays it as RX_DRP, so a port doing exactly what it was configured to do reports a large and growing discard count. This is most visible on a dual-ToR standby mux port, where the standby ToR is *supposed* to drop the traffic it receives from the server: the port reports a ~99% RX_DRP rate while forwarding correctly. RFC 1213 defines ifInDiscards as packets discarded despite no error being detected, to free buffer space - i.e. resource exhaustion, not policy. SONiC itself already treats ACL drops as RX_ERR rather than RX_DRP. Add VNET_BUFFER_F_POLICY_DROP (bit 19, the first free bit) and set it at the ACL deny choke point in acl_fa_inner_node_fn(). interface_drop_punt() then discounts flagged buffers from the interface drop counter while still counting the per-node/per-ACE error, so denies remain fully observable via "show errors" and "show acl-plugin acl". The flag is set only for ACL_FA_ERROR_ACL_DROP. Drops caused by a lack of resources, such as ACL_FA_ERROR_ACL_TOO_MANY_SESSIONS, are genuine discards and continue to increment the interface counter. Verified on a KVM dual-ToR testbed running this patch: the standby ToR received 5762 packets on the mux port with RX_DRP=0 (previously RX_DRP tracked the full received count), while "show errors" still reported the denies against acl-plugin-in-ip4-l2. This makes dualtor_io/test_normal_op.py::test_upstream_standby_rx_drop_check pass. Bump VPP_VERSION to 2606-0.7. The suffix is the cache key vppbld/Makefile uses to fetch pre-built debs, so it has to move whenever the patch series changes content. 0.6 is already published by patch 0018, which merged as PR sonic-net#278 while this one was open; leaving it at 0.6 would make downstream sonic-buildimage builds pull the stale 0.6 debs that pre-date this patch. Note that git does not flag this: both branches bumped 0.5 to 0.6 independently, so the identical edits merge without a conflict. Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

) Approach What is the motivation for this PR? Fixes: sonic-net/sonic-buildimage#29077 IN_PORTS is how orchagent scopes an ACL entry to a subset of the ports its table is bound to. aclorch restricts it to physical interfaces, and the dual ToR standby DENY entries rely on it. Silently widening those entries to the whole table is worse than not supporting the field at all, because nothing in the logs distinguishes a rule that was scoped from one that was not. Work item tracking Microsoft ADO (number only): 39466368 How did you do it? A VPP ACL rule matches one interface, so a SAI entry naming N ports becomes N VPP rules: acl_entry_in_ports_get() finds the attribute and resolves it. The cached attribute only carries the list count, its list pointer is still null, so the entry is read again with a buffer of the right size. Each port OID is then resolved to a VPP interface name via vpp_get_hwif_name(). fill_acl_rules() pops the rules it just generated for the entry and re-adds one copy per resolved interface. This is the same shape as the existing fan-out where a port based entry with no protocol becomes a UDP rule and a TCP rule. vpp_acl_rule_t gains in_hwif_name. The SAI layer keeps working in interface names, as it already does for binding, and vpp_acl_add_replace() resolves the name to a sw_if_index when it builds the API message. Empty means any. SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS becomes an explicit no-op in acl_rule_field_update(), since it is not a per rule field. The fanned out rules stay contiguous from ace.vpp_rule_base_index, so num_rules still describes the entry and the counter aggregation in getAclEntryStats() sums over them unchanged. Failure handling is deliberate, since three different things can leave the interface set empty and they do not mean the same thing: Case Behaviour Field absent or not enabled Not scoped, one rule as before Enabled but names no port Scoped to nothing, no rule emitted Port list unreadable, or no port resolves Error, ACL programming fails The last row matters. On failure the scope of the entry is simply unknown, and installing it unscoped would re-introduce exactly the blackhole this PR is fixing, so fill_acl_rules() fails and CHECK_STATUS_ACLTBLCONFIG leaves the previous ACL in place. The empty list case is left as "matches nothing" on purpose: no ingress interface can be a member of an empty list, so emitting no rule is the faithful translation. How did you verify/test it? Built into docker-syncd-vpp and deployed to a vms-kvm-dual-vpp-t0-1 dual ToR testbed, then checked the programmed rules against the mux state: vlab-vpp-03: 18 standby ports, 18 matching ingress scoped deny rules. vlab-vpp-04: 6 standby ports, 6 matching ingress scoped deny rules. ACL 3 stayed bound to all 32 interfaces in both cases, confirming the scoping comes from the rules and not from the binding. Rule counts tracking the standby set on two ToRs with different mux distributions is the behaviour this PR is after, and is what regressed before. Still outstanding: live downstream forwarding under mux toggles (standby → active → standby) has not been run yet, so the dataplane effect is verified by rule inspection rather than by packets. Any platform specific information? VPP only. This depends on sonic-net/sonic-platform-vpp#278 and the two have to merge together. Adding the field changes the acl_add_replace message, and therefore its CRC, so a syncd built against one VPP version cannot program ACLs on the other. That PR also bumps VPP_VERSION to 2606-0.6. A scale note for reviewers: an entry naming N ports costs N rules, so a table bound to many mux ports grows proportionally. VPP's tuplemerge can additionally fold the fanned out copies into one less specific mask type, and split_partition() has no interface dimension to split them back apart on, so they can end up on a single collision chain. Correctness is unaffected, single_rule_match_5tuple() in the platform PR re-checks the interface on the collision path, but it is worth knowing before scaling the port count up. Documentation Design is written up in sonic-net/SONiC#2506.

lolyu requested review from yue-fred-gao and a lite review from Copilot August 29, 2026 08:14

Copilot started reviewing on behalf of lolyu August 29, 2026 08:15 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Comment thread vppbld/patches/0018-acl-match-on-ingress-interface.patch

lolyu requested a lite review from Copilot August 29, 2026 08:57

Copilot started reviewing on behalf of lolyu August 29, 2026 08:57 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Comment thread vppbld/patches/0018-acl-match-on-ingress-interface.patch Outdated

lolyu force-pushed the acl_match_in_port branch from 5d2c9c1 to 9a0c22e Compare August 29, 2026 09:03

lolyu force-pushed the acl_match_in_port branch from 9a0c22e to 5bc79f7 Compare August 29, 2026 09:04

lolyu requested a lite review from Copilot August 29, 2026 09:04

Copilot started reviewing on behalf of lolyu August 29, 2026 09:05 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

This was referenced Aug 29, 2026

[vpp][acl] match on ingress interface to support SAI ACL IN_PORTS sonic-net/sonic-sairedis#2064

Merged

[vpp][acl] add vpp acl IN_PORT support HLD sonic-net/SONiC#2506

Merged

[DONT MERGE] Test VPP ACL IN_PORTS support sonic-net/sonic-buildimage#29248

Closed

lolyu requested a lite review from Copilot September 1, 2026 12:51

Copilot started reviewing on behalf of lolyu September 1, 2026 12:51 View session

Copilot AI reviewed Sep 1, 2026

Copilot stopped reviewing on behalf of lolyu due to an error September 1, 2026 12:52
Error performing PR review: HttpError: Bad credentials - https://docs.github.com/rest

lolyu mentioned this pull request Sep 1, 2026

[vpp][acl] do not count policy denies as interface drops #280

Open

lolyu force-pushed the acl_match_in_port branch from 3f6f3f8 to db1c792 Compare September 1, 2026 14:13

lolyu force-pushed the acl_match_in_port branch from db1c792 to 44838ed Compare September 1, 2026 14:34

lolyu requested a lite review from Copilot September 2, 2026 05:36

Copilot started reviewing on behalf of lolyu September 2, 2026 05:36 View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

Comment thread vppbld/patches/series Outdated

yue-fred-gao approved these changes Sep 2, 2026

View reviewed changes

lolyu force-pushed the acl_match_in_port branch from 44838ed to 5bc79f7 Compare September 3, 2026 04:18

lolyu requested a lite review from Copilot September 3, 2026 04:18

Copilot started reviewing on behalf of lolyu September 3, 2026 04:19 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

lolyu merged commit 7fb27b0 into sonic-net:master Sep 3, 2026
5 checks passed

Uh oh!

Conversation

lolyu commented Aug 29, 2026

Why I did it

How I did it

Uh oh!

azure-pipelines Bot commented Aug 29, 2026

Uh oh!

mssonicbld commented Aug 29, 2026

Uh oh!

azure-pipelines Bot commented Aug 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

mssonicbld commented Aug 29, 2026

Uh oh!

azure-pipelines Bot commented Aug 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

mssonicbld commented Aug 29, 2026

Uh oh!

azure-pipelines Bot commented Aug 29, 2026

Uh oh!

mssonicbld commented Aug 29, 2026

Uh oh!

azure-pipelines Bot commented Aug 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mssonicbld commented Sep 1, 2026

Uh oh!

azure-pipelines Bot commented Sep 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

lolyu commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mssonicbld commented Sep 1, 2026

Uh oh!

azure-pipelines Bot commented Sep 1, 2026

Uh oh!

mssonicbld commented Sep 1, 2026

Uh oh!

azure-pipelines Bot commented Sep 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

mssonicbld commented Sep 3, 2026

Uh oh!

azure-pipelines Bot commented Sep 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lolyu commented Sep 1, 2026 •

edited

Loading