Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 55 additions & 10 deletions bgpd/bgp_attr.c
Original file line number Diff line number Diff line change
Expand Up @@ -1049,6 +1049,9 @@ unsigned int attrhash_key_make(const void *p)
MIX3(attr->bh_type, attr->otc, bgp_attr_get_aigp_metric(attr));
MIX3(attr->mm_seqnum, attr->df_alg, attr->df_pref);
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);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

key = jhash(&attr->rmac, sizeof(attr->rmac), key);
if (bgp_attr_get_nhc(attr))
MIX(bgp_nhc_hash_key_make(bgp_attr_get_nhc(attr)));
Expand Down Expand Up @@ -1104,7 +1107,9 @@ bool attrhash_cmp(const void *p1, const void *p2)
attr1->bh_type == attr2->bh_type && attr1->otc == attr2->otc &&
!memcmp(&attr1->rmac, &attr2->rmac, sizeof(struct ethaddr)) &&
bgp_nhc_same(bgp_attr_get_nhc(attr1), bgp_attr_get_nhc(attr2)) &&
bgp_ls_attr_same(attr1->ls_attr, attr2->ls_attr))
bgp_ls_attr_same(attr1->ls_attr, attr2->ls_attr) &&
(attr1->pmsi_tnl_type == attr2->pmsi_tnl_type) &&
IPV6_ADDR_SAME(&attr1->tunn_id, &attr2->tunn_id))
return true;
}

Expand Down Expand Up @@ -3751,6 +3756,7 @@ bgp_attr_pmsi_tunnel(struct bgp_attr_parser_args *args)
const bgp_size_t length = args->length;
uint8_t tnl_type;
int attr_parse_len = 2 + BGP_LABEL_BYTES;
struct in_addr tunn_id;

if (peer->discard_attrs[args->type] || peer->withdraw_attrs[args->type])
goto pmsi_tunnel_ignore;
Expand All @@ -3773,9 +3779,14 @@ bgp_attr_pmsi_tunnel(struct bgp_attr_parser_args *args)
args->total);
}
if (tnl_type == PMSI_TNLTYPE_INGR_REPL) {
if (length != 9) {
/* The length should be one of these values:
* 1 (Flags) + 1 (Tunnel Type) + 3 (Label) + 4 (IPv4 address) = 9 octets
* 1 (Flags) + 1 (Tunnel Type) + 3 (Label) + 16 (IPv6 address) = 21 octets
*/
if (length != BGP_ATTR_PMSI_TUNNEL_V4_LENGTH &&
length != BGP_ATTR_PMSI_TUNNEL_V6_LENGTH) {
flog_err(EC_BGP_ATTR_PMSI_LEN,
"Bad PMSI tunnel attribute length %d for IR",
"Bad PMSI tunnel attribute length %d for Ingress Replication",
length);
return bgp_attr_malformed(
args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
Expand All @@ -3787,7 +3798,19 @@ bgp_attr_pmsi_tunnel(struct bgp_attr_parser_args *args)
bgp_attr_set_pmsi_tnl_type(attr, tnl_type);
stream_get(&attr->label, connection->curr, BGP_LABEL_BYTES);

/* Forward read pointer of input stream. */
/* Decode ingress-replication tunnel id */
if (tnl_type == PMSI_TNLTYPE_INGR_REPL) {
if (length == BGP_ATTR_PMSI_TUNNEL_V4_LENGTH) {
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;
}
}

/* Forward read pointer of input stream to skip anything we didn't parse */
stream_forward_getp(connection->curr, length - attr_parse_len);

return BGP_ATTR_PARSE_PROCEED;
Expand Down Expand Up @@ -5652,15 +5675,37 @@ bgp_size_t bgp_packet_attribute(struct bgp *bgp, struct peer *peer, struct strea

/* PMSI Tunnel */
if (bgp_attr_exists(attr, BGP_ATTR_PMSI_TUNNEL)) {
uint8_t tunn_id_len = 0;
uint8_t *nh;
struct in_addr tunn_id;

stream_putc(s, BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS);
stream_putc(s, BGP_ATTR_PMSI_TUNNEL);
stream_putc(s, 9); // Length
stream_putc(s, 0); // Flags

/* Encode tunnel id for known tunnel type */
if (bgp_attr_get_pmsi_tnl_type(attr) == PMSI_TNLTYPE_INGR_REPL) {
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);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shit I read this wrong. The ipv4_mapped_ipv6_to_ipv4 sets tunn_id.

nh = (uint8_t *)&tunn_id;
} else {
stream_putc(s, BGP_ATTR_PMSI_TUNNEL_V6_LENGTH);
tunn_id_len = IPV6_MAX_BYTELEN;
nh = (uint8_t *)&attr->tunn_id;
}
} else {
/* Encode label part only */
stream_putc(s, BGP_ATTR_PMSI_TUNNEL_LBL_ONLY_LEN);
}

stream_putc(s, 0); /* Flags */
stream_putc(s, bgp_attr_get_pmsi_tnl_type(attr));
stream_put(s, &(attr->label),
BGP_LABEL_BYTES); // MPLS Label / VXLAN VNI
stream_put_ipv4(s, attr->nexthop.s_addr);
// Unicast tunnel endpoint IP address
stream_put(s, &(attr->label), BGP_LABEL_BYTES); /* MPLS Label/VXLAN VNI */

/* Unicast tunnel endpoint IP address */
if (tunn_id_len > 0)
stream_put(s, nh, tunn_id_len);
}

/* OTC */
Expand Down
8 changes: 7 additions & 1 deletion bgpd/bgp_attr.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,11 @@ enum pta_type {
PMSI_TNLTYPE_MAX = PMSI_TNLTYPE_MLDP_MP2MP
};

/* PMSI lengths for label-only, ipv4, and ipv6 "identifier" */
#define BGP_ATTR_PMSI_TUNNEL_LBL_ONLY_LEN 5
#define BGP_ATTR_PMSI_TUNNEL_V4_LENGTH 9
#define BGP_ATTR_PMSI_TUNNEL_V6_LENGTH 21

/*
* Prefix-SID type-4
* SRv6-VPN-SID-TLV
Expand Down Expand Up @@ -217,6 +222,7 @@ struct attr {

/* PMSI tunnel type (RFC 6514). */
enum pta_type pmsi_tnl_type;
struct in6_addr tunn_id; /* PMSI Tunnel Id */

/* Multi-Protocol Nexthop, AFI IPv6 */
struct in6_addr mp_nexthop_global;
Expand Down Expand Up @@ -509,7 +515,7 @@ static inline uint32_t mac_mobility_seqnum(struct attr *attr)
return (attr) ? attr->mm_seqnum : 0;
}

static inline enum pta_type bgp_attr_get_pmsi_tnl_type(struct attr *attr)
static inline enum pta_type bgp_attr_get_pmsi_tnl_type(const struct attr *attr)
{
return attr->pmsi_tnl_type;
}
Expand Down
4 changes: 4 additions & 0 deletions bgpd/bgp_evpn.c
Original file line number Diff line number Diff line change
Expand Up @@ -2318,6 +2318,10 @@ static int update_evpn_route(struct bgp *bgp, struct bgpevpn *vpn,
if (p->prefix.route_type == BGP_EVPN_IMET_ROUTE) {
SET_FLAG(attr.flag, ATTR_FLAG_BIT(BGP_ATTR_PMSI_TUNNEL));
bgp_attr_set_pmsi_tnl_type(&attr, PMSI_TNLTYPE_INGR_REPL);
if (attr.mp_nexthop_len == BGP_ATTR_NHLEN_IPV4)
ipv4_to_ipv4_mapped_ipv6(&attr.tunn_id, attr.mp_nexthop_global_in);
else
IPV6_ADDR_COPY(&attr.tunn_id, &attr.mp_nexthop_global);
}

/* router mac is only needed for type-2 routes here. */
Expand Down
37 changes: 30 additions & 7 deletions bgpd/bgp_route.c
Original file line number Diff line number Diff line change
Expand Up @@ -12276,6 +12276,7 @@ void route_vty_out_detail(struct vty *vty, struct bgp *bgp, struct bgp_dest *bn,
bool nexthop_self =
CHECK_FLAG(path->flags, BGP_PATH_ANNC_NH_SELF) ? true : false;
int i;
const char *msgstr;
char *nexthop_hostname = bgp_nexthop_hostname(path->peer, path->nexthop);
char time_buf[64];
struct bgp_path_info *bpi_ultimate =
Expand Down Expand Up @@ -13318,19 +13319,41 @@ void route_vty_out_detail(struct vty *vty, struct bgp *bgp, struct bgp_dest *bn,

/* Line 10 display PMSI tunnel attribute, if present */
if (CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_PMSI_TUNNEL))) {
const char *str = lookup_msg(bgp_pmsi_tnltype_str,
bgp_attr_get_pmsi_tnl_type(attr),
PMSI_TNLTYPE_STR_DEFAULT);
msgstr = lookup_msg(bgp_pmsi_tnltype_str, bgp_attr_get_pmsi_tnl_type(attr),
PMSI_TNLTYPE_STR_DEFAULT);

if (json_paths) {
json_pmsi = json_object_new_object();
json_object_string_add(json_pmsi, "tunnelType", str);
json_object_string_add(json_pmsi, "tunnelType", msgstr);
json_object_int_add(json_pmsi, "label",
label2vni(&attr->label));

if (bgp_attr_get_pmsi_tnl_type(attr) == PMSI_TNLTYPE_INGR_REPL) {
if (IS_MAPPED_IPV6(&attr->tunn_id)) {
json_object_string_addf(
json_pmsi, "id", "%pI4",
(in_addr_t *)&attr->tunn_id.s6_addr32[3]);
} else {
json_object_string_addf(json_pmsi, "id", "%pI6",
&attr->tunn_id);
}
}
json_object_object_add(json_path, "pmsi", json_pmsi);
} else
vty_out(vty, " PMSI Tunnel Type: %s, label: %d\n",
str, label2vni(&attr->label));
} else if (bgp_attr_get_pmsi_tnl_type(attr) == PMSI_TNLTYPE_INGR_REPL) {
/* Include tunnel ID for known types */
if (IS_MAPPED_IPV6(&attr->tunn_id)) {
vty_out(vty, " PMSI Tunnel Type: %s, label: %d ID:%pI4\n",
msgstr, label2vni(&attr->label),
(in_addr_t *)&attr->tunn_id.s6_addr32[3]);
} else {
vty_out(vty, " PMSI Tunnel Type: %s, label: %d ID:%pI6\n",
msgstr, label2vni(&attr->label), &attr->tunn_id);
}
} else {
/* Label only */
vty_out(vty, " PMSI Tunnel Type: %s, label: %d\n", msgstr,
label2vni(&attr->label));
}
}

if (path->peer->connection->t_gr_restart &&
Expand Down
9 changes: 9 additions & 0 deletions tests/topotests/bgp_evpn_v6_pmsi_tunnel/r1/evpn_type3.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"101": {
"type": "L2",
"numRemoteVteps": 1,
"remoteVteps": [
"fd00:100::2"
]
}
}
15 changes: 15 additions & 0 deletions tests/topotests/bgp_evpn_v6_pmsi_tunnel/r1/frr.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
interface r1-eth0
ipv6 address fd00:100::1/64
!
router bgp 65000
bgp router-id 10.10.10.1
bgp log-neighbor-changes
no bgp default ipv4-unicast
neighbor fd00:100::2 remote-as 65000
neighbor fd00:100::2 capability extended-nexthop
!
address-family l2vpn evpn
neighbor fd00:100::2 activate
advertise-all-vni
exit-address-family
!
9 changes: 9 additions & 0 deletions tests/topotests/bgp_evpn_v6_pmsi_tunnel/r2/evpn_type3.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"101": {
"type": "L2",
"numRemoteVteps": 1,
"remoteVteps": [
"fd00:100::1"
]
}
}
15 changes: 15 additions & 0 deletions tests/topotests/bgp_evpn_v6_pmsi_tunnel/r2/frr.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
interface r2-eth0
ipv6 address fd00:100::2/64
!
router bgp 65000
bgp router-id 10.10.10.2
bgp log-neighbor-changes
no bgp default ipv4-unicast
neighbor fd00:100::1 remote-as 65000
neighbor fd00:100::1 capability extended-nexthop
!
address-family l2vpn evpn
neighbor fd00:100::1 activate
advertise-all-vni
exit-address-family
!
Loading
Loading