Skip to content

[vpp] add plugins for CoPP dataplane enforcement - #281

Open
nhegde-microsoft wants to merge 4 commits into
sonic-net:masterfrom
nhegde-microsoft:copp-vpp-enablement
Open

nhegde-microsoft wants to merge 4 commits into
sonic-net:masterfrom
nhegde-microsoft:copp-vpp-enablement

Conversation

@nhegde-microsoft

@nhegde-microsoft nhegde-microsoft commented Sep 3, 2026

Copy link
Copy Markdown

What

Two VPP plugins for CoPP dataplane enforcement:

  • copp_punt_policer: ARP/LACP/LLDP/UDLD/TTL_ERROR on device-input.
  • copp_ip2me_policer (new): IP2ME/SNMP/SSH on ip4-punt.

Why

sonic-buildimage#25801. Ethertype-keyed traps never reach VPP's existing classify arcs on these linux-cp-paired L3 ports. IP2ME/SNMP/SSH have no ethertype to match on and are keyed by destination IP after routing, so they need a separate policer path.

How

copp_ip2me_policer registers on ip4-punt (global arc, reached only after routing decides a packet is host-bound — no per-interface binding needed). Tracks router-owned IPv4 addresses, meters matches via the existing SAI-created policer (vnet_police_packet()). Conform/unmatched → ip4-punt-redirect; exceed/violate → ip4-drop.

Design: https://github.com/nhegde-microsoft/SONiC/blob/copp-vpp-enablement/doc/vpp/vpp_copp_HLD.md

Companion PRs

Test

vlab-vpp-01: all 24 tests/copp/test_copp.py subtests pass, zero skips.

@azure-pipelines

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

@mssonicbld

Copy link
Copy Markdown

/azp run

@azure-pipelines

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

@mssonicbld

Copy link
Copy Markdown

/azp run

@azure-pipelines

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

@azure-pipelines

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

@aaronber0614

Copy link
Copy Markdown
Contributor

Multi-model review: sonic-platform-vpp #281 (copp_punt_policer plugin)

I ran this through a three model consensus review (Claude Opus, GPT, Grok, all at high reasoning) and then a second adversarial pass whose job was to disprove each finding. Every item below is grounded against the pinned VPP revision 3f9e978d (policer lives at src/plugins/policer/) and the local sonic-sairedis tree. CI on this PR is green and the plugin itself is well structured. The blocker below is a deploy wiring gap, not a defect in the plugin C code.

Blocker 1: startup.conf.tmpl never enables the native policer_plugin.so

docker-syncd-vpp/conf/startup.conf.tmpl uses plugin default { disable } and then enables an explicit allow list. This PR adds plugin copp_punt_policer_plugin.so { enable } but does not add plugin policer_plugin.so { enable }. In the pinned VPP, policer is add_vpp_plugin(policer ...) in src/plugins/policer/CMakeLists.txt, so it is a loadable policer_plugin.so, not core vnet. Its binary API messages register only when that plugin loads.

The sonic-sairedis side (#2070) then does, in get_base_msg_id():

policer_msg_id_base = vl_client_get_first_plugin_msg_id("policer_%08x...");
assert(policer_msg_id_base != (u16)~0);

On a clean build and deploy where policer_plugin.so is not loaded, that lookup returns ~0 and the assert aborts syncd at connect time (this aborts all of syncd, not just CoPP). configure.ac does not define NDEBUG, so the assert is live; even if it were compiled out, a ~0 base makes every POLICER_ADD a garbage message id that never reaches the real handler, so no policer is ever programmed.

I confirmed the plugin data path does NOT crash in this scenario: copp_punt_policer_resolve_index() null checks policer_get_main() and returns DROP_UNRESOLVED, so bound ethertypes are dropped rather than dereferencing NULL. The failure is purely the syncd side abort above.

I suspect the manual validation passed because the test box had policer_plugin.so enabled out of band. Fix: add plugin policer_plugin.so { enable } to startup.conf.tmpl (and to the docker-sonic-vpp startup template if CoPP is meant to work in the single container image too).

Blocker 2 (shared with #2070): SAI policer counters read stats that this plugin never increments

copp_punt_policer_node.c meters with the raw vnet_police_packet(policer, 256, ...). That call computes color and updates token buckets only. The stats segment counters /net/policer/{conform,exceed,violate} are incremented ONLY by the policer_police() wrapper (src/plugins/policer/police_inlines.h, vlib_increment_combined_counter), which this node never calls. sonic-sairedis getPolicerStats reads exactly those /net/policer/* counters, so SAI_POLICER_STAT_GREEN/YELLOW/RED_* return 0 for every CoPP ethertype. The plugin does keep its own per ethertype counters, but it exposes them through a separate API that getPolicerStats does not read. test_policer does not catch this because it measures PTF rx_pps, not SAI stats. Fix options: have the node call policer_police() so the combined counters increment, or point getPolicerStats at this plugin's own counters. This ties directly to HLD REQ-5 (see my #2539 comment).

Non blocking notes

  1. Worker safety: copp_punt_policer_bind mutates cpm->entries[] and n_entries from the main thread with no vlib_worker_thread_barrier_sync, and the counter flush is non atomic. This is fine on a single worker t1-lag-vpp KVM but will race on a multi worker setup. Worth a comment noting the single worker assumption, or a barrier if multi worker is in scope.
  2. The per packet ELOG in the node path is not gated on VLIB_NODE_FLAG_TRACE, so it fires unconditionally and wraps the elog ring under load. Consider gating it on the trace flag.
  3. Nit: the SAI to VPP policer mode mapping keys the 1R3C (SR_TCM) case on has_pir, but RFC2697 uses PBS, not PIR. Harmless for CoPP (CIR/CBS only), but the discriminator is slightly off.

Confirmed correct (I checked these and they are fine)

The fixed 256 byte metering length matches VPP's QOS_POLICER_FIXED_PKT_SIZE pps calibration and is intentional. Mixing vnet_feature_next with the static next_nodes[] (DROP=0, INTERFACE_OUTPUT=1) is the same idiom used by ip_validate and the VPP sample plugin, so that is correct.

Merge order

This PR should land first so platform-vpp master republishes the VPP debs and the generated copp_punt_policer.api_enum.h header, then #2070 (which currently red builds only because that header is not yet on master), then the sonic-mgmt skip lift. Please address Blocker 1 (and ideally Blocker 2) before merge.

Reviewed by AI agent on behalf of aaronber0614.

@mssonicbld

Copy link
Copy Markdown

/azp run

@azure-pipelines

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

@nhegde-microsoft

Copy link
Copy Markdown
Author

@aaronber0614
Thanks — both blockers check out. Digging into why testing didn't catch Blocker 1: my earlier validation ran against the DUT's live VPP, which turned out to be pinned at the old 435fda042 (pre-pluginification), not the actual 3f9e978d7 this PR targets. On the old build policer was still core vnet, always loaded — so that gap couldn't have surfaced there. That means the earlier "8/8 passing" evidence doesn't validate against the real pinned VPP; I'll redeploy against 3f9e978d7 and re-run before restating any pass results.

Blocker 1: Fixed — added plugin policer_plugin.so { enable } to docker-syncd-vpp/conf/startup.conf.tmpl. Left docker-sonic-vpp's template alone; its plugin list was already out of sync with docker-syncd-vpp before this PR (missing sflow/sonic_ext too), so bringing just the policer line in line there felt like a separate pre-existing issue — happy to open a follow-up if that image matters for CoPP.

Blocker 2: Fixed — the node now calls policer_get_counters() + vlib_increment_combined_counter() right after computing the verdict, using the same 256-byte metered length, so the stats-segment counters getPolicerStats() reads are no longer stuck at zero.

Non-blocking notes: addressed all three — ELOG now gated on VLIB_NODE_FLAG_TRACE, a short comment documenting the single-writer (syncd, main thread) assumption instead of an unverified barrier, and the SR_TCM/PIR-vs-PBS discriminator nit is noted for a follow-up.

Will hold off claiming this is validated again until I've re-run the full suite against a real 3f9e978d7 build.

New VPP plugin classifying and rate-limiting control-plane traffic
(ARP, LACP, LLDP, UDLD, TTL_ERROR) on the device-input feature arc.
Conforming packets are delivered directly to each port's paired TAP interface.
Enabled in docker-syncd-vpp's VPP startup config.

Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
The pinned VPP commit (3f9e978d7) includes upstream's 'policer:
pluginify policer' refactor, which moved policer support from
src/vnet/policer/ into its own out-of-tree-style plugin at
src/plugins/policer/. This broke the CI build:

  fatal error: 'vnet/policer/policer.h' file not found

Fix:
- Update includes from <vnet/policer/policer.h> to <policer/policer.h>
  (the new location, matching how src/plugins/unittest/policer_test.c
  already references it).
- The policer plugin's main-struct global (vnet_policer_main) is no
  longer directly linkable from an external plugin -- it's only
  extern-visible from inside the policer plugin itself
  (POLICER_PLUGIN_INTERNAL). External plugins must resolve it via the
  exported policer_get_main() accessor (a vlib_get_plugin_symbol()
  runtime lookup), same pattern policer_test.c uses. Switch both call
  sites and add a defensive null check, since the lookup can now
  legitimately fail (missing/not-yet-loaded policer plugin) where a
  direct global reference never could.

Verified via a standalone syntax-only compile of both changed .c files
against a clean checkout of the exact pinned VPP commit (3f9e978d7) --
0 errors, only the pre-existing -Waddress-of-packed-member warnings
already suppressed by the project's real build flags.

Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
- docker-syncd-vpp/conf/startup.conf.tmpl: enable policer_plugin.so.
  The pinned VPP version loads policer as its own plugin (no longer
  core vnet); without this, sonic-sairedis's policer_msg_id_base
  lookup returns ~0 and aborts syncd at connect.

- copp_punt_policer_node.c: call policer_get_counters() +
  vlib_increment_combined_counter() after computing the verdict, so
  the VPP stats-segment counters (/net/policer/{conform,exceed,violate})
  that SwitchVppPolicer.cpp's getPolicerStats() reads are actually
  populated. vnet_police_packet() alone only updates the token bucket
  and never touches these counters.

- copp_punt_policer_node.c: gate the per-packet ELOG call on
  VLIB_NODE_FLAG_TRACE instead of firing unconditionally.

- copp_punt_policer.c: document the lack of a
  vlib_worker_thread_barrier_sync() around the bind-table mutation --
  the only caller in this environment is SONiC's syncd (single VAPI
  client, main thread only), so this is unlikely to be an issue.

Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
Polices traffic destined to router-owned IPv4 addresses on the
ip4-punt arc, ahead of ip4-punt-redirect -- a single global arc, so no
per-interface binding is needed. Reuses VPP's existing policer objects.

Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
@mssonicbld

Copy link
Copy Markdown

/azp run

@azure-pipelines

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

@nhegde-microsoft nhegde-microsoft changed the title vppbld: add copp_punt_policer plugin for CoPP dataplane enforcement vppbld: add copp_punt_policer and copp_ip2me_policer plugins for CoPP dataplane enforcement Sep 10, 2026
@nhegde-microsoft nhegde-microsoft changed the title vppbld: add copp_punt_policer and copp_ip2me_policer plugins for CoPP dataplane enforcement [vpp] add plugins for CoPP dataplane enforcement Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants