bgpd: check and set extra num_labels

The handling of MPLS labels in BGP faces an issue due to the way labels
are stored in memory. They are stored in bgp_path_info but not in
bgp_adj_in and bgp_adj_out structures. As a consequence, some
configuration changes result in losing labels or even a bgpd crash. For
example, when retrieving routes from the Adj-RIB-in table
("soft-reconfiguration inbound" enabled), labels are missing.

bgp_path_info stores the MPLS labels, as shown below:

> struct bgp_path_info {
>   struct bgp_path_info_extra *extra;
>   [...]
> struct bgp_path_info_extra {
>	mpls_label_t label[BGP_MAX_LABELS];
>	uint32_t num_labels;
>   [...]

To solve those issues, a solution would be to set label data to the
bgp_adj_in and bgp_adj_out structures in addition to the
bgp_path_info_extra structure. The idea is to reference a common label
pointer in all these three structures. And to store the data in a hash
list in order to save memory.

However, an issue in the code prevents us from setting clean data
without a rework. The extra->num_labels field, which is intended to
indicate the number of labels in extra->label[], is not reliably checked
or set. The code often incorrectly assumes that if the extra pointer is
present, then a label must also be present, leading to direct access to
extra->label[] without verifying extra->num_labels. This assumption
usually works because extra->label[0] is set to MPLS_INVALID_LABEL when
a new bgp_path_info_extra is created, but it is technically incorrect.

Cleanup the label code by setting num_labels each time values are set in
extra->label[] and checking extra->num_labels before accessing the
labels.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>

diff --git a/bgpd/bgp_label.c b/bgpd/bgp_label.c
index 68104967b2fde21b0fb34e0d5a18d92cb5a3208e..d5732b5677214f33c134d9b578ed8bfcd340a888 100644 (file)
--- a/bgpd/bgp_label.c
+++ b/bgpd/bgp_label.c
@@ -89,7 +89,9 @@ mpls_label_t bgp_adv_label(struct bgp_dest *dest, struct bgp_path_info *pi,
        if (!dest || !pi || !to)
                return MPLS_INVALID_LABEL;
 
-       remote_label = pi->extra ? pi->extra->label[0] : MPLS_INVALID_LABEL;
+       remote_label = (pi->extra && pi->extra->num_labels)
+                              ? pi->extra->label[0]
+                              : MPLS_INVALID_LABEL;
        from = pi->peer;
        reflect =
                ((from->sort == BGP_PEER_IBGP) && (to->sort == BGP_PEER_IBGP));

diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c
index 39f8f84a02ebd1baa7b301e8f2fa8634867ef324..7fe4d6ece5a7da408f7d63a9e838fb1316904d0b 100644 (file)
--- a/bgpd/bgp_mplsvpn.c
+++ b/bgpd/bgp_mplsvpn.c
@@ -4130,7 +4130,8 @@ bool bgp_mplsvpn_path_uses_valid_mpls_label(struct bgp_path_info *pi)
                /* prefix_sid attribute */
                return false;
 
-       if (!pi->extra || !bgp_is_valid_label(&pi->extra->label[0]))
+       if (!pi->extra || !pi->extra->num_labels ||
+           !bgp_is_valid_label(&pi->extra->label[0]))
                /* invalid MPLS label */
                return false;
        return true;
@@ -4237,14 +4238,16 @@ void bgp_mplsvpn_nh_label_bind_register_local_label(struct bgp *bgp,
 {
        struct bgp_mplsvpn_nh_label_bind_cache *bmnc;
        struct bgp_mplsvpn_nh_label_bind_cache_head *tree;
+       mpls_label_t label;
+
+       label = pi->extra->num_labels ? decode_label(&pi->extra->label[0])
+                                     : MPLS_INVALID_LABEL;
 
        tree = &bgp->mplsvpn_nh_label_bind;
-       bmnc = bgp_mplsvpn_nh_label_bind_find(
-               tree, &pi->nexthop->prefix, decode_label(&pi->extra->label[0]));
+       bmnc = bgp_mplsvpn_nh_label_bind_find(tree, &pi->nexthop->prefix, label);
        if (!bmnc) {
-               bmnc = bgp_mplsvpn_nh_label_bind_new(
-                       tree, &pi->nexthop->prefix,
-                       decode_label(&pi->extra->label[0]));
+               bmnc = bgp_mplsvpn_nh_label_bind_new(tree, &pi->nexthop->prefix,
+                                                    label);
                bmnc->bgp_vpn = bgp;
                bmnc->allocation_in_progress = true;
                bgp_lp_get(LP_TYPE_BGP_L3VPN_BIND, bmnc,

diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 2309b710ed2cb0491eb848441c8cf6358c9d55fd..af440fce474b3fdc2a44f09186ad42949a074986 100644 (file)
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -1359,25 +1359,20 @@ int bgp_path_info_cmp(struct bgp *bgp, struct bgp_path_info *new,
                /* If one path has a label but the other does not, do not treat
                 * them as equals for multipath
                 */
-               int newl, existl;
-
-               newl = existl = 0;
-
-               if (new->extra)
-                       newl = new->extra->num_labels;
-               if (exist->extra)
-                       existl = exist->extra->num_labels;
-               if (((new->extra &&bgp_is_valid_label(&new->extra->label[0])) !=
-                    (exist->extra &&
-                     bgp_is_valid_label(&exist->extra->label[0]))) ||
-                   (newl != existl)) {
+               bool new_label_valid, exist_label_valid;
+
+               new_label_valid = new->extra && new->extra->num_labels &&
+                                   bgp_is_valid_label(&new->extra->label[0]);
+               exist_label_valid = exist->extra && exist->extra->num_labels &&
+                                   bgp_is_valid_label(&exist->extra->label[0]);
+
+               if (new_label_valid != exist_label_valid) {
                        if (debug)
                                zlog_debug(
                                        "%s: %s and %s cannot be multipath, one has a label while the other does not",
                                        pfx_buf, new_buf, exist_buf);
                } else if (CHECK_FLAG(bgp->flags,
                                      BGP_FLAG_ASPATH_MULTIPATH_RELAX)) {
-
                        /*
                         * For the two paths, all comparison steps till IGP
                         * metric
@@ -3455,7 +3450,7 @@ static bool bgp_lu_need_null_label(struct bgp *bgp,
            || new_select->sub_type == BGP_ROUTE_AGGREGATE
            || new_select->sub_type == BGP_ROUTE_REDISTRIBUTE)
                goto need_null_label;
-       else if (new_select->extra &&
+       else if (new_select->extra && new_select->extra->num_labels &&
                 bgp_is_valid_label(&new_select->extra->label[0]))
                return false;
 need_null_label:
@@ -6678,7 +6673,7 @@ void bgp_static_update(struct bgp *bgp, const struct prefix *p,
        route_map_result_t ret;
 #ifdef ENABLE_BGP_VNC
        int vnc_implicit_withdraw = 0;
-       mpls_label_t label = 0;
+       mpls_label_t label = MPLS_INVALID_LABEL;
 #endif
        uint32_t num_labels = 0;
        struct bgp *bgp_nexthop = bgp;
@@ -6831,7 +6826,7 @@ void bgp_static_update(struct bgp *bgp, const struct prefix *p,
                                                bgp, p, pi);
                                }
                        } else {
-                               if (pi->extra)
+                               if (pi->extra && pi->extra->num_labels)
                                        label = decode_label(
                                                &pi->extra->label[0]);
                        }
@@ -10069,7 +10064,8 @@ void route_vty_out_tag(struct vty *vty, const struct prefix *p,
                }
        }
 
-       if (bgp_is_valid_label(&path->extra->label[0])) {
+       if (path->extra->num_labels &&
+           bgp_is_valid_label(&path->extra->label[0])) {
                label = decode_label(&path->extra->label[0]);
                if (json) {
                        json_object_int_add(json_out, "notag", label);

diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c
index 1c2f08465d70b5ff76c181f1d68474e7b65300df..49b3ef371270511d9c1996ab0b59046ba0a53beb 100644 (file)
--- a/bgpd/bgp_zebra.c
+++ b/bgpd/bgp_zebra.c
@@ -1341,15 +1341,13 @@ static void bgp_zebra_announce_parse_nexthop(
                api_nh->srte_color = bgp_attr_get_color(info->attr);
 
                if (bgp_debug_zebra(&api->prefix)) {
-                       if (mpinfo->extra) {
+                       if (mpinfo->extra && mpinfo->extra->num_labels) {
                                zlog_debug("%s: p=%pFX, bgp_is_valid_label: %d",
                                           __func__, p,
                                           bgp_is_valid_label(
                                                   &mpinfo->extra->label[0]));
                        } else {
-                               zlog_debug(
-                                       "%s: p=%pFX, extra is NULL, no label",
-                                       __func__, p);
+                               zlog_debug("%s: p=%pFX, no label", __func__, p);
                        }
                }
 

diff --git a/bgpd/rfapi/rfapi.c b/bgpd/rfapi/rfapi.c
index ae899daf82d3e97868408ed80575858166bfa140..bea9bfb618fbcd5da5801757c5064ca09045df69 100644 (file)
--- a/bgpd/rfapi/rfapi.c
+++ b/bgpd/rfapi/rfapi.c
@@ -592,7 +592,7 @@ void add_vnc_route(struct rfapi_descriptor *rfd, /* cookie, VPN UN addr, peer */
                }
        }
 
-       if (label)
+       if (label && *label != MPLS_INVALID_LABEL)
                label_val = *label;
        else
                label_val = MPLS_LABEL_IMPLICIT_NULL;
@@ -1020,7 +1020,9 @@ void add_vnc_route(struct rfapi_descriptor *rfd, /* cookie, VPN UN addr, peer */
        new->extra->vnc = XCALLOC(MTYPE_BGP_ROUTE_EXTRA_VNC,
                                  sizeof(struct bgp_path_info_extra_vnc));
        new->extra->vnc->vnc.export.rfapi_handle = (void *)rfd;
+
        encode_label(label_val, &new->extra->label[0]);
+       new->extra->num_labels = 1;
 
        /* debug */
 
@@ -1043,6 +1045,7 @@ void add_vnc_route(struct rfapi_descriptor *rfd, /* cookie, VPN UN addr, peer */
                                bgp, prd, table, p, new);
                bgp_dest_unlock_node(pdest);
                encode_label(label_val, &bn->local_label);
+               new->extra->num_labels = 1;
        }
 
        bgp_dest_unlock_node(bn);

diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c
index 168b8e4c1e2e37592eb8b0c13fb9276325c4455f..7fb7d5d7aac532f15b79a25a32c0c2559da0f9aa 100644 (file)
--- a/bgpd/rfapi/rfapi_import.c
+++ b/bgpd/rfapi/rfapi_import.c
@@ -454,8 +454,11 @@ static struct bgp_path_info *rfapiBgpInfoCreate(struct attr *attr,
                new->extra->vnc->vnc.import.rd = *prd;
                new->extra->vnc->vnc.import.create_time = monotime(NULL);
        }
-       if (label)
+       if (label && *label != MPLS_INVALID_LABEL) {
                encode_label(*label, &new->extra->label[0]);
+               new->extra->num_labels = 1;
+       } else
+               new->extra->num_labels = 0;
 
        peer_lock(peer);
 
@@ -1267,7 +1270,10 @@ rfapiRouteInfo2NextHopEntry(struct rfapi_ip_prefix *rprefix,
                        bpi->extra->vnc->vnc.import.rd.val[1];
 
                /* label comes from MP_REACH_NLRI label */
-               vo->v.l2addr.label = decode_label(&bpi->extra->label[0]);
+               vo->v.l2addr.label =
+                       bpi->extra->num_labels
+                               ? decode_label(&bpi->extra->label[0])
+                               : MPLS_INVALID_LABEL;
 
                new->vn_options = vo;
 
@@ -4154,13 +4160,13 @@ static void rfapiBgpTableFilteredImport(struct bgp *bgp,
 
                                for (bpi = bgp_dest_get_bgp_path_info(dest2);
                                     bpi; bpi = bpi->next) {
-                                       uint32_t label = 0;
+                                       uint32_t label = MPLS_INVALID_LABEL;
 
                                        if (CHECK_FLAG(bpi->flags,
                                                       BGP_PATH_REMOVED))
                                                continue;
 
-                                       if (bpi->extra)
+                                       if (bpi->extra && bpi->extra->num_labels)
                                                label = decode_label(
                                                        &bpi->extra->label[0]);
                                        (*rfapiBgpInfoFilteredImportFunction(

diff --git a/bgpd/rfapi/rfapi_rib.c b/bgpd/rfapi/rfapi_rib.c
index 316904e45b8a8605944a614fdd3fce2fffc9215e..11dccbf1b5f524f617e2feda4bcb86cc4abc3644 100644 (file)
--- a/bgpd/rfapi/rfapi_rib.c
+++ b/bgpd/rfapi/rfapi_rib.c
@@ -691,7 +691,10 @@ static void rfapiRibBi2Ri(struct bgp_path_info *bpi, struct rfapi_info *ri,
                        bpi->extra->vnc->vnc.import.rd.val[1];
 
                /* label comes from MP_REACH_NLRI label */
-               vo->v.l2addr.label = decode_label(&bpi->extra->label[0]);
+               vo->v.l2addr.label =
+                       bpi->extra->num_labels
+                               ? decode_label(&bpi->extra->label[0])
+                               : MPLS_INVALID_LABEL;
 
                rfapi_vn_options_free(
                        ri->vn_options); /* maybe free old version */

diff --git a/bgpd/rfapi/rfapi_vty.c b/bgpd/rfapi/rfapi_vty.c
index 5da99dbc4e0948beb092be5eaf98fc554109c335..2576a283f522564a88c3919064617ecde83a6a7d 100644 (file)
--- a/bgpd/rfapi/rfapi_vty.c
+++ b/bgpd/rfapi/rfapi_vty.c
@@ -413,7 +413,7 @@ void rfapi_vty_out_vncinfo(struct vty *vty, const struct prefix *p,
                XFREE(MTYPE_ECOMMUNITY_STR, s);
        }
 
-       if (bpi->extra != NULL) {
+       if (bpi->extra != NULL && bpi->extra->num_labels) {
                if (bpi->extra->label[0] == BGP_PREVENT_VRF_2_VRF_LEAK)
                        vty_out(vty, " label=VRF2VRF");
                else
@@ -1052,7 +1052,7 @@ static int rfapiPrintRemoteRegBi(struct bgp *bgp, void *stream,
                snprintf(buf_un, sizeof(buf_un), "%s",
                         inet_ntop(pfx_vn.family, &pfx_vn.u.prefix, buf_ntop,
                                   sizeof(buf_ntop)));
-               if (bpi->extra) {
+               if (bpi->extra && bpi->extra->num_labels) {
                        uint32_t l = decode_label(&bpi->extra->label[0]);
                        snprintf(buf_vn, sizeof(buf_vn), "Label: %d", l);
                } else /* should never happen */
@@ -1161,7 +1161,8 @@ static int rfapiPrintRemoteRegBi(struct bgp *bgp, void *stream,
                        }
                }
        }
-       if (tun_type != BGP_ENCAP_TYPE_MPLS && bpi->extra) {
+       if (tun_type != BGP_ENCAP_TYPE_MPLS && bpi->extra &&
+           bpi->extra->num_labels) {
                uint32_t l = decode_label(&bpi->extra->label[0]);
 
                if (!MPLS_LABEL_IS_NULL(l)) {

diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c
index c067b7a610c11902ce4704326a7c72462735bcea..09260dbca18a1c66d568aba308ba658d76ba31e3 100644 (file)
--- a/bgpd/rfapi/vnc_import_bgp.c
+++ b/bgpd/rfapi/vnc_import_bgp.c
@@ -414,7 +414,7 @@ static void vnc_import_bgp_add_route_mode_resolve_nve_one_bi(
        uint32_t lifetime;
        uint32_t *plifetime;
        struct bgp_attr_encap_subtlv *encaptlvs;
-       uint32_t label = 0;
+       uint32_t label = MPLS_INVALID_LABEL;
 
        struct rfapi_un_option optary[3];
        struct rfapi_un_option *opt = NULL;
@@ -470,16 +470,17 @@ static void vnc_import_bgp_add_route_mode_resolve_nve_one_bi(
        if (bgp_attr_get_ecommunity(bpi->attr))
                ecommunity_merge(new_ecom, bgp_attr_get_ecommunity(bpi->attr));
 
-       if (bpi->extra)
+       if (bpi->extra && bpi->extra->num_labels)
                label = decode_label(&bpi->extra->label[0]);
 
        add_vnc_route(&vncHDResolveNve, bgp, SAFI_MPLS_VPN,
-                     prefix,     /* unicast route prefix */
+                     prefix,          /* unicast route prefix */
                      prd, &nexthop_h, /* new nexthop */
                      local_pref, plifetime,
                      (struct bgp_tea_options *)encaptlvs, /* RFP options */
                      opt, NULL, new_ecom, med, /* NULL => don't set med */
-                     (label ? &label : NULL),  /* NULL= default */
+                     ((label != MPLS_INVALID_LABEL) ? &label
+                                                    : NULL), /* NULL= default */
                      ZEBRA_ROUTE_BGP_DIRECT, BGP_ROUTE_REDISTRIBUTE,
                      RFAPI_AHR_RFPOPT_IS_VNCTLV); /* flags */
 
@@ -1678,7 +1679,7 @@ static void vnc_import_bgp_exterior_add_route_it(
                             bpi_interior = bpi_interior->next) {
                                struct prefix_rd *prd;
                                struct attr new_attr;
-                               uint32_t label = 0;
+                               uint32_t label = MPLS_INVALID_LABEL;
 
                                if (!is_usable_interior_route(bpi_interior))
                                        continue;
@@ -1698,8 +1699,10 @@ static void vnc_import_bgp_exterior_add_route_it(
                                if (bpi_interior->extra) {
                                        prd = &bpi_interior->extra->vnc->vnc
                                                       .import.rd;
-                                       label = decode_label(
-                                               &bpi_interior->extra->label[0]);
+                                       if (bpi_interior->extra->num_labels)
+                                               label = decode_label(
+                                                       &bpi_interior->extra
+                                                                ->label[0]);
                                } else
                                        prd = NULL;
 
@@ -1851,7 +1854,7 @@ void vnc_import_bgp_exterior_del_route(
                        for (bpi_interior = rn->info; bpi_interior;
                             bpi_interior = bpi_interior->next) {
                                struct prefix_rd *prd;
-                               uint32_t label = 0;
+                               uint32_t label = MPLS_INVALID_LABEL;
 
                                if (!is_usable_interior_route(bpi_interior))
                                        continue;
@@ -1867,8 +1870,10 @@ void vnc_import_bgp_exterior_del_route(
                                if (bpi_interior->extra) {
                                        prd = &bpi_interior->extra->vnc->vnc
                                                       .import.rd;
-                                       label = decode_label(
-                                               &bpi_interior->extra->label[0]);
+                                       if (bpi_interior->extra->num_labels)
+                                               label = decode_label(
+                                                       &bpi_interior->extra
+                                                                ->label[0]);
                                } else
                                        prd = NULL;
 
@@ -2007,15 +2012,16 @@ void vnc_import_bgp_exterior_add_route_interior(
 
                        struct prefix_rd *prd;
                        struct attr new_attr;
-                       uint32_t label = 0;
+                       uint32_t label = MPLS_INVALID_LABEL;
 
                        assert(bpi_exterior);
                        assert(pfx_exterior);
 
                        if (bpi_interior->extra) {
                                prd = &bpi_interior->extra->vnc->vnc.import.rd;
-                               label = decode_label(
-                                       &bpi_interior->extra->label[0]);
+                               if (bpi_interior->extra->num_labels)
+                                       label = decode_label(
+                                               &bpi_interior->extra->label[0]);
                        } else
                                prd = NULL;
 
@@ -2097,7 +2103,7 @@ void vnc_import_bgp_exterior_add_route_interior(
                                struct bgp_path_info *bpi;
                                struct prefix_rd *prd;
                                struct attr new_attr;
-                               uint32_t label = 0;
+                               uint32_t label = MPLS_INVALID_LABEL;
 
                                /* do pull-down */
 
@@ -2128,8 +2134,10 @@ void vnc_import_bgp_exterior_add_route_interior(
                                        if (bpi->extra) {
                                                prd = &bpi->extra->vnc->vnc
                                                               .import.rd;
-                                               label = decode_label(
-                                                       &bpi->extra->label[0]);
+                                               if (bpi->extra->num_labels)
+                                                       label = decode_label(
+                                                               &bpi->extra->label
+                                                                        [0]);
                                        } else
                                                prd = NULL;
 
@@ -2150,8 +2158,10 @@ void vnc_import_bgp_exterior_add_route_interior(
                                if (bpi_interior->extra) {
                                        prd = &bpi_interior->extra->vnc->vnc
                                                       .import.rd;
-                                       label = decode_label(
-                                               &bpi_interior->extra->label[0]);
+                                       if (bpi_interior->extra->num_labels)
+                                               label = decode_label(
+                                                       &bpi_interior->extra
+                                                                ->label[0]);
                                } else
                                        prd = NULL;
 
@@ -2237,7 +2247,7 @@ void vnc_import_bgp_exterior_add_route_interior(
 
                        struct prefix_rd *prd;
                        struct attr new_attr;
-                       uint32_t label = 0;
+                       uint32_t label = MPLS_INVALID_LABEL;
 
                        /* do pull-down */
 
@@ -2268,8 +2278,9 @@ void vnc_import_bgp_exterior_add_route_interior(
                         */
                        if (bpi_interior->extra) {
                                prd = &bpi_interior->extra->vnc->vnc.import.rd;
-                               label = decode_label(
-                                       &bpi_interior->extra->label[0]);
+                               if (bpi_interior->extra->num_labels)
+                                       label = decode_label(
+                                               &bpi_interior->extra->label[0]);
                        } else
                                prd = NULL;
 
@@ -2372,11 +2383,13 @@ void vnc_import_bgp_exterior_del_route_interior(
                                &cursor)) {
 
                struct prefix_rd *prd;
-               uint32_t label = 0;
+               uint32_t label = MPLS_INVALID_LABEL;
 
                if (bpi_interior->extra) {
                        prd = &bpi_interior->extra->vnc->vnc.import.rd;
-                       label = decode_label(&bpi_interior->extra->label[0]);
+                       if (bpi_interior->extra->num_labels)
+                               label = decode_label(
+                                       &bpi_interior->extra->label[0]);
                } else
                        prd = NULL;
 
@@ -2446,15 +2459,16 @@ void vnc_import_bgp_exterior_del_route_interior(
 
                                struct prefix_rd *prd;
                                struct attr new_attr;
-                               uint32_t label = 0;
+                               uint32_t label = MPLS_INVALID_LABEL;
 
                                if (bpi->type == ZEBRA_ROUTE_BGP_DIRECT_EXT)
                                        continue;
 
                                if (bpi->extra) {
                                        prd = &bpi->extra->vnc->vnc.import.rd;
-                                       label = decode_label(
-                                               &bpi->extra->label[0]);
+                                       if (bpi->extra->num_labels)
+                                               label = decode_label(
+                                                       &bpi->extra->label[0]);
                                } else
                                        prd = NULL;