From 081466bd57cd441632e244292036f886ceac9112 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Oct 2022 10:44:06 +0900 Subject: [PATCH 1/5] network: drop redundant condition The function `link_reconfigure_impl()` has the same condition at the beginning. --- src/network/networkd-link.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 00e4e451ef..e901c62fbf 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -1537,15 +1537,12 @@ static int link_carrier_gained(Link *link) { force_reconfigure = link->previous_ssid && !streq_ptr(link->previous_ssid, link->ssid); link->previous_ssid = mfree(link->previous_ssid); - if (!IN_SET(link->state, LINK_STATE_PENDING, LINK_STATE_FAILED, LINK_STATE_LINGER)) { - /* At this stage, both wlan and link information should be up-to-date. Hence, - * it is not necessary to call RTM_GETLINK, NL80211_CMD_GET_INTERFACE, or - * NL80211_CMD_GET_STATION commands, and simply call link_reconfigure_impl(). - * Note, link_reconfigure_impl() returns 1 when the link is reconfigured. */ - r = link_reconfigure_impl(link, force_reconfigure); - if (r != 0) - return r; - } + /* At this stage, both wlan and link information should be up-to-date. Hence, it is not necessary to + * call RTM_GETLINK, NL80211_CMD_GET_INTERFACE, or NL80211_CMD_GET_STATION commands, and simply call + * link_reconfigure_impl(). Note, link_reconfigure_impl() returns 1 when the link is reconfigured. */ + r = link_reconfigure_impl(link, force_reconfigure); + if (r != 0) + return r; r = link_handle_bound_by_list(link); if (r < 0) From 4faca0a361011806e546f35e8f49467be3951acb Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Oct 2022 10:46:08 +0900 Subject: [PATCH 2/5] network: allow to (automatically) reconfigure failed interface We have already allowed to reconfigure failed interface manually, but not allowed to do automatically, e.g. on carrier gain. This makes that failed interfaces also reconfigured automatically. Note, the condition is inversed to shorten the condition. --- src/network/networkd-link.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index e901c62fbf..0759fbd9eb 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -1185,7 +1185,7 @@ static int link_reconfigure_impl(Link *link, bool force) { assert(link); - if (!IN_SET(link->state, LINK_STATE_INITIALIZED, LINK_STATE_CONFIGURING, LINK_STATE_CONFIGURED, LINK_STATE_UNMANAGED)) + if (IN_SET(link->state, LINK_STATE_PENDING, LINK_STATE_LINGER)) return 0; r = netdev_get(link->manager, link->ifname, &netdev); From 6625c39e70128d5aa8d3791c8232e33f624cfe49 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Oct 2022 11:43:46 +0900 Subject: [PATCH 3/5] network: simplify the logic of reading driver and permanent HW address No functional changes, just refactoring and preparation for later commits. Note, `link->dev` should always exist when link state is initialized or later. --- src/network/networkd-link.c | 54 +++++++++++++++++++++++++++---------- src/network/networkd-link.h | 4 +++ 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 0759fbd9eb..d3dafb2458 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -2028,15 +2028,17 @@ static int link_update_driver(Link *link, sd_netlink_message *message) { assert(message); /* Driver is already read. Assuming the driver is never changed. */ - if (link->driver) + if (link->ethtool_driver_read) return 0; /* When udevd is running, read the driver after the interface is initialized by udevd. * Otherwise, ethtool may not work correctly. See issue #22538. * When udevd is not running, read the value when the interface is detected. */ - if (link->state != (udev_available() ? LINK_STATE_INITIALIZED : LINK_STATE_PENDING)) + if (udev_available() && !link->dev) return 0; + link->ethtool_driver_read = true; + r = ethtool_get_driver(&link->manager->ethtool_fd, link->ifname, &link->driver); if (r < 0) { log_link_debug_errno(link, r, "Failed to get driver, continuing without: %m"); @@ -2064,6 +2066,38 @@ static int link_update_driver(Link *link, sd_netlink_message *message) { return 0; } +static int link_update_permanent_hardware_address_from_ethtool(Link *link, sd_netlink_message *message) { + int r; + + assert(link); + assert(link->manager); + assert(message); + + if (link->ethtool_permanent_hw_addr_read) + return 0; + + /* When udevd is running, read the permanent hardware address after the interface is + * initialized by udevd. Otherwise, ethtool may not work correctly. See issue #22538. + * When udevd is not running, read the value when the interface is detected. */ + if (udev_available() && !link->dev) + return 0; + + /* If the interface does not have a hardware address, then it will not have a permanent address either. */ + r = netlink_message_read_hw_addr(message, IFLA_ADDRESS, NULL); + if (r == -ENODATA) + return 0; + if (r < 0) + return log_link_debug_errno(link, r, "Failed to read IFLA_ADDRESS attribute: %m"); + + link->ethtool_permanent_hw_addr_read = true; + + r = ethtool_get_permanent_hw_addr(&link->manager->ethtool_fd, link->ifname, &link->permanent_hw_addr); + if (r < 0) + log_link_debug_errno(link, r, "Permanent hardware address not found, continuing without: %m"); + + return 0; +} + static int link_update_permanent_hardware_address(Link *link, sd_netlink_message *message) { int r; @@ -2074,23 +2108,15 @@ static int link_update_permanent_hardware_address(Link *link, sd_netlink_message if (link->permanent_hw_addr.length > 0) return 0; - /* When udevd is running, read the permanent hardware address after the interface is - * initialized by udevd. Otherwise, ethtool may not work correctly. See issue #22538. - * When udevd is not running, read the value when the interface is detected. */ - if (link->state != (udev_available() ? LINK_STATE_INITIALIZED : LINK_STATE_PENDING)) - return 0; - r = netlink_message_read_hw_addr(message, IFLA_PERM_ADDRESS, &link->permanent_hw_addr); if (r < 0) { if (r != -ENODATA) return log_link_debug_errno(link, r, "Failed to read IFLA_PERM_ADDRESS attribute: %m"); - if (netlink_message_read_hw_addr(message, IFLA_ADDRESS, NULL) >= 0) { - /* Fallback to ethtool, if the link has a hardware address. */ - r = ethtool_get_permanent_hw_addr(&link->manager->ethtool_fd, link->ifname, &link->permanent_hw_addr); - if (r < 0) - log_link_debug_errno(link, r, "Permanent hardware address not found, continuing without: %m"); - } + /* Fallback to ethtool for older kernels. */ + r = link_update_permanent_hardware_address_from_ethtool(link, message); + if (r < 0) + return r; } if (link->permanent_hw_addr.length > 0) diff --git a/src/network/networkd-link.h b/src/network/networkd-link.h index 9f1cdca312..4d397da79a 100644 --- a/src/network/networkd-link.h +++ b/src/network/networkd-link.h @@ -72,6 +72,10 @@ typedef struct Link { sd_device *dev; char *driver; + /* to prevent multiple ethtool calls */ + bool ethtool_driver_read; + bool ethtool_permanent_hw_addr_read; + /* link-local addressing */ IPv6LinkLocalAddressGenMode ipv6ll_address_gen_mode; From 40695457271dbc92b8556f017c30170a078fbae0 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Oct 2022 11:08:17 +0900 Subject: [PATCH 4/5] network: try to reconfigure when some information is updated When at least one of the name, MAC address, udev properties, and so on for an interface is updated, try to find a matching .network file, and reconfigure if a new .network file is assigned. Fixes #24975. --- src/network/networkd-link.c | 61 ++++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 25 deletions(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index d3dafb2458..f4c5b2def6 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -1367,21 +1367,18 @@ static int link_initialized_and_synced(Link *link) { return 0; } - /* This may get called either from the asynchronous netlink callback, - * or directly from link_check_initialized() if running in a container. */ - if (!IN_SET(link->state, LINK_STATE_PENDING, LINK_STATE_INITIALIZED)) - return 0; + if (link->state == LINK_STATE_PENDING) { + log_link_debug(link, "Link state is up-to-date"); + link_set_state(link, LINK_STATE_INITIALIZED); - log_link_debug(link, "Link state is up-to-date"); - link_set_state(link, LINK_STATE_INITIALIZED); + r = link_new_bound_by_list(link); + if (r < 0) + return r; - r = link_new_bound_by_list(link); - if (r < 0) - return r; - - r = link_handle_bound_by_list(link); - if (r < 0) - return r; + r = link_handle_bound_by_list(link); + if (r < 0) + return r; + } return link_reconfigure_impl(link, /* force = */ false); } @@ -1414,14 +1411,10 @@ static int link_initialized(Link *link, sd_device *device) { if (r < 0) log_link_warning_errno(link, r, "Failed to manage SR-IOV PF and VF ports, ignoring: %m"); - /* Do not ignore unamanaged state case here. If an interface is renamed after being once - * configured, and the corresponding .network file has Name= in [Match] section, then the - * interface may be already in unmanaged state. See #20657. */ - if (!IN_SET(link->state, LINK_STATE_PENDING, LINK_STATE_UNMANAGED)) - return 0; + if (link->state != LINK_STATE_PENDING) + return link_reconfigure(link, /* force = */ false); log_link_debug(link, "udev initialized link"); - link_set_state(link, LINK_STATE_INITIALIZED); /* udev has initialized the link, but we don't know if we have yet * processed the NEWLINK messages with the latest state. Do a GETLINK, @@ -2063,7 +2056,7 @@ static int link_update_driver(Link *link, sd_netlink_message *message) { link->dsa_master_ifindex = (int) dsa_master_ifindex; } - return 0; + return 1; /* needs reconfigure */ } static int link_update_permanent_hardware_address_from_ethtool(Link *link, sd_netlink_message *message) { @@ -2122,7 +2115,7 @@ static int link_update_permanent_hardware_address(Link *link, sd_netlink_message if (link->permanent_hw_addr.length > 0) log_link_debug(link, "Saved permanent hardware address: %s", HW_ADDR_TO_STR(&link->permanent_hw_addr)); - return 0; + return 1; /* needs reconfigure */ } static int link_update_hardware_address(Link *link, sd_netlink_message *message) { @@ -2205,7 +2198,7 @@ static int link_update_hardware_address(Link *link, sd_netlink_message *message) return log_link_debug_errno(link, r, "Could not update MAC address for LLDP Tx: %m"); } - return 0; + return 1; /* needs reconfigure */ } static int link_update_mtu(Link *link, sd_netlink_message *message) { @@ -2297,7 +2290,7 @@ static int link_update_alternative_names(Link *link, sd_netlink_message *message return log_link_debug_errno(link, r, "Failed to manage link by its new alternative names: %m"); } - return 0; + return 1; /* needs reconfigure */ } static int link_update_name(Link *link, sd_netlink_message *message) { @@ -2393,10 +2386,11 @@ static int link_update_name(Link *link, sd_netlink_message *message) { if (r < 0) return log_link_debug_errno(link, r, "Failed to update interface name in IPv4ACD client: %m"); - return 0; + return 1; /* needs reconfigure */ } static int link_update(Link *link, sd_netlink_message *message) { + bool needs_reconfigure = false; int r; assert(link); @@ -2405,10 +2399,12 @@ static int link_update(Link *link, sd_netlink_message *message) { r = link_update_name(link, message); if (r < 0) return r; + needs_reconfigure = needs_reconfigure || r > 0; r = link_update_alternative_names(link, message); if (r < 0) return r; + needs_reconfigure = needs_reconfigure || r > 0; r = link_update_mtu(link, message); if (r < 0) @@ -2417,14 +2413,17 @@ static int link_update(Link *link, sd_netlink_message *message) { r = link_update_driver(link, message); if (r < 0) return r; + needs_reconfigure = needs_reconfigure || r > 0; r = link_update_permanent_hardware_address(link, message); if (r < 0) return r; + needs_reconfigure = needs_reconfigure || r > 0; r = link_update_hardware_address(link, message); if (r < 0) return r; + needs_reconfigure = needs_reconfigure || r > 0; r = link_update_master(link, message); if (r < 0) @@ -2434,7 +2433,11 @@ static int link_update(Link *link, sd_netlink_message *message) { if (r < 0) return r; - return link_update_flags(link, message); + r = link_update_flags(link, message); + if (r < 0) + return r; + + return needs_reconfigure; } static Link *link_drop_or_unref(Link *link) { @@ -2623,6 +2626,14 @@ int manager_rtnl_process_link(sd_netlink *rtnl, sd_netlink_message *message, Man link_enter_failed(link); return 0; } + if (r > 0) { + r = link_reconfigure_impl(link, /* force = */ false); + if (r < 0) { + log_link_warning_errno(link, r, "Failed to reconfigure interface: %m"); + link_enter_failed(link); + return 0; + } + } } break; From fa4d3fed46a97ee45a4aeba4d9540015bf5cab7b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 28 Oct 2022 06:25:56 +0900 Subject: [PATCH 5/5] test-network: add testcase for reconfiguring interface --- test/test-network/conf/12-dummy-altname.link | 7 +++ test/test-network/conf/12-dummy-mac.netdev | 5 ++ .../conf/12-dummy-match-altname.network | 9 ++++ .../conf/12-dummy-match-mac-01.network | 9 ++++ .../conf/12-dummy-match-mac-02.network | 9 ++++ .../conf/12-dummy-match-renamed.network | 9 ++++ test/test-network/systemd-networkd-tests.py | 48 +++++++++++++++++++ 7 files changed, 96 insertions(+) create mode 100644 test/test-network/conf/12-dummy-altname.link create mode 100644 test/test-network/conf/12-dummy-mac.netdev create mode 100644 test/test-network/conf/12-dummy-match-altname.network create mode 100644 test/test-network/conf/12-dummy-match-mac-01.network create mode 100644 test/test-network/conf/12-dummy-match-mac-02.network create mode 100644 test/test-network/conf/12-dummy-match-renamed.network diff --git a/test/test-network/conf/12-dummy-altname.link b/test/test-network/conf/12-dummy-altname.link new file mode 100644 index 0000000000..ff47fec7d4 --- /dev/null +++ b/test/test-network/conf/12-dummy-altname.link @@ -0,0 +1,7 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +[Match] +OriginalName=dummy98-2 +Driver=dummy + +[Link] +AlternativeName=dummy98-2-altname diff --git a/test/test-network/conf/12-dummy-mac.netdev b/test/test-network/conf/12-dummy-mac.netdev new file mode 100644 index 0000000000..254ec94293 --- /dev/null +++ b/test/test-network/conf/12-dummy-mac.netdev @@ -0,0 +1,5 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +[NetDev] +Name=dummy98 +Kind=dummy +MACAddress=12:34:56:78:9a:01 diff --git a/test/test-network/conf/12-dummy-match-altname.network b/test/test-network/conf/12-dummy-match-altname.network new file mode 100644 index 0000000000..28d98c8edb --- /dev/null +++ b/test/test-network/conf/12-dummy-match-altname.network @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +[Match] +Name=dummy98-2-altname +Driver=dummy +MACAddress=12:34:56:78:9a:02 + +[Network] +IPv6AcceptRA=no +Address=10.0.2.2/16 diff --git a/test/test-network/conf/12-dummy-match-mac-01.network b/test/test-network/conf/12-dummy-match-mac-01.network new file mode 100644 index 0000000000..300bad6f9d --- /dev/null +++ b/test/test-network/conf/12-dummy-match-mac-01.network @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +[Match] +Name=dummy98 +Driver=dummy +MACAddress=12:34:56:78:9a:01 + +[Network] +IPv6AcceptRA=no +Address=10.0.0.1/16 diff --git a/test/test-network/conf/12-dummy-match-mac-02.network b/test/test-network/conf/12-dummy-match-mac-02.network new file mode 100644 index 0000000000..328469193a --- /dev/null +++ b/test/test-network/conf/12-dummy-match-mac-02.network @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +[Match] +Name=dummy98 +Driver=dummy +MACAddress=12:34:56:78:9a:02 + +[Network] +IPv6AcceptRA=no +Address=10.0.0.2/16 diff --git a/test/test-network/conf/12-dummy-match-renamed.network b/test/test-network/conf/12-dummy-match-renamed.network new file mode 100644 index 0000000000..bdac469bc3 --- /dev/null +++ b/test/test-network/conf/12-dummy-match-renamed.network @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +[Match] +Name=dummy98-1 +Driver=dummy +MACAddress=12:34:56:78:9a:02 + +[Network] +IPv6AcceptRA=no +Address=10.0.1.2/16 diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 2f7614428c..7059711f1d 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -1062,6 +1062,54 @@ class NetworkctlTests(unittest.TestCase, Utilities): self.check_link_exists('veth99', expected=False) self.check_link_exists('veth-peer', expected=False) +class NetworkdMatchTests(unittest.TestCase, Utilities): + + def setUp(self): + setup_common() + + def tearDown(self): + tear_down_common() + + def test_match(self): + copy_network_unit('12-dummy-mac.netdev', + '12-dummy-match-mac-01.network', + '12-dummy-match-mac-02.network', + '12-dummy-match-renamed.network', + '12-dummy-match-altname.network', + '12-dummy-altname.link') + start_networkd() + + self.wait_online(['dummy98:routable']) + output = check_output(*networkctl_cmd, '-n', '0', 'status', 'dummy98', env=env) + self.assertIn('Network File: /run/systemd/network/12-dummy-match-mac-01.network', output) + output = check_output('ip -4 address show dev dummy98') + self.assertIn('10.0.0.1/16', output) + + check_output('ip link set dev dummy98 down') + check_output('ip link set dev dummy98 address 12:34:56:78:9a:02') + + self.wait_address('dummy98', '10.0.0.2/16', ipv='-4', timeout_sec=10) + self.wait_online(['dummy98:routable']) + output = check_output(*networkctl_cmd, '-n', '0', 'status', 'dummy98', env=env) + self.assertIn('Network File: /run/systemd/network/12-dummy-match-mac-02.network', output) + + check_output('ip link set dev dummy98 down') + check_output('ip link set dev dummy98 name dummy98-1') + + self.wait_address('dummy98-1', '10.0.1.2/16', ipv='-4', timeout_sec=10) + self.wait_online(['dummy98-1:routable']) + output = check_output(*networkctl_cmd, '-n', '0', 'status', 'dummy98-1', env=env) + self.assertIn('Network File: /run/systemd/network/12-dummy-match-renamed.network', output) + + check_output('ip link set dev dummy98-1 down') + check_output('ip link set dev dummy98-1 name dummy98-2') + check_output(*udevadm_cmd, 'trigger', '--action=add', '/sys/class/net/dummy98-2') + + self.wait_address('dummy98-2', '10.0.2.2/16', ipv='-4', timeout_sec=10) + self.wait_online(['dummy98-2:routable']) + output = check_output(*networkctl_cmd, '-n', '0', 'status', 'dummy98-2', env=env) + self.assertIn('Network File: /run/systemd/network/12-dummy-match-altname.network', output) + class NetworkdNetDevTests(unittest.TestCase, Utilities): def setUp(self):