From 96099b4030d374cf1d34b3a6f3fa0e2dc1ed7a99 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Thu, 7 Sep 2017 14:54:42 +0200 Subject: [PATCH 1/6] bgpd: add safety check on ATTR_FLAG_BIT Signed-off-by: David Lamparter --- bgpd/bgp_attr.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h index 4dd38459f8..ea1e736520 100644 --- a/bgpd/bgp_attr.h +++ b/bgpd/bgp_attr.h @@ -207,7 +207,12 @@ struct transit { u_char *val; }; -#define ATTR_FLAG_BIT(X) (1ULL << ((X) - 1)) +/* "(void) 0" will generate a compiler error. this is a safety check to + * ensure we're not using a value that exceeds the bit size of attr->flag. */ +#define ATTR_FLAG_BIT(X) \ + __builtin_choose_expr((X) >= 1 && (X) <= 64, \ + 1ULL << ((X) - 1), \ + (void) 0) #define BGP_CLUSTER_LIST_LENGTH(attr) \ (((attr)->flag & ATTR_FLAG_BIT(BGP_ATTR_CLUSTER_LIST)) \ From 7c87afac92b94cd26c1a74df0e499a7a05603aa7 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Thu, 7 Sep 2017 14:24:00 +0200 Subject: [PATCH 2/6] bgpd: kill bgp_attr_refcount() This attempt at optimization has cost us more than a week's worth of time on several people hunting down the subtle bug that it was missing an increment on attr->lcommunity. This is absolutely not worth the maintenance cost. Signed-off-by: David Lamparter --- bgpd/bgp_attr.c | 39 --------------------------------------- bgpd/bgp_attr.h | 1 - bgpd/bgp_updgrp.c | 2 +- 3 files changed, 1 insertion(+), 41 deletions(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 03cf3625b7..d3f1294751 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -771,45 +771,6 @@ struct attr *bgp_attr_intern(struct attr *attr) return find; } -/** - * Increment the refcount on various structures that attr holds. - * Note on usage: call _only_ when the 'attr' object has already - * been 'intern'ed and exists in 'attrhash' table. The function - * serves to hold a reference to that (real) object. - * Note also that the caller can safely call bgp_attr_unintern() - * after calling bgp_attr_refcount(). That would release the - * reference and could result in a free() of the attr object. - */ -struct attr *bgp_attr_refcount(struct attr *attr) -{ - /* Intern referenced strucutre. */ - if (attr->aspath) - attr->aspath->refcnt++; - - if (attr->community) - attr->community->refcnt++; - - if (attr->ecommunity) - attr->ecommunity->refcnt++; - - if (attr->cluster) - attr->cluster->refcnt++; - - if (attr->transit) - attr->transit->refcnt++; - - if (attr->encap_subtlvs) - attr->encap_subtlvs->refcnt++; - -#if ENABLE_BGP_VNC - if (attr->vnc_subtlvs) - attr->vnc_subtlvs->refcnt++; -#endif - - attr->refcnt++; - return attr; -} - /* Make network statement's attribute. */ struct attr *bgp_attr_default_set(struct attr *attr, u_char origin) { diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h index ea1e736520..d404c9046e 100644 --- a/bgpd/bgp_attr.h +++ b/bgpd/bgp_attr.h @@ -241,7 +241,6 @@ extern void bgp_attr_dup(struct attr *, struct attr *); extern void bgp_attr_deep_dup(struct attr *, struct attr *); extern void bgp_attr_deep_free(struct attr *); extern struct attr *bgp_attr_intern(struct attr *attr); -extern struct attr *bgp_attr_refcount(struct attr *attr); extern void bgp_attr_unintern_sub(struct attr *); extern void bgp_attr_unintern(struct attr **); extern void bgp_attr_flush(struct attr *); diff --git a/bgpd/bgp_updgrp.c b/bgpd/bgp_updgrp.c index 9630afb717..05395a583a 100644 --- a/bgpd/bgp_updgrp.c +++ b/bgpd/bgp_updgrp.c @@ -1185,7 +1185,7 @@ static void update_subgroup_copy_adj_out(struct update_subgroup *source, aout_copy = bgp_adj_out_alloc(dest, aout->rn, aout->addpath_tx_id); aout_copy->attr = - aout->attr ? bgp_attr_refcount(aout->attr) : NULL; + aout->attr ? bgp_attr_intern(aout->attr) : NULL; } } From b4cb15c6670f649b62595793857f794fad41f884 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Thu, 7 Sep 2017 15:19:06 +0200 Subject: [PATCH 3/6] bgpd: rip out bgp_attr_deep_dup(), fix table-map bgp_attr_deep_dup is based on a misunderstanding of how route-maps work. They never change actual data, just pointers & fields in "struct attr". The correct thing to do is copy struct attr and call bgp_attr_flush() afterwards. Signed-off-by: David Lamparter --- bgpd/bgp_attr.c | 44 ----------------- bgpd/bgp_attr.h | 2 - bgpd/bgp_zebra.c | 121 +++++++++++++++++------------------------------ 3 files changed, 43 insertions(+), 124 deletions(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index d3f1294751..338fb6cd40 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -508,50 +508,6 @@ void bgp_attr_dup(struct attr *new, struct attr *orig) *new = *orig; } -void bgp_attr_deep_dup(struct attr *new, struct attr *orig) -{ - if (orig->aspath) - new->aspath = aspath_dup(orig->aspath); - - if (orig->community) - new->community = community_dup(orig->community); - - if (orig->ecommunity) - new->ecommunity = ecommunity_dup(orig->ecommunity); - if (orig->cluster) - new->cluster = cluster_dup(orig->cluster); - if (orig->transit) - new->transit = transit_dup(orig->transit); - if (orig->encap_subtlvs) - new->encap_subtlvs = encap_tlv_dup(orig->encap_subtlvs); -#if ENABLE_BGP_VNC - if (orig->vnc_subtlvs) - new->vnc_subtlvs = encap_tlv_dup(orig->vnc_subtlvs); -#endif -} - -void bgp_attr_deep_free(struct attr *attr) -{ - if (attr->aspath) - aspath_free(attr->aspath); - - if (attr->community) - community_free(attr->community); - - if (attr->ecommunity) - ecommunity_free(&attr->ecommunity); - if (attr->cluster) - cluster_free(attr->cluster); - if (attr->transit) - transit_free(attr->transit); - if (attr->encap_subtlvs) - encap_free(attr->encap_subtlvs); -#if ENABLE_BGP_VNC - if (attr->vnc_subtlvs) - encap_free(attr->vnc_subtlvs); -#endif -} - unsigned long int attr_count(void) { return attrhash->count; diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h index d404c9046e..6edbf43902 100644 --- a/bgpd/bgp_attr.h +++ b/bgpd/bgp_attr.h @@ -238,8 +238,6 @@ extern bgp_attr_parse_ret_t bgp_attr_parse(struct peer *, struct attr *, bgp_size_t, struct bgp_nlri *, struct bgp_nlri *); extern void bgp_attr_dup(struct attr *, struct attr *); -extern void bgp_attr_deep_dup(struct attr *, struct attr *); -extern void bgp_attr_deep_free(struct attr *); extern struct attr *bgp_attr_intern(struct attr *attr); extern void bgp_attr_unintern_sub(struct attr *); extern void bgp_attr_unintern(struct attr **); diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c index 8b9d55295d..5d9d0ddd1a 100644 --- a/bgpd/bgp_zebra.c +++ b/bgpd/bgp_zebra.c @@ -58,41 +58,6 @@ /* All information about zebra. */ struct zclient *zclient = NULL; -/* These array buffers are used in making a copy of the attributes for - route-map apply. Arrays are being used here to minimize mallocs and - frees for the temporary copy of the attributes. - Given the zapi api expects the nexthop buffer to contain pointer to - pointers for nexthops, we couldnt have used a single nexthop variable - on the stack, hence we had two options: - 1. maintain a linked-list and free it after zapi_*_route call - 2. use an array to avoid number of mallocs. - Number of supported next-hops are finite, use of arrays should be ok. */ -struct attr attr_cp[MULTIPATH_NUM]; -unsigned int attr_index = 0; - -/* Once per address-family initialization of the attribute array */ -#define BGP_INFO_ATTR_BUF_INIT() \ - do { \ - memset(attr_cp, 0, MULTIPATH_NUM * sizeof(struct attr)); \ - attr_index = 0; \ - } while (0) - -#define BGP_INFO_ATTR_BUF_COPY(info_src, info_dst) \ - do { \ - *info_dst = *info_src; \ - assert(attr_index != multipath_num); \ - bgp_attr_dup(&attr_cp[attr_index], info_src->attr); \ - bgp_attr_deep_dup(&attr_cp[attr_index], info_src->attr); \ - info_dst->attr = &attr_cp[attr_index]; \ - attr_index++; \ - } while (0) - -#define BGP_INFO_ATTR_BUF_FREE(info) \ - do { \ - bgp_attr_deep_free(info->attr); \ - } while (0) - - /* Can we install into zebra? */ static inline int bgp_install_info_to_zebra(struct bgp *bgp) { @@ -950,7 +915,12 @@ static struct in6_addr *bgp_info_to_ipv6_nexthop(struct bgp_info *info) static int bgp_table_map_apply(struct route_map *map, struct prefix *p, struct bgp_info *info) { - if (route_map_apply(map, p, RMAP_BGP, info) != RMAP_DENYMATCH) + route_map_result_t ret; + + ret = route_map_apply(map, p, RMAP_BGP, info); + bgp_attr_flush(info->attr); + + if (ret != RMAP_DENYMATCH) return 1; if (bgp_debug_zebra(p)) { @@ -992,8 +962,9 @@ void bgp_zebra_announce(struct bgp_node *rn, struct prefix *p, struct peer *peer; struct bgp_info *mpinfo; u_int32_t metric; + struct attr local_attr; struct bgp_info local_info; - struct bgp_info *info_cp = &local_info; + struct bgp_info *mpinfo_cp = &local_info; route_tag_t tag; mpls_label_t label; @@ -1046,37 +1017,34 @@ void bgp_zebra_announce(struct bgp_node *rn, struct prefix *p, else return; - if (bgp->table_map[afi][safi].name) - BGP_INFO_ATTR_BUF_INIT(); - /* Metric is currently based on the best-path only */ metric = info->attr->med; for (mpinfo = info; mpinfo; mpinfo = bgp_info_mpath_next(mpinfo)) { + *mpinfo_cp = *mpinfo; + if (nh_family == AF_INET) { struct in_addr *nexthop; - nexthop = NULL; - if (bgp->table_map[afi][safi].name) { /* Copy info and attributes, so the route-map - apply doesn't modify the - BGP route info. */ - BGP_INFO_ATTR_BUF_COPY(mpinfo, info_cp); - if (bgp_table_map_apply( - bgp->table_map[afi][safi].map, p, - info_cp)) { - if (mpinfo == info) { - metric = info_cp->attr->med; - tag = info_cp->attr->tag; - } - nexthop = &info_cp->attr->nexthop; - } - BGP_INFO_ATTR_BUF_FREE(info_cp); - } else - nexthop = &mpinfo->attr->nexthop; + apply doesn't modify the BGP route info. */ + local_attr = *mpinfo->attr; + mpinfo_cp->attr = &local_attr; - if (nexthop == NULL) - continue; + if (!bgp_table_map_apply( + bgp->table_map[afi][safi].map, p, + mpinfo_cp)) + continue; + + /* metric/tag is only allowed to be + * overridden on 1st nexthop */ + if (mpinfo == info) { + metric = mpinfo_cp->attr->med; + tag = mpinfo_cp->attr->tag; + } + } + + nexthop = &mpinfo_cp->attr->nexthop; api_nh = &api.nexthops[valid_nh_count]; api_nh->gate.ipv4 = *nexthop; @@ -1086,29 +1054,26 @@ void bgp_zebra_announce(struct bgp_node *rn, struct prefix *p, struct in6_addr *nexthop; ifindex = 0; - nexthop = NULL; if (bgp->table_map[afi][safi].name) { /* Copy info and attributes, so the route-map - apply doesn't modify the - BGP route info. */ - BGP_INFO_ATTR_BUF_COPY(mpinfo, info_cp); - if (bgp_table_map_apply( - bgp->table_map[afi][safi].map, p, - info_cp)) { - if (mpinfo == info) { - metric = info_cp->attr->med; - tag = info_cp->attr->tag; - } - nexthop = bgp_info_to_ipv6_nexthop( - info_cp); - } - BGP_INFO_ATTR_BUF_FREE(info_cp); - } else - nexthop = bgp_info_to_ipv6_nexthop(mpinfo); + apply doesn't modify the BGP route info. */ + local_attr = *mpinfo->attr; + mpinfo_cp->attr = &local_attr; - if (nexthop == NULL) - continue; + if (!bgp_table_map_apply( + bgp->table_map[afi][safi].map, p, + mpinfo_cp)) + continue; + + /* metric/tag is only allowed to be + * overridden on 1st nexthop */ + if (mpinfo == info) { + metric = mpinfo_cp->attr->med; + tag = mpinfo_cp->attr->tag; + } + } + nexthop = bgp_info_to_ipv6_nexthop(mpinfo_cp); if ((mpinfo == info) && mpinfo->attr->mp_nexthop_len From 821127e041849eacca89d052d543f50a02124823 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Thu, 7 Sep 2017 15:28:35 +0200 Subject: [PATCH 4/6] bgpd: remove transit_dup() & cluster_dup() These are now unused. route-maps can't modify these attributes, so there is no need for _dup functions. Signed-off-by: David Lamparter --- bgpd/bgp_attr.c | 31 ------------------------------- 1 file changed, 31 deletions(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 338fb6cd40..fe43afeb39 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -161,22 +161,6 @@ static void cluster_free(struct cluster_list *cluster) XFREE(MTYPE_CLUSTER, cluster); } -static struct cluster_list *cluster_dup(struct cluster_list *cluster) -{ - struct cluster_list *new; - - new = XCALLOC(MTYPE_CLUSTER, sizeof(struct cluster_list)); - new->length = cluster->length; - - if (cluster->length) { - new->list = XMALLOC(MTYPE_CLUSTER_VAL, cluster->length); - memcpy(new->list, cluster->list, cluster->length); - } else - new->list = NULL; - - return new; -} - static struct cluster_list *cluster_intern(struct cluster_list *cluster) { struct cluster_list *find; @@ -422,21 +406,6 @@ static void transit_free(struct transit *transit) XFREE(MTYPE_TRANSIT, transit); } -static struct transit *transit_dup(struct transit *transit) -{ - struct transit *new; - - new = XCALLOC(MTYPE_TRANSIT, sizeof(struct transit)); - new->length = transit->length; - if (new->length) { - new->val = XMALLOC(MTYPE_TRANSIT_VAL, transit->length); - memcpy(new->val, transit->val, transit->length); - } else - new->val = NULL; - - return new; -} - static void *transit_hash_alloc(void *p) { /* Transit structure is already allocated. */ From dbbac180c111d55b473b2352d95ef3a17695a7db Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Thu, 7 Sep 2017 15:27:12 +0200 Subject: [PATCH 5/6] bgpd: add comment on bgp_attr_intern This is confusing enough to warrant a comment. Signed-off-by: David Lamparter --- bgpd/bgp_attr.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index fe43afeb39..97b6273cb4 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -690,6 +690,12 @@ struct attr *bgp_attr_intern(struct attr *attr) } #endif + /* At this point, attr only contains intern'd pointers. that means + * if we find it in attrhash, it has all the same pointers and we + * correctly updated the refcounts on these. + * If we don't find it, we need to allocate a one because in all + * cases this returns a new reference to a hashed attr, but the input + * wasn't on hash. */ find = (struct attr *)hash_get(attrhash, attr, bgp_attr_hash_alloc); find->refcnt++; From be054588b43a222a128840011259b5f8e6c8a2cd Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Thu, 7 Sep 2017 15:31:00 +0200 Subject: [PATCH 6/6] bgpd: fix compiler warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bgp_route.c:6393:7: error: ‘len’ may be used uninitialized in this function gcc 5.4.0 isn't intelligent enough to notice it's set on all paths. Signed-off-by: David Lamparter --- 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 f354071654..fabe4483e5 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -6339,7 +6339,7 @@ void bgp_redistribute_withdraw(struct bgp *bgp, afi_t afi, int type, static void route_vty_out_route(struct prefix *p, struct vty *vty, json_object *json) { - int len; + int len = 0; u_int32_t destination; char buf[BUFSIZ];