From e64e052e17b2f3a7a19c3bcbc3b326088f388f3a Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 23 Feb 2021 03:05:24 +0900 Subject: [PATCH 1/4] sd-netlink: introduce sd_rtnl_message_nexthop_get_protocol() --- src/libsystemd/sd-netlink/rtnl-message.c | 16 ++++++++++++++++ src/systemd/sd-netlink.h | 1 + 2 files changed, 17 insertions(+) diff --git a/src/libsystemd/sd-netlink/rtnl-message.c b/src/libsystemd/sd-netlink/rtnl-message.c index b8c11d1181..d14fa1a5cb 100644 --- a/src/libsystemd/sd-netlink/rtnl-message.c +++ b/src/libsystemd/sd-netlink/rtnl-message.c @@ -329,6 +329,8 @@ int sd_rtnl_message_nexthop_get_family(const sd_netlink_message *m, uint8_t *fam assert_return(m, -EINVAL); assert_return(m->hdr, -EINVAL); + assert_return(rtnl_message_type_is_nexthop(m->hdr->nlmsg_type), -EINVAL); + assert_return(family, -EINVAL); nhm = NLMSG_DATA(m->hdr); *family = nhm->nh_family; @@ -336,6 +338,20 @@ int sd_rtnl_message_nexthop_get_family(const sd_netlink_message *m, uint8_t *fam return 0; } +int sd_rtnl_message_nexthop_get_protocol(const sd_netlink_message *m, uint8_t *protocol) { + struct nhmsg *nhm; + + assert_return(m, -EINVAL); + assert_return(m->hdr, -EINVAL); + assert_return(rtnl_message_type_is_nexthop(m->hdr->nlmsg_type), -EINVAL); + assert_return(protocol, -EINVAL); + + nhm = NLMSG_DATA(m->hdr); + *protocol = nhm->nh_protocol; + + return 0; +} + int sd_rtnl_message_neigh_set_flags(sd_netlink_message *m, uint8_t flags) { struct ndmsg *ndm; diff --git a/src/systemd/sd-netlink.h b/src/systemd/sd-netlink.h index 8820c65e23..276108b38d 100644 --- a/src/systemd/sd-netlink.h +++ b/src/systemd/sd-netlink.h @@ -184,6 +184,7 @@ int sd_rtnl_message_route_get_type(const sd_netlink_message *m, unsigned char *t int sd_rtnl_message_new_nexthop(sd_netlink *rtnl, sd_netlink_message **ret, uint16_t nhmsg_type, int nh_family, unsigned char nh_protocol); int sd_rtnl_message_nexthop_set_flags(sd_netlink_message *m, uint8_t flags); int sd_rtnl_message_nexthop_get_family(const sd_netlink_message *m, uint8_t *family); +int sd_rtnl_message_nexthop_get_protocol(const sd_netlink_message *m, uint8_t *protocol); int sd_rtnl_message_new_neigh(sd_netlink *nl, sd_netlink_message **ret, uint16_t msg_type, int index, int nda_family); int sd_rtnl_message_neigh_set_flags(sd_netlink_message *m, uint8_t flags); From 0e9d129c1603a0768adcb4e6b8218c16a07ac82f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 23 Feb 2021 03:10:16 +0900 Subject: [PATCH 2/4] network: nexthop: read protocol in received netlink message Preparation of later commits. --- src/network/networkd-nexthop.c | 12 ++++++++++++ src/network/networkd-nexthop.h | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index 0b598823f6..29ed5b5ef9 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -102,6 +102,7 @@ static int nexthop_new_static(Network *network, const char *filename, unsigned s static void nexthop_hash_func(const NextHop *nexthop, struct siphash *state) { assert(nexthop); + siphash24_compress(&nexthop->protocol, sizeof(nexthop->protocol), state); siphash24_compress(&nexthop->id, sizeof(nexthop->id), state); siphash24_compress(&nexthop->blackhole, sizeof(nexthop->blackhole), state); siphash24_compress(&nexthop->family, sizeof(nexthop->family), state); @@ -121,6 +122,10 @@ static void nexthop_hash_func(const NextHop *nexthop, struct siphash *state) { static int nexthop_compare_func(const NextHop *a, const NextHop *b) { int r; + r = CMP(a->protocol, b->protocol); + if (r != 0) + return r; + r = CMP(a->id, b->id); if (r != 0) return r; @@ -152,6 +157,7 @@ static void nexthop_copy(NextHop *dest, const NextHop *src) { /* This only copies entries used in the above hash and compare functions. */ + dest->protocol = src->protocol; dest->id = src->id; dest->blackhole = src->blackhole; dest->family = src->family; @@ -530,6 +536,12 @@ int manager_rtnl_process_nexthop(sd_netlink *rtnl, sd_netlink_message *message, } else if (!IN_SET(tmp->family, AF_INET, AF_INET6)) return log_link_debug(link, "rtnl: received nexthop message with invalid family %d, ignoring.", tmp->family); + r = sd_rtnl_message_nexthop_get_protocol(message, &tmp->protocol); + if (r < 0) { + log_link_warning_errno(link, r, "rtnl: could not get nexthop protocol, ignoring: %m"); + return 0; + } + r = netlink_message_read_in_addr_union(message, NHA_GATEWAY, tmp->family, &tmp->gw); if (r < 0 && r != -ENODATA) { log_link_warning_errno(link, r, "rtnl: could not get NHA_GATEWAY attribute, ignoring: %m"); diff --git a/src/network/networkd-nexthop.h b/src/network/networkd-nexthop.h index 9ead5fc95c..3c4e560c40 100644 --- a/src/network/networkd-nexthop.h +++ b/src/network/networkd-nexthop.h @@ -23,7 +23,7 @@ typedef struct NextHop { Manager *manager; Link *link; - unsigned char protocol; + uint8_t protocol; uint32_t id; bool blackhole; From 25b82b6e0ef02e3c0b58a3f8e253ae3236c2b72b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 15 Feb 2021 10:00:14 +0900 Subject: [PATCH 3/4] network: nexthop: drop unnecessary nexthops Similar to addresses or routes, this makes networkd drops unnecessary nexthops on configuring links or when a link is dropped. --- src/network/networkd-link.c | 8 ++ src/network/networkd-nexthop.c | 178 +++++++++++++++++++++++++++++++++ src/network/networkd-nexthop.h | 2 + 3 files changed, 188 insertions(+) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index b2949051bd..182c075a53 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -1980,6 +1980,10 @@ static int link_drop_foreign_config(Link *link) { if (k < 0 && r >= 0) r = k; + k = link_drop_foreign_nexthops(link); + if (k < 0 && r >= 0) + r = k; + k = manager_drop_foreign_routing_policy_rules(link->manager); if (k < 0 && r >= 0) r = k; @@ -2003,6 +2007,10 @@ static int link_drop_config(Link *link) { if (k < 0 && r >= 0) r = k; + k = link_drop_nexthops(link); + if (k < 0 && r >= 0) + r = k; + k = manager_drop_routing_policy_rules(link->manager, link); if (k < 0 && r >= 0) r = k; diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index 29ed5b5ef9..470095c897 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -151,6 +151,16 @@ DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR( nexthop_compare_func, nexthop_free); +static bool nexthop_equal(const NextHop *a, const NextHop *b) { + if (a == b) + return true; + + if (!a || !b) + return false; + + return nexthop_compare_func(a, b) == 0; +} + static void nexthop_copy(NextHop *dest, const NextHop *src) { assert(dest); assert(src); @@ -345,6 +355,56 @@ static void log_nexthop_debug(const NextHop *nexthop, uint32_t id, const char *s } } +static int nexthop_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) { + int r; + + assert(m); + + /* Note that link may be NULL. */ + if (link && IN_SET(link->state, LINK_STATE_FAILED, LINK_STATE_LINGER)) + return 1; + + r = sd_netlink_message_get_errno(m); + if (r < 0 && r != -ENOENT) + log_link_message_warning_errno(link, m, r, "Could not drop nexthop, ignoring"); + + return 1; +} + +static int nexthop_remove(const NextHop *nexthop, Manager *manager, Link *link) { + _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *req = NULL; + int r; + + assert(nexthop); + assert(manager); + + /* link may be NULL. */ + + if (nexthop->id == 0) { + log_link_debug(link, "Cannot remove nexthop without valid ID, ignoring."); + return 0; + } + + log_nexthop_debug(nexthop, nexthop->id, "Removing", link); + + r = sd_rtnl_message_new_nexthop(manager->rtnl, &req, RTM_DELNEXTHOP, AF_UNSPEC, RTPROT_UNSPEC); + if (r < 0) + return log_link_error_errno(link, r, "Could not create RTM_DELNEXTHOP message: %m"); + + r = sd_netlink_message_append_u32(req, NHA_ID, nexthop->id); + if (r < 0) + return log_link_error_errno(link, r, "Could not append NHA_ID attribute: %m"); + + r = netlink_call_async(manager->rtnl, NULL, req, nexthop_remove_handler, + link_netlink_destroy_callback, link); + if (r < 0) + return log_link_error_errno(link, r, "Could not send rtnetlink message: %m"); + + link_ref(link); /* link may be NULL, link_ref() is OK with that */ + + return 0; +} + static int nexthop_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) { int r; @@ -478,6 +538,124 @@ int link_set_nexthops(Link *link) { return 0; } +static bool link_has_nexthop(const Link *link, const NextHop *nexthop) { + NextHop *net_nexthop; + + assert(link); + assert(nexthop); + + if (!link->network) + return false; + + HASHMAP_FOREACH(net_nexthop, link->network->nexthops_by_section) + if (nexthop_equal(net_nexthop, nexthop)) + return true; + + return false; +} + +static bool links_have_nexthop(const Manager *manager, const NextHop *nexthop, const Link *except) { + Link *link; + + assert(manager); + + HASHMAP_FOREACH(link, manager->links) { + if (link == except) + continue; + + if (link_has_nexthop(link, nexthop)) + return true; + } + + return false; +} + +static int manager_drop_nexthops_internal(Manager *manager, bool foreign, const Link *except) { + NextHop *nexthop; + Set *nexthops; + int k, r = 0; + + assert(manager); + + nexthops = foreign ? manager->nexthops_foreign : manager->nexthops; + SET_FOREACH(nexthop, nexthops) { + /* do not touch nexthop created by the kernel */ + if (nexthop->protocol == RTPROT_KERNEL) + continue; + + /* The nexthop will be configured later, or already configured by a link. */ + if (links_have_nexthop(manager, nexthop, except)) + continue; + + /* The existing links do not have the nexthop. Let's drop this now. It may be + * re-configured later. */ + k = nexthop_remove(nexthop, manager, NULL); + if (k < 0 && r >= 0) + r = k; + } + + return r; +} + +static int manager_drop_foreign_nexthops(Manager *manager) { + return manager_drop_nexthops_internal(manager, true, NULL); +} + +static int manager_drop_nexthops(Manager *manager, const Link *except) { + return manager_drop_nexthops_internal(manager, false, except); +} + +int link_drop_foreign_nexthops(Link *link) { + NextHop *nexthop; + int k, r = 0; + + assert(link); + assert(link->manager); + + SET_FOREACH(nexthop, link->nexthops_foreign) { + /* do not touch nexthop created by the kernel */ + if (nexthop->protocol == RTPROT_KERNEL) + continue; + + if (link_has_nexthop(link, nexthop)) + k = nexthop_add(link, nexthop, NULL); + else + k = nexthop_remove(nexthop, link->manager, link); + if (k < 0 && r >= 0) + r = k; + } + + k = manager_drop_foreign_nexthops(link->manager); + if (k < 0 && r >= 0) + r = k; + + return r; +} + +int link_drop_nexthops(Link *link) { + NextHop *nexthop; + int k, r = 0; + + assert(link); + assert(link->manager); + + SET_FOREACH(nexthop, link->nexthops) { + /* do not touch nexthop created by the kernel */ + if (nexthop->protocol == RTPROT_KERNEL) + continue; + + k = nexthop_remove(nexthop, link->manager, link); + if (k < 0 && r >= 0) + r = k; + } + + k = manager_drop_nexthops(link->manager, link); + if (k < 0 && r >= 0) + r = k; + + return r; +} + int manager_rtnl_process_nexthop(sd_netlink *rtnl, sd_netlink_message *message, Manager *m) { _cleanup_(nexthop_freep) NextHop *tmp = NULL; NextHop *nexthop = NULL; diff --git a/src/network/networkd-nexthop.h b/src/network/networkd-nexthop.h index 3c4e560c40..cf06b7e86b 100644 --- a/src/network/networkd-nexthop.h +++ b/src/network/networkd-nexthop.h @@ -37,6 +37,8 @@ NextHop *nexthop_free(NextHop *nexthop); void network_drop_invalid_nexthops(Network *network); int link_set_nexthops(Link *link); +int link_drop_nexthops(Link *link); +int link_drop_foreign_nexthops(Link *link); int manager_get_nexthop_by_id(Manager *manager, uint32_t id, NextHop **ret); int manager_rtnl_process_nexthop(sd_netlink *rtnl, sd_netlink_message *message, Manager *m); From 9947c7bad1691a656aebe160ee3a7503061ae1c2 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 23 Feb 2021 18:45:48 +0900 Subject: [PATCH 4/4] test-network: add tests for dropping unnecessary nexthops --- .../conf/25-nexthop-nothing.network | 8 ++++++ test/test-network/systemd-networkd-tests.py | 28 +++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 test/test-network/conf/25-nexthop-nothing.network diff --git a/test/test-network/conf/25-nexthop-nothing.network b/test/test-network/conf/25-nexthop-nothing.network new file mode 100644 index 0000000000..fdaf32d534 --- /dev/null +++ b/test/test-network/conf/25-nexthop-nothing.network @@ -0,0 +1,8 @@ +[Match] +Name=veth99 + +[Network] +IPv6AcceptRA=no +Address=2001:1234:5:8f63::1/120 +Address=192.168.5.10/24 +Gateway=192.168.5.1 diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 6c17b204a1..70285d2a89 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -1801,6 +1801,7 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): '25-neighbor-ipv6.network', '25-neighbor-ip-dummy.network', '25-neighbor-ip.network', + '25-nexthop-nothing.network', '25-nexthop.network', '25-qdisc-cake.network', '25-qdisc-clsact-and-htb.network', @@ -2868,6 +2869,33 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): print(output) self.assertEqual('blackhole 2001:1234:5:8f62::2 nhid 7 dev lo proto static metric 1024 pref medium', output) + remove_unit_from_networkd_path(['25-nexthop.network']) + copy_unit_to_networkd_unit_path('25-nexthop-nothing.network') + rc = call(*networkctl_cmd, 'reload', env=env) + self.assertEqual(rc, 0) + time.sleep(1) + + output = check_output('ip nexthop list dev veth99') + print(output) + self.assertEqual(output, '') + output = check_output('ip nexthop list dev lo') + print(output) + self.assertEqual(output, '') + + remove_unit_from_networkd_path(['25-nexthop-nothing.network']) + copy_unit_to_networkd_unit_path('25-nexthop.network') + rc = call(*networkctl_cmd, 'reload', env=env) + self.assertEqual(rc, 0) + time.sleep(1) + + rc = call('ip link del veth99') + self.assertEqual(rc, 0) + time.sleep(2) + + output = check_output('ip nexthop list dev lo') + print(output) + self.assertEqual(output, '') + def test_qdisc(self): copy_unit_to_networkd_unit_path('25-qdisc-clsact-and-htb.network', '12-dummy.netdev', '25-qdisc-ingress-netem-compat.network', '11-dummy.netdev')