From a7eb77260ae6cfc56313e99f6178daa0b8283226 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Thu, 1 Sep 2022 10:21:20 +0200 Subject: [PATCH 1/2] dhcp: decline IPv6 lease if all adresses fail DAD Currently we accept the DHCPv6 just after addresses are configured on kernel, without waiting DAD result. Instead, wait that DAD completes and decline the lease if all addresses are detected as duplicate. Note that when an address has non-infinite lifetime and fails DAD, kernel removes it automatically. With iproute2 we see something like: 602: testX6 inet6 2620::1234:5678/128 scope global tentative dynamic noprefixroute valid_lft 7500sec preferred_lft 7200sec Deleted 602: testX6 inet6 2620::1234:5678/128 scope global dadfailed tentative dynamic noprefixroute valid_lft 7500sec preferred_lft 7200sec Since the address gets removed from the platform cache, at the moment we don't have a way to check the flags of the removal message. Therefore, we assume that any address that goes away in tentative state was detected as duplicate. https://bugzilla.redhat.com/show_bug.cgi?id=2096386 --- src/core/dhcp/nm-dhcp-client.c | 134 +++++++++++++++++++++++---------- 1 file changed, 95 insertions(+), 39 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 34873ac2e3..9ca8f77684 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -1043,8 +1043,11 @@ ipv6_lladdr_find(NMDhcpClient *self) return NULL; } -static const NMPlatformIP6Address * -ipv6_tentative_addr_find(NMDhcpClient *self) +static void +ipv6_tentative_addr_check(NMDhcpClient *self, + GPtrArray **tentative, + GPtrArray **missing, + const NMPlatformIP6Address **valid) { NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); NMDedupMultiIter iter; @@ -1062,16 +1065,26 @@ ipv6_tentative_addr_find(NMDhcpClient *self) NMP_CACHE_ID_TYPE_OBJECT_TYPE, &needle)); if (!pladdr) { - /* Address was removed from platform */ + /* address removed: we assume that's because DAD failed */ + if (missing) { + if (!*missing) + *missing = g_ptr_array_new(); + g_ptr_array_add(*missing, (gpointer) addr); + } continue; } if (NM_FLAGS_HAS(pladdr->n_ifa_flags, IFA_F_TENTATIVE) - && !NM_FLAGS_HAS(pladdr->n_ifa_flags, IFA_F_OPTIMISTIC)) - return pladdr; - } + && !NM_FLAGS_HAS(pladdr->n_ifa_flags, IFA_F_OPTIMISTIC)) { + if (tentative) { + if (!*tentative) + *tentative = g_ptr_array_new(); + g_ptr_array_add(*tentative, (gpointer) addr); + } + } - return NULL; + NM_SET_OUT(valid, addr); + } } static void @@ -1108,21 +1121,61 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp if (notify_data->notify_type == NM_L3_CONFIG_NOTIFY_TYPE_PLATFORM_CHANGE_ON_IDLE && priv->l3cfg_notify.wait_ipv6_dad) { - const NMPlatformIP6Address *tentative; + gs_unref_ptrarray GPtrArray *tentative = NULL; + gs_unref_ptrarray GPtrArray *missing = NULL; + const NMPlatformIP6Address *valid = NULL; + char str[NM_UTILS_TO_STRING_BUFFER_SIZE]; + guint i; + gs_free_error GError *error = NULL; + + ipv6_tentative_addr_check(self, &tentative, &missing, &valid); + if (tentative) { + for (i = 0; i < tentative->len; i++) { + _LOGD("still waiting DAD for address: %s", + nm_platform_ip6_address_to_string(tentative->pdata[i], str, sizeof(str))); + } + } else { + /* done */ - tentative = ipv6_tentative_addr_find(self); - if (!tentative) { - _LOGD("addresses in the lease completed DAD"); priv->l3cfg_notify.wait_ipv6_dad = FALSE; nm_clear_g_source_inst(&priv->v6.dad_timeout_source); l3_cfg_notify_check_connected(self); - _emit_notify( - self, - &((NMDhcpClientNotifyData){.notify_type = NM_DHCP_CLIENT_NOTIFY_TYPE_LEASE_UPDATE, - .lease_update = { - .l3cd = priv->l3cd_curr, - .accepted = TRUE, - }})); + + if (missing) { + for (i = 0; i < missing->len; i++) { + _LOGE("DAD failed for address: %s", + nm_platform_ip6_address_to_string(missing->pdata[i], str, sizeof(str))); + } + } + + if (valid) { + /* at least one non-duplicate address */ + _LOGD("addresses in the lease completed DAD: accept the lease"); + + if (_dhcp_client_accept(self, priv->l3cd_curr, &error)) { + _emit_notify(self, + &((NMDhcpClientNotifyData){ + .notify_type = NM_DHCP_CLIENT_NOTIFY_TYPE_LEASE_UPDATE, + .lease_update = { + .l3cd = priv->l3cd_curr, + .accepted = TRUE, + }})); + } else { + gs_free char *reason = + g_strdup_printf("error accepting lease: %s", error->message); + + _LOGD("accept failed: %s", error->message); + _emit_notify(self, + &((NMDhcpClientNotifyData){ + .notify_type = NM_DHCP_CLIENT_NOTIFY_TYPE_IT_LOOKS_BAD, + .it_looks_bad.reason = reason, + })); + } + } else { + _LOGD("decline the lease"); + if (!_dhcp_client_decline(self, priv->l3cd_curr, "DAD failed", &error)) + _LOGD("decline failed: %s", error->message); + } } } @@ -1155,20 +1208,23 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp address4->peer_address)) goto wait_dhcp_commit_done; } else { - const NMPlatformIP6Address *address6 = (const NMPlatformIP6Address *) lease_address; - const NMPlatformIP6Address *tentative; - char str[NM_UTILS_TO_STRING_BUFFER_SIZE]; + const NMPlatformIP6Address *address6 = (const NMPlatformIP6Address *) lease_address; + gs_unref_ptrarray GPtrArray *tentative = NULL; + char str[NM_UTILS_TO_STRING_BUFFER_SIZE]; + guint i; if (!nm_l3_config_data_lookup_address_6(committed_l3cd, &address6->address)) goto wait_dhcp_commit_done; - tentative = ipv6_tentative_addr_find(self); + ipv6_tentative_addr_check(self, &tentative, NULL, NULL); if (tentative) { priv->l3cfg_notify.wait_ipv6_dad = TRUE; priv->v6.dad_timeout_source = nm_g_timeout_add_seconds_source(30, ipv6_dad_timeout, self); - _LOGD("wait DAD for address %s", - nm_platform_ip6_address_to_string(tentative, str, sizeof(str))); + for (i = 0; i < tentative->len; i++) { + _LOGD("wait DAD for address %s", + nm_platform_ip6_address_to_string(tentative->pdata[i], str, sizeof(str))); + } } else { priv->l3cfg_notify.wait_ipv6_dad = FALSE; nm_clear_g_source_inst(&priv->v6.dad_timeout_source); @@ -1179,22 +1235,22 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp l3_cfg_notify_check_connected(self); - _LOGD("accept lease"); - - if (!_dhcp_client_accept(self, priv->l3cd_curr, &error)) { - gs_free char *reason = g_strdup_printf("error accepting lease: %s", error->message); - - _LOGD("accept failed: %s", error->message); - - _emit_notify(self, - &((NMDhcpClientNotifyData){ - .notify_type = NM_DHCP_CLIENT_NOTIFY_TYPE_IT_LOOKS_BAD, - .it_looks_bad.reason = reason, - })); - goto wait_dhcp_commit_done; - } - if (priv->config.addr_family == AF_INET || !priv->l3cfg_notify.wait_ipv6_dad) { + _LOGD("accept lease"); + + if (!_dhcp_client_accept(self, priv->l3cd_curr, &error)) { + gs_free char *reason = g_strdup_printf("error accepting lease: %s", error->message); + + _LOGD("accept failed: %s", error->message); + + _emit_notify(self, + &((NMDhcpClientNotifyData){ + .notify_type = NM_DHCP_CLIENT_NOTIFY_TYPE_IT_LOOKS_BAD, + .it_looks_bad.reason = reason, + })); + goto wait_dhcp_commit_done; + } + _emit_notify( self, &((NMDhcpClientNotifyData){.notify_type = NM_DHCP_CLIENT_NOTIFY_TYPE_LEASE_UPDATE, From e4aefbc5561375bf89a09c051a9c48a16855d234 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 2 Sep 2022 09:43:15 +0200 Subject: [PATCH 2/2] dhcp: implement decline on IPv6 DAD failure with dhclient The dhclient plugin already supports sending a decline when IPv4 ACD fails. Also implement support for IPv6 DAD. See-also: 156d84217ced ("dhcp/dhclient: implement accept/decline (ACD) for dhclient plugin") --- src/core/dhcp/nm-dhcp-client.c | 47 ++++++++++++---------------------- src/core/dhcp/nm-dhcp-helper.c | 19 ++++++++++++-- 2 files changed, 33 insertions(+), 33 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 9ca8f77684..68246280e3 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -115,9 +115,6 @@ typedef struct _NMDhcpClientPrivate { in_addr_t addr; NMOptionBool state; } acd; - struct { - GDBusMethodInvocation *invocation; - } bound; } v4; struct { GSource *lladdr_timeout_source; @@ -125,6 +122,8 @@ typedef struct _NMDhcpClientPrivate { } v6; }; + GDBusMethodInvocation *invocation; + struct { gulong id; bool wait_dhcp_commit : 1; @@ -909,13 +908,10 @@ _accept(NMDhcpClient *self, const NML3ConfigData *l3cd, GError **error) { NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - if (!NM_IS_IPv4(priv->config.addr_family)) + if (!priv->invocation) return TRUE; - if (!priv->v4.bound.invocation) - return TRUE; - - g_dbus_method_invocation_return_value(g_steal_pointer(&priv->v4.bound.invocation), NULL); + g_dbus_method_invocation_return_value(g_steal_pointer(&priv->invocation), NULL); return TRUE; } @@ -939,20 +935,17 @@ decline(NMDhcpClient *self, const NML3ConfigData *l3cd, const char *error_messag { NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - if (!NM_IS_IPv4(priv->config.addr_family)) - return TRUE; - - if (!priv->v4.bound.invocation) { + if (!priv->invocation) { nm_utils_error_set(error, NM_UTILS_ERROR_UNKNOWN, "calling decline in unexpected script state"); return FALSE; } - - g_dbus_method_invocation_return_error(g_steal_pointer(&priv->v4.bound.invocation), + g_dbus_method_invocation_return_error(g_steal_pointer(&priv->invocation), NM_DEVICE_ERROR, NM_DEVICE_ERROR_FAILED, - "acd failed"); + NM_IS_IPv4(priv->config.addr_family) ? "ACD failed" + : "DAD failed"); return TRUE; } @@ -1424,8 +1417,8 @@ nm_dhcp_client_stop(NMDhcpClient *self, gboolean release) priv->is_stopped = TRUE; - if (NM_IS_IPv4(priv->config.addr_family) && priv->v4.bound.invocation) { - g_dbus_method_invocation_return_error(g_steal_pointer(&priv->v4.bound.invocation), + if (priv->invocation) { + g_dbus_method_invocation_return_error(g_steal_pointer(&priv->invocation), NM_DEVICE_ERROR, NM_DEVICE_ERROR_FAILED, "dhcp stopping"); @@ -1584,7 +1577,6 @@ nm_dhcp_client_handle_event(gpointer unused, NMPlatformIP6Address prefix = { 0, }; - int IS_IPv4; g_return_val_if_fail(NM_IS_DHCP_CLIENT(self), FALSE); g_return_val_if_fail(iface != NULL, FALSE); @@ -1681,16 +1673,13 @@ nm_dhcp_client_handle_event(gpointer unused, client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_FAIL; } - IS_IPv4 = NM_IS_IPv4(priv->config.addr_family); + if (priv->invocation) + g_dbus_method_invocation_return_value(g_steal_pointer(&priv->invocation), NULL); - if (IS_IPv4 && priv->v4.bound.invocation) - g_dbus_method_invocation_return_value(g_steal_pointer(&priv->v4.bound.invocation), NULL); - - if (IS_IPv4 - && NM_IN_SET(client_event_type, - NM_DHCP_CLIENT_EVENT_TYPE_BOUND, - NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED)) - priv->v4.bound.invocation = g_steal_pointer(&invocation); + if (NM_IN_SET(client_event_type, + NM_DHCP_CLIENT_EVENT_TYPE_BOUND, + NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED)) + priv->invocation = g_steal_pointer(&invocation); _nm_dhcp_client_notify(self, client_event_type, l3cd); @@ -1841,10 +1830,6 @@ set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *ps * explicitly initialize the respective union member. */ if (NM_IS_IPv4(priv->config.addr_family)) { priv->v4 = (typeof(priv->v4)){ - .bound = - { - .invocation = NULL, - }, .acd = { .addr = INADDR_ANY, diff --git a/src/core/dhcp/nm-dhcp-helper.c b/src/core/dhcp/nm-dhcp-helper.c index 5a17f4e8ab..78db617c6a 100644 --- a/src/core/dhcp/nm-dhcp-helper.c +++ b/src/core/dhcp/nm-dhcp-helper.c @@ -114,6 +114,8 @@ main(int argc, char *argv[]) gint64 time_start; gint64 time_end; gint64 remaining_time; + gboolean IS_IPv4; + const char *reason; /* Connecting to the unix socket can fail with EAGAIN if there are too * many pending connections and the server can't accept them in time @@ -124,7 +126,19 @@ main(int argc, char *argv[]) time_end = time_start + (5000 * 1000L); try_count = 0; - _LOGi("nm-dhcp-helper: event called"); + reason = getenv("reason"); + + _LOGi("nm-dhcp-helper: event called: %s", reason); + + IS_IPv4 = !NM_IN_STRSET(reason, + "PREINIT6", + "BOUND6", + "RENEW6", + "REBIND6", + "DEPREF6", + "EXPIRE6", + "RELEASE6", + "STOP6"); do_connect: try_count++; @@ -244,5 +258,6 @@ out: } _LOGi("success: %s", success ? "YES" : "NO"); - return success ? EXIT_SUCCESS : EXIT_FAILURE; + /* The error code to send a decline depends on the address family */ + return success ? EXIT_SUCCESS : (IS_IPv4 ? 1 : 3); }