bgpd: PMSI tunnel attribute compatibility#21507
Conversation
Greptile SummaryThis PR extends PMSI tunnel attribute handling to support IPv6 tunnel identifiers (length 21), covering parsing, encoding, and Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Receive PMSI Tunnel attribute] --> B{length < 5?}
B -- Yes --> ERR1[ATTR_LENG_ERR / malformed]
B -- No --> C[Read Flags + Tunnel Type + Label]
C --> D{tnl_type == INGR_REPL?}
D -- No --> E[skip remaining bytes\nstream_forward_getp\nlength - 5]
D -- Yes --> F{length == 9?}
F -- No, not 21 either --> ERR2[ATTR_LENG_ERR / malformed]
F -- Yes IPv4 --> G[stream_get_ipv4\nipv4_to_ipv4_mapped_ipv6\nattr_parse_len += 4]
F -- No, length == 21 --> H[stream_get IPv6\n16 bytes\nattr_parse_len += 16]
G --> I[stream_forward_getp 0\nno-op]
H --> I
subgraph Encode [Send PMSI Tunnel attribute]
J{tnl_type == INGR_REPL?} -- No --> K[write length=5\nFlags+Type+Label only]
J -- Yes --> L{IS_MAPPED_IPV6\ntunn_id?}
L -- Yes IPv4-mapped --> M[write length=9\nIPv4 Tunnel ID]
L -- No pure IPv6 --> N[write length=21\nIPv6 Tunnel ID]
end
Prompt To Fix All With AIThis is a comment left during a code review.
Path: tests/topotests/lib/evpn.py
Line: 1579-1583
Comment:
**Bare dict access raises `KeyError` instead of returning an error string**
The function's contract is to return an error string on failure (so `run_and_expect` can retry), but `paths[0][0]["pmsi"]` raises `KeyError` if the `pmsi` key is absent rather than returning a message. The exception propagates through `run_and_expect`, eventually becoming the opaque `result` string instead of a clean diagnostic.
```suggestion
pmsi = paths[0][0].get("pmsi")
if pmsi is None:
return f"Imet PMSI attribute not found in path for rd {rd} and {prefix}"
out_label = pmsi.get("label", 0)
if out_label != pmsi_label:
return f"Imet PMSI Label mismatch Expected {pmsi_label} Got {out_label}"
out_id = pmsi.get("id", "")
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: tests/topotests/bgp_evpn_vxlan_topo1/test_bgp_evpn_vxlan_v6_vtep.py
Line: 302-305
Comment:
**Missing `routers_have_failure()` guard**
The equivalent `test_imet` in `test_bgp_evpn_vxlan.py` skips early when routers are in a failed state, but this version does not. If a prior test left a router failed, this test runs against broken state and produces misleading failures.
```suggestion
def test_imet():
tgen = get_topogen()
if tgen.routers_have_failure():
pytest.skip(tgen.errors)
dut_name = "PE1"
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (3): Last reviewed commit: "tests: Add a v6 pmsi tunnel only test." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
bgpd/bgp_attr.c
Outdated
| if (length == 9) { | ||
| struct in_addr tunn_id; | ||
|
|
||
| tunn_id.s_addr = stream_get_ipv4(connection->curr); | ||
| ipv4_to_ipv4_mapped_ipv6(&attr->tunn_id, tunn_id); | ||
| attr_parse_len += IPV4_MAX_BYTELEN; | ||
| } else { | ||
| stream_get(&attr->tunn_id, connection->curr, IPV6_MAX_BYTELEN); | ||
| attr_parse_len += IPV6_MAX_BYTELEN; | ||
| } |
There was a problem hiding this comment.
PMSI_TNLTYPE_INGR_REPL
The tunnel ID parsing block runs unconditionally for all tnl_type values, but the length-vs-address-family contract (length == 9 → IPv4, else → IPv6) is only valid for PMSI_TNLTYPE_INGR_REPL. For any other tunnel type with length != 9 (e.g., PMSI_TNLTYPE_NO_INFO with the minimum length of 5), the else branch calls stream_get(..., IPV6_MAX_BYTELEN) when only 0 bytes remain; stream_get returns early and emits a warning, yet attr_parse_len is still incremented by 16. The subsequent stream_forward_getp(connection->curr, length - attr_parse_len) then receives a wrapped-around size_t value, fails bounds-checking, and leaves the stream's read pointer at the wrong position, corrupting the parse of every subsequent attribute in the UPDATE message.
| if (length == 9) { | |
| struct in_addr tunn_id; | |
| tunn_id.s_addr = stream_get_ipv4(connection->curr); | |
| ipv4_to_ipv4_mapped_ipv6(&attr->tunn_id, tunn_id); | |
| attr_parse_len += IPV4_MAX_BYTELEN; | |
| } else { | |
| stream_get(&attr->tunn_id, connection->curr, IPV6_MAX_BYTELEN); | |
| attr_parse_len += IPV6_MAX_BYTELEN; | |
| } | |
| stream_get(&attr->label, connection->curr, BGP_LABEL_BYTES); | |
| if (tnl_type == PMSI_TNLTYPE_INGR_REPL) { | |
| if (length == 9) { | |
| struct in_addr tunn_id; | |
| tunn_id.s_addr = stream_get_ipv4(connection->curr); | |
| ipv4_to_ipv4_mapped_ipv6(&attr->tunn_id, tunn_id); | |
| attr_parse_len += IPV4_MAX_BYTELEN; | |
| } else { | |
| stream_get(&attr->tunn_id, connection->curr, IPV6_MAX_BYTELEN); | |
| attr_parse_len += IPV6_MAX_BYTELEN; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: bgpd/bgp_attr.c
Line: 3798-3807
Comment:
**Tunnel ID parsing not guarded by `PMSI_TNLTYPE_INGR_REPL`**
The tunnel ID parsing block runs unconditionally for **all** `tnl_type` values, but the length-vs-address-family contract (`length == 9` → IPv4, else → IPv6) is only valid for `PMSI_TNLTYPE_INGR_REPL`. For any other tunnel type with `length != 9` (e.g., `PMSI_TNLTYPE_NO_INFO` with the minimum length of 5), the `else` branch calls `stream_get(..., IPV6_MAX_BYTELEN)` when only 0 bytes remain; `stream_get` returns early and emits a warning, yet `attr_parse_len` is still incremented by 16. The subsequent `stream_forward_getp(connection->curr, length - attr_parse_len)` then receives a wrapped-around `size_t` value, fails bounds-checking, and leaves the stream's read pointer at the wrong position, corrupting the parse of every subsequent attribute in the UPDATE message.
```suggestion
stream_get(&attr->label, connection->curr, BGP_LABEL_BYTES);
if (tnl_type == PMSI_TNLTYPE_INGR_REPL) {
if (length == 9) {
struct in_addr tunn_id;
tunn_id.s_addr = stream_get_ipv4(connection->curr);
ipv4_to_ipv4_mapped_ipv6(&attr->tunn_id, tunn_id);
attr_parse_len += IPV4_MAX_BYTELEN;
} else {
stream_get(&attr->tunn_id, connection->curr, IPV6_MAX_BYTELEN);
attr_parse_len += IPV6_MAX_BYTELEN;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
yes, that's valid: I'll update the logic here
tests/topotests/bgp_evpn_vxlan_topo1/test_bgp_evpn_vxlan_v6_vtep.py
Outdated
Show resolved
Hide resolved
| MIX(attr->encap_tunneltype); | ||
| MIX(bgp_attr_get_pmsi_tnl_type(attr)); | ||
| if (bgp_attr_get_pmsi_tnl_type(attr) == PMSI_TNLTYPE_INGR_REPL) | ||
| key = jhash(attr->tunn_id.s6_addr, IPV6_MAX_BYTELEN, key); |
There was a problem hiding this comment.
Donald had a comment in the original PR:
so from my perspective anytime we've had a situation where we had unset values as part of this attribute hash key making/comparison we've ended up with bugs. I would prefer that FRR has a v4 and a v6 side of this where we explicitly only create a key for the v4 side if v4 and if v6 use the v6 data. The way I am reading this is that the v4 and v6 share the same space, correct?
mjstapp
force-pushed
the
bgp_pmsi_tun_v6
branch
from
b748444 to
36b5734
Compare
|
Moved the dup topotest helper to the lib/evpn module. |
Presently EVPN IMET PMSI Tunnel attributes PMSI tunnel ID is not sent or processed properly. For sending side we try to send attr->nexthop which is not initialized for V6 nexthop, while on the receiving side the Tunnel ID is not processed. Here the fix has 3 functional parts: 1> Accept V6 PMSI tunnel length i.e 21 2> Send side: We encode the Tunnel ID with V4 or V6 Tunnel ID. this is based on retrieved tunnel Id or for local route tunnel Id fetched based on nexthop. 3> Receive side: Decode the tunnel Id to attribute tunnel_id new field. Signed-off-by: Mrinmoy Ghosh <mrinmoy_g@hotmail.com> Signed-off-by: Mark Stapp <mjs@cisco.com>
Display PMSI tunnel ID for V4 and V6 in vty and JSON Signed-off-by: Mrinmoy Ghosh <mrinmoy_g@hotmail.com> Signed-off-by: Mark Stapp <mjs@cisco.com>
Test 'test_imet' added for V4 and V4 to validate Tunnel ID PMSI Signed-off-by: Mrinmoy Ghosh <mrinmoy_g@hotmail.com> Signed-off-by: Mark Stapp <mjs@cisco.com>
mjstapp
force-pushed
the
bgp_pmsi_tun_v6
branch
from
36b5734 to
55461a9
Compare
|
I've replaced some magic numbers with defines, and updated the logic so that we only parse tunnel id addresses for the INGR_REPL tunnel type we think we understand. I also updated the show code so that the tunnel id address is only included for the known tunnel type(s). |
|
@greptileai review |
| if (IS_MAPPED_IPV6(&attr->tunn_id)) { | ||
| stream_putc(s, BGP_ATTR_PMSI_TUNNEL_V4_LENGTH); | ||
| tunn_id_len = IPV4_MAX_BYTELEN; | ||
| ipv4_mapped_ipv6_to_ipv4(&attr->tunn_id, &tunn_id); |
There was a problem hiding this comment.
this seems wrong. We are supposed to be writing to the stream, yet we are setting the attr->tunn_id not the stream, right? The non mapped case writes the value to the stream. Also I am dubious about any pattern where we are setting attr values in a write function to a stream. This is the wrong place to do it.
There was a problem hiding this comment.
shit I read this wrong. The ipv4_mapped_ipv6_to_ipv4 sets tunn_id.
|
I wrote a topotest that checks v6 to v6 only in my PR can you grab it and put it in this PR too? |
|
oh, yes, sure, will do
|
Add a v6 only test that shows that v6 encoding actually works. Signed-off-by: Donald Sharp <sharpd@nvidia.com>
|
I've pushed an update with that added topotest
|
|
LGTM. I'll bug Jafar to get this one in because I can no longer do so since it has a commit from me. |
|
@greptile review |
4 participants
Enhance support of the PMSI tunnel attribute, include both v4 and v6 endpoints. This isn't MVPN support (!) - just extended compatibility with the attribute for interop purposes.
This is an updated version of #19962 - I'll update this with comments from that original PR and see if we can converge.
The code here has 3 functional parts:
1> Accept V6 PMSI tunnel length i.e 21
2> Send side: We encode the Tunnel ID with V4 or V6 Tunnel ID.
this is based on retrieved tunnel Id or for local route tunnel Id fetched based on nexthop.
3> Receive side: Decode the tunnel Id to attribute tunnel_id new field.
In addition, show output is updated to display Tunnel ID in vty and json
Topotest added to validate V4 and V4 Tunnel ID for Ingress replication tunnel type