Merge pull request #20892 from yuwata/test-network-preferred-lifetime-zero

network: drop and warn duplicated address settings

src/network/networkd-address.c

@@ -314,7 +314,18 @@ 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(
+        address_hash_ops,
+        Address,
+        address_hash_func,
+        address_compare_func);
+
+DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR(
+        address_hash_ops_free,
+        Address,
+        address_hash_func,
+        address_compare_func,
+        address_free);
 
 int address_dup(const Address *src, Address **ret) {
         _cleanup_(address_freep) Address *dest = NULL;
@@ -388,7 +399,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)
@@ -398,7 +409,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 +417,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 +1363,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 +1387,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);
 
@@ -1906,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;
 }

src/network/networkd-address.h

@@ -93,11 +93,10 @@ 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);
-extern const struct hash_ops address_hash_ops;
 
 DEFINE_NETWORK_CONFIG_STATE_FUNCTIONS(Address, address);
 static inline void address_enter_probing(Address *address) {

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,

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;
         }
@@ -121,6 +120,8 @@ static int network_resolve_stacked_netdevs(Network *network) {
 }
 
 int network_verify(Network *network) {
+        int r;
+
         assert(network);
         assert(network->filename);
 
@@ -160,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);
@@ -299,7 +302,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);
@@ -530,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;
 
@@ -565,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;

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,

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,

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

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

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):