bgpd: anchor parsed attr in bgp_nlri_parse_vpn to preserve srv6_l3service (backport #22492)#22620
Open
mergify[bot] wants to merge 2 commits into
Open
bgpd: anchor parsed attr in bgp_nlri_parse_vpn to preserve srv6_l3service (backport #22492)#22620mergify[bot] wants to merge 2 commits into
mergify[bot] wants to merge 2 commits into
Conversation
…vice The MPLS VPN NLRI parser passes the same parent attr through the loop to bgp_update() once per prefix. After commit 862af5d("bgpd: Move srv6_l3service from attr to attr_extra") moved srv6_l3service into attr->extra, multiple downstream paths use bgp_attr_owns_extra() to decide whether to discard or transfer attr->extra. That check returns true when parsed_attr != self, which is the default (zeroed) state for the parent attr in this function. bgp_nlri_parse_ip already self-anchors attr->attr_intern_reuse.parsed_attr to attr at the top of the NLRI loop and clears it at the bottom. Mirror the same pattern here so the parent attr's extra (containing srv6_l3service) is preserved across all NLRIs in a single UPDATE. Without this, only the first prefix in a multi-NLRI VPN UPDATE retains the SRv6 SID; subsequent prefixes lose it and end up missing in the importing VRF, which prevents correct VPN route install. Signed-off-by: Yuqing Zhao <galadriel.zhao@alibaba-inc.com> (cherry picked from commit d2a0fbc)
Reproduce the case where a single BGP UPDATE carries multiple VPN NLRIs sharing one parsed attribute set. Without the companion fix, only the first prefix keeps its srv6_l3service after bgp_attr_intern() and the rest lose their SRv6 SID context, so VPN-to-VRF leak collapses 10 prefixes onto a single VRF entry on PE2. Topology: ce -- pe1 -- pe2, eBGP between pe1/pe2 carrying vpnv4 over IPv6 next-hop with SRv6 per-VRF SIDs. CE injects 10 IPv4 /32 prefixes; PE2 must end up with all 10 in VRF Vrf1's RIB. Without the fix: - test_multi_nlri_vpn_rib PASSES (BGP VPN table sees all 10) - test_multi_nlri_vrf_rib FAILS (only 192.100.0.1/32 lands in zebra) This test verifies the correctness of the fix. Signed-off-by: Yuqing Zhao <galadriel.zhao@alibaba-inc.com> (cherry picked from commit af3f9cf)
|
Target branch is not in the allowed branches list. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Symptom
On a PE receiving a BGP UPDATE that carries multiple VPN NLRIs sharing one parsed attribute set, only 1 of N prefixes gets installed in the importing VRF outing table.
show bgp sumshowsPfxRcd N(all received) but the VRF RIB ends up with a single entry. We see the following outputs:Root cause
Commit 862af5d ("bgpd: Move srv6_l3service from attr to attr_extra") moved srv6_l3service from a direct field of struct attr into the heap-allocated
attr->extracontainer.attr->extrafollows ownership-transfer semantics:bgp_attr_hash_alloc()transfers extra out of attrs thatbgp_attr_owns_extra()considers transient (owned, not self-anchored).In
bgp_nlri_parse_vpn(), a single BGP UPDATE carries N NLRIs sharing one parsed parent attr. Without the self-anchor:so
bgp_attr_owns_extra()returns true on the first prefix'sbgp_attr_intern()call.bgp_attr_hash_alloc()then takesval->extraand sets it to NULL. Because val was the result ofbgp_attr_dup_into(&new_attr, parent_attr)and the parent's extra was shallow-copied, the parent attr's extra is now effectively gone. Prefixes 2..N see extra == NULL → no srv6_l3service → no SRv6 SID → they either fail VPN-to-VRF leak or collapse onto the same RIB entry.bgp_nlri_parse_ip()already setsattr->attr_intern_reuse.parsed_attr = attr, sobgp_attr_owns_extra()returns false there → the deep-copy branch runs → every NLRI gets its own extra. The VPN path was simply missing the same guard.Fix
Mirror the self-anchor pattern from
bgp_nlri_parse_ip()inbgp_nlri_parse_vpn(): seedattr->attr_intern_reuse.parsed_attr = attrbefore the prefix loop and reset the cache on the done: exit. if (attr) guards the withdraw path.Topotest
tests/topotests/bgp_srv6l3vpn_multi_nlri/:
ce --- pe1 --- pe2
Two assertions on PE2 after convergence:
Without the fix: test_multi_nlri_vrf_rib FAILS, only the first prefix installed in the VRF.
With the fix: both PASS.
Commits
This is an automatic backport of pull request bgpd: anchor parsed attr in bgp_nlri_parse_vpn to preserve srv6_l3service #22492 done by Mergify.