diff --git a/src/network/networkd-neighbor.c b/src/network/networkd-neighbor.c index c1629ae3e9a..c712954e0c8 100644 --- a/src/network/networkd-neighbor.c +++ b/src/network/networkd-neighbor.c @@ -16,7 +16,7 @@ Neighbor *neighbor_free(Neighbor *neighbor) { if (neighbor->network) { assert(neighbor->section); - hashmap_remove(neighbor->network->neighbors_by_section, neighbor->section); + ordered_hashmap_remove(neighbor->network->neighbors_by_section, neighbor->section); } config_section_free(neighbor->section); @@ -43,7 +43,7 @@ static int neighbor_new_static(Network *network, const char *filename, unsigned if (r < 0) return r; - neighbor = hashmap_get(network->neighbors_by_section, n); + neighbor = ordered_hashmap_get(network->neighbors_by_section, n); if (neighbor) { *ret = TAKE_PTR(neighbor); return 0; @@ -60,7 +60,7 @@ static int neighbor_new_static(Network *network, const char *filename, unsigned .source = NETWORK_CONFIG_SOURCE_STATIC, }; - r = hashmap_ensure_put(&network->neighbors_by_section, &config_section_hash_ops, neighbor->section, neighbor); + r = ordered_hashmap_ensure_put(&network->neighbors_by_section, &config_section_hash_ops, neighbor->section, neighbor); if (r < 0) return r; @@ -92,18 +92,13 @@ static void neighbor_hash_func(const Neighbor *neighbor, struct siphash *state) siphash24_compress(&neighbor->family, sizeof(neighbor->family), state); - switch (neighbor->family) { - case AF_INET: - case AF_INET6: - /* Equality of neighbors are given by the pair (addr,lladdr) */ - siphash24_compress(&neighbor->in_addr, FAMILY_ADDRESS_SIZE(neighbor->family), state); - break; - default: + if (!IN_SET(neighbor->family, AF_INET, AF_INET6)) /* treat any other address family as AF_UNSPEC */ - break; - } + return; - hw_addr_hash_func(&neighbor->ll_addr, state); + /* Equality of neighbors are given by the destination address. + * See neigh_lookup() in the kernel. */ + siphash24_compress(&neighbor->in_addr, FAMILY_ADDRESS_SIZE(neighbor->family), state); } static int neighbor_compare_func(const Neighbor *a, const Neighbor *b) { @@ -113,18 +108,49 @@ static int neighbor_compare_func(const Neighbor *a, const Neighbor *b) { if (r != 0) return r; - switch (a->family) { - case AF_INET: - case AF_INET6: - r = memcmp(&a->in_addr, &b->in_addr, FAMILY_ADDRESS_SIZE(a->family)); - if (r != 0) - return r; - } + if (!IN_SET(a->family, AF_INET, AF_INET6)) + /* treat any other address family as AF_UNSPEC */ + return 0; - return hw_addr_compare(&a->ll_addr, &b->ll_addr); + return memcmp(&a->in_addr, &b->in_addr, FAMILY_ADDRESS_SIZE(a->family)); } -DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR(neighbor_hash_ops, Neighbor, neighbor_hash_func, neighbor_compare_func, neighbor_free); +DEFINE_PRIVATE_HASH_OPS( + neighbor_hash_ops, + Neighbor, + neighbor_hash_func, + neighbor_compare_func); + +DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR( + neighbor_hash_ops_free, + Neighbor, + neighbor_hash_func, + neighbor_compare_func, + neighbor_free); + +static int neighbor_get_request(Link *link, const Neighbor *neighbor, Request **ret) { + Request *req; + + assert(link); + assert(link->manager); + assert(neighbor); + + req = ordered_set_get( + link->manager->request_queue, + &(Request) { + .link = link, + .type = REQUEST_TYPE_NEIGHBOR, + .userdata = (void*) neighbor, + .hash_func = (hash_func_t) neighbor_hash_func, + .compare_func = (compare_func_t) neighbor_compare_func, + }); + if (!req) + return -ENOENT; + + if (ret) + *ret = req; + return 0; +} static int neighbor_get(Link *link, const Neighbor *in, Neighbor **ret) { Neighbor *existing; @@ -147,7 +173,7 @@ static int neighbor_add(Link *link, Neighbor *neighbor) { assert(link); assert(neighbor); - r = set_ensure_put(&link->neighbors, &neighbor_hash_ops, neighbor); + r = set_ensure_put(&link->neighbors, &neighbor_hash_ops_free, neighbor); if (r < 0) return r; if (r == 0) @@ -209,6 +235,7 @@ static int neighbor_configure(Neighbor *neighbor, Link *link, Request *req) { } static int neighbor_process_request(Request *req, Link *link, Neighbor *neighbor) { + Neighbor *existing; int r; assert(req); @@ -223,6 +250,9 @@ static int neighbor_process_request(Request *req, Link *link, Neighbor *neighbor return log_link_warning_errno(link, r, "Failed to configure neighbor: %m"); neighbor_enter_configuring(neighbor); + if (neighbor_get(link, neighbor, &existing) >= 0) + neighbor_enter_configuring(existing); + return 1; } @@ -249,31 +279,36 @@ static int static_neighbor_configure_handler(sd_netlink *rtnl, sd_netlink_messag } static int link_request_neighbor(Link *link, const Neighbor *neighbor) { - Neighbor *existing; + _cleanup_(neighbor_freep) Neighbor *tmp = NULL; + Neighbor *existing = NULL; int r; assert(link); assert(neighbor); assert(neighbor->source != NETWORK_CONFIG_SOURCE_FOREIGN); - if (neighbor_get(link, neighbor, &existing) < 0) { - _cleanup_(neighbor_freep) Neighbor *tmp = NULL; + if (neighbor->ll_addr.length != link->hw_addr.length) { + log_link_debug(link, + "The link layer address lenght (%zu) for neighbor %s does not match with " + "the hardware address length (%zu), ignoring the setting.", + neighbor->ll_addr.length, + IN_ADDR_TO_STRING(neighbor->family, &neighbor->in_addr), + link->hw_addr.length); + return 0; + } - r = neighbor_dup(neighbor, &tmp); - if (r < 0) - return r; + r = neighbor_dup(neighbor, &tmp); + if (r < 0) + return r; - r = neighbor_add(link, tmp); - if (r < 0) - return r; + if (neighbor_get(link, neighbor, &existing) >= 0) + /* Copy state for logging below. */ + tmp->state = existing->state; - existing = TAKE_PTR(tmp); - } else - existing->source = neighbor->source; - - log_neighbor_debug(existing, "Requesting", link); + log_neighbor_debug(tmp, "Requesting", link); r = link_queue_request_safe(link, REQUEST_TYPE_NEIGHBOR, - existing, NULL, + tmp, + neighbor_free, neighbor_hash_func, neighbor_compare_func, neighbor_process_request, @@ -283,7 +318,11 @@ static int link_request_neighbor(Link *link, const Neighbor *neighbor) { if (r <= 0) return r; - neighbor_enter_requesting(existing); + neighbor_enter_requesting(tmp); + if (existing) + neighbor_enter_requesting(existing); + + TAKE_PTR(tmp); return 1; } @@ -297,7 +336,7 @@ int link_request_static_neighbors(Link *link) { link->static_neighbors_configured = false; - HASHMAP_FOREACH(neighbor, link->network->neighbors_by_section) { + ORDERED_HASHMAP_FOREACH(neighbor, link->network->neighbors_by_section) { r = link_request_neighbor(link, neighbor); if (r < 0) return log_link_warning_errno(link, r, "Could not request neighbor: %m"); @@ -333,6 +372,7 @@ static int neighbor_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, Link static int neighbor_remove(Neighbor *neighbor) { _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *m = NULL; + Request *req; Link *link; int r; @@ -362,6 +402,9 @@ static int neighbor_remove(Neighbor *neighbor) { link_ref(link); neighbor_enter_removing(neighbor); + if (neighbor_get_request(neighbor->link, neighbor, &req) >= 0) + neighbor_enter_removing(req->userdata); + return 0; } @@ -386,7 +429,7 @@ int link_drop_foreign_neighbors(Link *link) { } /* Next, unmark requested neighbors. They will be configured later. */ - HASHMAP_FOREACH(neighbor, link->network->neighbors_by_section) { + ORDERED_HASHMAP_FOREACH(neighbor, link->network->neighbors_by_section) { Neighbor *existing; if (neighbor_get(link, neighbor, &existing) >= 0) @@ -436,7 +479,9 @@ void link_foreignize_neighbors(Link *link) { int manager_rtnl_process_neighbor(sd_netlink *rtnl, sd_netlink_message *message, Manager *m) { _cleanup_(neighbor_freep) Neighbor *tmp = NULL; Neighbor *neighbor = NULL; + Request *req = NULL; uint16_t type, state; + bool is_new = false; int ifindex, r; Link *link; @@ -489,6 +534,7 @@ int manager_rtnl_process_neighbor(sd_netlink *rtnl, sd_netlink_message *message, tmp = new0(Neighbor, 1); + /* First, retrieve the fundamental information about the neighbor. */ r = sd_rtnl_message_neigh_get_family(message, &tmp->family); if (r < 0) { log_link_warning(link, "rtnl: received neighbor message without family, ignoring."); @@ -504,49 +550,52 @@ int manager_rtnl_process_neighbor(sd_netlink *rtnl, sd_netlink_message *message, return 0; } - r = netlink_message_read_hw_addr(message, NDA_LLADDR, &tmp->ll_addr); - if (r < 0) { - log_link_warning_errno(link, r, "rtnl: received neighbor message without valid link layer address, ignoring: %m"); - return 0; - } - + /* Then, find the managed Neighbor and Request objects corresponding to the netlink notification. */ (void) neighbor_get(link, tmp, &neighbor); + (void) neighbor_get_request(link, tmp, &req); - switch (type) { - case RTM_NEWNEIGH: - if (neighbor) { - neighbor_enter_configured(neighbor); - log_neighbor_debug(neighbor, "Received remembered", link); - } else { - neighbor_enter_configured(tmp); - log_neighbor_debug(tmp, "Remembering", link); - r = neighbor_add(link, tmp); - if (r < 0) { - log_link_warning_errno(link, r, "Failed to remember foreign neighbor, ignoring: %m"); - return 0; - } - TAKE_PTR(tmp); - } - - break; - - case RTM_DELNEIGH: + if (type == RTM_DELNEIGH) { if (neighbor) { neighbor_enter_removed(neighbor); - if (neighbor->state == 0) { - log_neighbor_debug(neighbor, "Forgetting", link); - neighbor_free(neighbor); - } else - log_neighbor_debug(neighbor, "Removed", link); + log_neighbor_debug(neighbor, "Forgetting removed", link); + neighbor_free(neighbor); } else log_neighbor_debug(tmp, "Kernel removed unknown", link); - break; + if (req) + neighbor_enter_removed(req->userdata); - default: - assert_not_reached(); + return 0; } + /* If we did not know the neighbor, then save it. */ + if (!neighbor) { + r = neighbor_add(link, tmp); + if (r < 0) { + log_link_warning_errno(link, r, "Failed to remember foreign neighbor, ignoring: %m"); + return 0; + } + neighbor = TAKE_PTR(tmp); + is_new = true; + } + + /* Also update information that cannot be obtained through netlink notification. */ + if (req && req->waiting_reply) { + Neighbor *n = ASSERT_PTR(req->userdata); + + neighbor->source = n->source; + } + + /* Then, update miscellaneous info. */ + r = netlink_message_read_hw_addr(message, NDA_LLADDR, &neighbor->ll_addr); + if (r < 0 && r != -ENODATA) + log_link_debug_errno(link, r, "rtnl: received neighbor message without valid link layer address, ignoring: %m"); + + neighbor_enter_configured(neighbor); + if (req) + neighbor_enter_configured(req->userdata); + + log_neighbor_debug(neighbor, is_new ? "Remembering" : "Received remembered", link); return 1; } @@ -576,14 +625,45 @@ static int neighbor_section_verify(Neighbor *neighbor) { return 0; } -void network_drop_invalid_neighbors(Network *network) { +int network_drop_invalid_neighbors(Network *network) { + _cleanup_set_free_ Set *neighbors = NULL; Neighbor *neighbor; + int r; assert(network); - HASHMAP_FOREACH(neighbor, network->neighbors_by_section) - if (neighbor_section_verify(neighbor) < 0) + ORDERED_HASHMAP_FOREACH(neighbor, network->neighbors_by_section) { + Neighbor *dup; + + if (neighbor_section_verify(neighbor) < 0) { + /* Drop invalid [Neighbor] sections. Note that neighbor_free() will drop the + * neighbor from neighbors_by_section. */ neighbor_free(neighbor); + continue; + } + + /* Always use the setting specified later. So, remove the previously assigned setting. */ + dup = set_remove(neighbors, neighbor); + if (dup) { + log_warning("%s: Duplicated neighbor settings for %s is specified at line %u and %u, " + "dropping the address setting specified at line %u.", + dup->section->filename, + IN_ADDR_TO_STRING(neighbor->family, &neighbor->in_addr), + neighbor->section->line, + dup->section->line, dup->section->line); + /* neighbor_free() will drop the address from neighbors_by_section. */ + neighbor_free(dup); + } + + /* Use neighbor_hash_ops, instead of neighbor_hash_ops_free. Otherwise, the Neighbor objects + * will be freed. */ + r = set_ensure_put(&neighbors, &neighbor_hash_ops, neighbor); + if (r < 0) + return log_oom(); + assert(r > 0); + } + + return 0; } diff --git a/src/network/networkd-neighbor.h b/src/network/networkd-neighbor.h index 758475a8ff6..683a310b3f3 100644 --- a/src/network/networkd-neighbor.h +++ b/src/network/networkd-neighbor.h @@ -28,7 +28,7 @@ typedef struct Neighbor { Neighbor *neighbor_free(Neighbor *neighbor); -void network_drop_invalid_neighbors(Network *network); +int network_drop_invalid_neighbors(Network *network); int link_drop_managed_neighbors(Link *link); int link_drop_foreign_neighbors(Link *link); diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index b353ea32feb..dbe8c59a625 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -317,7 +317,9 @@ int network_verify(Network *network) { network_drop_invalid_nexthops(network); network_drop_invalid_bridge_fdb_entries(network); network_drop_invalid_bridge_mdb_entries(network); - network_drop_invalid_neighbors(network); + r = network_drop_invalid_neighbors(network); + if (r < 0) + return r; network_drop_invalid_address_labels(network); network_drop_invalid_prefixes(network); network_drop_invalid_route_prefixes(network); @@ -772,7 +774,7 @@ static Network *network_free(Network *network) { hashmap_free_with_destructor(network->nexthops_by_section, nexthop_free); hashmap_free_with_destructor(network->bridge_fdb_entries_by_section, bridge_fdb_free); hashmap_free_with_destructor(network->bridge_mdb_entries_by_section, bridge_mdb_free); - hashmap_free_with_destructor(network->neighbors_by_section, neighbor_free); + ordered_hashmap_free_with_destructor(network->neighbors_by_section, neighbor_free); hashmap_free_with_destructor(network->address_labels_by_section, address_label_free); hashmap_free_with_destructor(network->prefixes_by_section, prefix_free); hashmap_free_with_destructor(network->route_prefixes_by_section, route_prefix_free); @@ -828,7 +830,7 @@ bool network_has_static_ipv6_configurations(Network *network) { if (mdb->family == AF_INET6) return true; - HASHMAP_FOREACH(neighbor, network->neighbors_by_section) + ORDERED_HASHMAP_FOREACH(neighbor, network->neighbors_by_section) if (neighbor->family == AF_INET6) return true; diff --git a/src/network/networkd-network.h b/src/network/networkd-network.h index 51caf4b9446..031ba398e7e 100644 --- a/src/network/networkd-network.h +++ b/src/network/networkd-network.h @@ -351,7 +351,7 @@ struct Network { Hashmap *nexthops_by_section; Hashmap *bridge_fdb_entries_by_section; Hashmap *bridge_mdb_entries_by_section; - Hashmap *neighbors_by_section; + OrderedHashmap *neighbors_by_section; Hashmap *address_labels_by_section; Hashmap *prefixes_by_section; Hashmap *route_prefixes_by_section; diff --git a/test/test-network/conf/25-neighbor-ip.network b/test/test-network/conf/25-neighbor-ip.network index 8ef6e3f28b5..5469333b92f 100644 --- a/test/test-network/conf/25-neighbor-ip.network +++ b/test/test-network/conf/25-neighbor-ip.network @@ -9,3 +9,8 @@ Address=10.0.0.21 [Neighbor] Address=10.0.0.22 LinkLayerAddress=10.65.223.239 + +[Neighbor] +# unmatching link layer address length +Address=10.0.0.23 +LinkLayerAddress=11:22:33:44:55:66 diff --git a/test/test-network/conf/25-neighbor-ipv6.network b/test/test-network/conf/25-neighbor-ipv6.network index f7480fa6a22..80f989d1cdc 100644 --- a/test/test-network/conf/25-neighbor-ipv6.network +++ b/test/test-network/conf/25-neighbor-ipv6.network @@ -10,3 +10,8 @@ LinkLocalAddressing=no [Neighbor] Address=2001:db8:0:f102::17 LinkLayerAddress=2a00:ffde:4567:edde::4988 + +[Neighbor] +# unmatching link layer address length +Address=2001:db8:0:f102::18 +LinkLayerAddress=11:22:33:44:55:66 diff --git a/test/test-network/conf/25-neighbor-section.network b/test/test-network/conf/25-neighbor-section.network index 38f6b025166..59e21ebf1ba 100644 --- a/test/test-network/conf/25-neighbor-section.network +++ b/test/test-network/conf/25-neighbor-section.network @@ -12,3 +12,16 @@ LinkLayerAddress=00:00:5e:00:02:65 [Neighbor] Address=2004:da8:1:0::1 LinkLayerAddress=00:00:5e:00:02:66 + +[Neighbor] +# unmatching link layer address length +Address=2004:da8:1:0::2 +LinkLayerAddress=192.168.0.1 + +[Neighbor] +# invalid setting (without LinkLayerAddress=) +Address=192.168.10.2 + +[Neighbor] +# invalid setting (without Address=) +LinkLayerAddress=00:00:5e:00:02:67 diff --git a/test/test-network/conf/25-neighbor-section.network.d/override.conf b/test/test-network/conf/25-neighbor-section.network.d/override.conf new file mode 100644 index 00000000000..01027e35c32 --- /dev/null +++ b/test/test-network/conf/25-neighbor-section.network.d/override.conf @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later + +[Neighbor] +Address=192.168.10.1 +LinkLayerAddress=00:00:5e:00:03:65 + +[Neighbor] +Address=2004:da8:1:0::1 +LinkLayerAddress=00:00:5e:00:03:66 diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 8fd9422a88e..c1a017d16b0 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -3125,40 +3125,56 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): self.assertRegex(output, f'2607:5300:203:5215:{i}::1 *proxy') def test_neighbor_section(self): - copy_network_unit('25-neighbor-section.network', '12-dummy.netdev') + copy_network_unit('25-neighbor-section.network', '12-dummy.netdev', copy_dropins=False) start_networkd() - self.wait_online(['dummy98:degraded'], timeout='40s') + self.wait_online(['dummy98:degraded']) print('### ip neigh list dev dummy98') output = check_output('ip neigh list dev dummy98') print(output) - self.assertRegex(output, '192.168.10.1.*00:00:5e:00:02:65.*PERMANENT') - self.assertRegex(output, '2004:da8:1::1.*00:00:5e:00:02:66.*PERMANENT') + self.assertIn('192.168.10.1 lladdr 00:00:5e:00:02:65 PERMANENT', output) + self.assertIn('2004:da8:1::1 lladdr 00:00:5e:00:02:66 PERMANENT', output) + self.assertNotIn('2004:da8:1:0::2', output) + self.assertNotIn('192.168.10.2', output) + self.assertNotIn('00:00:5e:00:02:67', output) output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) check_json(output) + copy_network_unit('25-neighbor-section.network.d/override.conf') + networkctl_reload() + self.wait_online(['dummy98:degraded']) + + print('### ip neigh list dev dummy98 (after reloading)') + output = check_output('ip neigh list dev dummy98') + print(output) + self.assertIn('192.168.10.1 lladdr 00:00:5e:00:03:65 PERMANENT', output) + self.assertIn('2004:da8:1::1 lladdr 00:00:5e:00:03:66 PERMANENT', output) + self.assertNotIn('2004:da8:1:0::2', output) + self.assertNotIn('192.168.10.2', output) + self.assertNotIn('00:00:5e:00:02', output) + def test_neighbor_reconfigure(self): - copy_network_unit('25-neighbor-section.network', '12-dummy.netdev') + copy_network_unit('25-neighbor-section.network', '12-dummy.netdev', copy_dropins=False) start_networkd() - self.wait_online(['dummy98:degraded'], timeout='40s') + self.wait_online(['dummy98:degraded']) print('### ip neigh list dev dummy98') output = check_output('ip neigh list dev dummy98') print(output) - self.assertRegex(output, '192.168.10.1.*00:00:5e:00:02:65.*PERMANENT') - self.assertRegex(output, '2004:da8:1::1.*00:00:5e:00:02:66.*PERMANENT') + self.assertIn('192.168.10.1 lladdr 00:00:5e:00:02:65 PERMANENT', output) + self.assertIn('2004:da8:1::1 lladdr 00:00:5e:00:02:66 PERMANENT', output) remove_network_unit('25-neighbor-section.network') copy_network_unit('25-neighbor-next.network') networkctl_reload() - self.wait_online(['dummy98:degraded'], timeout='40s') + self.wait_online(['dummy98:degraded']) print('### ip neigh list dev dummy98') output = check_output('ip neigh list dev dummy98') print(output) - self.assertNotRegex(output, '192.168.10.1.*00:00:5e:00:02:65.*PERMANENT') - self.assertRegex(output, '192.168.10.1.*00:00:5e:00:02:66.*PERMANENT') - self.assertNotRegex(output, '2004:da8:1::1.*PERMANENT') + self.assertNotIn('00:00:5e:00:02:65', output) + self.assertIn('192.168.10.1 lladdr 00:00:5e:00:02:66 PERMANENT', output) + self.assertNotIn('2004:da8:1::1', output) def test_neighbor_gre(self): copy_network_unit('25-neighbor-ip.network', '25-neighbor-ipv6.network', '25-neighbor-ip-dummy.network', @@ -3168,11 +3184,13 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): output = check_output('ip neigh list dev gretun97') print(output) - self.assertRegex(output, '10.0.0.22 lladdr 10.65.223.239 PERMANENT') + self.assertIn('10.0.0.22 lladdr 10.65.223.239 PERMANENT', output) + self.assertNotIn('10.0.0.23', output) output = check_output('ip neigh list dev ip6gretun97') print(output) self.assertRegex(output, '2001:db8:0:f102::17 lladdr 2a:?00:ff:?de:45:?67:ed:?de:[0:]*:49:?88 PERMANENT') + self.assertNotIn('2001:db8:0:f102::18', output) output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) check_json(output)