From f983155736ae702ea0bc08f88ddf374babd33756 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 1 Oct 2021 09:41:02 +0900 Subject: [PATCH 1/7] network: address: fix flags and lifetime in debugging logs Prompted by #20891. --- src/network/networkd-address.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index 37c5c3cc6c..b308e9ee62 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -398,7 +398,7 @@ static int address_add(Link *link, Address *address) { return 0; } -static int address_update(Address *address, const Address *src) { +static int address_update(Address *address) { Link *link; int r; @@ -406,11 +406,6 @@ static int address_update(Address *address, const Address *src) { assert(address->link); link = address->link; - if (src) { - address->flags = src->flags; - address->scope = src->scope; - address->cinfo = src->cinfo; - } if (address_is_ready(address) && address->family == AF_INET6 && @@ -1357,8 +1352,12 @@ int manager_rtnl_process_address(sd_netlink *rtnl, sd_netlink_message *message, switch (type) { case RTM_NEWADDR: if (address) { + /* update flags and etc. */ + address->flags = tmp->flags; + address->scope = tmp->scope; + address->cinfo = tmp->cinfo; address_enter_configured(address); - log_address_debug(address, "Remembering updated", link); + log_address_debug(address, "Received updated", link); } else { address_enter_configured(tmp); log_address_debug(tmp, "Received new", link); @@ -1377,7 +1376,7 @@ int manager_rtnl_process_address(sd_netlink *rtnl, sd_netlink_message *message, } /* address_update() logs internally, so we don't need to here. */ - r = address_update(address, tmp); + r = address_update(address); if (r < 0) link_enter_failed(link); From 6bf03f6f4a1d9b520454801c81576c1263caec4d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 1 Oct 2021 09:03:18 +0900 Subject: [PATCH 2/7] network: make several hash_ops static --- src/network/networkd-address.c | 7 ++++++- src/network/networkd-address.h | 1 - src/network/networkd-ndisc.c | 2 +- src/network/networkd-nexthop.c | 2 +- src/network/networkd-route.c | 2 +- src/network/networkd-route.h | 1 - 6 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index b308e9ee62..aaacab9a94 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -314,7 +314,12 @@ int address_compare_func(const Address *a1, const Address *a2) { } } -DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR(address_hash_ops, Address, address_hash_func, address_compare_func, address_free); +DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR( + address_hash_ops, + Address, + address_hash_func, + address_compare_func, + address_free); int address_dup(const Address *src, Address **ret) { _cleanup_(address_freep) Address *dest = NULL; diff --git a/src/network/networkd-address.h b/src/network/networkd-address.h index 67b7580be6..80ceda9427 100644 --- a/src/network/networkd-address.h +++ b/src/network/networkd-address.h @@ -97,7 +97,6 @@ void network_drop_invalid_addresses(Network *network); void address_hash_func(const Address *a, struct siphash *state); int address_compare_func(const Address *a1, const Address *a2); -extern const struct hash_ops address_hash_ops; DEFINE_NETWORK_CONFIG_STATE_FUNCTIONS(Address, address); static inline void address_enter_probing(Address *address) { diff --git a/src/network/networkd-ndisc.c b/src/network/networkd-ndisc.c index 4979d83f6c..6e6d656033 100644 --- a/src/network/networkd-ndisc.c +++ b/src/network/networkd-ndisc.c @@ -1245,7 +1245,7 @@ static int ipv6_token_compare_func(const IPv6Token *a, const IPv6Token *b) { return memcmp(&a->prefix, &b->prefix, sizeof(struct in6_addr)); } -DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR( +DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR( ipv6_token_hash_ops, IPv6Token, ipv6_token_hash_func, diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index d1ac730e15..267efb5620 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -149,7 +149,7 @@ int nexthop_compare_func(const NextHop *a, const NextHop *b) { return 0; } -DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR( +DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR( nexthop_hash_ops, NextHop, nexthop_hash_func, diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 009afa1b29..f7ef657bca 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -394,7 +394,7 @@ int route_compare_func(const Route *a, const Route *b) { } } -DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR( +DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR( route_hash_ops, Route, route_hash_func, diff --git a/src/network/networkd-route.h b/src/network/networkd-route.h index d3acdfa2e7..3e817de3ac 100644 --- a/src/network/networkd-route.h +++ b/src/network/networkd-route.h @@ -67,7 +67,6 @@ typedef struct Route { void route_hash_func(const Route *route, struct siphash *state); int route_compare_func(const Route *a, const Route *b); -extern const struct hash_ops route_hash_ops; int route_new(Route **ret); Route *route_free(Route *route); From 5b43c2c84312d399784b61fd44a00dc8d987f248 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 1 Oct 2021 09:09:48 +0900 Subject: [PATCH 3/7] network: rename address_hash_ops -> address_hash_ops_free Preparation for later commits. --- src/network/networkd-address.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index aaacab9a94..f162fd5334 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -315,7 +315,7 @@ int address_compare_func(const Address *a1, const Address *a2) { } DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR( - address_hash_ops, + address_hash_ops_free, Address, address_hash_func, address_compare_func, @@ -393,7 +393,7 @@ static int address_add(Link *link, Address *address) { assert(link); assert(address); - r = set_ensure_put(&link->addresses, &address_hash_ops, address); + r = set_ensure_put(&link->addresses, &address_hash_ops_free, address); if (r < 0) return r; if (r == 0) From 50783f91d44b1978c0e4ba62283131fac75d3745 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 1 Oct 2021 09:22:18 +0900 Subject: [PATCH 4/7] network: drop and warn duplicated Address= settings Fixes #20891. --- src/network/networkd-address.c | 43 +++++++++++++++++++++++++++++++--- src/network/networkd-address.h | 2 +- src/network/networkd-network.c | 6 ++++- 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index f162fd5334..002827eb78 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -314,6 +314,12 @@ int address_compare_func(const Address *a1, const Address *a2) { } } +DEFINE_PRIVATE_HASH_OPS( + address_hash_ops, + Address, + address_hash_func, + address_compare_func); + DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR( address_hash_ops_free, Address, @@ -1910,12 +1916,43 @@ static int address_section_verify(Address *address) { return 0; } -void network_drop_invalid_addresses(Network *network) { +int network_drop_invalid_addresses(Network *network) { + _cleanup_set_free_ Set *addresses = NULL; Address *address; + int r; assert(network); - ORDERED_HASHMAP_FOREACH(address, network->addresses_by_section) - if (address_section_verify(address) < 0) + ORDERED_HASHMAP_FOREACH(address, network->addresses_by_section) { + Address *dup; + + if (address_section_verify(address) < 0) { + /* Drop invalid [Address] sections or Address= settings in [Network]. + * Note that address_free() will drop the address from addresses_by_section. */ address_free(address); + continue; + } + + /* Always use the setting specified later. So, remove the previously assigned setting. */ + dup = set_remove(addresses, address); + if (dup) { + _cleanup_free_ char *buf = NULL; + + (void) in_addr_prefix_to_string(address->family, &address->in_addr, address->prefixlen, &buf); + log_warning("%s: Duplicated address %s is specified at line %u and %u, " + "dropping the address setting specified at line %u.", + dup->section->filename, strna(buf), address->section->line, + dup->section->line, dup->section->line); + /* address_free() will drop the address from addresses_by_section. */ + address_free(dup); + } + + /* Do not use address_hash_ops_free here. Otherwise, all address settings will be freed. */ + r = set_ensure_put(&addresses, &address_hash_ops, address); + if (r < 0) + return log_oom(); + assert(r > 0); + } + + return 0; } diff --git a/src/network/networkd-address.h b/src/network/networkd-address.h index 80ceda9427..0fd3163fc4 100644 --- a/src/network/networkd-address.h +++ b/src/network/networkd-address.h @@ -93,7 +93,7 @@ int request_process_address(Request *req); int manager_rtnl_process_address(sd_netlink *nl, sd_netlink_message *message, Manager *m); -void network_drop_invalid_addresses(Network *network); +int network_drop_invalid_addresses(Network *network); void address_hash_func(const Address *a, struct siphash *state); int address_compare_func(const Address *a1, const Address *a2); diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index 9c01d1cb89..bb7c1defe7 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -121,6 +121,8 @@ static int network_resolve_stacked_netdevs(Network *network) { } int network_verify(Network *network) { + int r; + assert(network); assert(network->filename); @@ -299,7 +301,9 @@ int network_verify(Network *network) { network->ipv6_proxy_ndp_addresses = set_free_free(network->ipv6_proxy_ndp_addresses); } - network_drop_invalid_addresses(network); + r = network_drop_invalid_addresses(network); + if (r < 0) + return r; network_drop_invalid_routes(network); network_drop_invalid_nexthops(network); network_drop_invalid_bridge_fdb_entries(network); From 8f9bdeabe3433b09bede744da3b213760a891f04 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 4 Oct 2021 18:22:49 +0900 Subject: [PATCH 5/7] network: downgrade log level for non-critical errors --- src/network/networkd-network.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index bb7c1defe7..358bbf5b5e 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -45,7 +45,7 @@ /* Let's assume that anything above this number is a user misconfiguration. */ #define MAX_NTP_SERVERS 128 -static int network_resolve_netdev_one(Network *network, const char *name, NetDevKind kind, NetDev **ret_netdev) { +static int network_resolve_netdev_one(Network *network, const char *name, NetDevKind kind, NetDev **ret) { const char *kind_string; NetDev *netdev; int r; @@ -57,22 +57,22 @@ static int network_resolve_netdev_one(Network *network, const char *name, NetDev assert(network); assert(network->manager); assert(network->filename); - assert(ret_netdev); + assert(ret); if (kind == _NETDEV_KIND_TUNNEL) kind_string = "tunnel"; else { kind_string = netdev_kind_to_string(kind); if (!kind_string) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "%s: Invalid NetDev kind of %s, ignoring assignment.", - network->filename, name); + return log_warning_errno(SYNTHETIC_ERRNO(EINVAL), + "%s: Invalid NetDev kind of %s, ignoring assignment.", + network->filename, name); } r = netdev_get(network->manager, name, &netdev); if (r < 0) - return log_error_errno(r, "%s: %s NetDev could not be found, ignoring assignment.", - network->filename, name); + return log_warning_errno(r, "%s: %s NetDev could not be found, ignoring assignment.", + network->filename, name); if (netdev->kind != kind && !(kind == _NETDEV_KIND_TUNNEL && IN_SET(netdev->kind, @@ -86,11 +86,11 @@ static int network_resolve_netdev_one(Network *network, const char *name, NetDev NETDEV_KIND_VTI6, NETDEV_KIND_IP6TNL, NETDEV_KIND_ERSPAN))) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "%s: NetDev %s is not a %s, ignoring assignment", - network->filename, name, kind_string); + return log_warning_errno(SYNTHETIC_ERRNO(EINVAL), + "%s: NetDev %s is not a %s, ignoring assignment", + network->filename, name, kind_string); - *ret_netdev = netdev_ref(netdev); + *ret = netdev_ref(netdev); return 1; } @@ -103,16 +103,15 @@ static int network_resolve_stacked_netdevs(Network *network) { HASHMAP_FOREACH_KEY(kind, name, network->stacked_netdev_names) { _cleanup_(netdev_unrefp) NetDev *netdev = NULL; - r = network_resolve_netdev_one(network, name, PTR_TO_INT(kind), &netdev); - if (r <= 0) + if (network_resolve_netdev_one(network, name, PTR_TO_INT(kind), &netdev) <= 0) continue; r = hashmap_ensure_put(&network->stacked_netdevs, &string_hash_ops, netdev->ifname, netdev); if (r == -ENOMEM) return log_oom(); if (r < 0) - return log_error_errno(r, "%s: Failed to add NetDev '%s' to network: %m", - network->filename, (const char *) name); + log_warning_errno(r, "%s: Failed to add NetDev '%s' to network, ignoring: %m", + network->filename, (const char *) name); netdev = NULL; } @@ -162,7 +161,9 @@ int network_verify(Network *network) { (void) network_resolve_netdev_one(network, network->bond_name, NETDEV_KIND_BOND, &network->bond); (void) network_resolve_netdev_one(network, network->bridge_name, NETDEV_KIND_BRIDGE, &network->bridge); (void) network_resolve_netdev_one(network, network->vrf_name, NETDEV_KIND_VRF, &network->vrf); - (void) network_resolve_stacked_netdevs(network); + r = network_resolve_stacked_netdevs(network); + if (r < 0) + return r; /* Free unnecessary entries. */ network->batadv_name = mfree(network->batadv_name); From 9202b567bcdd0c1f6a1fc2a5f36602e619960813 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 4 Oct 2021 18:26:24 +0900 Subject: [PATCH 6/7] network: do not ignore critical errors like OOM --- src/network/networkd-network.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index 358bbf5b5e..7611476ad6 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -535,14 +535,17 @@ int network_load_one(Manager *manager, OrderedHashmap **networks, const char *fi r = network_add_ipv4ll_route(network); if (r < 0) - log_warning_errno(r, "%s: Failed to add IPv4LL route, ignoring: %m", network->filename); + return log_warning_errno(r, "%s: Failed to add IPv4LL route: %m", network->filename); r = network_add_default_route_on_device(network); if (r < 0) - log_warning_errno(r, "%s: Failed to add default route on device, ignoring: %m", - network->filename); + return log_warning_errno(r, "%s: Failed to add default route on device: %m", + network->filename); - if (network_verify(network) < 0) + r = network_verify(network); + if (r == -ENOMEM) + return r; + if (r < 0) /* Ignore .network files that do not match the conditions. */ return 0; @@ -570,7 +573,7 @@ int network_load(Manager *manager, OrderedHashmap **networks) { STRV_FOREACH(f, files) { r = network_load_one(manager, networks, *f); if (r < 0) - log_error_errno(r, "Failed to load %s, ignoring: %m", *f); + return log_error_errno(r, "Failed to load %s: %m", *f); } return 0; From 40971657ceb9cedce87f3dab3c42e877a3acbc64 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 30 Sep 2021 19:22:22 +0900 Subject: [PATCH 7/7] test-network: add tests for duplicated address setting Also, add more tests for PreferredLifetime=0 C.f. #20891. --- .../conf/25-address-static.network | 8 ++++++++ test/test-network/systemd-networkd-tests.py | 20 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/test/test-network/conf/25-address-static.network b/test/test-network/conf/25-address-static.network index 3227fe8dfd..6cde669dc5 100644 --- a/test/test-network/conf/25-address-static.network +++ b/test/test-network/conf/25-address-static.network @@ -52,11 +52,19 @@ Peer=2001:db8:0:f103::10/128 [Address] Address=::/64 +[Network] +# this will later deduped by the following section +Address=10.7.8.9/16 + [Address] Address=10.7.8.9/16 PreferredLifetime=0 Scope=link +[Address] +# this will also deduped +Address=2001:0db8:1:f101::1/64 + [Address] Address=2001:0db8:1:f101::1/64 PreferredLifetime=0 diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 354a6255f2..5de2d3ae03 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -1993,9 +1993,29 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): self.assertIn('inet6 2001:db8:1:f101::1/64 scope global deprecated', output) self.assertRegex(output, r'inet6 fd[0-9a-f:]*1/64 scope global') + # Tests for #20891. + # 1. set preferred lifetime forever to drop the deprecated flag for testing #20891. + self.assertEqual(call('ip address change 10.7.8.9/16 dev dummy98 preferred_lft forever'), 0) + self.assertEqual(call('ip address change 2001:db8:1:f101::1/64 dev dummy98 preferred_lft forever'), 0) + output = check_output('ip -4 address show dev dummy98') + print(output) + self.assertNotIn('deprecated', output) + output = check_output('ip -6 address show dev dummy98') + print(output) + self.assertNotIn('deprecated', output) + + # 2. restart networkd to reconfigure the interface. restart_networkd() self.wait_online(['dummy98:routable']) + # 3. check the deprecated flag is set for the address configured with PreferredLifetime=0 + output = check_output('ip -4 address show dev dummy98') + print(output) + self.assertIn('inet 10.7.8.9/16 brd 10.7.255.255 scope link deprecated dummy98', output) + output = check_output('ip -6 address show dev dummy98') + print(output) + self.assertIn('inet6 2001:db8:1:f101::1/64 scope global deprecated', output) + # test for ENOBUFS issue #17012 output = check_output('ip -4 address show dev dummy98') for i in range(1,254):