From 0a584672517a73e9d5ed5485941143036b28d290 Mon Sep 17 00:00:00 2001 From: Carmine Scarpitta Date: Tue, 23 Aug 2022 22:40:47 +0200 Subject: [PATCH 1/8] zebra: Fix memory leak in SRv6 locator delete Running `srv6_locator` topotest with `--valgrind-memleaks` gives several memory leak errors. This is due to the way SRv6 locators are deleted: when an SRv6 locator is deleted, it is removed from the SRv6 locators list (`srv6->locators`), but the memory allocated for the SRv6 locator is not freed. This patch adds a call to the `srv6_locator_free()` function to properly free the allocated memory when an SRv6 locator is removed. Signed-off-by: Carmine Scarpitta --- zebra/zebra_srv6.c | 1 + 1 file changed, 1 insertion(+) diff --git a/zebra/zebra_srv6.c b/zebra/zebra_srv6.c index 219d047694..36506cacc7 100644 --- a/zebra/zebra_srv6.c +++ b/zebra/zebra_srv6.c @@ -162,6 +162,7 @@ void zebra_srv6_locator_delete(struct srv6_locator *locator) } listnode_delete(srv6->locators, locator); + srv6_locator_free(locator); } struct srv6_locator *zebra_srv6_locator_lookup(const char *name) From d0c775e3eb749e03f8414478b5c1fd26b9a5e726 Mon Sep 17 00:00:00 2001 From: Carmine Scarpitta Date: Tue, 23 Aug 2022 23:27:47 +0200 Subject: [PATCH 2/8] lib: Fix memory leak in `zclient_send_localsid()` Running `bgp_srv6l3vpn_to_bgp_vrf` and `bgp_srv6l3vpn_to_bgp_vrf2` topotests with `--valgrind-memleaks` gives several memory leak errors. This is due to the way FRR daemons pass local SIDs to zebra: to send a local SID to zebra, FRR daemons call the `zclient_send_localsid()` function. The `zclient_send_localsid()` function performs the following sequence of operations: * create a temporary `struct nexthop`; * call `nexthop_add_srv6_seg6local()` to fill the `struct nexthop` with the proper local SID information; * create a `struct zapi_route` and call `zapi_nexthop_from_nexthop()` to copy the information from the `struct nexthop` to the `struct zapi_route`; * send the `struct zapi_route` to zebra through the ZAPI. The `nexthop_add_srv6_seg6local()` function uses `XCALLOC()` to allocate memory for the SRv6 nexthop. This memory is never freed. Creating a temporary `struct nexthop` is unnecessary, as the local SID information can be pushed directly to the `struct zapi_route`. This patch simplifies the implementation of `zclient_send_localsid()` by avoiding using the temporary `struct nexthop`. This eliminates the need to use `nexthop_add_srv6_seg6local()` to fill the `struct nexthop` and consequently fixes the memory leak. Signed-off-by: Carmine Scarpitta --- lib/zclient.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/zclient.c b/lib/zclient.c index e556b768ac..8ec82ab7bb 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -447,7 +447,7 @@ enum zclient_send_status zclient_send_localsid(struct zclient *zclient, { struct prefix_ipv6 p = {}; struct zapi_route api = {}; - struct nexthop nh = {}; + struct zapi_nexthop *znh; p.family = AF_INET6; p.prefixlen = IPV6_MAX_BITLEN; @@ -465,12 +465,16 @@ enum zclient_send_status zclient_send_localsid(struct zclient *zclient, SET_FLAG(api.flags, ZEBRA_FLAG_ALLOW_RECURSION); SET_FLAG(api.message, ZAPI_MESSAGE_NEXTHOP); - nh.type = NEXTHOP_TYPE_IFINDEX; - nh.ifindex = oif; - SET_FLAG(nh.flags, ZAPI_NEXTHOP_FLAG_SEG6LOCAL); - nexthop_add_srv6_seg6local(&nh, action, context); + znh = &api.nexthops[0]; + + memset(znh, 0, sizeof(*znh)); + + znh->type = NEXTHOP_TYPE_IFINDEX; + znh->ifindex = oif; + SET_FLAG(znh->flags, ZAPI_NEXTHOP_FLAG_SEG6LOCAL); + znh->seg6local_action = action; + memcpy(&znh->seg6local_ctx, context, sizeof(struct seg6local_context)); - zapi_nexthop_from_nexthop(&api.nexthops[0], &nh); api.nexthop_num = 1; return zclient_route_send(ZEBRA_ROUTE_ADD, zclient, &api); From 03852f673b571fc7f5d815a3f00429533f38d2aa Mon Sep 17 00:00:00 2001 From: Carmine Scarpitta Date: Tue, 23 Aug 2022 23:55:05 +0200 Subject: [PATCH 3/8] bgpd: Fix memory leak in SRv6 locator delete/unset Running `bgp_srv6l3vpn_to_bgp_vrf` and `bgp_srv6l3vpn_to_bgp_vrf2` topotests with `--valgrind-memleaks` gives several memory leak errors. This is due to the way SRv6 locators are deleted/unset in bgpd: when an SRv6 locator is deleted/unset, all the chunks of the locator are removed from the SRv6 locator chunks list (`bgp->srv6_locator_chunks`). However, the memory allocated for the chunks is not freed. This patch adds a call to the `srv6_locator_chunk_free()` function to properly free the allocated memory when an SRv6 locator is removed or unset. Signed-off-by: Carmine Scarpitta --- bgpd/bgp_vty.c | 4 +++- bgpd/bgp_zebra.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 0eba5ea447..c7ef76297a 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -306,8 +306,10 @@ static int bgp_srv6_locator_unset(struct bgp *bgp) return -1; /* refresh chunks */ - for (ALL_LIST_ELEMENTS(bgp->srv6_locator_chunks, node, nnode, chunk)) + for (ALL_LIST_ELEMENTS(bgp->srv6_locator_chunks, node, nnode, chunk)) { listnode_delete(bgp->srv6_locator_chunks, chunk); + srv6_locator_chunk_free(chunk); + } /* refresh functions */ for (ALL_LIST_ELEMENTS(bgp->srv6_functions, node, nnode, func)) diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c index 9c9b88e125..8a19a1400b 100644 --- a/bgpd/bgp_zebra.c +++ b/bgpd/bgp_zebra.c @@ -3218,8 +3218,10 @@ static int bgp_zebra_process_srv6_locator_delete(ZAPI_CALLBACK_ARGS) // refresh chunks for (ALL_LIST_ELEMENTS(bgp->srv6_locator_chunks, node, nnode, chunk)) if (prefix_match((struct prefix *)&loc.prefix, - (struct prefix *)&chunk->prefix)) + (struct prefix *)&chunk->prefix)) { listnode_delete(bgp->srv6_locator_chunks, chunk); + srv6_locator_chunk_free(chunk); + } // refresh functions for (ALL_LIST_ELEMENTS(bgp->srv6_functions, node, nnode, func)) { From bda15542f425bff67b4f821f2e475f4e330696dd Mon Sep 17 00:00:00 2001 From: Carmine Scarpitta Date: Wed, 24 Aug 2022 00:02:22 +0200 Subject: [PATCH 4/8] bgpd: Fix memory leak when an SRv6 SID is removed Running `bgp_srv6l3vpn_to_bgp_vrf` and `bgp_srv6l3vpn_to_bgp_vrf2` topotests with `--valgrind-memleaks` gives several memory leak errors. This is due to the way SRv6 SIDs are removed in bgpd: when an SRv6 locator is deleted/unset, all the SIDs allocated from that locator are removed from the SRv6 functions list (`bgp->srv6_functions`),but the memory allocated for the SIDs is not freed. This patch adds a call to `XFREE()` to properly free the allocated memory when an SRv6 SID is removed. Signed-off-by: Carmine Scarpitta --- bgpd/bgp_vty.c | 4 +++- bgpd/bgp_zebra.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index c7ef76297a..6488c94075 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -312,8 +312,10 @@ static int bgp_srv6_locator_unset(struct bgp *bgp) } /* refresh functions */ - for (ALL_LIST_ELEMENTS(bgp->srv6_functions, node, nnode, func)) + for (ALL_LIST_ELEMENTS(bgp->srv6_functions, node, nnode, func)) { listnode_delete(bgp->srv6_functions, func); + XFREE(MTYPE_BGP_SRV6_FUNCTION, func); + } /* refresh tovpn_sid */ for (ALL_LIST_ELEMENTS_RO(bm->bgp, node, bgp_vrf)) { diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c index 8a19a1400b..33e8895bec 100644 --- a/bgpd/bgp_zebra.c +++ b/bgpd/bgp_zebra.c @@ -3229,8 +3229,10 @@ static int bgp_zebra_process_srv6_locator_delete(ZAPI_CALLBACK_ARGS) tmp_prefi.prefixlen = 128; tmp_prefi.prefix = func->sid; if (prefix_match((struct prefix *)&loc.prefix, - (struct prefix *)&tmp_prefi)) + (struct prefix *)&tmp_prefi)) { listnode_delete(bgp->srv6_functions, func); + XFREE(MTYPE_BGP_SRV6_FUNCTION, func); + } } // refresh tovpn_sid From f8e9c702a17d6b5642078305436ad9b8920a119a Mon Sep 17 00:00:00 2001 From: Carmine Scarpitta Date: Wed, 24 Aug 2022 00:13:25 +0200 Subject: [PATCH 5/8] bgpd: Fix memory leak in SRv6 locator delete Running `bgp_srv6l3vpn_to_bgp_vrf` and `bgp_srv6l3vpn_to_bgp_vrf2` topotests with `--valgrind-memleaks` gives several memory leak errors. This is due to the way SRv6 locators are removed/unset in bgpd: when an SRv6 locator is deleted or unset, the memory allocated for the locator prefix (`tovpn_sid_locator`) is not freed. This patch adds a `for` loop that iterates over the list of BGP instances. For each BGP instance using the SRv6 locator to be removed/unset, we use `XFREE()` to properly free the memory allocated for `tovpn_sid_locator` after the SRv6 locator is removed or unset. The memory allocated for `tovpn_sid_locator` cannot be freed before calling `vpn_leak_postchange_all()`. This is because after deleting an SRv6 locator, we call `vpn_leak_postchange_all()` to handle the SRv6 locator deletion and send a BGP Prefix SID withdraw message. `tovpn_sid_locator` is required to properly build the BGP Prefix SID withdraw message. After calling `vpn_leak_postchange_all()` we can safely remove the `tovpn_sid_locator` and free the allocated memory. Signed-off-by: Carmine Scarpitta --- bgpd/bgp_vty.c | 14 ++++++++++++++ bgpd/bgp_zebra.c | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 6488c94075..ab3a24770f 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -338,6 +338,20 @@ static int bgp_srv6_locator_unset(struct bgp *bgp) /* update vpn bgp processes */ vpn_leak_postchange_all(); + /* refresh tovpn_sid_locator */ + for (ALL_LIST_ELEMENTS_RO(bm->bgp, node, bgp_vrf)) { + if (bgp_vrf->inst_type != BGP_INSTANCE_TYPE_VRF) + continue; + + /* refresh vpnv4 tovpn_sid_locator */ + XFREE(MTYPE_BGP_SRV6_SID, + bgp_vrf->vpn_policy[AFI_IP].tovpn_sid_locator); + + /* refresh vpnv6 tovpn_sid_locator */ + XFREE(MTYPE_BGP_SRV6_SID, + bgp_vrf->vpn_policy[AFI_IP6].tovpn_sid_locator); + } + /* clear locator name */ memset(bgp->srv6_locator_name, 0, sizeof(bgp->srv6_locator_name)); diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c index 33e8895bec..077785b55a 100644 --- a/bgpd/bgp_zebra.c +++ b/bgpd/bgp_zebra.c @@ -3209,7 +3209,7 @@ static int bgp_zebra_process_srv6_locator_delete(ZAPI_CALLBACK_ARGS) struct srv6_locator_chunk *chunk; struct bgp_srv6_function *func; struct bgp *bgp_vrf; - struct in6_addr *tovpn_sid; + struct in6_addr *tovpn_sid, *tovpn_sid_locator; struct prefix_ipv6 tmp_prefi; if (zapi_srv6_locator_decode(zclient->ibuf, &loc) < 0) @@ -3266,6 +3266,37 @@ static int bgp_zebra_process_srv6_locator_delete(ZAPI_CALLBACK_ARGS) } vpn_leak_postchange_all(); + + /* refresh tovpn_sid_locator */ + for (ALL_LIST_ELEMENTS_RO(bm->bgp, node, bgp_vrf)) { + if (bgp_vrf->inst_type != BGP_INSTANCE_TYPE_VRF) + continue; + + /* refresh vpnv4 tovpn_sid_locator */ + tovpn_sid_locator = + bgp_vrf->vpn_policy[AFI_IP].tovpn_sid_locator; + if (tovpn_sid_locator) { + tmp_prefi.family = AF_INET6; + tmp_prefi.prefixlen = IPV6_MAX_BITLEN; + tmp_prefi.prefix = *tovpn_sid_locator; + if (prefix_match((struct prefix *)&loc.prefix, + (struct prefix *)&tmp_prefi)) + XFREE(MTYPE_BGP_SRV6_SID, tovpn_sid_locator); + } + + /* refresh vpnv6 tovpn_sid_locator */ + tovpn_sid_locator = + bgp_vrf->vpn_policy[AFI_IP6].tovpn_sid_locator; + if (tovpn_sid_locator) { + tmp_prefi.family = AF_INET6; + tmp_prefi.prefixlen = IPV6_MAX_BITLEN; + tmp_prefi.prefix = *tovpn_sid_locator; + if (prefix_match((struct prefix *)&loc.prefix, + (struct prefix *)&tmp_prefi)) + XFREE(MTYPE_BGP_SRV6_SID, tovpn_sid_locator); + } + } + return 0; } From a0c47583a43138f32cf4c1a3691c289d46ba2933 Mon Sep 17 00:00:00 2001 From: Carmine Scarpitta Date: Wed, 24 Aug 2022 00:25:17 +0200 Subject: [PATCH 6/8] sharpd: Fix memory leak in release-locator-chunk Running the `zebra_seg6local_route` topotest with `--valgrind-memleaks` gives several memory leak errors. This is due to the way SRv6 chunks are released: when the user executes the CLI command `sharp srv6-manager release-locator-chunk` to release the chunks of an SRv6 locator, all the chunks are removed from the list `loc->chunks`. Also, the locator is removed from the SRv6 locators list `sg.srv6_locators`, but the memory allocated for the locator is not freed. This patch adds a call to `XFREE()` to properly free the allocated memory when all the chunks of an SRv6 locator are removed and the locator is removed as well. Signed-off-by: Carmine Scarpitta --- sharpd/sharp_vty.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sharpd/sharp_vty.c b/sharpd/sharp_vty.c index 2281b3ce26..b3ac8949e6 100644 --- a/sharpd/sharp_vty.c +++ b/sharpd/sharp_vty.c @@ -1096,6 +1096,7 @@ DEFPY (sharp_srv6_manager_release_locator_chunk, list_delete_all_node(loc->chunks); list_delete(&loc->chunks); listnode_delete(sg.srv6_locators, loc); + XFREE(MTYPE_SRV6_LOCATOR, loc); break; } } From 6dece5ac29320c720e2f79ca0f52ebdad7615fc8 Mon Sep 17 00:00:00 2001 From: Carmine Scarpitta Date: Wed, 24 Aug 2022 00:35:43 +0200 Subject: [PATCH 7/8] sharpd: Fix memory leak in release-locator-chunk Running the `zebra_seg6local_route` topotest with `--valgrind-memleaks` gives several memory leak errors. This is due to the way SRv6 chunks are released: when the user executes the CLI command `sharp srv6-manager release-locator-chunk` to release the chunks of an SRv6 locator, the `list_delete()` function is called to delete the chunks list (`loc->chunks`), but the memory allocated for the chunks is not freed. This patch defines a new callback `sharp_srv6_locator_chunk_free()`. This callback takes care of freeing the memory allocated for a given chunk. When `list_delete()` is called to remove the chunk list `loc->chunks`, it automatically calls `sharp_srv6_locator_chunk_free()` on each element of the list to free the allocated memory before deleting the list. Signed-off-by: Carmine Scarpitta --- sharpd/sharp_vty.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sharpd/sharp_vty.c b/sharpd/sharp_vty.c index b3ac8949e6..cfde0749b1 100644 --- a/sharpd/sharp_vty.c +++ b/sharpd/sharp_vty.c @@ -924,6 +924,11 @@ DEFPY (import_te, return CMD_SUCCESS; } +static void sharp_srv6_locator_chunk_free(struct prefix_ipv6 *chunk) +{ + prefix_ipv6_free((struct prefix_ipv6 **)&chunk); +} + DEFPY (sharp_srv6_manager_get_locator_chunk, sharp_srv6_manager_get_locator_chunk_cmd, "sharp srv6-manager get-locator-chunk NAME$locator_name", @@ -947,6 +952,8 @@ DEFPY (sharp_srv6_manager_get_locator_chunk, loc = XCALLOC(MTYPE_SRV6_LOCATOR, sizeof(struct sharp_srv6_locator)); loc->chunks = list_new(); + loc->chunks->del = + (void (*)(void *))sharp_srv6_locator_chunk_free; snprintf(loc->name, SRV6_LOCNAME_SIZE, "%s", locator_name); listnode_add(sg.srv6_locators, loc); } From 877682e328254e9f30f6734b2a736bee42f3fcc1 Mon Sep 17 00:00:00 2001 From: Carmine Scarpitta Date: Wed, 24 Aug 2022 00:53:19 +0200 Subject: [PATCH 8/8] sharpd: Fix memory leaks related to SRv6 nexthops Running the `zebra_seg6local_route` topotest with `--valgrind-memleaks` gives several memory leak errors. This is due to the way SRv6 routes (seg6 and seg6local routes) are handled: when the user executes the CLI command `sharp install seg6-routes` or `sharp install seg6local-routes` to create a seg6 or seg6local route, sharpd calls `nexthop_add_srv6_seg6` or `nexthop_add_srv6_seg6local` to create an SRv6 nexthop. A pointer to the SRv6 nexthop is stored in the global data structure `sg.r.nhop`. If you call `sharp install routes`, `sharp install seg6-routes` or `sharp install seg6local-routes` to create more routes, `sg.r.nhop` is set to zero and the pointer to the SRv6 nexthop contained in `sg.r.nhop` is definitely lost and the allocated memory is never freed. This patch adds calls to `nexthop_del_srv6_seg6()` and `nexthop_del_srv6_seg6local()` to free the memory allocated for the SRv6 nexthop before clearing the `sg.r.nhop` data structure. Signed-off-by: Carmine Scarpitta --- sharpd/sharp_vty.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sharpd/sharp_vty.c b/sharpd/sharp_vty.c index cfde0749b1..3853df7cb0 100644 --- a/sharpd/sharp_vty.c +++ b/sharpd/sharp_vty.c @@ -234,6 +234,8 @@ DEFPY (install_routes, memset(&prefix, 0, sizeof(prefix)); memset(&sg.r.orig_prefix, 0, sizeof(sg.r.orig_prefix)); + nexthop_del_srv6_seg6local(&sg.r.nhop); + nexthop_del_srv6_seg6(&sg.r.nhop); memset(&sg.r.nhop, 0, sizeof(sg.r.nhop)); memset(&sg.r.nhop_group, 0, sizeof(sg.r.nhop_group)); memset(&sg.r.backup_nhop, 0, sizeof(sg.r.nhop)); @@ -376,6 +378,8 @@ DEFPY (install_seg6_routes, memset(&prefix, 0, sizeof(prefix)); memset(&sg.r.orig_prefix, 0, sizeof(sg.r.orig_prefix)); + nexthop_del_srv6_seg6local(&sg.r.nhop); + nexthop_del_srv6_seg6(&sg.r.nhop); memset(&sg.r.nhop, 0, sizeof(sg.r.nhop)); memset(&sg.r.nhop_group, 0, sizeof(sg.r.nhop_group)); memset(&sg.r.backup_nhop, 0, sizeof(sg.r.nhop)); @@ -467,6 +471,8 @@ DEFPY (install_seg6local_routes, sg.r.repeat = 0; memset(&sg.r.orig_prefix, 0, sizeof(sg.r.orig_prefix)); + nexthop_del_srv6_seg6local(&sg.r.nhop); + nexthop_del_srv6_seg6(&sg.r.nhop); memset(&sg.r.nhop, 0, sizeof(sg.r.nhop)); memset(&sg.r.nhop_group, 0, sizeof(sg.r.nhop_group)); memset(&sg.r.backup_nhop, 0, sizeof(sg.r.nhop));