From ebf66d0c05cf8eb7b58036e38b8be8bbf7c4dc34 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 21 Aug 2024 07:17:19 +0900 Subject: [PATCH 1/8] network: introduce reconfigure_data_free() and _freep() No functional change, just refactoring. --- src/network/networkd-link.c | 42 ++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 7dcc641d7e..3940aa79b3 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -1463,17 +1463,26 @@ typedef struct ReconfigureData { sd_bus_message *message; } ReconfigureData; +static ReconfigureData* reconfigure_data_free(ReconfigureData *data) { + if (!data) + return NULL; + + link_unref(data->link); + sd_bus_message_unref(data->message); + + return mfree(data); +} + +DEFINE_TRIVIAL_CLEANUP_FUNC(ReconfigureData*, reconfigure_data_free); + static void reconfigure_data_destroy_callback(ReconfigureData *data) { int r; assert(data); - assert(data->link); assert(data->manager); assert(data->manager->reloading > 0); assert(data->message); - link_unref(data->link); - data->manager->reloading--; if (data->manager->reloading <= 0) { r = sd_bus_reply_method_return(data->message, NULL); @@ -1481,8 +1490,7 @@ static void reconfigure_data_destroy_callback(ReconfigureData *data) { log_warning_errno(r, "Failed to send reply for 'Reload' DBus method, ignoring: %m"); } - sd_bus_message_unref(data->message); - free(data); + reconfigure_data_free(data); } static int reconfigure_handler_on_bus_method_reload(sd_netlink *rtnl, sd_netlink_message *m, ReconfigureData *data) { @@ -1493,7 +1501,7 @@ static int reconfigure_handler_on_bus_method_reload(sd_netlink *rtnl, sd_netlink int link_reconfigure_on_bus_method_reload(Link *link, sd_bus_message *message) { _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *req = NULL; - _cleanup_free_ ReconfigureData *data = NULL; + _cleanup_(reconfigure_data_freep) ReconfigureData *data = NULL; int r; assert(link); @@ -1505,29 +1513,29 @@ int link_reconfigure_on_bus_method_reload(Link *link, sd_bus_message *message) { if (IN_SET(link->state, LINK_STATE_PENDING, LINK_STATE_INITIALIZED, LINK_STATE_LINGER)) return 0; - r = sd_rtnl_message_new_link(link->manager->rtnl, &req, RTM_GETLINK, link->ifindex); - if (r < 0) - return r; - data = new(ReconfigureData, 1); if (!data) return -ENOMEM; - r = netlink_call_async(link->manager->rtnl, NULL, req, - reconfigure_handler_on_bus_method_reload, - reconfigure_data_destroy_callback, data); - if (r < 0) - return r; - *data = (ReconfigureData) { .link = link_ref(link), .manager = link->manager, .message = sd_bus_message_ref(message), }; - link->manager->reloading++; + r = sd_rtnl_message_new_link(link->manager->rtnl, &req, RTM_GETLINK, link->ifindex); + if (r < 0) + return r; + + r = netlink_call_async(link->manager->rtnl, NULL, req, + reconfigure_handler_on_bus_method_reload, + reconfigure_data_destroy_callback, data); + if (r < 0) + return r; TAKE_PTR(data); + link->manager->reloading++; + return 0; } From 685fd0d729718aab45a19567a889c8baa2e6cc65 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 21 Aug 2024 07:01:47 +0900 Subject: [PATCH 2/8] network: enter initialized state when the interface will be reconfigured When the interface is in the failed state, link_getlink_handler_internal() will do nothing and return zero, thus the interface will not be reconfigured, especially when the reconfiguration is triggered in link_enter_failed(). Follow-up for c2eb7753dd47ec04ae0d66400e70bc87fbf1adcc. --- src/network/networkd-link-bus.c | 1 - src/network/networkd-link.c | 6 ++++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/network/networkd-link-bus.c b/src/network/networkd-link-bus.c index 84ed42987c..ef2e811255 100644 --- a/src/network/networkd-link-bus.c +++ b/src/network/networkd-link-bus.c @@ -669,7 +669,6 @@ int bus_link_method_reconfigure(sd_bus_message *message, void *userdata, sd_bus_ if (r < 0) return r; if (r > 0) { - link_set_state(l, LINK_STATE_INITIALIZED); r = link_save_and_clean_full(l, /* also_save_manager = */ true); if (r < 0) return r; diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 3940aa79b3..2f7aaec2e5 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -1454,6 +1454,9 @@ int link_reconfigure(Link *link, bool force) { if (r < 0) return r; + if (force || link->state == LINK_STATE_FAILED) + link_set_state(link, LINK_STATE_INITIALIZED); + return 1; /* 1 means the interface will be reconfigured. */ } @@ -1536,6 +1539,9 @@ int link_reconfigure_on_bus_method_reload(Link *link, sd_bus_message *message) { TAKE_PTR(data); link->manager->reloading++; + if (link->state == LINK_STATE_FAILED) + link_set_state(link, LINK_STATE_INITIALIZED); + return 0; } From 021d39d3d19a910e195f8e68e7c86bb6ca1a5554 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 21 Aug 2024 07:07:05 +0900 Subject: [PATCH 3/8] network: log and enter failed state in link_reconfigure() No functional change, just refactoring. --- src/network/networkd-link-bus.c | 7 ++----- src/network/networkd-link.c | 31 +++++++++++++++++-------------- src/network/networkd-manager.c | 17 ++++------------- 3 files changed, 23 insertions(+), 32 deletions(-) diff --git a/src/network/networkd-link-bus.c b/src/network/networkd-link-bus.c index ef2e811255..cb114929fa 100644 --- a/src/network/networkd-link-bus.c +++ b/src/network/networkd-link-bus.c @@ -666,13 +666,10 @@ int bus_link_method_reconfigure(sd_bus_message *message, void *userdata, sd_bus_ return 1; /* Polkit will call us back */ r = link_reconfigure(l, /* force = */ true); + if (r > 0) + r = link_save_and_clean_full(l, /* also_save_manager = */ true); if (r < 0) return r; - if (r > 0) { - r = link_save_and_clean_full(l, /* also_save_manager = */ true); - if (r < 0) - return r; - } return sd_bus_reply_method_return(message, NULL); } diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 2f7aaec2e5..1c6f20695f 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -427,8 +427,6 @@ int link_stop_engines(Link *link, bool may_keep_dhcp) { } void link_enter_failed(Link *link) { - int r; - assert(link); if (IN_SET(link->state, LINK_STATE_FAILED, LINK_STATE_LINGER)) @@ -444,13 +442,8 @@ void link_enter_failed(Link *link) { } log_link_info(link, "Trying to reconfigure the interface."); - r = link_reconfigure(link, /* force = */ true); - if (r < 0) { - log_link_warning_errno(link, r, "Failed to reconfigure interface: %m"); - goto stop; - } - - return; + if (link_reconfigure(link, /* force = */ true) > 0) + return; stop: (void) link_stop_engines(link, /* may_keep_dhcp = */ false); @@ -1451,8 +1444,11 @@ int link_reconfigure(Link *link, bool force) { return 0; /* 0 means no-op. */ r = link_call_getlink(link, force ? link_force_reconfigure_handler : link_reconfigure_handler); - if (r < 0) + if (r < 0) { + log_link_warning_errno(link, r, "Failed to reconfigure interface: %m"); + link_enter_failed(link); return r; + } if (force || link->state == LINK_STATE_FAILED) link_set_state(link, LINK_STATE_INITIALIZED); @@ -1517,8 +1513,10 @@ int link_reconfigure_on_bus_method_reload(Link *link, sd_bus_message *message) { return 0; data = new(ReconfigureData, 1); - if (!data) - return -ENOMEM; + if (!data) { + r = -ENOMEM; + goto failed; + } *data = (ReconfigureData) { .link = link_ref(link), @@ -1528,13 +1526,13 @@ int link_reconfigure_on_bus_method_reload(Link *link, sd_bus_message *message) { r = sd_rtnl_message_new_link(link->manager->rtnl, &req, RTM_GETLINK, link->ifindex); if (r < 0) - return r; + goto failed; r = netlink_call_async(link->manager->rtnl, NULL, req, reconfigure_handler_on_bus_method_reload, reconfigure_data_destroy_callback, data); if (r < 0) - return r; + goto failed; TAKE_PTR(data); link->manager->reloading++; @@ -1543,6 +1541,11 @@ int link_reconfigure_on_bus_method_reload(Link *link, sd_bus_message *message) { link_set_state(link, LINK_STATE_INITIALIZED); return 0; + +failed: + log_link_warning_errno(link, r, "Failed to reconfigure interface: %m"); + link_enter_failed(link); + return r; } static int link_initialized_and_synced(Link *link) { diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 6893ade66a..4ebecaac59 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -87,13 +87,8 @@ static int match_prepare_for_sleep(sd_bus_message *message, void *userdata, sd_b log_debug("Coming back from suspend, reconfiguring all connections..."); - HASHMAP_FOREACH(link, m->links_by_index) { - r = link_reconfigure(link, /* force = */ true); - if (r < 0) { - log_link_warning_errno(link, r, "Failed to reconfigure interface: %m"); - link_enter_failed(link); - } - } + HASHMAP_FOREACH(link, m->links_by_index) + (void) link_reconfigure(link, /* force = */ true); return 0; } @@ -1143,13 +1138,9 @@ int manager_reload(Manager *m, sd_bus_message *message) { HASHMAP_FOREACH(link, m->links_by_index) { if (message) - r = link_reconfigure_on_bus_method_reload(link, message); + (void) link_reconfigure_on_bus_method_reload(link, message); else - r = link_reconfigure(link, /* force = */ false); - if (r < 0) { - log_link_warning_errno(link, r, "Failed to reconfigure the interface: %m"); - link_enter_failed(link); - } + (void) link_reconfigure(link, /* force = */ false); } r = 0; From e5b19cbed2032db214eec9a6191866f6f96355ef Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 21 Aug 2024 11:24:14 +0900 Subject: [PATCH 4/8] sd-dhcp-client: do not call callback with SD_DHCP_CLIENT_EVENT_STOP if already stopped When an interface enters the failed state, even if the DHCP client is stopped, the acquired DHCP lease is not unreferenced, as the callback dhcp4_handler() do nothing in that case. When the failed interface is being reconfigured after that, the DHCP client is stopped again (though it is already stopped), and SD_DHCP_CLIENT_EVENT_STOP event is triggered and sd_dhcp_client_send_release() is called, and the assertion in the function is triggered. E.g. === systemd-networkd[98588]: wlp59s0: DHCPv4 address 192.168.86.250/24, gateway 192.168.86.1 acquired from 192.168.86.1 systemd-networkd[98588]: wlp59s0: Could not set DHCPv4 route: Nexthop has invalid gateway. Network is unreachable systemd-networkd[98588]: wlp59s0: Failed systemd-networkd[98588]: wlp59s0: State changed: configuring -> failed systemd-networkd[98588]: wlp59s0: The interface entered the failed state frequently, refusing to reconfigure it automatically. systemd-networkd[98588]: wlp59s0: DHCPv4 client: STOPPED systemd-networkd[98588]: wlp59s0: DHCPv4 client: State changed: bound -> stopped systemd-networkd[98588]: Got message type=method_call sender=:1.449 destination=org.freedesktop.network1 path=/org/freedesktop/network1 interface=org.freedesktop.network1.Manager member=ReconfigureLink ... systemd-networkd[98588]: wlp59s0: State changed: failed -> initialized systemd-networkd[98588]: wlp59s0: found matching network '/etc/systemd/network/50-wifi.network'. systemd-networkd[98588]: wlp59s0: Configuring with /etc/systemd/network/50-wifi.network. systemd-networkd[98588]: wlp59s0: DHCPv4 client: STOPPED systemd-networkd[98588]: Assertion 'sd_dhcp_client_is_running(client)' failed at src/libsystemd-network/sd-dhcp-client.c:2197, function sd_dhcp_client_send_release(). Aborting. === --- src/libsystemd-network/sd-dhcp-client.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c index 1eb8509f0f..8b8d0611a2 100644 --- a/src/libsystemd-network/sd-dhcp-client.c +++ b/src/libsystemd-network/sd-dhcp-client.c @@ -683,15 +683,21 @@ static int client_initialize(sd_dhcp_client *client) { static void client_stop(sd_dhcp_client *client, int error) { assert(client); + DHCP_CLIENT_DONT_DESTROY(client); - if (error < 0) - log_dhcp_client_errno(client, error, "STOPPED: %m"); - else if (error == SD_DHCP_CLIENT_EVENT_STOP) - log_dhcp_client(client, "STOPPED"); - else - log_dhcp_client(client, "STOPPED: Unknown event"); + if (sd_dhcp_client_is_running(client)) { + if (error < 0) + log_dhcp_client_errno(client, error, "STOPPED: %m"); + else if (error == SD_DHCP_CLIENT_EVENT_STOP) + log_dhcp_client(client, "STOPPED"); + else + log_dhcp_client(client, "STOPPED: Unknown event"); - client_notify(client, error); + client_notify(client, error); + } else if (error < 0) { + log_dhcp_client_errno(client, error, "FAILED: %m"); + client_notify(client, error); + } client_initialize(client); } From f8cdd37d0aed8423d6744dd6637955760346939a Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 21 Aug 2024 10:23:06 +0900 Subject: [PATCH 5/8] sd-dhcp-client: refuse to send RELEASE or friends gracefully when the daemon is stopped or so We can easily hit the assertions without checking the internal states of the DHCP client before calling these functions. That's annoying. Let's do more gracefully. --- src/libsystemd-network/sd-dhcp-client.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c index 8b8d0611a2..94ad5bbea9 100644 --- a/src/libsystemd-network/sd-dhcp-client.c +++ b/src/libsystemd-network/sd-dhcp-client.c @@ -2138,14 +2138,8 @@ static int client_receive_message_raw( } int sd_dhcp_client_send_renew(sd_dhcp_client *client) { - assert_return(client, -EINVAL); - assert_return(sd_dhcp_client_is_running(client), -ESTALE); - assert_return(client->fd >= 0, -EINVAL); - - if (client->state != DHCP_STATE_BOUND) - return 0; - - assert(client->lease); + if (!sd_dhcp_client_is_running(client) || client->state != DHCP_STATE_BOUND) + return 0; /* do nothing */ client->start_delay = 0; client->discover_attempt = 1; @@ -2199,14 +2193,13 @@ int sd_dhcp_client_start(sd_dhcp_client *client) { } int sd_dhcp_client_send_release(sd_dhcp_client *client) { - assert_return(client, -EINVAL); - assert_return(sd_dhcp_client_is_running(client), -ESTALE); - assert_return(client->lease, -EUNATCH); - _cleanup_free_ DHCPPacket *release = NULL; size_t optoffset, optlen; int r; + if (!sd_dhcp_client_is_running(client) || !client->lease) + return 0; /* do nothing */ + r = client_message_init(client, &release, DHCP_RELEASE, &optlen, &optoffset); if (r < 0) return r; @@ -2234,14 +2227,13 @@ int sd_dhcp_client_send_release(sd_dhcp_client *client) { } int sd_dhcp_client_send_decline(sd_dhcp_client *client) { - assert_return(client, -EINVAL); - assert_return(sd_dhcp_client_is_running(client), -ESTALE); - assert_return(client->lease, -EUNATCH); - _cleanup_free_ DHCPPacket *release = NULL; size_t optoffset, optlen; int r; + if (!sd_dhcp_client_is_running(client) || !client->lease) + return 0; /* do nothing */ + r = client_message_init(client, &release, DHCP_DECLINE, &optlen, &optoffset); if (r < 0) return r; From 2bb7559ade518bb0f27ec6d0212ddb8bcc470677 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 21 Aug 2024 10:24:44 +0900 Subject: [PATCH 6/8] sd-dhcp-client: stop client without calling notification after sending RELEASE Otherwise, even the acquired lease is released, the client may be in e.g. BOUND state or so, and may send renew or rebind after timeout later. --- src/libsystemd-network/sd-dhcp-client.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c index 94ad5bbea9..1a1499fa2c 100644 --- a/src/libsystemd-network/sd-dhcp-client.c +++ b/src/libsystemd-network/sd-dhcp-client.c @@ -2223,7 +2223,11 @@ int sd_dhcp_client_send_release(sd_dhcp_client *client) { log_dhcp_client(client, "RELEASE"); - return 0; + /* This function is mostly called when stopping daemon. Hence, do not call client_stop() or + * client_restart(). Otherwise, the notification callback will be called again and we may easily + * enter an infinite loop. */ + client_initialize(client); + return 1; /* sent and stopped. */ } int sd_dhcp_client_send_decline(sd_dhcp_client *client) { From 22c864d88556186a82b525b009c6dc5599a87e9b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 21 Aug 2024 10:31:34 +0900 Subject: [PATCH 7/8] sd-dhcp-client: actually restart daemon after sending DECLINE message client_stop() sets DHCP_STATE_STOPPED to client->state, thus the server never restarted. --- src/libsystemd-network/sd-dhcp-client.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c index 1a1499fa2c..41638ac715 100644 --- a/src/libsystemd-network/sd-dhcp-client.c +++ b/src/libsystemd-network/sd-dhcp-client.c @@ -2260,15 +2260,13 @@ int sd_dhcp_client_send_decline(sd_dhcp_client *client) { log_dhcp_client(client, "DECLINE"); - client_stop(client, SD_DHCP_CLIENT_EVENT_STOP); + /* This function is mostly called when the acquired address conflicts with another host. + * Restarting the daemon to acquire another address. */ + r = client_restart(client); + if (r < 0) + return r; - if (client->state != DHCP_STATE_STOPPED) { - r = sd_dhcp_client_start(client); - if (r < 0) - return r; - } - - return 0; + return 1; /* sent and restarted. */ } int sd_dhcp_client_stop(sd_dhcp_client *client) { From 7321a87b533abff9629a0c39426964c9196137fd Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 21 Aug 2024 10:47:09 +0900 Subject: [PATCH 8/8] network/ipv4acd: adjust comment and logging --- src/network/networkd-ipv4acd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/network/networkd-ipv4acd.c b/src/network/networkd-ipv4acd.c index 6b1abc42e8..4785b96f9f 100644 --- a/src/network/networkd-ipv4acd.c +++ b/src/network/networkd-ipv4acd.c @@ -111,20 +111,20 @@ static int dhcp4_address_on_conflict(Link *link) { assert(link); assert(link->dhcp_client); + log_link_warning(link, "Dropping DHCPv4 lease, as an address conflict was detected."); + r = sd_dhcp_client_send_decline(link->dhcp_client); if (r < 0) log_link_warning_errno(link, r, "Failed to send DHCP DECLINE, ignoring: %m"); if (!link->dhcp_lease) - /* Unlikely, but during probing the address, the lease may be lost. */ return 0; - log_link_warning(link, "Dropping DHCPv4 lease, as an address conflict was detected."); r = dhcp4_lease_lost(link); if (r < 0) return log_link_warning_errno(link, r, "Failed to drop DHCPv4 lease: %m"); - /* It is not necessary to call address_remove() here, as dhcp4_lease_lost() removes it. */ + /* It is not necessary to call address_remove() here, as dhcp4_lease_lost() removes the address. */ return 0; }