Skip to content

bgpd: Fix SRv6 SID export route-map update not taking effect - #21283

Merged
donaldsharp merged 2 commits into
FRRouting:masterfrom
cscarpitta:fix_srv6_bgp_grt_rmap_change
Mar 22, 2026
Merged

bgpd: Fix SRv6 SID export route-map update not taking effect#21283
donaldsharp merged 2 commits into
FRRouting:masterfrom
cscarpitta:fix_srv6_bgp_grt_rmap_change

Conversation

@cscarpitta

Copy link
Copy Markdown
Contributor

When sid export ... route-map <name> is reconfigured with a different route-map on an already-configured SID export, the new route-map is silently ignored. The code skips the early-return (rmap_str != rmap_name), but then calls bgp_srv6_unicast_announce() without updating rmap_name, so the old route-map remains in effect.

Fix this by replacing rmap_name before triggering the re-announce: decrement the reference count on the old route-map, free its name string, assign the new name, and increment the reference count on the new route-map.

@frrbot frrbot Bot added bgp tests Topotests, make check, etc labels Mar 22, 2026
When `sid export ... route-map <name>` is reconfigured with a
different route-map on an already-configured SID export, the new
route-map is silently ignored. The code skips the early-return
(rmap_str != rmap_name), but then calls
bgp_srv6_unicast_announce() without updating rmap_name, so the
old route-map remains in effect.

Fix this by replacing rmap_name before triggering the re-announce:
decrement the reference count on the old route-map, free its name
string, assign the new name, and increment the reference count on
the new route-map.

Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
@greptile-apps

greptile-apps Bot commented Mar 22, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a bug where reconfiguring sid export ... route-map <name> with a different route-map on an already-configured SID export silently kept the old route-map in effect. The fix correctly updates rmap_name β€” decrementing the old route-map's reference count, freeing its name string, duplicating the new name, and incrementing the new route-map's reference count β€” before calling bgp_srv6_unicast_announce(). A new topotest validates the fix end-to-end.

Changes:

  • bgpd/bgp_vty.c: In the sid_export DEFPY handler, adds the missing rmap_name update steps (ref decrement, free, strdup, ref increment) in the "already configured, rmap changed" branch before triggering re-announcement.
  • tests/topotests/bgp_srv6_unicast/test_bgp_srv6_unicast.py: Adds test_bgp_srv6_sid_rmap_update() which swaps the route-map on a live SID export and confirms the policy change propagates to peers R2 and R3, then restores the original and confirms recovery.

Confidence Score: 5/5

  • This PR is safe to merge β€” it is a minimal, well-targeted bug fix with correct memory and reference-count management, mirroring the established pattern used elsewhere in the same function.
  • The fix is logically sound: at the point the new code executes, rmap_str is guaranteed non-NULL (the early-return gate handles the NULL case), and the if (bgp->srv6_unicast[afi].rmap_name) guard safely covers the case where no previous route-map was set. The new code exactly mirrors the decrement/free/strdup/increment pattern used in the no branch and the initial-configuration path in the same function. The topotest exercises the precise failure scenario from the bug report and also verifies rollback. No unrelated code is touched.
  • No files require special attention.

Important Files Changed

Filename Overview
bgpd/bgp_vty.c Inserts proper rmap_name update (decrement old ref, free, strdup new, increment new ref) before re-announcing, fixing the bug where a reconfigured route-map was silently ignored.
tests/topotests/bgp_srv6_unicast/test_bgp_srv6_unicast.py Adds test_bgp_srv6_sid_rmap_update() to verify that replacing a route-map on an existing SID export actually takes effect, directly exercising the fixed code path.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[sid export route-map VTY command] --> B{SID already configured?}
    B -- No --> C[Initial setup: XSTRDUP rmap_name, increment ref, ensure SID]
    B -- Yes --> D{rmap_str same as current rmap_name?}
    D -- Same or NULL --> E[Return CMD_SUCCESS no-op]
    D -- Different --> F[Decrement old rmap ref count]
    F --> G[XFREE old rmap_name]
    G --> H[XSTRDUP new rmap_name]
    H --> I[Increment new rmap ref count]
    I --> J[bgp_srv6_unicast_announce with new route-map]
    J --> K[Return CMD_SUCCESS - new route-map in effect]
Loading

Reviews (1): Last reviewed commit: "tests: Add test for sid export route-map..." | Re-trigger Greptile

Add test_bgp_srv6_sid_rmap_update() to verify that replacing an
already-configured sid export route-map with a different one
correctly applies the new policy.

The test reconfigures R1 with a new route-map (filter2) that
excludes 10.0.0.1/32 from SID assignment, confirms the prefix
loses its SRv6 encapsulation on R2 and becomes uninstalled on
R3, then restores the original route-map (filter) and verifies
that SRv6 encapsulation is re-established on both peers.

Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
@cscarpitta
cscarpitta force-pushed the fix_srv6_bgp_grt_rmap_change branch from 78b5149 to 141ca52 Compare March 22, 2026 09:02
@donaldsharp
donaldsharp merged commit ae7c04c into FRRouting:master Mar 22, 2026
20 checks passed
cscarpitta added a commit to cscarpitta/sonic-buildimage that referenced this pull request Apr 1, 2026
Port the following patches from FRR mainline:
- bgpd: add remoteTransposedSid value in json output of vpn paths (FRRouting/frr#20255)
- SID extension to 32bits (FRRouting/frr#19223)
- various srv6 fixes (FRRouting/frr#20256)
- bgpd: Fix SRv6 SID/locator memory leak in SID notify handler (FRRouting/frr#21049)
- bgpd: Track route-map references for srv6 when rmap is used (FRRouting/frr#19868)
- bgpd: Fix route-map cleanup ordering in SRv6 unicast SID export (FRRouting/frr#21191)
- bgpd: Fix SRv6 SID export route-map update not taking effect (FRRouting/frr#21283)
- bgpd: Add SRv6 uDT46 SID support for GRT (FRRouting/frr#21041)

Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bgp master rebase PR needs rebase size/M tests Topotests, make check, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants