From 5993ee8a8a4338bc8b8e1e836c1abae8882283c9 Mon Sep 17 00:00:00 2001 From: Wen Liang Date: Tue, 17 Dec 2024 14:40:59 -0500 Subject: [PATCH 1/4] nm-dhcp-client: add argument controlling whether to get next or current lease In the scenario for sending the release message, we need to guarantee that NM only sends the release message when the client received a lease from the server. However, there is some distinction between the `l3cd_curr` and `l3cd_next` when ACD is pending, because `l3cd_curr` is NULL but `l3cd_next` is not NULL when ACD is pending. Regardless of whether ACD is pending or completed, these are all considered the client have received the release from the server. Therefore, adapt the function `nm_dhcp_client_get_lease()` to control whether to get next or current lease. --- src/core/devices/nm-device.c | 2 +- src/core/dhcp/nm-dhcp-client.c | 19 +++++++++++++++++-- src/core/dhcp/nm-dhcp-client.h | 2 +- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index b7896fdc0b..45170d31ba 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -11617,7 +11617,7 @@ _dev_ipdhcpx_start(NMDevice *self, int addr_family) /* Take the NML3ConfigData from the previous lease (if any) that was passed to the NMDhcpClient. * This may be the old lease only used during the duration of a reapply until we get the * new lease. */ - previous_lease = nm_dhcp_client_get_lease(priv->ipdhcp_data_x[IS_IPv4].client); + previous_lease = nm_dhcp_client_get_lease(priv->ipdhcp_data_x[IS_IPv4].client, TRUE); if (!priv->ipdhcp_data_x[IS_IPv4].config) { priv->ipdhcp_data_x[IS_IPv4].config = nm_dhcp_config_new(addr_family, previous_lease); diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index f50a8e9fda..18ad4024b4 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -280,10 +280,25 @@ nm_dhcp_client_create_options_dict(NMDhcpClient *self, gboolean static_keys) return options; } +/** + * nm_dhcp_client_get_lease(): + * @self: the client + * @ignore_acd_pending: FALSE means to only return the lease that already + * passed ACD, thus it is in use by us. TRUE means to return a new lease + * that might still be pending of Address Collision Detection (ACD) check, + * if there is one, or return the current lease that passed ACD if not. + * + * Returns the current lease that passed ACD or a pending lease still under + * ACD check. + * + */ const NML3ConfigData * -nm_dhcp_client_get_lease(NMDhcpClient *self) +nm_dhcp_client_get_lease(NMDhcpClient *self, gboolean ignore_acd_pending) { - return NM_DHCP_CLIENT_GET_PRIVATE(self)->l3cd_curr; + if (ignore_acd_pending) + return NM_DHCP_CLIENT_GET_PRIVATE(self)->l3cd_curr; + else + return NM_DHCP_CLIENT_GET_PRIVATE(self)->l3cd_next; } /*****************************************************************************/ diff --git a/src/core/dhcp/nm-dhcp-client.h b/src/core/dhcp/nm-dhcp-client.h index 431d08f383..f26d88d86f 100644 --- a/src/core/dhcp/nm-dhcp-client.h +++ b/src/core/dhcp/nm-dhcp-client.h @@ -246,7 +246,7 @@ const NMDhcpClientConfig *nm_dhcp_client_get_config(NMDhcpClient *self); pid_t nm_dhcp_client_get_pid(NMDhcpClient *self); -const NML3ConfigData *nm_dhcp_client_get_lease(NMDhcpClient *self); +const NML3ConfigData *nm_dhcp_client_get_lease(NMDhcpClient *self, gboolean ignore_acd_pending); void nm_dhcp_client_stop(NMDhcpClient *self, gboolean release); From 96ff17fd483b0a176ae19c057fb97bdcfdcd6d4c Mon Sep 17 00:00:00 2001 From: Wen Liang Date: Thu, 5 Dec 2024 15:25:09 -0500 Subject: [PATCH 2/4] dhcp-client4: do not send release message when there is no lease The daemon crashes when NM tries to send the release message when there is no lease yet and the UDP socket is still in the PACKET state, which causes an assertion failure as the result. Add the condition to guarantee that n-dhcp4 only sends the release message when there is a lease. Resolves: https://issues.redhat.com/browse/RHEL-69132 Stack trace of the crash: 0 0x00007f5ac248bacc __pthread_kill_implementation (libc.so.6 + 0x8bacc) 1 0x00007f5ac243e686 __GI_raise (libc.so.6 + 0x3e686) 2 0x00007f5ac2428833 __GI_abort (libc.so.6 + 0x28833) 3 0x00007f5ac242875b __assert_fail_base (libc.so.6 + 0x2875b) 4 0x00007f5ac24373c6 __assert_fail (libc.so.6 + 0x373c6) 5 0x00005607ec7f194a n_dhcp4_c_connection_udp_send (NetworkManager + 0x8594a) 6 0x00005607eca228cc n_dhcp4_c_connection_start_request (NetworkManager + 0x2b68cc) 7 0x00005607eca14b31 nm_dhcp_client_stop (NetworkManager + 0x2a8b31) 8 0x00005607eca8a4ce _dev_ipdhcpx_cleanup (NetworkManager + 0x31e4ce) 9 0x00005607ecac144d _cleanup_ip_pre (NetworkManager + 0x35544d) 10 0x00005607ecac3f04 _cleanup_generic_pre (NetworkManager + 0x357f04) 11 0x00005607ecad5006 nm_device_cleanup (NetworkManager + 0x369006) 12 0x00005607ecac5230 _set_state_full (NetworkManager + 0x359230) 13 0x00005607ecac8c4a nm_device_state_changed (NetworkManager + 0x35cc4a) 14 0x00007f5ac2daa47b g_idle_dispatch (libglib-2.0.so.0 + 0x5147b) 15 0x00007f5ac2dadf4f g_main_dispatch (libglib-2.0.so.0 + 0x54f4f) 16 0x00007f5ac2e03268 g_main_context_iterate.constprop.0 (libglib-2.0.so.0 + 0xaa268) 17 0x00007f5ac2dad5a3 g_main_loop_run (libglib-2.0.so.0 + 0x545a3) 18 0x00005607ec7c3eed main (NetworkManager + 0x57eed) 19 0x00007f5ac24295d0 __libc_start_call_main (libc.so.6 + 0x295d0) 20 0x00007f5ac2429680 __libc_start_main_impl (libc.so.6 + 0x29680) 21 0x00005607ec7c43f5 _start (NetworkManager + 0x583f5) --- src/core/dhcp/nm-dhcp-nettools.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/core/dhcp/nm-dhcp-nettools.c b/src/core/dhcp/nm-dhcp-nettools.c index 61490fb662..27bb136b0a 100644 --- a/src/core/dhcp/nm-dhcp-nettools.c +++ b/src/core/dhcp/nm-dhcp-nettools.c @@ -1542,7 +1542,10 @@ stop(NMDhcpClient *client, gboolean release) NMDhcpNettoolsPrivate *priv = NM_DHCP_NETTOOLS_GET_PRIVATE(self); if (release) { - if (n_dhcp4_client_probe_release(priv->probe)) + /* After we receive a lease from server, it doesn't matter if we completed ACD, + * we should send the release message. */ + if (nm_dhcp_client_get_lease(client, FALSE) + && n_dhcp4_client_probe_release(priv->probe) < 0) _LOGT("dhcp-client4: failed to send request with RELEASE message"); } From bccf03159115fcbdc94d2a96d54f80b1aff7d625 Mon Sep 17 00:00:00 2001 From: Wen Liang Date: Thu, 5 Dec 2024 17:13:22 -0500 Subject: [PATCH 3/4] n-dhcp4: do not send the release message in wrong connection state We should not send the DHCP release message when udp socket is still in the PACKET state, this state is typically used during the discovery and offer phases, where the client broadcasts DHCP packets like DHCPDISCOVER and receives responses like DHCPOFFER. At this point, the client has no lease because it has not yet completed the DHCP handshake. --- src/n-dhcp4/src/n-dhcp4-c-probe.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/n-dhcp4/src/n-dhcp4-c-probe.c b/src/n-dhcp4/src/n-dhcp4-c-probe.c index ee3a8886bd..c37f1e207b 100644 --- a/src/n-dhcp4/src/n-dhcp4-c-probe.c +++ b/src/n-dhcp4/src/n-dhcp4-c-probe.c @@ -1332,6 +1332,10 @@ int n_dhcp4_client_probe_release(NDhcp4ClientProbe *probe) { _c_cleanup_(n_dhcp4_outgoing_freep) NDhcp4Outgoing *request_out = NULL; int r; + if (probe->connection.state != N_DHCP4_C_CONNECTION_STATE_DRAINING + && probe->connection.state != N_DHCP4_C_CONNECTION_STATE_UDP) + return -ENOTRECOVERABLE; + r = n_dhcp4_c_connection_release_new(&probe->connection, &request_out, NULL); if (r) return r; From 55710d3d7ce548c36a337482511cad6bb331572a Mon Sep 17 00:00:00 2001 From: Wen Liang Date: Mon, 16 Dec 2024 10:03:51 -0500 Subject: [PATCH 4/4] n-dhcp4: send request directly to avoid unnecessary retransmission timeout Using `n_dhcp4_c_connection_start_request()` will cause staying in `connection->request`, as a result, it will cause the resending of DHCPRELEASE and DHCPDECLINE message, thus, use `n_dhcp4_c_connection_send_request()` directly instead to avoid unnecessary retransmission timeout, as suggested by https://github.com/nettools/n-dhcp4/pull/44/commits/f030927a54b2292f02ab519485997e702749aa54#r1531834009. --- src/n-dhcp4/src/n-dhcp4-c-connection.c | 6 +++--- src/n-dhcp4/src/n-dhcp4-c-probe.c | 4 ++-- src/n-dhcp4/src/n-dhcp4-private.h | 3 +++ 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/n-dhcp4/src/n-dhcp4-c-connection.c b/src/n-dhcp4/src/n-dhcp4-c-connection.c index 7024b71ac9..e76ea16592 100644 --- a/src/n-dhcp4/src/n-dhcp4-c-connection.c +++ b/src/n-dhcp4/src/n-dhcp4-c-connection.c @@ -1010,9 +1010,9 @@ static const char *message_type_to_str(uint8_t type) { } } -static int n_dhcp4_c_connection_send_request(NDhcp4CConnection *connection, - NDhcp4Outgoing *request, - uint64_t timestamp) { +int n_dhcp4_c_connection_send_request(NDhcp4CConnection *connection, + NDhcp4Outgoing *request, + uint64_t timestamp) { char server_addr[INET_ADDRSTRLEN]; char client_addr[INET_ADDRSTRLEN]; char error_msg[128]; diff --git a/src/n-dhcp4/src/n-dhcp4-c-probe.c b/src/n-dhcp4/src/n-dhcp4-c-probe.c index c37f1e207b..7a6def340c 100644 --- a/src/n-dhcp4/src/n-dhcp4-c-probe.c +++ b/src/n-dhcp4/src/n-dhcp4-c-probe.c @@ -1137,7 +1137,7 @@ int n_dhcp4_client_probe_transition_decline(NDhcp4ClientProbe *probe, NDhcp4Inco if (r) return r; - r = n_dhcp4_c_connection_start_request(&probe->connection, request, ns_now); + r = n_dhcp4_c_connection_send_request(&probe->connection, request, ns_now); if (r) return r; else @@ -1340,7 +1340,7 @@ int n_dhcp4_client_probe_release(NDhcp4ClientProbe *probe) { if (r) return r; - r = n_dhcp4_c_connection_start_request(&probe->connection, request_out, 0); + r = n_dhcp4_c_connection_send_request(&probe->connection, request_out, 0); if (r) return r; diff --git a/src/n-dhcp4/src/n-dhcp4-private.h b/src/n-dhcp4/src/n-dhcp4-private.h index 40404001a8..dbfe9b4ad0 100644 --- a/src/n-dhcp4/src/n-dhcp4-private.h +++ b/src/n-dhcp4/src/n-dhcp4-private.h @@ -642,6 +642,9 @@ int n_dhcp4_c_connection_release_new(NDhcp4CConnection *connection, int n_dhcp4_c_connection_start_request(NDhcp4CConnection *connection, NDhcp4Outgoing *request, uint64_t timestamp); +int n_dhcp4_c_connection_send_request(NDhcp4CConnection *connection, + NDhcp4Outgoing *request, + uint64_t timestamp); int n_dhcp4_c_connection_dispatch_timer(NDhcp4CConnection *connection, uint64_t timestamp); int n_dhcp4_c_connection_dispatch_io(NDhcp4CConnection *connection,