vpp: support binding multiple ACL tables by priority by yue-fred-gao · Pull Request #1732 · sonic-net/sonic-sairedis

yue-fred-gao · 2025-12-19T21:43:09Z

why

currently vpp doesn't support binding multiple ACL tables. Each table is appended with default permit-all rules. With multiple tables, this may cause acl matched by such rules and skip the actual rule to make in the tables after this one.

what this PR does

remove the default permit-all rules for each table
If a table is empty, create a dummy rule that won't match any traffic because vpp doesn't allow empty table. The dummy rule matches dest-ip to 0.0.0.0/32
sort all the tables by priority in the table group. vpp doesn't support parallel matching
added catch-all acl group to the end. vpp default behavior of no match is drop but sonic is accept.
Fix sonic-vpp crashing due to race condition during stats pull. If the interface to get stats has been removed, stat_segment_ls_r returns null.

- Added catch-all acl group - Add a dummy rule because vpp doesn't allow empty table Signed-off-by: Yue Gao <[email protected]>

mssonicbld · 2025-12-19T21:43:17Z

/azp run

azure-pipelines · 2025-12-19T21:43:28Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2026-02-05T15:29:10Z

/azp run

azure-pipelines · 2026-02-05T15:29:22Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Yue Gao <[email protected]>

mssonicbld · 2026-02-05T20:57:45Z

/azp run

azure-pipelines · 2026-02-05T20:57:56Z

Azure Pipelines successfully started running 1 pipeline(s).

- This could happen at race condition when the interface is removed during shutdown Signed-off-by: Yue Gao <[email protected]>

This reverts commit 633c0d7. Signed-off-by: Yue Gao <[email protected]>

mssonicbld · 2026-02-10T21:01:43Z

/azp run

azure-pipelines · 2026-02-10T21:01:54Z

Azure Pipelines successfully started running 1 pipeline(s).

dypet

LGTM

AkeelAli

I would just add a comment in the PR description about the changes in SaiVppStats.c.

Copilot

Pull request overview

This PR updates the VPP-backed VS ACL implementation to support binding multiple ACL tables to a port by ordering them via table-group-member priority, while preserving SONiC’s default “permit if no match” behavior. It also hardens VPP stats dumping to avoid crashes/leaks when queried stats paths disappear.

Changes:

Remove per-table default permit-all rules and introduce an “empty table” placeholder ACL rule to satisfy VPP’s non-empty requirement.
Bind multiple ACL tables for a table group by sorting members by priority and appending a shared catch-all default permit ACL at the end.
Fix stats dump error handling/cleanup to avoid races and leaks when VPP stat directory listing returns null/empty.

Reviewed changes

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

File	Description
vslib/vpp/vppxlate/SaiVppStats.c	Frees VPP vectors and disconnects cleanly on null/empty stat listings to avoid crash/leaks.
vslib/vpp/SwitchVppAcl.cpp	Reworks ACL table defaults, introduces empty-table placeholder ACL, sorts/binds group members by priority, adds shared default permit ACL binding.
vslib/vpp/SwitchVpp.h	Adds state for the shared default permit ACL (swindex + created flag) and new helper declarations.

Copilot · 2026-02-12T23:53:20Z

vslib/vpp/SwitchVppAcl.cpp

+    if (get(SAI_OBJECT_TYPE_ACL_TABLE_GROUP, tbl_grp_oid, 1, &attr) != SAI_STATUS_SUCCESS) {
+        SWSS_LOG_INFO("ACL table group %s direction not found",
+                      sai_serialize_object_id(tbl_grp_oid).c_str());
+        return SAI_STATUS_FAILURE;


addRemoveAclGrpMbr fails the whole operation if SAI_ACL_TABLE_GROUP_ATTR_ACL_STAGE is missing. Elsewhere (aclBindUnbindPorts) the code treats a missing stage as a no-op and returns success; consider keeping the behavior consistent to avoid group member add/remove failing due to missing/unset stage metadata.

Suggested change

return SAI_STATUS_FAILURE;

// Missing stage metadata: behave as no-op and return success, consistent with aclBindUnbindPorts

return SAI_STATUS_SUCCESS;

Copilot · 2026-02-12T23:53:20Z

vslib/vpp/SwitchVppAcl.cpp

+            sai_status_t status = aclBindUnbindPort(port_oid, tbl_grp_oid, is_input, false);
+            if (status != SAI_STATUS_SUCCESS) {
+                SWSS_LOG_ERROR("Failed to unbind ACL group %s from port %s",
+                               sai_serialize_object_id(tbl_grp_oid).c_str(),
+                               sai_serialize_object_id(port_oid).c_str());


During step 1 unbind, aclBindUnbindPort mutates m_acl_tbl_grp_ports_map (removes ports) as a side effect. If an unbind fails and this function returns early, the ports map can be left partially modified, desynchronizing internal bookkeeping from VPP state. Consider unbinding without updating the ports map (or rolling back map updates on failure).

Copilot · 2026-02-12T23:53:20Z

vslib/vpp/SwitchVppAcl.cpp

        attr.id = SAI_ACL_TABLE_GROUP_MEMBER_ATTR_ACL_TABLE_ID;
        if (get(SAI_OBJECT_TYPE_ACL_TABLE_GROUP_MEMBER, member_oid, 1, &attr) != SAI_STATUS_SUCCESS) {
-            auto sid = sai_serialize_object_id(member_oid);
-
-            SWSS_LOG_INFO("ACL table oid for acl grp member id %s not found", sid.c_str());
-            continue;
+            SWSS_LOG_INFO("ACL table oid for acl grp member id %s not found",
+                          sai_serialize_object_id(member_oid).c_str());
+            return SAI_STATUS_FAILURE;


If fetching SAI_ACL_TABLE_GROUP_MEMBER_ATTR_ACL_TABLE_ID fails, this now returns SAI_STATUS_FAILURE immediately. Since aclBindUnbindPort already updated the port↔group bookkeeping at the top of the function, this can leave internal state indicating the port is bound/unbound even though no VPP bind/unbind was performed. Consider treating this as a skip (or rolling back the bookkeeping) to avoid desynchronizing state on transient/stale members.

Copilot · 2026-02-12T23:53:20Z

vslib/vpp/SwitchVppAcl.cpp

+            SWSS_LOG_INFO("VS swindex for ACL table oid %s not found",
+                          sai_serialize_object_id(tbl_oid).c_str());
+            return SAI_STATUS_FAILURE;


Similarly, returning failure when the ACL table swindex isn’t found (m_acl_swindex_map.end()) can desynchronize bookkeeping (port already recorded as bound/unbound) and may block binding other valid tables. Consider handling this more gracefully (skip the stale member and/or roll back the port-map change) so a single missing table doesn’t break the entire group bind/unbind.

Suggested change

SWSS_LOG_INFO("VS swindex for ACL table oid %s not found",

sai_serialize_object_id(tbl_oid).c_str());

return SAI_STATUS_FAILURE;

SWSS_LOG_INFO("VS swindex for ACL table oid %s not found, skipping member %s",

sai_serialize_object_id(tbl_oid).c_str(),

sai_serialize_object_id(member_oid).c_str());

continue;

Copilot · 2026-02-12T23:53:21Z

vslib/vpp/SwitchVppAcl.cpp

+    // Create an ACL with 1 rule matching dest IP 0.0.0.0/32 (will never match real traffic) but vpp doesn't allow 0 rule table
+    acl = (vpp_acl_t *) calloc(1, sizeof(vpp_acl_t) + sizeof(vpp_acl_rule_t));


The comment in emptyAclCreate says the dummy rule “will never match real traffic”, but matching dst IP 0.0.0.0/32 can still match packets destined to 0.0.0.0 (e.g., malformed or unexpected traffic). Either adjust the wording (e.g., “unlikely”) or pick a match condition that is guaranteed not to occur in your environment.

why currently vpp doesn't support binding multiple ACL tables. Each table is appended with default permit-all rules. With multiple tables, this may cause acl matched by such rules and skip the actual rule to make in the tables after this one. what this PR does remove the default permit-all rules for each table If a table is empty, create a dummy rule that won't match any traffic because vpp doesn't allow empty table. The dummy rule matches dest-ip to 0.0.0.0/32 sort all the tables by priority in the table group. vpp doesn't support parallel matching added catch-all acl group to the end. vpp default behavior of no match is drop but sonic is accept. Fix sonic-vpp crashing due to race condition during stats pull. If the interface to get stats has been removed, stat_segment_ls_r returns null. Signed-off-by: Yue Gao <[email protected]> Signed-off-by: Vivek Reddy <[email protected]>

why currently vpp doesn't support binding multiple ACL tables. Each table is appended with default permit-all rules. With multiple tables, this may cause acl matched by such rules and skip the actual rule to make in the tables after this one. what this PR does remove the default permit-all rules for each table If a table is empty, create a dummy rule that won't match any traffic because vpp doesn't allow empty table. The dummy rule matches dest-ip to 0.0.0.0/32 sort all the tables by priority in the table group. vpp doesn't support parallel matching added catch-all acl group to the end. vpp default behavior of no match is drop but sonic is accept. Fix sonic-vpp crashing due to race condition during stats pull. If the interface to get stats has been removed, stat_segment_ls_r returns null. Signed-off-by: Yue Gao <[email protected]>

* [DPU] Add support for Flow bulk session get notifications Signed-off-by: Vivek Reddy <[email protected]> * Remove obvious comments Signed-off-by: Vivek Reddy <[email protected]> * Add processMetadata method to validate object id Signed-off-by: Vivek Reddy <[email protected]> * Handled comments Signed-off-by: Vivek Reddy <[email protected]> * Minor fixes Signed-off-by: Vivek Reddy <[email protected]> * Use get_stats_ext instead of get_stats for switch counters (#1757) Signed-off-by: Ryan Garofano <[email protected]> Signed-off-by: Vivek Reddy <[email protected]> * [test] Fix flaky FlexCounter.bulkChunksize by replacing usleep with poll-wait (#1766) Signed-off-by: Vivek Reddy <[email protected]> * Add .github/copilot-instructions.md for AI-assisted development (#1764) Add .github/copilot-instructions.md to provide AI-assisted development guidance for contributors using GitHub Copilot, Copilot Chat, and other AI coding tools. This file helps AI tools understand the repo's architecture, coding conventions, and contribution workflow, leading to more accurate suggestions and fewer review cycles. What's included: Repository architecture and component overview Coding standards and naming conventions Testing requirements and patterns Build system integration notes Common pitfalls and best practices This file has no impact on builds, tests, or runtime behavior — it is purely developer guidance metadata. Signed-off-by: Rustiqly <[email protected]> Signed-off-by: Vivek Reddy <[email protected]> * vpp: support binding multiple ACL tables by priority (#1732) why currently vpp doesn't support binding multiple ACL tables. Each table is appended with default permit-all rules. With multiple tables, this may cause acl matched by such rules and skip the actual rule to make in the tables after this one. what this PR does remove the default permit-all rules for each table If a table is empty, create a dummy rule that won't match any traffic because vpp doesn't allow empty table. The dummy rule matches dest-ip to 0.0.0.0/32 sort all the tables by priority in the table group. vpp doesn't support parallel matching added catch-all acl group to the end. vpp default behavior of no match is drop but sonic is accept. Fix sonic-vpp crashing due to race condition during stats pull. If the interface to get stats has been removed, stat_segment_ls_r returns null. Signed-off-by: Yue Gao <[email protected]> Signed-off-by: Vivek Reddy <[email protected]> * Update syncd/NotificationProcessor.cpp Co-authored-by: Copilot <[email protected]> Signed-off-by: Vivek Reddy <[email protected]> * Update syncd/FlowDump.cpp Co-authored-by: Copilot <[email protected]> Signed-off-by: Vivek Reddy <[email protected]> * Update syncd/FlowDump.cpp Co-authored-by: Copilot <[email protected]> Signed-off-by: Vivek Reddy <[email protected]> * Update meta/SaiSerialize.cpp Co-authored-by: Copilot <[email protected]> Signed-off-by: Vivek Reddy <[email protected]> * Handle co-pilot comments Signed-off-by: Vivek Reddy <[email protected]> --------- Signed-off-by: Vivek Reddy <[email protected]> Signed-off-by: Ryan Garofano <[email protected]> Signed-off-by: Rustiqly <[email protected]> Signed-off-by: Yue Gao <[email protected]> Co-authored-by: Ryan Garofano <[email protected]> Co-authored-by: rustiqly <[email protected]> Co-authored-by: yue-fred-gao <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Lihua Yuan <[email protected]>

* Support for sonic-mgmt ACL testing on VPP (#1599) * Enabling sonic-mgmt ACL testing for Sonic-VPP * Use retval in debugs * Graceful shutdown vpp to avoid core dump (#1714) Signed-off-by: Yue Gao <[email protected]> * vpp: support ACL attached to LAG and UDP in ACE (#1718) * Handle acl attachment through LAG update * Add default permit-all rules * Support ACL with UDP protocol * if protocol is not specified but port or port-range is create 2 rules with proto UDP and TCP. vpp requires proto to be set if port or port-range is used * realign ace stats index because each ace can map to multiple acl rules Signed-off-by: Yue Gao <[email protected]> * vpp: support binding multiple ACL tables by priority (#1732) why currently vpp doesn't support binding multiple ACL tables. Each table is appended with default permit-all rules. With multiple tables, this may cause acl matched by such rules and skip the actual rule to make in the tables after this one. what this PR does remove the default permit-all rules for each table If a table is empty, create a dummy rule that won't match any traffic because vpp doesn't allow empty table. The dummy rule matches dest-ip to 0.0.0.0/32 sort all the tables by priority in the table group. vpp doesn't support parallel matching added catch-all acl group to the end. vpp default behavior of no match is drop but sonic is accept. Fix sonic-vpp crashing due to race condition during stats pull. If the interface to get stats has been removed, stat_segment_ls_r returns null. Signed-off-by: Yue Gao <[email protected]> * changes for vpp release 202510 --------- Signed-off-by: Yue Gao <[email protected]> Co-authored-by: AkeelAli <[email protected]> Co-authored-by: Mihut Aronovici <[email protected]>

Bind multiple ACL tables by priority

075ccdb

- Added catch-all acl group - Add a dummy rule because vpp doesn't allow empty table Signed-off-by: Yue Gao <[email protected]>

yue-fred-gao marked this pull request as ready for review December 19, 2025 21:58

Merge branch 'master' into acl_multi_table

e8a1fbb

yue-fred-gao added 2 commits February 5, 2026 12:57

Return proper status

7c1557d

Signed-off-by: Yue Gao <[email protected]>

Skip binding an empty table to ports

633c0d7

Signed-off-by: Yue Gao <[email protected]>

yue-fred-gao added 2 commits February 10, 2026 16:01

Fix core dump if the stats polling failed to find the right path

f41fd09

- This could happen at race condition when the interface is removed during shutdown Signed-off-by: Yue Gao <[email protected]>

Revert "Skip binding an empty table to ports"

8dd75c4

This reverts commit 633c0d7. Signed-off-by: Yue Gao <[email protected]>

yue-fred-gao requested a review from lolyu February 11, 2026 14:37

dypet approved these changes Feb 12, 2026

View reviewed changes

AkeelAli mentioned this pull request Feb 12, 2026

VPP platform test_acl.py changes sonic-net/sonic-mgmt#18313

Merged

11 tasks

AkeelAli approved these changes Feb 12, 2026

View reviewed changes

lolyu requested a review from Copilot February 12, 2026 23:47

Copilot started reviewing on behalf of lolyu February 12, 2026 23:47 View session

Copilot AI reviewed Feb 12, 2026

View reviewed changes

lguohan approved these changes Feb 14, 2026

View reviewed changes

lguohan merged commit 566c04a into sonic-net:master Feb 14, 2026
22 checks passed

yue-fred-gao added the Request for 202511 Branch label Mar 2, 2026

mssonicbld added the Cherry Pick Conflict_202511 label Mar 2, 2026

	return SAI_STATUS_FAILURE;
	// Missing stage metadata: behave as no-op and return success, consistent with aclBindUnbindPorts
	return SAI_STATUS_SUCCESS;

		// Create an ACL with 1 rule matching dest IP 0.0.0.0/32 (will never match real traffic) but vpp doesn't allow 0 rule table
		acl = (vpp_acl_t *) calloc(1, sizeof(vpp_acl_t) + sizeof(vpp_acl_rule_t));

Conversation

yue-fred-gao commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

why

what this PR does

Uh oh!

mssonicbld commented Dec 19, 2025

Uh oh!

azure-pipelines bot commented Dec 19, 2025

Uh oh!

mssonicbld commented Feb 5, 2026

Uh oh!

azure-pipelines bot commented Feb 5, 2026

Uh oh!

mssonicbld commented Feb 5, 2026

Uh oh!

azure-pipelines bot commented Feb 5, 2026

Uh oh!

mssonicbld commented Feb 10, 2026

Uh oh!

azure-pipelines bot commented Feb 10, 2026

Uh oh!

dypet left a comment

Choose a reason for hiding this comment

Uh oh!

AkeelAli left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yue-fred-gao commented Dec 19, 2025 •

edited

Loading