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 <equinox@opensourcerouting.org>

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index d3f129475137ab6be4ea1e3a52e411f701aa1bc7..338fb6cd4062df1364fa34eb04604d085be75d2b 100644 (file)
--- 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 d404c9046e59b1e704cced00f8533b05c7b8a03e..6edbf439025ad9cce9b15509a51aec32c8688edd 100644 (file)
--- 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 8b9d55295d123b863226e4833cdc8a25f7519024..5d9d0ddd1ad448760268badc709dfb16b94c6d47 100644 (file)
--- 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(
+                                  apply doesn't modify the BGP route info. */
+                               local_attr = *mpinfo->attr;
+                               mpinfo_cp->attr = &local_attr;
+
+                               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;
+                                           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;
                                }
-                               BGP_INFO_ATTR_BUF_FREE(info_cp);
-                       } else
-                               nexthop = &mpinfo->attr->nexthop;
+                       }
 
-                       if (nexthop == NULL)
-                               continue;
+                       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(
+                                  apply doesn't modify the BGP route info. */
+                               local_attr = *mpinfo->attr;
+                               mpinfo_cp->attr = &local_attr;
+
+                               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);
+                                           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;
                                }
-                               BGP_INFO_ATTR_BUF_FREE(info_cp);
-                       } else
-                               nexthop = bgp_info_to_ipv6_nexthop(mpinfo);
-
-                       if (nexthop == NULL)
-                               continue;
+                       }
+                       nexthop = bgp_info_to_ipv6_nexthop(mpinfo_cp);
 
                        if ((mpinfo == info)
                            && mpinfo->attr->mp_nexthop_len