Skip to content

Commit

Permalink
bgpd: fix bgp crash after BGP peer goes down
Browse files Browse the repository at this point in the history 
A BGP crash happens when the 'show bgp label-nexthop' is executed, on a
BGP VRF configuration with the 'label vpn export allocation-mode per-nexthop'
command configured.

> (gdb) bt
> #0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=140039420118912) at ./nptl/pthread_kill.c:44
> FRRouting#1  __pthread_kill_internal (signo=6, threadid=140039420118912) at ./nptl/pthread_kill.c:78
> FRRouting#2  __GI___pthread_kill (threadid=140039420118912, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
> FRRouting#3  0x00007f5d78063476 in __GI_raise (sig=6) at ../sysdeps/posix/raise.c:26
> FRRouting#4  0x00007f5d78448705 in core_handler (signo=6, siginfo=0x7ffffe2e0f70, context=0x7ffffe2e0e40)
>     at /build/make-pkg/output/_packages/cp-routing/src/lib/sigevent.c:262
> FRRouting#5  <signal handler called>
> FRRouting#6  __pthread_kill_implementation (no_tid=0, signo=6, threadid=140039420118912) at ./nptl/pthread_kill.c:44
> FRRouting#7  __pthread_kill_internal (signo=6, threadid=140039420118912) at ./nptl/pthread_kill.c:78
> FRRouting#8  __GI___pthread_kill (threadid=140039420118912, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
> FRRouting#9  0x00007f5d78063476 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
> FRRouting#10 0x00007f5d780497f3 in __GI_abort () at ./stdlib/abort.c:79
> FRRouting#11 0x00007f5d7848872f in _zlog_assert_failed (xref=0x5605f59a4ec0 <_xref.9>, extra=0x0)
>     at /build/make-pkg/output/_packages/cp-routing/src/lib/zlog.c:557
> FRRouting#12 0x00005605f57ad3ab in show_bgp_nexthop_label_afi (vty=0x5605f79138e0, afi=AFI_IP, bgp=0x5605f7855090, detail=true)
>     at /build/make-pkg/output/_packages/cp-routing/src/bgpd/bgp_labelpool.c:1071
> FRRouting#13 0x00005605f57ad62a in show_bgp_nexthop_label (self=0x5605f59a4920 <show_bgp_nexthop_label_cmd>, vty=0x5605f79138e0, argc=6, argv=0x5605f77c5cc0)
>     at /build/make-pkg/output/_packages/cp-routing/src/bgpd/bgp_labelpool.c:1116
> FRRouting#14 0x00007f5d783bb858 in cmd_execute_command_real (vline=0x5605f791c060, filter=FILTER_RELAXED, vty=0x5605f79138e0, cmd=0x0, up_level=0)
>     at /build/make-pkg/output/_packages/cp-routing/src/lib/command.c:1070
> FRRouting#15 0x00007f5d783bb9dd in cmd_execute_command (vline=0x5605f791c060, vty=0x5605f79138e0, cmd=0x0, vtysh=0)
>     at /build/make-pkg/output/_packages/cp-routing/src/lib/command.c:1130
> FRRouting#16 0x00007f5d783bbf8a in cmd_execute (vty=0x5605f79138e0, cmd=0x5605f791a010 "show bgp vrf vrf1 label-nexthop detail", matched=0x0, vtysh=0)
>     at /build/make-pkg/output/_packages/cp-routing/src/lib/command.c:1294
> FRRouting#17 0x00007f5d7846965c in vty_command (vty=0x5605f79138e0, buf=0x5605f791a010 "show bgp vrf vrf1 label-nexthop detail")
>     at /build/make-pkg/output/_packages/cp-routing/src/lib/vty.c:530
> FRRouting#18 0x00007f5d7846b52c in vty_execute (vty=0x5605f79138e0) at /build/make-pkg/output/_packages/cp-routing/src/lib/vty.c:1296
> FRRouting#19 0x00007f5d7846d508 in vtysh_read (thread=0x7ffffe2e4410) at /build/make-pkg/output/_packages/cp-routing/src/lib/vty.c:2137
> FRRouting#20 0x00007f5d78461fe6 in thread_call (thread=0x7ffffe2e4410) at /build/make-pkg/output/_packages/cp-routing/src/lib/thread.c:1825
>
> (gdb) frame 12
> FRRouting#12 0x00005605f57ad3ab in show_bgp_nexthop_label_afi (vty=0x5605f79138e0, afi=AFI_IP, bgp=0x5605f7855090, detail=true)
>     at /build/make-pkg/output/_packages/cp-routing/src/bgpd/bgp_labelpool.c:1071
>

This crash is a segmentation fault: the 'path' pointer is not a valid pointer;
consequently, the 'dest' and the 'table' pointer are also invalid. The 'path'
pointer is a 'bgp_path_info' structure previously freed when a peer down event
occured. The 'show bgp label-nexthop' command was attempting to dump the
'bgp_path_info' entries referenced in the 'bgp_label_per_nexthop_cache' entries.
As the 'bgp_path_info' entries were invalid, the crash happened. To illustrate,
the below dump shows 3 path entries linked to the '192.0.2.11' next-hop, whereas
the '192.0.2.11' peer has been removed.

> dut-vm# show bgp vrf vrf1 label-nexthop
> Current BGP label nexthop cache for IPv4, VRF VRF vrf1
>  192.0.2.11, label 20 #paths 3
>   if r1-eth1
>   Last update: Wed May 24 15:16:21 2023
> dut-vm# show bgp vrf vrf1 label-nexthop detail
> <-- crash

When the 'bgp_path_info' entries are freed, the 'bgp_mplsvpn_path_nh_label_unlink()'
function is called. The 'pi->net' pointer is needed to check the BGP RIB table is
the SAFI_UNICAST routing table, and to de-reference the 'bgp_labl_per_nexthop_cache'
entry from the 'bgp_path_info' entry. In this case, the 'pi->net' was unset.

Fix this by introducing a new 'mplsvpn_usage' field to determine if the 'bgp_path_info'
structure contains a 'bgp_mplsvpn_label_nh' structure.

Fixes: ("bgpd: allocate label bound to received mpls vpn routes")
Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
  • Loading branch information
@pguibert6WIND
pguibert6WIND committed May 24, 2023
1 parent 4a8c70c commit 75bd75e
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 8 deletions.
14 changes: 6 additions & 8 deletions bgpd/bgp_mplsvpn.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1338,10 +1338,7 @@ void bgp_mplsvpn_path_nh_label_unlink(struct bgp_path_info *pi)
{
struct bgp_label_per_nexthop_cache *blnc;

if (!pi || !pi->net || !bgp_dest_table(pi->net))
return;

if (bgp_dest_table(pi->net)->safi != SAFI_UNICAST)
if (!CHECK_FLAG(pi->mplsvpn_usage, BGP_MPLSVPN_LABEL_NH))
return;

blnc = pi->mplsvpn.blnc.label_nexthop_cache;
Expand All @@ -1352,6 +1349,7 @@ void bgp_mplsvpn_path_nh_label_unlink(struct bgp_path_info *pi)
LIST_REMOVE(pi, mplsvpn.blnc.label_nh_thread);
pi->mplsvpn.blnc.label_nexthop_cache->path_count--;
pi->mplsvpn.blnc.label_nexthop_cache = NULL;
UNSET_FLAG(pi->mplsvpn_usage, BGP_MPLSVPN_LABEL_NH);

if (LIST_EMPTY(&(blnc->paths)))
bgp_label_per_nexthop_free(blnc);
Expand Down Expand Up @@ -1473,6 +1471,7 @@ static mpls_label_t _vpn_leak_from_vrf_get_per_nexthop_label(
mplsvpn.blnc.label_nh_thread);
pi->mplsvpn.blnc.label_nexthop_cache = blnc;
pi->mplsvpn.blnc.label_nexthop_cache->path_count++;
SET_FLAG(pi->mplsvpn_usage, BGP_MPLSVPN_LABEL_NH);
blnc->last_update = monotime(NULL);
}

Expand Down Expand Up @@ -4093,10 +4092,7 @@ void bgp_mplsvpn_path_nh_label_bind_unlink(struct bgp_path_info *pi)
{
struct bgp_mplsvpn_nh_label_bind_cache *bmnc;

if (!pi || !pi->net || !bgp_dest_table(pi->net))
return;

if (bgp_dest_table(pi->net)->safi != SAFI_MPLS_VPN)
if (!CHECK_FLAG(pi->mplsvpn_usage, BGP_MPLSVPN_NH_LABEL_BIND))
return;

bmnc = pi->mplsvpn.bmnc.nh_label_bind_cache;
Expand All @@ -4107,6 +4103,7 @@ void bgp_mplsvpn_path_nh_label_bind_unlink(struct bgp_path_info *pi)
LIST_REMOVE(pi, mplsvpn.bmnc.nh_label_bind_thread);
pi->mplsvpn.bmnc.nh_label_bind_cache->path_count--;
pi->mplsvpn.bmnc.nh_label_bind_cache = NULL;
SET_FLAG(pi->mplsvpn_usage, BGP_MPLSVPN_NH_LABEL_BIND);

if (LIST_EMPTY(&(bmnc->paths)))
bgp_mplsvpn_nh_label_bind_free(bmnc);
Expand Down Expand Up @@ -4143,6 +4140,7 @@ void bgp_mplsvpn_nh_label_bind_register_local_label(struct bgp *bgp,
mplsvpn.bmnc.nh_label_bind_thread);
pi->mplsvpn.bmnc.nh_label_bind_cache = bmnc;
pi->mplsvpn.bmnc.nh_label_bind_cache->path_count++;
SET_FLAG(pi->mplsvpn_usage, BGP_MPLSVPN_NH_LABEL_BIND);
bmnc->last_update = monotime(NULL);
}

Expand Down
3 changes: 3 additions & 0 deletions bgpd/bgp_route.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,9 @@ struct bgp_path_info {
uint32_t addpath_rx_id;
struct bgp_addpath_info_data tx_addpath;

#define BGP_MPLSVPN_LABEL_NH (1<<1)
#define BGP_MPLSVPN_NH_LABEL_BIND (1<<2)
uint8_t mplsvpn_usage;
union {
struct bgp_mplsvpn_label_nh blnc;
struct bgp_mplsvpn_nh_label_bind bmnc;
Expand Down

0 comments on commit 75bd75e

Please sign in to comment.