From 4bcdc3c1ebe3e2e8a967ff067ecb2a8cbfc2f6ab Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 7 Jan 2020 12:01:39 +0100 Subject: [PATCH 1/3] n-dhcp4: allow calling listen() on already listening connection When the client enters the INIT state, it calls listen() on the connection connection to create the packet socket. However, if the client is coming from the REBOOTING state after a NAK, the connection is already in the listening state; do nothing in such case. --- shared/n-dhcp4/src/n-dhcp4-c-connection.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/shared/n-dhcp4/src/n-dhcp4-c-connection.c b/shared/n-dhcp4/src/n-dhcp4-c-connection.c index e51a3e3249..f3ae44e2d9 100644 --- a/shared/n-dhcp4/src/n-dhcp4-c-connection.c +++ b/shared/n-dhcp4/src/n-dhcp4-c-connection.c @@ -139,6 +139,9 @@ int n_dhcp4_c_connection_listen(NDhcp4CConnection *connection) { _c_cleanup_(c_closep) int fd_packet = -1; int r; + if (connection->state == N_DHCP4_C_CONNECTION_STATE_PACKET) + return 0; + c_assert(connection->state == N_DHCP4_C_CONNECTION_STATE_INIT || connection->state == N_DHCP4_C_CONNECTION_STATE_DRAINING || connection->state == N_DHCP4_C_CONNECTION_STATE_UDP); From 218782a9a3c326f5c8cc3ea40dc0cd039060b188 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 7 Jan 2020 12:02:55 +0100 Subject: [PATCH 2/3] n-dhcp4: restart the transaction after a NAK It is not enough to set the INIT state after a NAK; a timeout (ns_deferred) must be set so that it is added to the event fd. The client retries immediately the first time, so that in the successful case it gets an address quickly. To avoid flooding the network in case of servers always replying with NAKs, next attempts are done with intervals from 2 seconds to 5 minutes using exponential backoff. See also systemd commit [1]. [1] https://github.com/systemd/systemd/commit/1d1a3e0afb85478cda43670b8ed92a6db6c83f3e https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/325 --- shared/n-dhcp4/src/n-dhcp4-c-probe.c | 10 ++++++---- shared/n-dhcp4/src/n-dhcp4-private.h | 1 + 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/shared/n-dhcp4/src/n-dhcp4-c-probe.c b/shared/n-dhcp4/src/n-dhcp4-c-probe.c index 4fb7d3892a..82a089ee5f 100644 --- a/shared/n-dhcp4/src/n-dhcp4-c-probe.c +++ b/shared/n-dhcp4/src/n-dhcp4-c-probe.c @@ -946,7 +946,7 @@ static int n_dhcp4_client_probe_transition_ack(NDhcp4ClientProbe *probe, NDhcp4I n_dhcp4_client_lease_unref(probe->current_lease); probe->current_lease = n_dhcp4_client_lease_ref(lease); probe->state = N_DHCP4_CLIENT_PROBE_STATE_BOUND; - + probe->ns_nak_restart_delay = 0; break; case N_DHCP4_CLIENT_PROBE_STATE_REQUESTING: @@ -969,7 +969,7 @@ static int n_dhcp4_client_probe_transition_ack(NDhcp4ClientProbe *probe, NDhcp4I node->event.granted.lease = n_dhcp4_client_lease_ref(lease); probe->current_lease = n_dhcp4_client_lease_ref(lease); probe->state = N_DHCP4_CLIENT_PROBE_STATE_GRANTED; - + probe->ns_nak_restart_delay = 0; break; case N_DHCP4_CLIENT_PROBE_STATE_INIT: @@ -1004,9 +1004,11 @@ static int n_dhcp4_client_probe_transition_nak(NDhcp4ClientProbe *probe) { return r; probe->state = N_DHCP4_CLIENT_PROBE_STATE_INIT; - + probe->ns_deferred = n_dhcp4_gettime(CLOCK_BOOTTIME) + probe->ns_nak_restart_delay; + probe->ns_nak_restart_delay = c_clamp(probe->ns_nak_restart_delay * 2, + UINT64_C(1000000000 * 2), + UINT64_C(1000000000 * 300)); break; - case N_DHCP4_CLIENT_PROBE_STATE_SELECTING: case N_DHCP4_CLIENT_PROBE_STATE_INIT_REBOOT: case N_DHCP4_CLIENT_PROBE_STATE_INIT: diff --git a/shared/n-dhcp4/src/n-dhcp4-private.h b/shared/n-dhcp4/src/n-dhcp4-private.h index fcfb0f35b8..c092ae8fc3 100644 --- a/shared/n-dhcp4/src/n-dhcp4-private.h +++ b/shared/n-dhcp4/src/n-dhcp4-private.h @@ -352,6 +352,7 @@ struct NDhcp4ClientProbe { unsigned int state; /* current probe state */ uint64_t ns_deferred; /* timeout for deferred action */ uint64_t ns_reinit; + uint64_t ns_nak_restart_delay; /* restart delay after a nak */ NDhcp4ClientLease *current_lease; /* current lease */ NDhcp4CConnection connection; /* client connection wrapper */ From 2523000b36b113fd7b94ae6b7178f6fd33908ef9 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 7 Jan 2020 12:03:21 +0100 Subject: [PATCH 3/3] dhcp: nettools: handle 'retracted' event as 'expired' The 'retracted' event is emitted when the client receives a NAK in the rebooting, requesting, renewing or rebinding state, while 'expired' means that the client wasn't able to renew the lease before expiry. In both cases the old lease is no longer valid and n-dhcp4 keep trying to get a lease, so the two events should be handlded in the same way. Note that the systemd client doesn't have a 'retracted' event and considers all NAKs as 'expired' events. --- src/dhcp/nm-dhcp-nettools.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dhcp/nm-dhcp-nettools.c b/src/dhcp/nm-dhcp-nettools.c index 30820c615a..76a2c2e0aa 100644 --- a/src/dhcp/nm-dhcp-nettools.c +++ b/src/dhcp/nm-dhcp-nettools.c @@ -1028,10 +1028,10 @@ dhcp4_event_handle (NMDhcpNettools *self, _LOGW ("selecting lease failed: %d", r); } break; + case N_DHCP4_CLIENT_EVENT_RETRACTED: case N_DHCP4_CLIENT_EVENT_EXPIRED: nm_dhcp_client_set_state (NM_DHCP_CLIENT (self), NM_DHCP_STATE_EXPIRE, NULL, NULL); break; - case N_DHCP4_CLIENT_EVENT_RETRACTED: case N_DHCP4_CLIENT_EVENT_CANCELLED: nm_dhcp_client_set_state (NM_DHCP_CLIENT (self), NM_DHCP_STATE_FAIL, NULL, NULL); break;