bgpd: pin the DIMT UMH from the UMH large community (BLO-36558)

[kkroo]

What

The DIMT pin path now also reads the UMH from the UMH large community <origin AS>:<function>:<UMH IPv4>, through one decoder shared with the MVPN Type-7 lane. This is BLO-36558 steps 2–3; the contract is doc/dimt-lc-umh-mapping.md (#99).

• Shared decoder bgp_umh_lc_decode() (bgp_dimt.c): applies the origin-AS trust rule and the usable-address rule, and the lowest tuple wins. Each lane keeps its own reject counter and throttle. The MVPN lane now calls it and its log lines are byte-identical; vector p6 (a v4 UMH on a v6 C-S route) still resolves there.
• DIMT call site (bgp_dimt_umh_lc_resolve()): a new knob bgp dimt umh-large-community <fn> (default instance only, separate from the MVPN knob). The checks run in order: neighbour trust (dimt-trusted, bgpd: gate the DIMT UMH extended community on neighbor trust + origin AS (BLO-36553) #100) before decoding, then IPv4 only, then the decoder. An untrusted LC is also charged to dimtUmhRejected (CTO ruling D1).
• Precedence: a valid LC maps as pim, preference 0, and wins over the 0x80 EC, unless the EC's selected tuple is amt-relay. The disagreement is logged under debug bgp zebra.
• Dedup: each attribute set is counted once (pi->dimt_umh_lc_counted). A knob change re-evaluates every unicast route without a session reset.
• New show command: show bgp [vrf NAME] umh-large-community [json] (dimtFunction, dimtUmhLcRejected, mvpnFunction, mvpnUmhLcRejected).

Scope note (CTO ruling D2 on BLO-39485): transitive DIMT through an IX uses bilateral member sessions. The trust gate still refuses route-server and multi-AS paths by design; a route-server variant is deferred to BLO-39562.

Tests (devbox, CI image docker/ubuntu-ci 24.04)

• tests/bgpd/test_dimt_umh_lc: 12 cases, all pass. test_dimt_umh_trust passes too.
• tests/topotests/bgp_dimt_umh_lc T1–T9 plus regressions bgp_dimt_umh_trust, bgp_mvpn_gtm_umh_{lc,ebgp,ibgp,bestpath,dimt} and pim_dimt_umh: 87 passed, 3 skipped, 0 failed.
• Failing-first: the new topotest against a master image (d4467836c8) fails at T1 on behaviour: r2 has no UMH mapping for 10.10.1.0/24 (mapping absent). The unit test does not link on master.

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-39562
🔗 Paperclip issue: BLO-36558
🔗 Paperclip issue: BLO-39485

[kkroo]

@ally please review head 40db35f (BLO-36558 steps 2-3: the DIMT pin path reads the UMH large community through the decoder shared with MVPN; CTO rulings D1/D2 on BLO-39485 applied).

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 40db35f

Clean. I traced the decoder, the two call sites, the attr-reference lifecycle and the MVPN refactor against the files at this head, and verified the load-bearing assumptions rather than taking the comments' word for them.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

• [code] bgpd/bgp_dimt.c:1092 — the ec_amt || disjunct in the disagreement-log guard is redundant: ec_amt implies new_type == ZAPI_UMH_TYPE_AMT_RELAY, which already satisfies new_type != ZAPI_UMH_TYPE_PIM immediately after it. No behavioural difference; dropping it removes a term a future reader has to prove harmless.

  • Keep the comment above it — the intent (compare type as well as address, so an identical-address amt-relay override is still logged) is the non-obvious part and is worth preserving.

• [code] bgpd/bgp_dimt.c:1236-1254 — bgp_dimt_umh_lc_set_function() walks every IPv4 and IPv6 unicast dest synchronously on the vty/main thread, and calls bgp_dimt_route_update() for every dest with a selected path. On a box carrying a full table most of those dests have no UMH of either encoding, and each still pays a cross-family bgp_dimt_umh_decode() plus a dimt_sent lookup; every dest that does have an EC-derived mapping is re-ADDed to pimd even when the knob change cannot affect it.

  • This matches FRR precedent (bgp_route_map_process_update_cb walks tables synchronously too) and the header comment acknowledges the cost, so it is defensible as-is. If full-table deployments are in scope, the cheap win is to skip the bgp_dimt_route_update() call when the path had no dimt_umh_lc_counted record and resolves none under the new function — that leaves the correctness-relevant cases (newly resolves, no longer resolves, record needs clearing) untouched.

• [code] bgpd/bgp_dimt.c:1220 — the default-instance invariant is enforced only by the single vty caller, while the function's own comment is what documents it. An early if (bgp->inst_type != BGP_INSTANCE_TYPE_DEFAULT) return; would keep the rule with the function that states it. Harmless today — bgp_dimt_route_update() filters non-default instances before touching dimt_sent, so a VRF call is wasted work rather than a bug.

Strengths

• The security-critical ordering is right. bgp_dimt_umh_lc_resolve() runs bgp_dimt_peer_is_trusted() before decoding, so the LC cannot be used to launder a claim the 0x80 EC would have been refused for on the same route. That gate is itself a fail-closed allow-list over sub_type, and reusing it rather than re-deriving a second trust check is the correct call.
• "Lowest tuple wins" is actually sound, not just asserted. I verified it: lcommunity_parse() runs lcommunity_uniq_sort() on every received attribute, and lcommunity_add_val() inserts by memcmp over the full 12 bytes — so ordering is numeric on (GA, function, parameter), making the Global Administrator the primary sort key exactly as the decoder comment claims. The reasoning for counting post-winner tuples (a crafted high-GA tuple sorts behind a legitimate one, so an early skip would never count it) follows from that and is correct.
• The attr-reference lifecycle is handled completely. dimt_umh_lc_counted is unterned in both bgp_path_info_free_with_caller() and the bgp_update() discard path, and bgp_dimt_umh_lc_from_path() reassigns unconditionally rather than relying on bgp_attr_unintern() to NULL the pointer — which it only does when the refcount reaches zero. The knob-change walk gets that right too, with the explicit = NULL. This is the failure mode that usually slips through a change of this shape.
• The refactor tightens two latent NULL derefs rather than carrying them over: the old MVPN path dereferenced pi->peer->sort unguarded and checked !lcom without !lcom->val; the shared decoder guards both.
• Counting semantics are documented where operators will actually hit them. doc/user/bgp.rst states plainly that dimtUmhLcRejected and mvpnUmhLcRejected are not the same kind of number (per-attribute-set vs per-evaluation) and that neither is comparable with the per-neighbor dimtUmhRejected — the exact ambiguity that otherwise turns into a misread dashboard.
• Failing-first evidence is stated concretely (new topotest fails at T1 against master image d4467836c8 on behaviour, not on a link error), and the unit test asserts throttle independence between lanes rather than only the happy path.

One behavioural note for the release notes rather than a finding: moving the found check to after the GA and address checks means an MVPN route carrying a valid winner plus a later invalid tuple now emits notice-level rejects and increments the new counter, where it previously emitted only a debug "lower tuple already won". That is deliberate and documented in the decoder comment, and it is the better behaviour — but the PR body's "its log lines are byte-identical" describes the format strings, not which lines fire for a given route.

Recommended Action

1. No blocking changes requested.
2. Merge once the remaining required CI checks finish green.

[kkroo]

CI on 40db35f is red only on pim_dimt_tunnel_normal::test_jp_upstream_is_hello_neighbor (22.04 and 24.04 shard 1/2, in both the parallel run and the serial rerun). The other two shard-1 failures, bgp_gr_functionality_topo2 and ospf_topo2, passed on the serial rerun.

That failure is in master, not in this diff. The test's J/P parser skips the Num Joins: heading that tshark 3.6.2 and 4.2.2 both print, so F9 cannot pass on any run. #134's 23:08Z comment saw the same thing.

The fix is #135, which was squash-merged into #134's branch (fbc3617), not into master. Plan: once #134 lands on master, update this branch with a merge from master (no force-push), wait for CI, then request a fresh Ally review at the new head. Nothing in this PR changes.

[kkroo]

Lease: kkroo drive session (Omar, 7ee27e), about 30 min, at 40db35f5. Pushing a prepared change (finding fix or conflict resolution); disposition follows.

[kkroo]

Merged master at cb92d0f9

The only red check at the previous head was the topotest that asserts the tshark Num Joins count. That failure was not caused by this PR: frr #134 fixed the assertion and landed on master at 1d27120. This commit merges master so the lanes run against the fixed test. The merge was clean. No code in this PR changed.

[kkroo]

Lease released (kkroo drive session 7ee27e). Pushed cb92d0f9; disposition above.

[kkroo]

/test

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: cb92d0f

Clean. This head is the previous one plus a merge of master (#134's CI/topotest fixes); I verified the author's "no code in this PR changed" claim rather than taking it, then re-ran the lenses over the code itself.

Identity check. compare 40db35f5...cb92d0f9 is ahead_by: 2, and those two commits touch only .github/workflows/{buildcache-seed,github-ci}.yml, tests/topotests/ospf_basic_functionality/test_ospf_authentication.py and tests/topotests/pim_dimt_tunnel_normal/test_pim_dimt_tunnel_normal.py — none of them this PR's. Blob SHAs for all 12 substantive PR files (bgpd/bgp_dimt.{c,h}, bgp_mvpn.c, bgp_route.{c,h}, bgp_vty.c, bgpd.h, both docs, the unit test, the topotest, tests/bgpd/subdir.am) are byte-identical across the two heads. The merge was clean and the PR diff is unchanged.

Critical Issues (0)

Important Issues (0)

Suggestions (4)

• [code] bgpd/bgp_dimt.c:1092 — the ec_amt || disjunct in the disagreement-log guard is redundant: ec_amt implies new_type == ZAPI_UMH_TYPE_AMT_RELAY, which already satisfies the new_type != ZAPI_UMH_TYPE_PIM term immediately after it. No behavioural difference; dropping it removes a term a future reader has to prove harmless. Keep the comment above it — the intent (compare type as well as address, so an identical-address amt-relay override is still logged) is the non-obvious part.

• [code] bgpd/bgp_dimt.c:1230-1256 — bgp_dimt_umh_lc_set_function() walks every IPv4 and IPv6 unicast dest synchronously on the vty/main thread and calls bgp_dimt_route_update() for every dest with a selected path. On a full table most dests carry no UMH of either encoding and still pay a cross-family decode plus a dimt_sent lookup. This matches FRR precedent (bgp_route_map_process_update_cb walks tables synchronously too) and the header comment acknowledges the cost, so it is defensible as-is. If full-table deployments are in scope, the cheap win is to skip the bgp_dimt_route_update() call when the path had no dimt_umh_lc_counted record and resolves none under the new function — that leaves the three correctness-relevant cases (newly resolves, no longer resolves, record needs clearing) untouched.

• [code] bgpd/bgp_dimt.c:1220 — the default-instance invariant is enforced only by the single vty caller, while the function's own comment is what documents it. An early if (bgp->inst_type != BGP_INSTANCE_TYPE_DEFAULT) return; would keep the rule with the function that states it. Harmless today — bgp_dimt_route_update() filters non-default instances before touching dimt_sent, so a VRF call is wasted work rather than a bug.

• [code] bgpd/bgp_vty.c:23502 — show bgp vrf VRFNAME umh-large-community always prints the DIMT row as disabled with Rejected 0, because both the knob and the counting call site are default-instance only. That is correct but reads like a per-VRF fact that could have been otherwise. Either suppress the DIMT row under a non-default instance or label it (DIMT n/a (default instance only)); the MVPN row is the only genuinely per-VRF one here.

Strengths

• The security-critical ordering is right, and survives the refactor. bgp_dimt_umh_lc_resolve() runs bgp_dimt_peer_is_trusted() before decoding, so the LC cannot launder a claim the 0x80 EC would have been refused for on the same route. Reusing that fail-closed sub_type allow-list rather than re-deriving a second trust check is the correct call.
• "Lowest tuple wins" is sound, not merely asserted. lcommunity_parse() runs lcommunity_uniq_sort() on every received attribute and lcommunity_add_val() inserts by memcmp over all 12 bytes, so ordering is numeric on (GA, function, parameter) and the Global Administrator is the primary sort key exactly as the decoder comment claims. The reasoning for counting post-winner tuples follows from that and is correct: a crafted high-GA tuple sorts behind a legitimate one, so an early skip would never count it.
• The attr-reference lifecycle is complete. dimt_umh_lc_counted is unterned in bgp_path_info_free_with_caller() and in the bgp_update() discard path, and bgp_dimt_umh_lc_from_path() reassigns unconditionally rather than relying on bgp_attr_unintern() to NULL the pointer — which it only does at refcount zero. The knob-change walk gets that right too, with the explicit = NULL. This is the failure mode that usually slips through a change of this shape.
• The dedup record is keyed so it cannot over- or under-count. A path carrying no DIMT tuple stores no reference and re-enters as "fresh" on every pass, but short-circuits at the has_function gate before any counter moves; a path on the lane that is refused (untrusted peer, or v6) sets on_lane before those gates, so its record is taken and the refusal is charged exactly once per attribute set.
• The MVPN refactor tightens two latent NULL derefs rather than carrying them over: the old path dereferenced pi->peer->sort unguarded and checked !lcom without !lcom->val; the shared decoder guards both.
• I re-verified the load-bearing externals rather than assuming them. IPV4_NET0/NET127/LINKLOCAL/CLASS_D/CLASS_E all exist at lib/prefix.h:381-386 and are host-order macros, matching the host-order param passed to them; aspath_origin_as()'s three-argument form at bgpd/bgp_aspath.h:145 matches both call sites. enum bgp_umh_lc_lane_id has both values designated in umh_lc_lane_name[] and umh_lc_lane_noun[], and both call sites pass a literal, so the lane lookup cannot run off the end.
• The unit test is a matrix, not a happy path. Twelve cases covering function matching, known answer, lowest-tuple-plus-mismatches, AS_SET, foreign function, every unusable range, untrusted peer, v6, throttle independence between lanes, pure query, reprocess-once, and two knobs side by side — including negative assertions on on_lane and on the counters, which is what makes the dedup contract testable at all.
• Counting semantics are documented where operators hit them. doc/user/bgp.rst states plainly that dimtUmhLcRejected and mvpnUmhLcRejected are different kinds of number (per-attribute-set vs per-evaluation) and that neither is comparable with the per-neighbor dimtUmhRejected — the exact ambiguity that otherwise becomes a misread dashboard.

One behavioural note for the release notes rather than a finding, unchanged from the previous head: moving the found check to after the GA and address checks means an MVPN route carrying a valid winner plus a later invalid tuple now emits notice-level rejects and increments the new counter, where it previously emitted only a debug "lower tuple already won". That is deliberate, documented in the decoder comment, and the better behaviour — but the PR body's "its log lines are byte-identical" describes the format strings, not which lines fire for a given route.

CI state at this head when I read it: the four Ubuntu test shards were queued under run 37222642153, so the lanes had not yet reported. Nothing to act on — just the reason item 2 below is phrased as a wait.

Recommended Action

1. No blocking changes requested.
2. Merge once the remaining required CI checks finish green.