From f4d7cb0e9b365347015455d42ea69b115303da76 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Mon, 14 Oct 2019 20:44:56 -0400 Subject: [PATCH 1/3] bgpd: Properly lock parent node for type4 routes When creating a bgp_path_info for a type 4 route the pi->extra->parent and the route node for the originating table were not being locked properly. This will prevent BGP from not properly cleaning up the data structures on cleanup. Possibly every one of the functions that we use to create the new bgp_path_info's should use an abstracted version of this code, but I am unsure at this point in time if a type 4 should use the same or not. Signed-off-by: Donald Sharp --- bgpd/bgp_evpn.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index 50fef00a96..a65a80aeba 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -2424,7 +2424,8 @@ static int install_evpn_route_entry_in_es(struct bgp *bgp, struct evpnes *es, parent_pi->peer, attr_new, rn); SET_FLAG(pi->flags, BGP_PATH_VALID); bgp_path_info_extra_get(pi); - pi->extra->parent = parent_pi; + pi->extra->parent = bgp_path_info_lock(parent_pi); + bgp_lock_node((struct bgp_node *)parent_pi->net); bgp_path_info_add(rn, pi); } else { if (attrhash_cmp(pi->attr, parent_pi->attr) From 6b74234908e9d25857dfdc4f13ef3951092b663d Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Mon, 14 Oct 2019 21:05:15 -0400 Subject: [PATCH 2/3] bgpd: Refactor bgp_path_info creation We are doing the same thing in multiple places. Refactor. Signed-off-by: Donald Sharp --- bgpd/bgp_evpn.c | 66 ++++++++++++++++++++++--------------------------- 1 file changed, 30 insertions(+), 36 deletions(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index a65a80aeba..f8cd3de68c 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -2394,6 +2394,30 @@ static int handle_tunnel_ip_change(struct bgp *bgp, struct bgpevpn *vpn, return 0; } +static void bgp_create_evpn_bgp_path_info(struct bgp_path_info *parent_pi, + struct bgp_node *rn) +{ + struct attr *attr_new; + struct bgp_path_info *pi; + + /* Add (or update) attribute to hash. */ + attr_new = bgp_attr_intern(parent_pi->attr); + + /* Create new route with its attribute. */ + pi = info_make(parent_pi->type, BGP_ROUTE_IMPORTED, 0, parent_pi->peer, + attr_new, rn); + SET_FLAG(pi->flags, BGP_PATH_VALID); + bgp_path_info_extra_get(pi); + pi->extra->parent = bgp_path_info_lock(parent_pi); + bgp_lock_node((struct bgp_node *)parent_pi->net); + if (parent_pi->extra) { + memcpy(&pi->extra->label, &parent_pi->extra->label, + sizeof(pi->extra->label)); + pi->extra->num_labels = parent_pi->extra->num_labels; + } + bgp_path_info_add(rn, pi); +} + /* Install EVPN route entry in ES */ static int install_evpn_route_entry_in_es(struct bgp *bgp, struct evpnes *es, struct prefix_evpn *p, @@ -2517,24 +2541,9 @@ static int install_evpn_route_entry_in_vrf(struct bgp *bgp_vrf, && (struct bgp_path_info *)pi->extra->parent == parent_pi) break; - if (!pi) { - /* Add (or update) attribute to hash. */ - attr_new = bgp_attr_intern(&attr); - - /* Create new route with its attribute. */ - pi = info_make(parent_pi->type, BGP_ROUTE_IMPORTED, 0, - parent_pi->peer, attr_new, rn); - SET_FLAG(pi->flags, BGP_PATH_VALID); - bgp_path_info_extra_get(pi); - pi->extra->parent = bgp_path_info_lock(parent_pi); - bgp_lock_node((struct bgp_node *)parent_pi->net); - if (parent_pi->extra) { - memcpy(&pi->extra->label, &parent_pi->extra->label, - sizeof(pi->extra->label)); - pi->extra->num_labels = parent_pi->extra->num_labels; - } - bgp_path_info_add(rn, pi); - } else { + if (!pi) + bgp_create_evpn_bgp_path_info(parent_pi, rn); + else { if (attrhash_cmp(pi->attr, &attr) && !CHECK_FLAG(pi->flags, BGP_PATH_REMOVED)) { bgp_unlock_node(rn); @@ -2596,24 +2605,9 @@ static int install_evpn_route_entry(struct bgp *bgp, struct bgpevpn *vpn, && (struct bgp_path_info *)pi->extra->parent == parent_pi) break; - if (!pi) { - /* Add (or update) attribute to hash. */ - attr_new = bgp_attr_intern(parent_pi->attr); - - /* Create new route with its attribute. */ - pi = info_make(parent_pi->type, BGP_ROUTE_IMPORTED, 0, - parent_pi->peer, attr_new, rn); - SET_FLAG(pi->flags, BGP_PATH_VALID); - bgp_path_info_extra_get(pi); - pi->extra->parent = bgp_path_info_lock(parent_pi); - bgp_lock_node((struct bgp_node *)parent_pi->net); - if (parent_pi->extra) { - memcpy(&pi->extra->label, &parent_pi->extra->label, - sizeof(pi->extra->label)); - pi->extra->num_labels = parent_pi->extra->num_labels; - } - bgp_path_info_add(rn, pi); - } else { + if (!pi) + bgp_create_evpn_bgp_path_info(parent_pi, rn); + else { if (attrhash_cmp(pi->attr, parent_pi->attr) && !CHECK_FLAG(pi->flags, BGP_PATH_REMOVED)) { bgp_unlock_node(rn); From a51743300c3a05fba849b45099b8095a8610a905 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Mon, 14 Oct 2019 21:09:55 -0400 Subject: [PATCH 3/3] bgpd: Be careful about displaying vni's as labels. When a type 2/3 or 5 route is received, verified and the resulting route generated is pushed into the appropriate vrf the vni's associated with the route are also passed in. This is showing up as a Remote label when you dump the route in bgp: BGP routing table entry for 0.0.0.0/0^M Paths: (1 available, best #1, table third) Advertised to non peer-group peers: 10.10.120.22 42001 42005 42006 42055 10.10.120.22 from 10.10.120.22 (10.10.255.193) Origin IGP, valid, external, bestpath-from-AS 42001, best Remote label: 62750 AddPath ID: RX 0, TX 2 Last update: Fri Oct 11 12:59:56 2019 The `Remote label: 62750` is the mpls label version of the vni passed in. This is meaningless and confusing to the end user. Do not display this information. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index fa64d3dd62..1305f18fe8 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -8967,7 +8967,7 @@ void route_vty_out_detail(struct vty *vty, struct bgp *bgp, /* Remote Label */ if (path->extra && bgp_is_valid_label(&path->extra->label[0]) - && safi != SAFI_EVPN) { + && (safi != SAFI_EVPN && !is_route_parent_evpn(path))) { mpls_label_t label = label_pton(&path->extra->label[0]); if (json_paths)