From 70ac630b399a007ae396eab42217d98638162a71 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Wed, 24 Apr 2024 16:59:25 +0300 Subject: [PATCH] bgpd: Pass the right reuse_list when handling it via bgp_reuse_timer thread This fixes the crash: ``` ==14759== Invalid read of size 8 ==14759== at 0x31032B: bgp_reuselist_del (bgp_damp.c:51) ==14759== by 0x310392: bgp_damp_info_unclaim (bgp_damp.c:69) ==14759== by 0x310CD6: bgp_damp_info_free (bgp_damp.c:387) ==14759== by 0x311016: bgp_reuse_timer (bgp_damp.c:230) ==14759== by 0x4F227CC: thread_call (thread.c:2008) ==14759== by 0x4EDB7D7: frr_run (libfrr.c:1216) ==14759== by 0x1EF748: main (bgp_main.c:525) ==14759== Address 0x48 is not stack'd, malloc'd or (recently) free'd ==14759== ==14759== ==14759== Process terminating with default action of signal 11 (SIGSEGV) ==14759== at 0x59CC7F5: raise (raise.c:46) ==14759== by 0x4F10CEB: core_handler (sigevent.c:261) ==14759== by 0x59CC97F: ??? (in /lib/x86_64-linux-gnu/libpthread-2.27.so) ==14759== by 0x31032A: bgp_reuselist_del (bgp_damp.c:51) ==14759== by 0x310392: bgp_damp_info_unclaim (bgp_damp.c:69) ==14759== by 0x310CD6: bgp_damp_info_free (bgp_damp.c:387) ==14759== by 0x311016: bgp_reuse_timer (bgp_damp.c:230) ==14759== by 0x4F227CC: thread_call (thread.c:2008) ==14759== by 0x4EDB7D7: frr_run (libfrr.c:1216) ==14759== by 0x1EF748: main (bgp_main.c:525) ``` Signed-off-by: Donatas Abraitis --- bgpd/bgp_damp.c | 39 +++++++++++++++++---------------------- bgpd/bgp_damp.h | 3 ++- bgpd/bgp_route.c | 12 +++++------- 3 files changed, 24 insertions(+), 30 deletions(-) diff --git a/bgpd/bgp_damp.c b/bgpd/bgp_damp.c index ddf7e0db8f..6b9b12dd4d 100644 --- a/bgpd/bgp_damp.c +++ b/bgpd/bgp_damp.c @@ -43,13 +43,16 @@ static void bgp_reuselist_switch(struct reuselist *source, SLIST_INSERT_HEAD(target, info, entry); } -static void bgp_damp_info_unclaim(struct bgp_damp_info *bdi) +static void bgp_damp_info_unclaim(struct bgp_damp_info *bdi, + struct reuselist *list) { assert(bdi && bdi->config); if (bdi->index == BGP_DAMP_NO_REUSE_LIST_INDEX) bgp_reuselist_del(&bdi->config->no_reuse_list, bdi); else - bgp_reuselist_del(&bdi->config->reuse_list[bdi->index], bdi); + bgp_reuselist_del(list ? list + : &bdi->config->reuse_list[bdi->index], + bdi); bdi->config = NULL; } @@ -61,7 +64,7 @@ static void bgp_damp_info_claim(struct bgp_damp_info *bdi, bdi->config = bdc; return; } - bgp_damp_info_unclaim(bdi); + bgp_damp_info_unclaim(bdi, NULL); bdi->config = bdc; bdi->afi = bdc->afi; bdi->safi = bdc->safi; @@ -119,7 +122,7 @@ static void bgp_reuse_list_add(struct bgp_damp_info *bdi, /* Delete BGP dampening information from reuse list. */ static void bgp_reuse_list_delete(struct bgp_damp_info *bdi) { - bgp_damp_info_unclaim(bdi); + bgp_damp_info_unclaim(bdi, NULL); } static void bgp_no_reuse_list_add(struct bgp_damp_info *bdi, @@ -132,7 +135,7 @@ static void bgp_no_reuse_list_add(struct bgp_damp_info *bdi, static void bgp_no_reuse_list_delete(struct bgp_damp_info *bdi) { - bgp_damp_info_unclaim(bdi); + bgp_damp_info_unclaim(bdi, NULL); } /* Return decayed penalty value. */ @@ -210,8 +213,7 @@ static void bgp_reuse_timer(struct event *t) } if (bdi->penalty <= bdc->reuse_limit / 2.0) { - bgp_reuselist_del(&plist, bdi); - bgp_damp_info_free(bdi, 1); + bgp_damp_info_free(bdi, &plist, 1); } else { bdi->index = BGP_DAMP_NO_REUSE_LIST_INDEX; bgp_reuselist_switch(&plist, bdi, @@ -357,22 +359,18 @@ int bgp_damp_update(struct bgp_path_info *path, struct bgp_dest *dest, if (bdi->penalty > bdc->reuse_limit / 2.0) bdi->t_updated = t_now; - else { - bgp_damp_info_unclaim(bdi); - bgp_damp_info_free(bdi, 0); - } + else + bgp_damp_info_free(bdi, NULL, 0); return status; } -void bgp_damp_info_free(struct bgp_damp_info *bdi, int withdraw) +void bgp_damp_info_free(struct bgp_damp_info *bdi, struct reuselist *list, + int withdraw) { assert(bdi); - if (bdi->path == NULL) { - XFREE(MTYPE_BGP_DAMP_INFO, bdi); - return; - } + bgp_damp_info_unclaim(bdi, list); bdi->path->extra->damp_info = NULL; bgp_path_info_unset_flag(bdi->dest, bdi->path, @@ -495,15 +493,12 @@ void bgp_damp_info_clean(struct bgp *bgp, struct bgp_damp_config *bdc, bgp_process(bgp, bdi->dest, bdi->path, bdi->afi, bdi->safi); } - bgp_reuselist_del(list, bdi); - bgp_damp_info_free(bdi, 1); + bgp_damp_info_free(bdi, list, 1); } } - while ((bdi = SLIST_FIRST(&bdc->no_reuse_list)) != NULL) { - bgp_reuselist_del(&bdc->no_reuse_list, bdi); - bgp_damp_info_free(bdi, 1); - } + while ((bdi = SLIST_FIRST(&bdc->no_reuse_list)) != NULL) + bgp_damp_info_free(bdi, &bdc->no_reuse_list, 1); /* Free decay array */ XFREE(MTYPE_BGP_DAMP_ARRAY, bdc->decay_array); diff --git a/bgpd/bgp_damp.h b/bgpd/bgp_damp.h index 2578e25954..851c6f9e85 100644 --- a/bgpd/bgp_damp.h +++ b/bgpd/bgp_damp.h @@ -130,7 +130,8 @@ extern int bgp_damp_withdraw(struct bgp_path_info *path, struct bgp_dest *dest, afi_t afi, safi_t safi, int attr_change); extern int bgp_damp_update(struct bgp_path_info *path, struct bgp_dest *dest, afi_t afi, safi_t saff); -extern void bgp_damp_info_free(struct bgp_damp_info *bdi, int withdraw); +extern void bgp_damp_info_free(struct bgp_damp_info *bdi, + struct reuselist *list, int withdraw); extern void bgp_damp_info_clean(struct bgp *bgp, struct bgp_damp_config *bdc, afi_t afi, safi_t safi); extern void bgp_damp_config_clean(struct bgp_damp_config *bdc); diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 9697245eea..5022ba36ab 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -251,7 +251,7 @@ void bgp_path_info_extra_free(struct bgp_path_info_extra **extra) e = *extra; if (e->damp_info) - bgp_damp_info_free(e->damp_info, 0); + bgp_damp_info_free(e->damp_info, NULL, 0); e->damp_info = NULL; if (e->vrfleak && e->vrfleak->parent) { struct bgp_path_info *bpi = @@ -15827,9 +15827,8 @@ static int bgp_clear_damp_route(struct vty *vty, const char *view_name, while (pi) { if (pi->extra && pi->extra->damp_info) { pi_temp = pi->next; - bgp_damp_info_free( - pi->extra->damp_info, - 1); + bgp_damp_info_free(pi->extra->damp_info, + NULL, 1); pi = pi_temp; } else pi = pi->next; @@ -15866,9 +15865,8 @@ static int bgp_clear_damp_route(struct vty *vty, const char *view_name, bdi->afi, bdi->safi); } - bgp_damp_info_free( - pi->extra->damp_info, - 1); + bgp_damp_info_free(pi->extra->damp_info, + NULL, 1); pi = pi_temp; } else pi = pi->next; -- 2.39.5