Skip to content

Commit

Permalink
bgpd: snmp, fix possible issues when default vrf is not set.
Browse files Browse the repository at this point in the history 
When importing VPN updates in the default VRF, a crash happens
when dumping the L3VPN MIB. To illustrate, the following crash log example is displayed:

BGP[7301]: Received signal 11 at 1684255316 (si_addr 0x0, PC 0x7fa09b992b05); aborting...
BGP[7301]: /lib/x86_64-linux-gnu/libfrr.so.0(zlog_backtrace_sigsafe+0xb3) [0x7fa09bbd93f2]
BGP[7301]: /lib/x86_64-linux-gnu/libfrr.so.0(zlog_signal+0x1b4) [0x7fa09bbd9257]
BGP[7301]: /lib/x86_64-linux-gnu/libfrr.so.0(+0x10568d) [0x7fa09bc1c68d]
BGP[7301]: /lib/x86_64-linux-gnu/libc.so.6(+0x42520) [0x7fa09b837520]
BGP[7301]: /lib/x86_64-linux-gnu/libc.so.6(+0x19db05) [0x7fa09b992b05]
BGP[7301]: /usr/lib/x86_64-linux-gnu/frr/modules/bgpd_snmp.so(+0x62ff) [0x7fa09b5f42ff]
BGP[7301]: /usr/lib/x86_64-linux-gnu/frr/modules/bgpd_snmp.so(+0x647c) [0x7fa09b5f447c]
BGP[7301]: /usr/bin/bgpd(+0x1e1aab) [0x55b1ce417aab]
BGP[7301]: /usr/bin/bgpd(+0x1e261b) [0x55b1ce41861b]
BGP[7301]: /lib/x86_64-linux-gnu/libfrr.so.0(if_up_via_zapi+0x2c) [0x7fa09bbbe43c]
BGP[7301]: /lib/x86_64-linux-gnu/libfrr.so.0(+0x13df63) [0x7fa09bc54f63]
BGP[7301]: /lib/x86_64-linux-gnu/libfrr.so.0(+0x14267e) [0x7fa09bc5967e]
BGP[7301]: /lib/x86_64-linux-gnu/libfrr.so.0(thread_call+0x1c2) [0x7fa09bc35fcc]
BGP[7301]: /lib/x86_64-linux-gnu/libfrr.so.0(frr_run+0x257) [0x7fa09bbcd6e8]
BGP[7301]: /usr/bin/bgpd(main+0x4f4) [0x55b1ce2ef98c]
BGP[7301]: /lib/x86_64-linux-gnu/libc.so.6(+0x29d90) [0x7fa09b81ed90]
BGP[7301]: /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80) [0x7fa09b81ee40]
BGP[7301]: /usr/bin/bgpd(_start+0x25) [0x55b1ce2edae5]
BGP[7301]: in thread zclient_read scheduled from /build/make-pkg/output/_packages/cp-routing/src/lib/zclient.c:4397 zclient_event()

The 'name' attribute of the BGP instance is attempted to be accessed,
in multiple functions.

Fix this by using the "default" vrf name, which is returned by the
VRF_DEFAULT_NAME macro, wherever the default BGP instance may be
used.

Signed-off-by: Dmytro Shytyi <[email protected]>
Signed-off-by: Philippe guibert <[email protected]>
  • Loading branch information
@dmytroshytyi-6WIND @pguibert6WIND
dmytroshytyi-6WIND authored and pguibert6WIND committed Jun 7, 2023
1 parent 45d9af1 commit 967f02b
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 23 deletions.
103 changes: 83 additions & 20 deletions bgpd/bgp_mplsvpn_snmp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -554,16 +554,23 @@ static bool is_bgp_vrf_active(struct bgp *bgp)
{
struct vrf *vrf;
struct interface *ifp;
const char *vrf_name;

/* if there is one interface in the vrf which is up then it is deemed
* active
*/
vrf = vrf_lookup_by_id(bgp->vrf_id);
if (vrf == NULL)
return false;

if (bgp->name)
vrf_name = bgp->name;
else
vrf_name = VRF_DEFAULT_NAME;

RB_FOREACH (ifp, if_name_head, &vrf->ifaces_by_name) {
/* if we are in a vrf skip the l3mdev */
if (bgp->name && strncmp(ifp->name, bgp->name, VRF_NAMSIZ) == 0)
if (vrf_name && strncmp(ifp->name, vrf_name, VRF_NAMSIZ) == 0)
continue;

if (if_is_up(ifp))
Expand Down Expand Up @@ -674,14 +681,20 @@ static uint8_t *mplsL3vpnConnectedInterfaces(struct variable *v, oid name[],
struct bgp *bgp;
uint32_t count = 0;
struct vrf *vrf;
char *vrf_name;

if (smux_header_generic(v, name, length, exact, var_len, write_method)
== MATCH_FAILED)
return NULL;

for (ALL_LIST_ELEMENTS(bm->bgp, node, nnode, bgp)) {
if (bgp->name)
vrf_name = bgp->name;
else
vrf_name = (char *)VRF_DEFAULT_NAME;

if (is_bgp_vrf_mplsvpn(bgp)) {
vrf = vrf_lookup_by_name(bgp->name);
vrf = vrf_lookup_by_name(vrf_name);
if (vrf == NULL)
continue;

Expand Down Expand Up @@ -765,6 +778,8 @@ static struct bgp *bgp_lookup_by_name_next(char *vrf_name)
struct bgp *bgp, *bgp_next = NULL;
struct listnode *node, *nnode;
bool first = false;
char *vrf_name_selected;
char *bgp_next_name;

/*
* the vrfs are not stored alphabetically but since we are using the
Expand All @@ -779,11 +794,23 @@ static struct bgp *bgp_lookup_by_name_next(char *vrf_name)
bgp_next = bgp;
continue;
}
if (first || strncmp(bgp->name, vrf_name, VRF_NAMSIZ) > 0) {

if (bgp->name)
vrf_name_selected = bgp->name;
else
vrf_name_selected = (char *)VRF_DEFAULT_NAME;

if (bgp_next && bgp_next->name)
bgp_next_name = bgp_next->name;
else
bgp_next_name = (char *)VRF_DEFAULT_NAME;

if (first ||
strncmp(vrf_name_selected, vrf_name, VRF_NAMSIZ) > 0) {
if (bgp_next == NULL)
bgp_next = bgp;
else if (strncmp(bgp->name, bgp_next->name, VRF_NAMSIZ)
< 0)
else if (strncmp(vrf_name_selected, bgp_next_name,
VRF_NAMSIZ) < 0)
bgp_next = bgp;
}
}
Expand All @@ -800,6 +827,7 @@ static struct bgp *bgpL3vpnIfConf_lookup(struct variable *v, oid name[],
size_t namelen = v ? v->namelen : IFCONFTAB_NAMELEN;
struct interface *ifp;
int vrf_name_len, len;
char *bgp_name;

/* too long ? */
if (*length - namelen > (VRF_NAMSIZ + sizeof(uint32_t)))
Expand Down Expand Up @@ -831,11 +859,15 @@ static struct bgp *bgpL3vpnIfConf_lookup(struct variable *v, oid name[],
while (bgp) {
ifp = if_vrf_lookup_by_index_next(*ifindex,
bgp->vrf_id);
if (bgp->name)
bgp_name = bgp->name;
else
bgp_name = (char *)VRF_DEFAULT_NAME;
if (ifp) {
vrf_name_len = strnlen(bgp->name, VRF_NAMSIZ);
vrf_name_len = strnlen(bgp_name, VRF_NAMSIZ);
*ifindex = ifp->ifindex;
len = vrf_name_len + sizeof(ifindex_t);
oid_copy_str(name + namelen, bgp->name,
oid_copy_str(name + namelen, bgp_name,
vrf_name_len);
oid_copy_int(name + namelen + vrf_name_len,
ifindex);
Expand All @@ -844,7 +876,7 @@ static struct bgp *bgpL3vpnIfConf_lookup(struct variable *v, oid name[],
return bgp;
}
*ifindex = 0;
bgp = bgp_lookup_by_name_next(bgp->name);
bgp = bgp_lookup_by_name_next(bgp_name);
}

return NULL;
Expand Down Expand Up @@ -890,6 +922,7 @@ static struct bgp *bgpL3vpnVrf_lookup(struct variable *v, oid name[],
struct bgp *bgp = NULL;
size_t namelen = v ? v->namelen : VRFTAB_NAMELEN;
int len;
const char *bgp_name;

if (*length - namelen > VRF_NAMSIZ)
return NULL;
Expand All @@ -905,8 +938,12 @@ static struct bgp *bgpL3vpnVrf_lookup(struct variable *v, oid name[],
if (bgp == NULL)
return NULL;

len = strnlen(bgp->name, VRF_NAMSIZ);
oid_copy_str(name + namelen, bgp->name, len);
if (bgp->name)
bgp_name = bgp->name;
else
bgp_name = VRF_DEFAULT_NAME;
len = strnlen(bgp_name, VRF_NAMSIZ);
oid_copy_str(name + namelen, bgp_name, len);
*length = len + namelen;
}
return bgp;
Expand All @@ -918,6 +955,7 @@ static uint8_t *mplsL3vpnVrfTable(struct variable *v, oid name[],
{
char vrf_name[VRF_NAMSIZ];
struct bgp *l3vpn_bgp;
const char *bgp_name;

if (smux_header_table(v, name, length, exact, var_len, write_method)
== MATCH_FAILED)
Expand All @@ -934,8 +972,12 @@ static uint8_t *mplsL3vpnVrfTable(struct variable *v, oid name[],
*var_len = 0;
return NULL;
case MPLSL3VPNVRFDESC:
*var_len = strnlen(l3vpn_bgp->name, VRF_NAMSIZ);
return (uint8_t *)l3vpn_bgp->name;
if (l3vpn_bgp->name)
bgp_name = l3vpn_bgp->name;
else
bgp_name = VRF_DEFAULT_NAME;
*var_len = strnlen(bgp_name, VRF_NAMSIZ);
return (uint8_t *)bgp_name;
case MPLSL3VPNVRFRD:
/*
* this is a horror show but the MIB dicates one RD per vrf
Expand Down Expand Up @@ -999,6 +1041,7 @@ static struct bgp *bgpL3vpnVrfRt_lookup(struct variable *v, oid name[],
struct bgp *l3vpn_bgp;
size_t namelen = v ? v->namelen : VRFRTTAB_NAMELEN;
int vrf_name_len, len;
char *l3vpn_bgp_name;

/* too long ? */
if (*length - namelen
Expand Down Expand Up @@ -1052,6 +1095,10 @@ static struct bgp *bgpL3vpnVrfRt_lookup(struct variable *v, oid name[],
continue;
}
if (*rt_index) {
if (l3vpn_bgp->name)
l3vpn_bgp_name = l3vpn_bgp->name;
else
l3vpn_bgp_name = (char *)VRF_DEFAULT_NAME;
switch (*rt_type) {
case 0:
*rt_type = MPLSVPNVRFRTTYPEIMPORT;
Expand Down Expand Up @@ -1091,18 +1138,18 @@ static struct bgp *bgpL3vpnVrfRt_lookup(struct variable *v, oid name[],

/* we have a match copy the oid info */
vrf_name_len =
strnlen(l3vpn_bgp->name, VRF_NAMSIZ);
strnlen(l3vpn_bgp_name, VRF_NAMSIZ);
len = vrf_name_len + sizeof(uint32_t)
+ sizeof(uint8_t);
oid_copy_str(name + namelen, l3vpn_bgp->name,
oid_copy_str(name + namelen, l3vpn_bgp_name,
vrf_name_len);
oid_copy_int(name + namelen + vrf_name_len,
(int *)rt_index);
name[(namelen + len) - 1] = *rt_type;
*length = len + namelen;
return l3vpn_bgp;
}
l3vpn_bgp = bgp_lookup_by_name_next(l3vpn_bgp->name);
l3vpn_bgp = bgp_lookup_by_name_next(l3vpn_bgp_name);
}
}
return NULL;
Expand Down Expand Up @@ -1131,6 +1178,7 @@ static uint8_t *mplsL3vpnVrfRtTable(struct variable *v, oid name[],
uint8_t rt_type = 0;
char *rt_b = NULL;
static char rt_b_str[BUFSIZ] = {};
const char *l3vpn_bgp_name;

if (smux_header_table(v, name, length, exact, var_len, write_method)
== MATCH_FAILED)
Expand All @@ -1143,6 +1191,10 @@ static uint8_t *mplsL3vpnVrfRtTable(struct variable *v, oid name[],
if (!l3vpn_bgp)
return NULL;

if (l3vpn_bgp->name)
l3vpn_bgp_name = l3vpn_bgp->name;
else
l3vpn_bgp_name = VRF_DEFAULT_NAME;
switch (v->magic) {
case MPLSL3VPNVRFRT:
switch (rt_type) {
Expand Down Expand Up @@ -1177,7 +1229,7 @@ static uint8_t *mplsL3vpnVrfRtTable(struct variable *v, oid name[],
memset(rt_description, 0, VRF_NAMSIZ + RT_PREAMBLE_SIZE);
snprintf(rt_description, VRF_NAMSIZ + RT_PREAMBLE_SIZE,
"RT %s for VRF %s", rt_type2str(rt_type),
l3vpn_bgp->name);
l3vpn_bgp_name);
*var_len =
strnlen(rt_description, VRF_NAMSIZ + RT_PREAMBLE_SIZE);
return (uint8_t *)rt_description;
Expand Down Expand Up @@ -1319,8 +1371,7 @@ static struct bgp_path_info *bgp_lookup_route_next(struct bgp **l3vpn_bgp,
}

/* No more paths in the input route so find the next route */
for (; *l3vpn_bgp;
*l3vpn_bgp = bgp_lookup_by_name_next((*l3vpn_bgp)->name)) {
while (*l3vpn_bgp) {
*policy = 0;
if (!*dest) {
table = (*l3vpn_bgp)->rib[AFI_IP][SAFI_UNICAST];
Expand All @@ -1345,6 +1396,12 @@ static struct bgp_path_info *bgp_lookup_route_next(struct bgp **l3vpn_bgp,
}
break;
}
if ((*l3vpn_bgp)->name)
*l3vpn_bgp =
bgp_lookup_by_name_next((*l3vpn_bgp)->name);
else
*l3vpn_bgp = bgp_lookup_by_name_next(
(char *)VRF_DEFAULT_NAME);
}

return NULL;
Expand Down Expand Up @@ -1378,6 +1435,7 @@ static struct bgp_path_info *bgpL3vpnRte_lookup(struct variable *v, oid name[],
struct ipaddr nexthop = {0};
uint8_t prefix_type;
uint8_t nexthop_type;
const char *l3vpn_bgp_name;

if ((uint32_t)(*length - namelen) > (VRF_NAMSIZ + 37))
return NULL;
Expand Down Expand Up @@ -1457,11 +1515,16 @@ static struct bgp_path_info *bgpL3vpnRte_lookup(struct variable *v, oid name[],
if (*l3vpn_bgp == NULL)
return NULL;

if ((*l3vpn_bgp)->name)
l3vpn_bgp_name = (*l3vpn_bgp)->name;
else
l3vpn_bgp_name = VRF_DEFAULT_NAME;

pi = bgp_lookup_route_next(l3vpn_bgp, dest, &prefix, policy,
&nexthop);
if (pi) {
uint8_t vrf_name_len =
strnlen((*l3vpn_bgp)->name, VRF_NAMSIZ);
strnlen(l3vpn_bgp_name, VRF_NAMSIZ);
const struct prefix *p = bgp_dest_get_prefix(*dest);
uint8_t oid_index;
bool v4 = (p->family == AF_INET);
Expand All @@ -1470,7 +1533,7 @@ static struct bgp_path_info *bgpL3vpnRte_lookup(struct variable *v, oid name[],
struct attr *attr = pi->attr;

/* copy the index parameters */
oid_copy_str(&name[namelen], (*l3vpn_bgp)->name,
oid_copy_str(&name[namelen], l3vpn_bgp_name,
vrf_name_len);
oid_index = namelen + vrf_name_len;
if (v4) {
Expand Down
5 changes: 3 additions & 2 deletions bgpd/bgpd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3451,8 +3451,9 @@ struct bgp *bgp_lookup_by_name(const char *name)
struct listnode *node, *nnode;

for (ALL_LIST_ELEMENTS(bm->bgp, node, nnode, bgp))
if ((bgp->name == NULL && name == NULL)
|| (bgp->name && name && strcmp(bgp->name, name) == 0))
if ((bgp->name == NULL &&
(name == NULL || strcmp(name, VRF_DEFAULT_NAME) == 0)) ||
(bgp->name && name && strcmp(bgp->name, name) == 0))
return bgp;
return NULL;
}
Expand Down
6 changes: 5 additions & 1 deletion bgpd/bgpd.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2599,7 +2599,11 @@ static inline uint32_t bgp_vrf_interfaces(struct bgp *bgp, bool active)
if (vrf == NULL)
return 0;
RB_FOREACH (ifp, if_name_head, &vrf->ifaces_by_name) {
if (strcmp(ifp->name, bgp->name) == 0)
/* vrf interface */
if (bgp->name && strcmp(ifp->name, bgp->name) == 0)
continue;
/* loopback interface */
if (!bgp->name && if_is_loopback_exact(ifp))
continue;
if (!active || if_is_up(ifp))
count++;
Expand Down

0 comments on commit 967f02b

Please sign in to comment.