Merge pull request #15285 from idryzhov/staticd-nexthop-refcounter

staticd: fix nexthop tracking memory leak and add a topotest for VRFs

staticd/static_nht.c

@@ -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,
+				   uint32_t nh_num, vrf_id_t nh_vrf_id)
-				   struct vrf *vrf)
 {
 	struct static_nexthop *nh;
 
@@ -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;
@@ -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);
 		}
@@ -83,7 +77,7 @@ 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);
 		}
 	}
 }
@@ -91,29 +85,23 @@ static void static_nht_update_safi(struct prefix *sp, struct prefix *nhp,
 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 (svrf, svrf_name_head, &svrfs)
-
+		static_nht_update_safi(sp, nhp, nh_num, afi, safi, svrf,
-	RB_FOREACH (vrf, vrf_name_head, &vrfs_by_name)
-		static_nht_update_safi(sp, nhp, nh_num, afi, safi, vrf,
 				       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;
@@ -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)
+	RB_FOREACH (svrf, svrf_name_head, &svrfs)
-		static_nht_reset_start_safi(nhp, afi, safi, vrf, nh_vrf_id);
+		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,

staticd/static_routes.c

@@ -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);
 }
@@ -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;
@@ -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)
+	static_uninstall_nexthop(nh);
-		goto EXIT;
 
-	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);
@@ -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);
@@ -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);
 			}
 		}
 	}
@@ -518,8 +514,6 @@ 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;
 
@@ -527,24 +521,10 @@ static void static_enable_vrf(struct route_table *stable, afi_t afi, safi_t safi
 		si = static_route_info_from_rnode(rn);
 		if (!si)
 			continue;
-		frr_each(static_path_list, &si->path_list, pn) {
+		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);
 	}
 }
-	}
-}
 
 /*
  * When a vrf is being enabled by the kernel, go through all the
@@ -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;
@@ -625,7 +605,6 @@ 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;
 
@@ -633,16 +612,10 @@ static void static_disable_vrf(struct route_table *stable,
 		si = static_route_info_from_rnode(rn);
 		if (!si)
 			continue;
-		frr_each(static_path_list, &si->path_list, pn) {
+		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);
 	}
 }
-	}
-}
 
 /*
  * When the disable_svrf is shutdown by the kernel, we call

staticd/static_routes.h

@@ -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);
 

staticd/static_zebra.c

@@ -390,7 +390,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;

tests/topotests/static_vrf/r1/frr.conf

@@ -0,0 +1,18 @@
+interface r1-eth0 vrf red
+  ip address 192.0.2.1/23
+exit
+
+interface r1-eth1 vrf blue
+  ip address 192.0.2.129/24
+exit
+
+ip route 198.51.100.1/32 192.0.2.2 nexthop-vrf red
+ip route 198.51.100.1/32 192.0.2.130 nexthop-vrf blue
+ip route 198.51.100.2/32 r1-eth0 nexthop-vrf red
+ip route 198.51.100.2/32 r1-eth1 nexthop-vrf blue
+
+ip route 203.0.113.1/32 192.0.2.130 vrf red nexthop-vrf blue
+ip route 203.0.113.2/32 r1-eth1 vrf red nexthop-vrf blue
+
+ip route 203.0.113.129/32 192.0.2.2 vrf blue nexthop-vrf red
+ip route 203.0.113.130/32 r1-eth0 vrf blue nexthop-vrf red

tests/topotests/static_vrf/test_static_vrf.py

@@ -0,0 +1,126 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 eval: (blacken-mode 1) -*-
+# SPDX-License-Identifier: ISC
+#
+# Copyright (c) 2024 NFWare Inc.
+#
+# noqa: E501
+#
+"""
+Test static route functionality
+"""
+
+import ipaddress
+
+import pytest
+from lib.topogen import Topogen
+from lib.common_config import retry
+
+pytestmark = [pytest.mark.staticd, pytest.mark.mgmtd]
+
+
+@pytest.fixture(scope="module")
+def tgen(request):
+    "Setup/Teardown the environment and provide tgen argument to tests"
+
+    topodef = {"s1": ("r1",), "s2": ("r1",)}
+
+    tgen = Topogen(topodef, request.module.__name__)
+    tgen.start_topology()
+
+    router_list = tgen.routers()
+    for rname, router in router_list.items():
+        # Setup VRF red
+        router.net.add_l3vrf("red", 10)
+        router.net.attach_iface_to_l3vrf(rname + "-eth0", "red")
+        # Setup VRF blue
+        router.net.add_l3vrf("blue", 20)
+        router.net.attach_iface_to_l3vrf(rname + "-eth1", "blue")
+        # Load configuration
+        router.load_frr_config("frr.conf")
+
+    tgen.start_router()
+    yield tgen
+    tgen.stop_topology()
+
+
+@retry(retry_timeout=1, initial_wait=0.1)
+def check_kernel(r1, prefix, nexthops, vrf, expected_p=True, expected_nh=True):
+    vrfstr = f" vrf {vrf}" if vrf else ""
+
+    net = ipaddress.ip_network(prefix)
+    if net.version == 6:
+        kernel = r1.run(f"ip -6 route show{vrfstr} {prefix}")
+    else:
+        kernel = r1.run(f"ip -4 route show{vrfstr} {prefix}")
+
+    if expected_p:
+        assert prefix in kernel, f"Failed to find \n'{prefix}'\n in \n'{kernel:.1920}'"
+    else:
+        assert (
+            prefix not in kernel
+        ), f"Failed found \n'{prefix}'\n in \n'{kernel:.1920}'"
+
+    if not expected_p:
+        return
+
+    for nh in nexthops:
+        if expected_nh:
+            assert f"{nh}" in kernel, f"Failed to find \n'{nh}'\n in \n'{kernel:.1920}'"
+        else:
+            assert (
+                f"{nh}" not in kernel
+            ), f"Failed found \n'{nh}'\n in \n'{kernel:.1920}'"
+
+
+def test_static_vrf(tgen):
+    if tgen.routers_have_failure():
+        pytest.skip(tgen.errors)
+
+    r1 = tgen.gears["r1"]
+
+    # Check initial configuration
+    check_kernel(r1, "198.51.100.1", ["192.0.2.2", "192.0.2.130"], None)
+    check_kernel(r1, "198.51.100.2", ["r1-eth0", "r1-eth1"], None)
+    check_kernel(r1, "203.0.113.1", ["192.0.2.130"], "red")
+    check_kernel(r1, "203.0.113.2", ["r1-eth1"], "red")
+    check_kernel(r1, "203.0.113.129", ["192.0.2.2"], "blue")
+    check_kernel(r1, "203.0.113.130", ["r1-eth0"], "blue")
+
+    # Delete VRF red
+    r1.net.del_iface("red")
+
+    # Check that "red" nexthops are removed, "blue" nexthops are still there
+    check_kernel(r1, "198.51.100.1", ["192.0.2.2"], None, expected_nh=False)
+    check_kernel(r1, "198.51.100.1", ["192.0.2.130"], None)
+    check_kernel(r1, "198.51.100.2", ["r1-eth0"], None, expected_nh=False)
+    check_kernel(r1, "198.51.100.2", ["r1-eth1"], None)
+    check_kernel(r1, "203.0.113.129", ["192.0.2.2"], "blue", expected_p=False)
+    check_kernel(r1, "203.0.113.130", ["r1-eth0"], "blue", expected_p=False)
+
+    # Delete VRF blue
+    r1.net.del_iface("blue")
+
+    # Check that "blue" nexthops are removed
+    check_kernel(r1, "198.51.100.1", ["192.0.2.130"], None, expected_p=False)
+    check_kernel(r1, "198.51.100.2", ["r1-eth1"], None, expected_p=False)
+
+    # Add VRF red back, attach "eth0" to it
+    r1.net.add_l3vrf("red", 10)
+    r1.net.attach_iface_to_l3vrf("r1-eth0", "red")
+
+    # Check that "red" nexthops are restored
+    check_kernel(r1, "198.51.100.1", ["192.0.2.2"], None)
+    check_kernel(r1, "198.51.100.2", ["r1-eth0"], None)
+
+    # Add VRF blue back, attach "eth1" to it
+    r1.net.add_l3vrf("blue", 20)
+    r1.net.attach_iface_to_l3vrf("r1-eth1", "blue")
+
+    # Check that everything is restored
+    check_kernel(r1, "198.51.100.1", ["192.0.2.2", "192.0.2.130"], None)
+    check_kernel(r1, "198.51.100.2", ["r1-eth0", "r1-eth1"], None)
+    check_kernel(r1, "203.0.113.1", ["192.0.2.130"], "red")
+    check_kernel(r1, "203.0.113.2", ["r1-eth1"], "red")
+    check_kernel(r1, "203.0.113.129", ["192.0.2.2"], "blue")
+    check_kernel(r1, "203.0.113.130", ["r1-eth0"], "blue")