[vpp] Permit local traffic ahead of ACL deny rules - #2027
Conversation
|
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
53f2997 to
7c724e1
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
make_local_permit left src_prefix unset for IPv6 local addresses. The xlate defaults an unset source prefix to IPv4, so an IPv6 dst produced a rule with src=IPv4 / dst=IPv6. VPP rejects mismatched-family ACL rules with VNET_API_ERROR_INVALID_SRC_ADDRESS (-57), which failed the entire acl_add_replace. As a result only the very first (IPv4-only) refresh committed and every later rebuild -- including the ones that would add the SoC loopback permit -- was rejected, so control traffic to those addresses kept hitting the mux drop. Set a same-family wildcard source (0.0.0.0/0 or ::/0) so every rule is family-consistent and accepted. Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
7c724e1 to
dbe8cbd
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR updates the SAI-VPP (libsaivs) datapath so that traffic destined to the switch’s own local IP addresses is permitted ahead of ACL deny rules, preventing control-plane (“for-us”) traffic from being dropped by broad VPP ACL denies (notably the dual-ToR mux drop ACL).
Changes:
- Track switch-owned local interface IP addresses as they are added/removed in VPP (
trackLocalIp). - When programming an ACL table that contains any deny rule, prepend per-local-IP “permit dst /32|/128” rules to the same VPP ACL (
injectLocalPermits) and keep these permits refreshed when local IPs change. - Offset VPP rule index bookkeeping to keep ACL counters aligned after permit injection.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| vslib/vpp/SwitchVppRif.cpp | Calls trackLocalIp() on successful interface IP add/del so local IPs are tracked from all relevant programming paths. |
| vslib/vpp/SwitchVppAcl.cpp | Adds local-IP tracking and ACL permit injection ahead of deny rules during ACL table (re)configuration. |
| vslib/vpp/SwitchVpp.h | Adds state (m_local_ips, m_local_deny_tables) and declares the new helper methods. |
Local permits are prepended only to the regular ACL rule list, but the base-index fixup shifted vpp_rule_base_index for every ACE, including tunnel-termination ACEs whose indices are relative to the separate tunterm ACL. That table does not gain the leading permit rules, so shifting its ACEs corrupts the ACL counter-to-rule mapping for tunterm entries. Only adjust regular (non-tunterm) ACEs. Addresses review feedback on sonic-net#2027.
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Local permits are prepended only to the regular ACL rule list, but the base-index fixup shifted vpp_rule_base_index for every ACE, including tunnel-termination ACEs whose indices are relative to the separate tunterm ACL. That table does not gain the leading permit rules, so shifting its ACEs corrupts the ACL counter-to-rule mapping for tunterm entries. Only adjust regular (non-tunterm) ACEs. Addresses review feedback on sonic-net#2027. Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
47ac022 to
2ba150f
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
2ba150f to
54d14c7
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
yue-fred-gao
left a comment
There was a problem hiding this comment.
vpp only supports about 80 rules. This can easily take half of that. Is there anyway to limit this feature only to where it is actually needed?
Have you run t1-lag sanity?
…addrs Address review feedback on rule-count budget: the previous logic injected a permit for every local interface address into any table containing a deny, which scales with fabric size and can consume a large share of the practical VPP per-interface ACL budget. Narrow the feature to exactly where it is needed: - Trigger only on the dual-ToR mux drop, identified as a DROP ACE scoped by SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS (the pattern VPP cannot represent and thus renders as a port-wide deny). Ordinary security/data ACLs and platforms with no mux drop (e.g. t1-lag) get nothing injected. - Track only mux-reachable local addresses -- the VLAN SVI (BVI) gateway and loopbacks reached through it. Routed port/portchannel (uplink) addresses never traverse a mux port, so they are skipped, bounding the permit count independent of uplink/fabric count. Addresses review feedback on sonic-net#2027.
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Thanks — both good points. Pushed a follow-up ( Rule-count / "limit it to where it's needed": the previous logic injected a permit for every local interface address into any table with a deny, which scales with fabric size. It's now scoped two ways:
Measured on a live dual-ToR DUT, the mux ACL went from 18 permits + 1 deny (19) to 10 permits + 1 deny (11) — the 8 uplink t1-lag sanity: I have not run a full t1-lag testbed yet (only dual-ToR DUTs are up in my environment). By construction, on t1-lag there is no IN_PORTS-scoped mux drop ACL, so |
Summary:
Fixes sonic-net/sonic-buildimage#28884
Type of change
Approach
What is the motivation for this PR?
the mux drop ACL blackholing ICMP replies at
l2-input.check issue: sonic-net/sonic-buildimage#28884 for more details.
Signed-off-by: Longxiang Lyu lolv@microsoft.com
Work item tracking
How did you do it?
The fix emulates hardware "CoPP before ACL" purely through ACL rule ordering. VPP evaluates the rules of a single ACL in order, first-match-wins, so if permit rules for the switch's own addresses are placed ahead of a deny in the same ACL, traffic to local RIF is allowed through to the BVI and punted at ip4-local, while all other traffic still falls through to the deny unchanged. No extra datapath node and no per-packet lookup are introduced.
Learn the switch's own addresses. The switch's local host addresses (loopback, SVI/BVI gateway, L3 router-interface) are exactly the interface addresses it programs into VPP, which VPP turns into local (ip2me) receive routes. The layer now records these addresses as they are added and removed, giving it an up-to-date set of "for-us" destinations for both IPv4 and IPv6.
Prepend permits when a table drops traffic. When an ACL table is (re)built and it contains any deny/drop rule, host-scoped permit rules (one per known local address) are inserted ahead of that table's rules. Because the permits live in the same ACL that carries the deny, they are automatically applied to exactly the interfaces the table is bound to, and only traffic destined to the switch's own addresses matches them — everything else is unaffected.
Keep the two in sync. The local-address set and the drop ACL are programmed by independent, unordered SAI events, so the solution reconciles them either way: tables that contain a deny are remembered, and a later change to the local-address set re-applies the permits to those tables. Equivalently, if the addresses are already known when the drop table is first built, the permits are emitted immediately. During bulk boot, before any drop ACL exists, addresses simply accumulate with no ACL churn.
How did you verify/test it?
Build and verify on dualtor testbed, the icmp replies are received on mux port:
Any platform specific information?
Documentation