From f450e1cda41f1b7576094a0b3017ba9849cd55ae Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 7 Jun 2024 12:30:59 -0400 Subject: [PATCH 1/5] zebra: Make p and src_p const for rib_delete The prefix'es p and src_p are not const. Let's make them so. Useful to signal that we will not change this data. Signed-off-by: Donald Sharp --- zebra/rib.h | 2 +- zebra/zebra_rib.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/zebra/rib.h b/zebra/rib.h index 84ea766c4733..7f4e3949e02d 100644 --- a/zebra/rib.h +++ b/zebra/rib.h @@ -395,7 +395,7 @@ extern int rib_add_multipath_nhe(afi_t afi, safi_t safi, struct prefix *p, extern void rib_delete(afi_t afi, safi_t safi, vrf_id_t vrf_id, int type, unsigned short instance, uint32_t flags, - struct prefix *p, struct prefix_ipv6 *src_p, + const struct prefix *p, const struct prefix_ipv6 *src_p, const struct nexthop *nh, uint32_t nhe_id, uint32_t table_id, uint32_t metric, uint8_t distance, bool fromkernel); diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index 59190e9dd330..c1bd61e1db31 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -4393,8 +4393,8 @@ int rib_add_multipath(afi_t afi, safi_t safi, struct prefix *p, } void rib_delete(afi_t afi, safi_t safi, vrf_id_t vrf_id, int type, - unsigned short instance, uint32_t flags, struct prefix *p, - struct prefix_ipv6 *src_p, const struct nexthop *nh, + unsigned short instance, uint32_t flags, const struct prefix *p, + const struct prefix_ipv6 *src_p, const struct nexthop *nh, uint32_t nhe_id, uint32_t table_id, uint32_t metric, uint8_t distance, bool fromkernel) { From bdfccf69fa128c51c45bbd3528788f72ac17d854 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 7 Jun 2024 12:56:35 -0400 Subject: [PATCH 2/5] zebra: Expose rib_update_handle_vrf_all This function will be used on interface down events to allow for kernel routes to be cleaned up. Signed-off-by: Donald Sharp --- zebra/rib.h | 2 ++ zebra/zebra_rib.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/zebra/rib.h b/zebra/rib.h index 7f4e3949e02d..8792fb7908ac 100644 --- a/zebra/rib.h +++ b/zebra/rib.h @@ -477,6 +477,8 @@ extern uint8_t route_distance(int type); extern void zebra_rib_evaluate_rn_nexthops(struct route_node *rn, uint32_t seq, bool rt_delete); +extern void rib_update_handle_vrf_all(enum rib_update_event event, int rtype); + /* * rib_find_rn_from_ctx * diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index c1bd61e1db31..649450b5c63c 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -4543,7 +4543,7 @@ void rib_update_table(struct route_table *table, enum rib_update_event event, } } -static void rib_update_handle_vrf_all(enum rib_update_event event, int rtype) +void rib_update_handle_vrf_all(enum rib_update_event event, int rtype) { struct zebra_router_table *zrt; From d528c02a204086da0d542d5655b8724de681a65c Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 7 Jun 2024 13:50:07 -0400 Subject: [PATCH 3/5] zebra: Handle kernel routes appropriately Current code intentionally ignores kernel routes. Modify zebra to allow these routes to be read in on linux. Also modify zebra to look to see if a route should be treated as a connected and mark it as such. Additionally this should properly handle some of the issues being seen with NOPREFIXROUTE. Signed-off-by: Donald Sharp --- zebra/interface.c | 2 + zebra/rib.h | 1 + zebra/rt_netlink.c | 2 - zebra/zebra_rib.c | 105 +++++++++++++++++++++++++++++++++++++++------ 4 files changed, 96 insertions(+), 14 deletions(-) diff --git a/zebra/interface.c b/zebra/interface.c index 03b710e1a0f9..d146004781a5 100644 --- a/zebra/interface.c +++ b/zebra/interface.c @@ -1058,6 +1058,8 @@ void if_down(struct interface *ifp) /* Delete all neighbor addresses learnt through IPv6 RA */ if_down_del_nbr_connected(ifp); + + rib_update_handle_vrf_all(RIB_UPDATE_INTERFACE_DOWN, ZEBRA_ROUTE_KERNEL); } void if_refresh(struct interface *ifp) diff --git a/zebra/rib.h b/zebra/rib.h index 8792fb7908ac..cd6efbfb36dd 100644 --- a/zebra/rib.h +++ b/zebra/rib.h @@ -326,6 +326,7 @@ typedef struct rib_tables_iter_t_ { /* Events/reasons triggering a RIB update. */ enum rib_update_event { + RIB_UPDATE_INTERFACE_DOWN, RIB_UPDATE_KERNEL, RIB_UPDATE_RMAP_CHANGE, RIB_UPDATE_OTHER, diff --git a/zebra/rt_netlink.c b/zebra/rt_netlink.c index c22145be693b..ddcb83cd8ce7 100644 --- a/zebra/rt_netlink.c +++ b/zebra/rt_netlink.c @@ -799,8 +799,6 @@ int netlink_route_change_read_unicast_internal(struct nlmsghdr *h, return 0; if (rtm->rtm_protocol == RTPROT_REDIRECT) return 0; - if (rtm->rtm_protocol == RTPROT_KERNEL) - return 0; selfroute = is_selfroute(rtm->rtm_protocol); diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index 649450b5c63c..2d6c5148833a 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -1619,6 +1619,10 @@ static bool rib_compare_routes(const struct route_entry *re1, * v6 link-locals, and we also support multiple addresses in the same * subnet on a single interface. */ + if (re1->type == ZEBRA_ROUTE_CONNECT && + (re1->nhe->nhg.nexthop->ifindex == re2->nhe->nhg.nexthop->ifindex)) + return true; + if (re1->type != ZEBRA_ROUTE_CONNECT && re1->type != ZEBRA_ROUTE_LOCAL) return true; @@ -2863,10 +2867,11 @@ static void process_subq_early_route_add(struct zebra_early_route *ere) /* Link new re to node.*/ if (IS_ZEBRA_DEBUG_RIB) { - rnode_debug( - rn, re->vrf_id, - "Inserting route rn %p, re %p (%s) existing %p, same_count %d", - rn, re, zebra_route_string(re->type), same, same_count); + rnode_debug(rn, re->vrf_id, + "Inserting route rn %p, re %p (%s/%s/%s) existing %p, same_count %d", + rn, re, zebra_route_string(re->type), + afi2str(ere->afi), safi2str(ere->safi), same, + same_count); if (IS_ZEBRA_DEBUG_RIB_DETAILED) route_entry_dump( @@ -4383,6 +4388,34 @@ int rib_add_multipath(afi_t afi, safi_t safi, struct prefix *p, nhe.id = re->nhe_id; n = zebra_nhe_copy(&nhe, 0); + + if (re->type == ZEBRA_ROUTE_KERNEL) { + struct interface *ifp; + struct connected *connected; + + if (p->family == AF_INET6 && + IN6_IS_ADDR_LINKLOCAL(&p->u.prefix6)) { + zebra_nhg_free(n); + zebra_rib_route_entry_free(re); + return -1; + } + + ifp = if_lookup_prefix(p, re->vrf_id); + if (ifp) { + connected = connected_lookup_prefix(ifp, p); + + if (connected && !CHECK_FLAG(connected->flags, + ZEBRA_IFA_NOPREFIXROUTE)) { + zebra_nhg_free(n); + zebra_rib_route_entry_free(re); + return -1; + } + + if (ifp->ifindex == ng->nexthop->ifindex) + re->type = ZEBRA_ROUTE_CONNECT; + } + } + ret = rib_add_multipath_nhe(afi, safi, p, src_p, re, n, startup); /* In error cases, free the route also */ @@ -4458,6 +4491,9 @@ static const char *rib_update_event2str(enum rib_update_event event) const char *ret = "UNKNOWN"; switch (event) { + case RIB_UPDATE_INTERFACE_DOWN: + ret = "RIB_UPDATE_INTERFACE_DOWN"; + break; case RIB_UPDATE_KERNEL: ret = "RIB_UPDATE_KERNEL"; break; @@ -4474,15 +4510,56 @@ static const char *rib_update_event2str(enum rib_update_event event) return ret; } +/* + * We now keep kernel routes, but we don't have any + * trigger events for them when they are implicitly + * deleted. Since we are already walking the + * entire table on a down event let's look at + * the few kernel routes we may have + */ +static void +rib_update_handle_kernel_route_down_possibility(struct route_node *rn, + struct route_entry *re) +{ + struct nexthop *nexthop = NULL; + bool alive = false; + + for (ALL_NEXTHOPS(re->nhe->nhg, nexthop)) { + struct interface *ifp = if_lookup_by_index(nexthop->ifindex, + nexthop->vrf_id); + + if (ifp && if_is_up(ifp)) { + alive = true; + break; + } + } + + if (!alive) { + struct rib_table_info *rib_table = srcdest_rnode_table_info(rn); + const struct prefix *p; + const struct prefix_ipv6 *src_p; + + srcdest_rnode_prefixes(rn, &p, (const struct prefix **)&src_p); + + rib_delete(rib_table->afi, rib_table->safi, re->vrf_id, + re->type, re->instance, re->flags, p, src_p, NULL, 0, + re->table, re->metric, re->distance, true); + } +} + /* Schedule route nodes to be processed if they match the type */ -static void rib_update_route_node(struct route_node *rn, int type) +static void rib_update_route_node(struct route_node *rn, int type, + enum rib_update_event event) { struct route_entry *re, *next; bool re_changed = false; RNODE_FOREACH_RE_SAFE (rn, re, next) { - if (type == ZEBRA_ROUTE_ALL || type == re->type) { + if (event == RIB_UPDATE_INTERFACE_DOWN && type == re->type && + type == ZEBRA_ROUTE_KERNEL) + rib_update_handle_kernel_route_down_possibility(rn, re); + else if (type == ZEBRA_ROUTE_ALL || type == re->type) { SET_FLAG(re->status, ROUTE_ENTRY_CHANGED); re_changed = true; } @@ -4522,20 +4599,24 @@ void rib_update_table(struct route_table *table, enum rib_update_event event, /* * If we are looking at a route node and the node * has already been queued we don't - * need to queue it up again + * need to queue it up again, unless it is + * an interface down event as that we need + * to process this no matter what. */ - if (rn->info - && CHECK_FLAG(rib_dest_from_rnode(rn)->flags, - RIB_ROUTE_ANY_QUEUED)) + if (rn->info && + CHECK_FLAG(rib_dest_from_rnode(rn)->flags, + RIB_ROUTE_ANY_QUEUED) && + event != RIB_UPDATE_INTERFACE_DOWN) continue; switch (event) { + case RIB_UPDATE_INTERFACE_DOWN: case RIB_UPDATE_KERNEL: - rib_update_route_node(rn, ZEBRA_ROUTE_KERNEL); + rib_update_route_node(rn, ZEBRA_ROUTE_KERNEL, event); break; case RIB_UPDATE_RMAP_CHANGE: case RIB_UPDATE_OTHER: - rib_update_route_node(rn, rtype); + rib_update_route_node(rn, rtype, event); break; case RIB_UPDATE_MAX: break; From 9bc0cd8241f39e4fd751edfa52c09fae6db2db1c Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 26 Jun 2024 13:21:38 -0400 Subject: [PATCH 4/5] zebra: Prevent accidental re memory leak in odd case There exists a path in rib_add_multipath where if a decision is made to not use the passed in re, we just drop the memory instead of freeing it. Let's free it. Signed-off-by: Donald Sharp --- zebra/zebra_rib.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index 2d6c5148833a..b4baee148aef 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -4375,8 +4375,10 @@ int rib_add_multipath(afi_t afi, safi_t safi, struct prefix *p, return -1; /* We either need nexthop(s) or an existing nexthop id */ - if (ng == NULL && re->nhe_id == 0) + if (ng == NULL && re->nhe_id == 0) { + zebra_rib_route_entry_free(re); return -1; + } /* * Use a temporary nhe to convey info to the common/main api. From 37dd51867f2b98f0fb616fc3cf9922240346fd19 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 15 Aug 2024 16:02:55 -0400 Subject: [PATCH 5/5] tests: Add some tests to show new behavior works as expected a) A noprefix address by itself should not create a connected route. This was pre-existing. b) A noprefix address with a corresponding route should result in a connected route. This is how NetworkManager appears to work. This is new behavior, so a new test. c) A route is added to the system from someone else. This is new behavior, so a new test. Signed-off-by: Donald Sharp --- .../r1/ip_route_connected.json | 24 +++++++++++ .../r1/ip_route_kernel.json | 24 +++++++++++ .../test_zebra_multiple_connected.py | 43 +++++++++++++++++++ 3 files changed, 91 insertions(+) create mode 100644 tests/topotests/zebra_multiple_connected/r1/ip_route_connected.json create mode 100644 tests/topotests/zebra_multiple_connected/r1/ip_route_kernel.json diff --git a/tests/topotests/zebra_multiple_connected/r1/ip_route_connected.json b/tests/topotests/zebra_multiple_connected/r1/ip_route_connected.json new file mode 100644 index 000000000000..db03ce84a6a4 --- /dev/null +++ b/tests/topotests/zebra_multiple_connected/r1/ip_route_connected.json @@ -0,0 +1,24 @@ +{ + "192.168.44.0/24":[ + { + "prefix":"192.168.44.0/24", + "prefixLen":24, + "protocol":"connected", + "vrfName":"default", + "selected":true, + "destSelected":true, + "distance":0, + "metric":0, + "installed":true, + "table":254, + "nexthops":[ + { + "fib":true, + "directlyConnected":true, + "interfaceName":"r1-eth1", + "active":true + } + ] + } + ] +} diff --git a/tests/topotests/zebra_multiple_connected/r1/ip_route_kernel.json b/tests/topotests/zebra_multiple_connected/r1/ip_route_kernel.json new file mode 100644 index 000000000000..22465cb477d3 --- /dev/null +++ b/tests/topotests/zebra_multiple_connected/r1/ip_route_kernel.json @@ -0,0 +1,24 @@ +{ + "4.5.6.7/32":[ + { + "prefix":"4.5.6.7/32", + "prefixLen":32, + "protocol":"kernel", + "vrfName":"default", + "selected":true, + "destSelected":true, + "distance":0, + "metric":0, + "installed":true, + "table":254, + "nexthops":[ + { + "fib":true, + "directlyConnected":true, + "interfaceName":"r1-eth1", + "active":true + } + ] + } + ] +} diff --git a/tests/topotests/zebra_multiple_connected/test_zebra_multiple_connected.py b/tests/topotests/zebra_multiple_connected/test_zebra_multiple_connected.py index dc47527c74c2..7dbeb6f1ccb3 100644 --- a/tests/topotests/zebra_multiple_connected/test_zebra_multiple_connected.py +++ b/tests/topotests/zebra_multiple_connected/test_zebra_multiple_connected.py @@ -19,6 +19,9 @@ import pytest import json from functools import partial +from lib.topolog import logger + +pytestmark = pytest.mark.random_order(disabled=True) # Save the Current Working Directory to find configuration files. CWD = os.path.dirname(os.path.realpath(__file__)) @@ -159,6 +162,46 @@ def test_zebra_noprefix_connected(): assert result, "Connected Route should not have been added" +def test_zebra_noprefix_connected_add(): + "Test that a noprefixroute created with a manual route works as expected, this is for NetworkManager" + + tgen = get_topogen() + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + router = tgen.gears["r1"] + router.run("ip route add 192.168.44.0/24 dev r1-eth1") + + connected = "{}/{}/ip_route_connected.json".format(CWD, router.name) + expected = json.loads(open(connected).read()) + + test_func = partial( + topotest.router_json_cmp, router, "show ip route 192.168.44.0/24 json", expected + ) + result, _ = topotest.run_and_expect(test_func, None, count=20, wait=1) + assert result, "Connected Route should have been added\n{}".format(_) + + +def test_zebra_kernel_route_add(): + "Test that a random kernel route is properly handled as expected" + + tgen = get_topogen() + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + router = tgen.gears["r1"] + router.run("ip route add 4.5.6.7/32 dev r1-eth1") + + kernel = "{}/{}/ip_route_kernel.json".format(CWD, router.name) + expected = json.loads(open(kernel).read()) + + test_func = partial( + topotest.router_json_cmp, router, "show ip route 4.5.6.7/32 json", expected + ) + result, _ = topotest.run_and_expect(test_func, None, count=20, wait=1) + assert result, "Connected Route should have been added\n{}".format(_) + + if __name__ == "__main__": args = ["-s"] + sys.argv[1:] sys.exit(pytest.main(args))