Skip to content

Commit

Permalink
staticd: fix nht memory leak
Browse files Browse the repository at this point in the history 
When a static route with a gateway nexthop is created, the nexthop is
sent to zebra for NHT, and added to a local hash. When the nexthop's VRF
is deleted from kernel, nexthop still stays in the hash. This is a
memory leak, because it is never deleted from there. Even if the VRF is
recreated in kernel, it is assigned with a new `vrf_id` so the old hash
entry is not reused, and a new one is created. To fix the issue, remove
all nexthops from the hash when the corresponding VRF is deleted, and
also do not add nexthops to the hash if their corresponding VRF doesn't
exist in kernel.

Signed-off-by: Igor Ryzhov <[email protected]>
  • Loading branch information
@idryzhov
idryzhov committed Feb 2, 2024
1 parent 3d57f04 commit 30cb582
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 69 deletions.
34 changes: 11 additions & 23 deletions staticd/static_nht.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@
#include "static_nht.h"

static void static_nht_update_path(struct static_path *pn, struct prefix *nhp,
uint32_t nh_num, vrf_id_t nh_vrf_id,
struct vrf *vrf)
uint32_t nh_num, vrf_id_t nh_vrf_id)
{
struct static_nexthop *nh;

Expand Down Expand Up @@ -49,18 +48,13 @@ static void static_nht_update_path(struct static_path *pn, struct prefix *nhp,

static void static_nht_update_safi(struct prefix *sp, struct prefix *nhp,
uint32_t nh_num, afi_t afi, safi_t safi,
struct vrf *vrf, vrf_id_t nh_vrf_id)
struct static_vrf *svrf, vrf_id_t nh_vrf_id)
{
struct route_table *stable;
struct static_vrf *svrf;
struct route_node *rn;
struct static_path *pn;
struct static_route_info *si;

svrf = vrf->info;
if (!svrf)
return;

stable = static_vrf_static_table(afi, safi, svrf);
if (!stable)
return;
Expand All @@ -71,7 +65,7 @@ static void static_nht_update_safi(struct prefix *sp, struct prefix *nhp,
si = static_route_info_from_rnode(rn);
frr_each(static_path_list, &si->path_list, pn) {
static_nht_update_path(pn, nhp, nh_num,
nh_vrf_id, vrf);
nh_vrf_id);
}
route_unlock_node(rn);
}
Expand All @@ -83,37 +77,31 @@ static void static_nht_update_safi(struct prefix *sp, struct prefix *nhp,
if (!si)
continue;
frr_each(static_path_list, &si->path_list, pn) {
static_nht_update_path(pn, nhp, nh_num, nh_vrf_id, vrf);
static_nht_update_path(pn, nhp, nh_num, nh_vrf_id);
}
}
}

void static_nht_update(struct prefix *sp, struct prefix *nhp, uint32_t nh_num,
afi_t afi, safi_t safi, vrf_id_t nh_vrf_id)
{
struct static_vrf *svrf;

struct vrf *vrf;

RB_FOREACH (vrf, vrf_name_head, &vrfs_by_name)
static_nht_update_safi(sp, nhp, nh_num, afi, safi, vrf,
RB_FOREACH (svrf, svrf_name_head, &svrfs)
static_nht_update_safi(sp, nhp, nh_num, afi, safi, svrf,
nh_vrf_id);
}

static void static_nht_reset_start_safi(struct prefix *nhp, afi_t afi,
safi_t safi, struct vrf *vrf,
safi_t safi, struct static_vrf *svrf,
vrf_id_t nh_vrf_id)
{
struct static_vrf *svrf;
struct route_table *stable;
struct static_nexthop *nh;
struct static_path *pn;
struct route_node *rn;
struct static_route_info *si;

svrf = vrf->info;
if (!svrf)
return;

stable = static_vrf_static_table(afi, safi, svrf);
if (!stable)
return;
Expand Down Expand Up @@ -153,10 +141,10 @@ static void static_nht_reset_start_safi(struct prefix *nhp, afi_t afi,
void static_nht_reset_start(struct prefix *nhp, afi_t afi, safi_t safi,
vrf_id_t nh_vrf_id)
{
struct vrf *vrf;
struct static_vrf *svrf;

RB_FOREACH (vrf, vrf_name_head, &vrfs_by_name)
static_nht_reset_start_safi(nhp, afi, safi, vrf, nh_vrf_id);
RB_FOREACH (svrf, svrf_name_head, &svrfs)
static_nht_reset_start_safi(nhp, afi, safi, svrf, nh_vrf_id);
}

static void static_nht_mark_state_safi(struct prefix *sp, afi_t afi,
Expand Down
63 changes: 18 additions & 45 deletions staticd/static_routes.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ void zebra_stable_node_cleanup(struct route_table *table,
/* Install static path into rib. */
void static_install_path(struct static_path *pn)
{
struct static_nexthop *nh;

frr_each(static_nexthop_list, &pn->nexthop_list, nh)
static_zebra_nht_register(nh, true);

if (static_nexthop_list_count(&pn->nexthop_list))
static_zebra_route_add(pn, true);
}
Expand Down Expand Up @@ -377,6 +372,17 @@ void static_install_nexthop(struct static_nexthop *nh)
}
}

void static_uninstall_nexthop(struct static_nexthop *nh)
{
struct static_path *pn = nh->pn;

if (nh->nh_vrf_id == VRF_UNKNOWN)
return;

static_zebra_nht_register(nh, false);
static_uninstall_path(pn);
}

void static_delete_nexthop(struct static_nexthop *nh)
{
struct static_path *pn = nh->pn;
Expand All @@ -386,17 +392,8 @@ void static_delete_nexthop(struct static_nexthop *nh)
/* Remove BFD session/configuration if any. */
bfd_sess_free(&nh->bsp);

if (nh->nh_vrf_id == VRF_UNKNOWN)
goto EXIT;
static_uninstall_nexthop(nh);

static_zebra_nht_register(nh, false);
/*
* If we have other si nodes then route replace
* else delete the route
*/
static_uninstall_path(pn);

EXIT:
route_unlock_node(rn);
/* Free static route configuration. */
XFREE(MTYPE_STATIC_NEXTHOP, nh);
Expand Down Expand Up @@ -490,7 +487,6 @@ static void static_fixup_vrf(struct vrf *vrf, struct route_table *stable,
continue;

nh->nh_vrf_id = vrf->vrf_id;
nh->nh_registered = false;
if (nh->ifname[0]) {
ifp = if_lookup_by_name(nh->ifname,
nh->nh_vrf_id);
Expand All @@ -500,7 +496,7 @@ static void static_fixup_vrf(struct vrf *vrf, struct route_table *stable,
continue;
}

static_install_path(pn);
static_install_nexthop(nh);
}
}
}
Expand All @@ -518,31 +514,15 @@ static void static_fixup_vrf(struct vrf *vrf, struct route_table *stable,
static void static_enable_vrf(struct route_table *stable, afi_t afi, safi_t safi)
{
struct route_node *rn;
struct static_nexthop *nh;
struct interface *ifp;
struct static_path *pn;
struct static_route_info *si;

for (rn = route_top(stable); rn; rn = route_next(rn)) {
si = static_route_info_from_rnode(rn);
if (!si)
continue;
frr_each(static_path_list, &si->path_list, pn) {
frr_each(static_nexthop_list, &pn->nexthop_list, nh) {
if (nh->nh_vrf_id == VRF_UNKNOWN)
continue;
if (nh->ifname[0]) {
ifp = if_lookup_by_name(nh->ifname,
nh->nh_vrf_id);
if (ifp)
nh->ifindex = ifp->ifindex;
else
continue;
}

static_install_path(pn);
}
}
frr_each(static_path_list, &si->path_list, pn)
static_install_path(pn);
}
}

Expand Down Expand Up @@ -604,7 +584,7 @@ static void static_cleanup_vrf(struct vrf *vrf, struct route_table *stable,
if (strcmp(vrf->name, nh->nh_vrfname) != 0)
continue;

static_uninstall_path(pn);
static_uninstall_nexthop(nh);

nh->nh_vrf_id = VRF_UNKNOWN;
nh->ifindex = IFINDEX_INTERNAL;
Expand All @@ -625,22 +605,15 @@ static void static_disable_vrf(struct route_table *stable,
afi_t afi, safi_t safi)
{
struct route_node *rn;
struct static_nexthop *nh;
struct static_path *pn;
struct static_route_info *si;

for (rn = route_top(stable); rn; rn = route_next(rn)) {
si = static_route_info_from_rnode(rn);
if (!si)
continue;
frr_each(static_path_list, &si->path_list, pn) {
frr_each(static_nexthop_list, &pn->nexthop_list, nh) {
if (nh->nh_vrf_id == VRF_UNKNOWN)
continue;

static_uninstall_path(pn);
}
}
frr_each(static_path_list, &si->path_list, pn)
static_uninstall_path(pn);
}
}

Expand Down
1 change: 1 addition & 0 deletions staticd/static_routes.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ static_add_nexthop(struct static_path *pn, enum static_nh_type type,
struct ipaddr *ipaddr, const char *ifname,
const char *nh_vrf, uint32_t color);
extern void static_install_nexthop(struct static_nexthop *nh);
extern void static_uninstall_nexthop(struct static_nexthop *nh);

extern void static_delete_nexthop(struct static_nexthop *nh);

Expand Down
2 changes: 1 addition & 1 deletion staticd/static_zebra.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ extern void static_zebra_route_add(struct static_path *pn, bool install)
struct zapi_route api;
uint32_t nh_num = 0;

if (!si->svrf->vrf)
if (!si->svrf->vrf || si->svrf->vrf->vrf_id == VRF_UNKNOWN)
return;

p = src_pp = NULL;
Expand Down

0 comments on commit 30cb582

Please sign in to comment.