From a55d2a50353890ba344bc9d80e3c74b8a7bd1537 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 19 May 2022 14:57:42 +0200 Subject: [PATCH 01/27] contrib: improve bashrc for nm-in-container.sh --- contrib/scripts/nm-in-container.sh | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/contrib/scripts/nm-in-container.sh b/contrib/scripts/nm-in-container.sh index 95fc306a95..6d21d5ce68 100755 --- a/contrib/scripts/nm-in-container.sh +++ b/contrib/scripts/nm-in-container.sh @@ -167,6 +167,12 @@ alias n="ninja -C build" alias l='ls -l --color=auto' +ulimit -c unlimited + +export G_DEBUG=fatal-warnings + +unset DEBUGINFOD_URLS + Clean() { systemctl stop NetworkManager rm -i -rf /run/NetworkManager @@ -233,8 +239,8 @@ nmcli connection add type pppoe con-name ppp-net1 ifname ppp-net1 pppoe.parent n for i in {1..9}; do nm-env-prepare.sh --prefix eth -i \$i; done systemctl status NetworkManager systemctl stop NetworkManager -systemctl stop NetworkManager; /opt/test/sbin/NetworkManager --debug 2>&1 | tee -a /tmp/nm-log.txt systemctl stop NetworkManager; gdb -ex run --args /opt/test/sbin/NetworkManager --debug +systemctl stop NetworkManager; /opt/test/sbin/NetworkManager --debug 2>&1 | tee -a ./nm-log.txt EOF cat < /etc/NetworkManager/conf.d/95-user.conf COPY data-bash_history /root/.bash_history COPY data-gdbinit /root/.gdbinit COPY data-gdb_history /root/.gdb_history From a6b8f050a0fa85daac2595f708643d53b2199b89 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 27 May 2022 13:14:31 +0200 Subject: [PATCH 02/27] contrib: add "journal" command to "nm-in-container.sh" for showing logs --- contrib/scripts/nm-in-container.sh | 40 ++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/contrib/scripts/nm-in-container.sh b/contrib/scripts/nm-in-container.sh index 6d21d5ce68..7907537fb6 100755 --- a/contrib/scripts/nm-in-container.sh +++ b/contrib/scripts/nm-in-container.sh @@ -9,14 +9,17 @@ set -e # - build: build a new image, named "$CONTAINER_NAME_REPOSITORY:$CONTAINER_NAME_TAG" ("nm:nm") # - run: start the container and tag it "$CONTAINER_NAME_NAME" ("nm") # - exec: run bash inside the container +# - journal|j: print the journal from inside the container # - stop: stop the container # - clean: delete the container and the image. # # Options: # --no-cleanup: don't delete the CONTAINERFILE and other artifacts # --stop: only has effect with "run". It will stop the container afterwards. -# -- [COMMAND]: with command "exec", provide a command to run in the container. -# Defaults to "bash". +# -- [EXTRA_ARGS]: +# - with command "exec", provide a command and arguments to run in the container. +# Defaults to "bash". +# - with command "journal", additional arguments that are passed to journalctl. # # It bind mounts the current working directory inside the container. # You can run `make install` and run tests. @@ -32,11 +35,13 @@ CONTAINER_NAME_REPOSITORY=${CONTAINER_NAME_REPOSITORY:-nm} CONTAINER_NAME_TAG=${CONTAINER_NAME_TAG:-nm} CONTAINER_NAME_NAME=${CONTAINER_NAME_NAME:-nm} +EXEC_ENV=() + ############################################################################### usage() { cat < Date: Fri, 27 May 2022 16:20:55 +0200 Subject: [PATCH 03/27] contrib: install black/clang-format in nm-in-container.sh It's just convenient to have some tools around, not only for testing, but also for (some limited) development. In particular, because we bind-mount .vimrc inside the container, and if I use vim, black/clang-format is just one key binding away. --- contrib/scripts/nm-in-container.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contrib/scripts/nm-in-container.sh b/contrib/scripts/nm-in-container.sh index 7907537fb6..ed859436fc 100755 --- a/contrib/scripts/nm-in-container.sh +++ b/contrib/scripts/nm-in-container.sh @@ -276,6 +276,7 @@ RUN dnf install -y \\ bash-completion \\ bind-utils \\ bluez-libs-devel \\ + clang-tools-extra \\ cscope \\ dbus-devel \\ dbus-x11 \\ @@ -315,6 +316,7 @@ RUN dnf install -y \\ ppp-devel \\ procps \\ python3-behave \\ + python3-black \\ python3-dbus \\ python3-devel \\ python3-gobject \\ From d81a9aec31bf71c9a53fed183092c7170a00a289 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 19 May 2022 12:27:32 +0200 Subject: [PATCH 04/27] glib-aux/logging: add LOGD_DHCP_af() helper macro --- src/libnm-glib-aux/nm-logging-fwd.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/libnm-glib-aux/nm-logging-fwd.h b/src/libnm-glib-aux/nm-logging-fwd.h index 0e715c5047..72e5723c28 100644 --- a/src/libnm-glib-aux/nm-logging-fwd.h +++ b/src/libnm-glib-aux/nm-logging-fwd.h @@ -59,7 +59,16 @@ typedef enum { LOGD_IP = LOGD_IP4 | LOGD_IP6, #define LOGD_DHCPX(is_ipv4) ((is_ipv4) ? LOGD_DHCP4 : LOGD_DHCP6) -#define LOGD_IPX(is_ipv4) ((is_ipv4) ? LOGD_IP4 : LOGD_IP6) + +#define LOGD_DHCP_af(addr_family) \ + ({ \ + const int _addr_family_1 = (addr_family); \ + \ + (_addr_family_1 == AF_UNSPEC ? LOGD_DHCP \ + : (NM_IS_IPv4(_addr_family_1) ? LOGD_DHCP4 : LOGD_DHCP6)); \ + }) + +#define LOGD_IPX(is_ipv4) ((is_ipv4) ? LOGD_IP4 : LOGD_IP6) } NMLogDomain; From 08c010cb2b700113ddb173eac8b12636b026699a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 26 May 2022 13:28:10 +0200 Subject: [PATCH 05/27] glib-aux: add nm_g_array_index_p() helper and cleanup nm_g_array*() helpers --- src/libnm-glib-aux/nm-shared-utils.h | 75 +++++++++++++++++----------- 1 file changed, 47 insertions(+), 28 deletions(-) diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 5552bf0567..51a3e0afa5 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -2194,38 +2194,57 @@ nm_g_array_unref(GArray *arr) g_array_unref(arr); } -#define nm_g_array_first(arr, type) \ - ({ \ - GArray *const _arr = (arr); \ - guint _len; \ - \ - nm_assert(_arr); \ - _len = _arr->len; \ - nm_assert(_len > 0); \ - &g_array_index(arr, type, 0); \ +#define nm_g_array_first(arr, Type) \ + ({ \ + GArray *const _arr = (arr); \ + \ + nm_assert(_arr); \ + nm_assert(sizeof(Type) == g_array_get_element_size(_arr)); \ + nm_assert(_arr->len > 0); \ + \ + &g_array_index(arr, Type, 0); \ }) -#define nm_g_array_last(arr, type) \ - ({ \ - GArray *const _arr = (arr); \ - guint _len; \ - \ - nm_assert(_arr); \ - _len = _arr->len; \ - nm_assert(_len > 0); \ - &g_array_index(arr, type, _len - 1u); \ +#define nm_g_array_last(arr, Type) \ + ({ \ + GArray *const _arr = (arr); \ + \ + nm_assert(_arr); \ + nm_assert(sizeof(Type) == g_array_get_element_size(_arr)); \ + nm_assert(_arr->len > 0); \ + \ + &g_array_index(arr, Type, _arr->len - 1u); \ }) -#define nm_g_array_append_new(arr, type) \ - ({ \ - GArray *const _arr = (arr); \ - guint _len; \ - \ - nm_assert(_arr); \ - _len = _arr->len; \ - nm_assert(_len < G_MAXUINT); \ - g_array_set_size(_arr, _len + 1u); \ - &g_array_index(arr, type, _len); \ +/* Similar to g_array_index(). The differences are + * - this does nm_assert() checks that the arguments are valid. + * - returns a pointer to the element. */ +#define nm_g_array_index_p(arr, Type, idx) \ + ({ \ + GArray *const _arr = (arr); \ + const guint _idx = (idx); \ + \ + nm_assert(_arr); \ + nm_assert(sizeof(Type) == g_array_get_element_size(_arr)); \ + nm_assert(_idx < _arr->len); \ + \ + &g_array_index(_arr, Type, _idx); \ + }) + +#define nm_g_array_append_new(arr, Type) \ + ({ \ + GArray *const _arr = (arr); \ + guint _len; \ + \ + nm_assert(_arr); \ + nm_assert(sizeof(Type) == g_array_get_element_size(_arr)); \ + \ + _len = _arr->len; \ + \ + nm_assert(_len < G_MAXUINT); \ + \ + g_array_set_size(_arr, _len + 1u); \ + &g_array_index(arr, Type, _len); \ }) /*****************************************************************************/ From eed9acc191cccc610aa09360eefb36263c1394b0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 26 May 2022 14:57:50 +0200 Subject: [PATCH 06/27] glib-aux: add assertions to nm_strvarray_*() helpers --- src/libnm-glib-aux/nm-shared-utils.h | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 51a3e0afa5..fddcc8fe26 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -3121,10 +3121,14 @@ gboolean nm_utils_ifname_valid(const char *name, NMUtilsIfaceType type, GError * static inline GArray * nm_strvarray_ensure(GArray **p) { + nm_assert(p); + if (!*p) { *p = g_array_new(TRUE, FALSE, sizeof(char *)); g_array_set_clear_func(*p, nm_indirect_g_free); - } + } else + nm_assert(g_array_get_element_size(*p) == sizeof(char *)); + return *p; } @@ -3133,6 +3137,9 @@ nm_strvarray_add(GArray *array, const char *str) { char *s; + nm_assert(array); + nm_assert(g_array_get_element_size(array) == sizeof(char *)); + s = g_strdup(str); g_array_append_val(array, s); } @@ -3140,15 +3147,14 @@ nm_strvarray_add(GArray *array, const char *str) static inline const char * nm_strvarray_get_idx(GArray *array, guint idx) { - nm_assert(array); - nm_assert(idx < array->len); - - return g_array_index(array, const char *, idx); + return *nm_g_array_index_p(array, const char *, idx); } static inline const char *const * nm_strvarray_get_strv_non_empty(GArray *arr, guint *length) { + nm_assert(!arr || g_array_get_element_size(arr) == sizeof(char *)); + if (!arr || arr->len == 0) { NM_SET_OUT(length, 0); return NULL; @@ -3163,6 +3169,8 @@ nm_strvarray_get_strv_non_empty_dup(GArray *arr, guint *length) { const char *const *strv; + nm_assert(!arr || g_array_get_element_size(arr) == sizeof(char *)); + if (!arr || arr->len == 0) { NM_SET_OUT(length, 0); return NULL; @@ -3181,6 +3189,8 @@ nm_strvarray_get_strv(GArray **arr, guint *length) return (const char *const *) arr; } + nm_assert(g_array_get_element_size(*arr) == sizeof(char *)); + NM_SET_OUT(length, (*arr)->len); return &g_array_index(*arr, const char *, 0); } @@ -3192,6 +3202,8 @@ nm_strvarray_set_strv(GArray **array, const char *const *strv) array_old = g_steal_pointer(array); + nm_assert(!array_old || g_array_get_element_size(array_old) == sizeof(char *)); + if (!strv || !strv[0]) return; @@ -3208,6 +3220,7 @@ nm_strvarray_find_first(GArray *strv, const char *needle) nm_assert(needle); if (strv) { + nm_assert(g_array_get_element_size(strv) == sizeof(char *)); for (i = 0; i < strv->len; i++) { if (nm_streq(needle, g_array_index(strv, const char *, i))) return i; From f9d601ef069c3f243f09bc75a7deeec978c2acce Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 May 2022 13:11:16 +0200 Subject: [PATCH 07/27] device: initialize full v4/v6 union of NMDhcpClientConfig in _dev_ipdhcpx_start() I think the previous was technically correct in any case too. Still change it, because I feel with union and struct initialization, we should always explicitly pick one union member that we fully initialize. --- src/core/devices/nm-device.c | 40 +++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 0b19d32601..7c77c9f39a 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -10296,7 +10296,10 @@ _dev_ipdhcpx_start(NMDevice *self, int addr_family) .vendor_class_identifier = vendor_class_identifier, .use_fqdn = hostname_is_fqdn, .reject_servers = reject_servers, - .v4.request_broadcast = request_broadcast, + .v4 = + { + .request_broadcast = request_broadcast, + }, }; priv->ipdhcp_data_4.client = @@ -10311,22 +10314,25 @@ _dev_ipdhcpx_start(NMDevice *self, int addr_family) duid = _prop_get_ipv6_dhcp_duid(self, connection, hwaddr, &enforce_duid); config = (NMDhcpClientConfig){ - .addr_family = AF_INET6, - .l3cfg = nm_device_get_l3cfg(self), - .iface = nm_device_get_ip_iface(self), - .uuid = nm_connection_get_uuid(connection), - .send_hostname = nm_setting_ip_config_get_dhcp_send_hostname(s_ip), - .hostname = nm_setting_ip_config_get_dhcp_hostname(s_ip), - .hostname_flags = _prop_get_ipvx_dhcp_hostname_flags(self, AF_INET6), - .client_id = duid, - .mud_url = _prop_get_connection_mud_url(self, s_con), - .timeout = no_lease_timeout_sec, - .anycast_address = _device_get_dhcp_anycast_address(self), - .v6.enforce_duid = enforce_duid, - .v6.iaid = iaid, - .v6.iaid_explicit = iaid_explicit, - .v6.info_only = (priv->ipdhcp_data_6.v6.mode == NM_NDISC_DHCP_LEVEL_OTHERCONF), - .v6.needed_prefixes = priv->ipdhcp_data_6.v6.needed_prefixes, + .addr_family = AF_INET6, + .l3cfg = nm_device_get_l3cfg(self), + .iface = nm_device_get_ip_iface(self), + .uuid = nm_connection_get_uuid(connection), + .send_hostname = nm_setting_ip_config_get_dhcp_send_hostname(s_ip), + .hostname = nm_setting_ip_config_get_dhcp_hostname(s_ip), + .hostname_flags = _prop_get_ipvx_dhcp_hostname_flags(self, AF_INET6), + .client_id = duid, + .mud_url = _prop_get_connection_mud_url(self, s_con), + .timeout = no_lease_timeout_sec, + .anycast_address = _device_get_dhcp_anycast_address(self), + .v6 = + { + .enforce_duid = enforce_duid, + .iaid = iaid, + .iaid_explicit = iaid_explicit, + .info_only = (priv->ipdhcp_data_6.v6.mode == NM_NDISC_DHCP_LEVEL_OTHERCONF), + .needed_prefixes = priv->ipdhcp_data_6.v6.needed_prefixes, + }, }; priv->ipdhcp_data_6.client = From d60ba91c87d198dd40dea33915d7887d8da28a18 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 19 May 2022 09:23:29 +0200 Subject: [PATCH 08/27] core: move NM_ACD_TIMEOUT_MAX_MSEC define to "nm-l3cfg.h" header for reuse --- src/core/nm-l3cfg.c | 9 ++++----- src/core/nm-l3cfg.h | 1 + 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/core/nm-l3cfg.c b/src/core/nm-l3cfg.c index 38b9d8224e..dff6959215 100644 --- a/src/core/nm-l3cfg.c +++ b/src/core/nm-l3cfg.c @@ -37,7 +37,6 @@ G_STATIC_ASSERT(NM_ACD_TIMEOUT_RFC5227_MSEC == N_ACD_TIMEOUT_RFC5227); #define ACD_ENSURE_RATELIMIT_MSEC ((guint32) 4000u) #define ACD_WAIT_PROBING_EXTRA_TIME_MSEC ((guint32) (1000u + ACD_ENSURE_RATELIMIT_MSEC)) #define ACD_WAIT_PROBING_EXTRA_TIME2_MSEC ((guint32) 1000u) -#define ACD_MAX_TIMEOUT_MSEC ((guint32) 30000u) #define ACD_WAIT_TIME_PROBING_FULL_RESTART_MSEC ((guint32) 30000u) #define ACD_WAIT_TIME_CONFLICT_RESTART_MSEC ((guint32) 120000u) #define ACD_WAIT_TIME_ANNOUNCE_RESTART_MSEC ((guint32) 30000u) @@ -1866,10 +1865,10 @@ _l3_acd_data_add(NML3Cfg *self, acd_data = _l3_acd_data_find(self, addr); - if (acd_timeout_msec > ACD_MAX_TIMEOUT_MSEC) { + if (acd_timeout_msec > NM_ACD_TIMEOUT_MAX_MSEC) { /* we limit the maximum timeout. Otherwise we have to handle integer overflow * when adding timeouts. */ - acd_timeout_msec = ACD_MAX_TIMEOUT_MSEC; + acd_timeout_msec = NM_ACD_TIMEOUT_MAX_MSEC; } if (!acd_data) { @@ -3229,8 +3228,8 @@ nm_l3cfg_add_config(NML3Cfg *self, nm_assert(l3cd); nm_assert(nm_l3_config_data_get_ifindex(l3cd) == self->priv.ifindex); - if (acd_timeout_msec > ACD_MAX_TIMEOUT_MSEC) - acd_timeout_msec = ACD_MAX_TIMEOUT_MSEC; + if (acd_timeout_msec > NM_ACD_TIMEOUT_MAX_MSEC) + acd_timeout_msec = NM_ACD_TIMEOUT_MAX_MSEC; nm_assert(NM_IN_SET(acd_defend_type, NM_L3_ACD_DEFEND_TYPE_NEVER, diff --git a/src/core/nm-l3cfg.h b/src/core/nm-l3cfg.h index f6ec39ced8..63ccd5a141 100644 --- a/src/core/nm-l3cfg.h +++ b/src/core/nm-l3cfg.h @@ -10,6 +10,7 @@ #define NM_L3CFG_CONFIG_PRIORITY_IPV6LL 1 #define NM_L3CFG_CONFIG_PRIORITY_VPN 9 #define NM_ACD_TIMEOUT_RFC5227_MSEC 9000u +#define NM_ACD_TIMEOUT_MAX_MSEC 30000u #define NM_TYPE_L3CFG (nm_l3cfg_get_type()) #define NM_L3CFG(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), NM_TYPE_L3CFG, NML3Cfg)) From cd09f3d364ed8810640ddab8a93b46be71c41b4e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 17 May 2022 19:03:09 +0200 Subject: [PATCH 09/27] dhcp: fix logging of event in _nm_dhcp_client_notify() --- src/core/dhcp/nm-dhcp-client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index b1e1f9ab3a..0c091e61ff 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -320,7 +320,7 @@ _nm_dhcp_client_notify(NMDhcpClient *self, _LOGT("notify: event=%s%s%s", nm_dhcp_client_event_type_to_string(client_event_type), - NM_PRINT_FMT_QUOTED2(l3cd, "", ", l3cd=", NM_HASH_OBFUSCATE_PTR_STR(l3cd, sbuf1))); + NM_PRINT_FMT_QUOTED2(l3cd, ", l3cd=", NM_HASH_OBFUSCATE_PTR_STR(l3cd, sbuf1), "")); if (l3cd) nm_l3_config_data_seal(l3cd); From 05cc160494206b8d8257692fddc2e514a655c674 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 17 May 2022 13:28:02 +0200 Subject: [PATCH 10/27] dhcp/trivial: drop obsolete code comment This is done already. --- src/core/dhcp/nm-dhcp-client.h | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.h b/src/core/dhcp/nm-dhcp-client.h index 274a954b6c..3479534de8 100644 --- a/src/core/dhcp/nm-dhcp-client.h +++ b/src/core/dhcp/nm-dhcp-client.h @@ -84,15 +84,6 @@ typedef struct { const char *nm_dhcp_client_event_type_to_string(NMDhcpClientEventType client_event_type); -/* FIXME(l3cfg:dhcp:config): nm_dhcp_manager_start_ip[46]() has a gazillion of parameters, - * those get passed on as CONSTRUCT_ONLY properties to the NMDhcpClient. Drop - * all these parameters, and let the caller provide one NMDhcpClientConfig - * instance. There will be only one GObject property (NM_DHCP_CLIENT_CONFIG), - * which is CONSTRUCT_ONLY and takes a (mandatory) G_TYPE_POINTER for the - * configuration. - * - * Since NMDhcpClientConfig has an addr_family, we also don't need separate - * nm_dhcp_manager_start_ip[46]() methods. */ typedef struct { int addr_family; From e756533002b299896d3b7f742169c7b1690e9baf Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 May 2022 13:29:28 +0200 Subject: [PATCH 11/27] dhcp: move addr-family specific data to union in NMDhcpClientPrivate --- src/core/dhcp/nm-dhcp-client.c | 41 +++++++++++++++++++++++++++------- src/core/dhcp/nm-dhcp-client.h | 5 +++-- 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 0c091e61ff..0fb69b711b 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -45,17 +45,26 @@ typedef struct _NMDhcpClientPrivate { NMDhcpClientConfig config; const NML3ConfigData *l3cd; GSource *no_lease_timeout_source; - GSource *ipv6_lladdr_timeout_source; GSource *watch_source; GBytes *effective_client_id; - pid_t pid; - bool iaid_explicit : 1; - bool is_stopped : 1; + + union { + struct { + int _unused; + } v4; + struct { + GSource *lladdr_timeout_source; + } v6; + }; + struct { gulong id; bool wait_dhcp_commit : 1; bool wait_ll_address : 1; } l3cfg_notify; + + pid_t pid; + bool is_stopped : 1; } NMDhcpClientPrivate; G_DEFINE_ABSTRACT_TYPE(NMDhcpClient, nm_dhcp_client, G_TYPE_OBJECT) @@ -528,7 +537,7 @@ ipv6_lladdr_timeout(gpointer user_data) NMDhcpClient *self = user_data; NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - nm_clear_g_source_inst(&priv->ipv6_lladdr_timeout_source); + nm_clear_g_source_inst(&priv->v6.lladdr_timeout_source); _emit_notify( self, @@ -548,6 +557,8 @@ ipv6_lladdr_find(NMDhcpClient *self) NMDedupMultiIter iter; const NMPObject *obj; + nm_assert(!NM_IS_IPv4(priv->config.addr_family)); + l3cfg = priv->config.l3cfg; nmp_lookup_init_object(&lookup, NMP_OBJECT_TYPE_IP6_ADDRESS, nm_l3cfg_get_ifindex(l3cfg)); @@ -585,7 +596,7 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp _LOGD("got IPv6LL address, starting transaction"); priv->l3cfg_notify.wait_ll_address = FALSE; connect_l3cfg_notify(self); - nm_clear_g_source_inst(&priv->ipv6_lladdr_timeout_source); + nm_clear_g_source_inst(&priv->v6.lladdr_timeout_source); _no_lease_timeout_schedule(self); @@ -697,7 +708,7 @@ nm_dhcp_client_start(NMDhcpClient *self, GError **error) _LOGD("waiting for IPv6LL address"); priv->l3cfg_notify.wait_ll_address = TRUE; connect_l3cfg_notify(self); - priv->ipv6_lladdr_timeout_source = + priv->v6.lladdr_timeout_source = nm_g_timeout_add_seconds_source(10, ipv6_lladdr_timeout, self); return TRUE; } @@ -1070,6 +1081,7 @@ config_init(NMDhcpClientConfig *config, const NMDhcpClientConfig *src) nm_assert(config); nm_assert(src); nm_assert(config != src); + nm_assert_addr_family(src->addr_family); *config = *src; @@ -1165,6 +1177,18 @@ set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *ps case PROP_CONFIG: /* construct-only */ config_init(&priv->config, g_value_get_pointer(value)); + + /* I know, this is technically not necessary. It just feels nicer to + * explicitly initialize the respective union member. */ + if (NM_IS_IPv4(priv->config.addr_family)) { + priv->v4 = (typeof(priv->v4)){ + ._unused = 0, + }; + } else { + priv->v6 = (typeof(priv->v6)){ + .lladdr_timeout_source = NULL, + }; + } break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); @@ -1196,7 +1220,8 @@ dispose(GObject *object) watch_cleanup(self); nm_clear_g_source_inst(&priv->no_lease_timeout_source); - nm_clear_g_source_inst(&priv->ipv6_lladdr_timeout_source); + if (!NM_IS_IPv4(priv->config.addr_family)) + nm_clear_g_source_inst(&priv->v6.lladdr_timeout_source); nm_clear_pointer(&priv->effective_client_id, g_bytes_unref); nm_assert(!priv->watch_source); diff --git a/src/core/dhcp/nm-dhcp-client.h b/src/core/dhcp/nm-dhcp-client.h index 3479534de8..ef735d7df2 100644 --- a/src/core/dhcp/nm-dhcp-client.h +++ b/src/core/dhcp/nm-dhcp-client.h @@ -147,12 +147,13 @@ typedef struct { union { struct { + /* The address from the previous lease */ + const char *last_address; + /* Set BOOTP broadcast flag in request packets, so that servers * will always broadcast replies. */ bool request_broadcast : 1; - /* The address from the previous lease */ - const char *last_address; } v4; struct { /* If set, the DUID from the connection is used; otherwise From 7db07faa5e31c077dd9a817cac52cc1e04a6e28b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 16 May 2022 21:41:33 +0200 Subject: [PATCH 12/27] dhcp: replace switch in l3_cfg_notify_cb() with if blocks The l3_cfg_notify_cb() handler is used for different purposes, and different events will be considered. Usually a switch statement is very nice for enums, especially if all enum values should be handled (because the compiler can warn about unhandled cases). In this case, not all events are supposed to be handled. At this point, it seems nicer to just use an if block. It better composes. The compiler should be able to optimize both variants to the same result. In any case, checking some integers for equality is in any case going to be efficient. --- src/core/dhcp/nm-dhcp-client.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 0fb69b711b..e5abbfa721 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -582,15 +582,11 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp nm_assert(l3cfg == priv->config.l3cfg); - switch (notify_data->notify_type) { - case NM_L3_CONFIG_NOTIFY_TYPE_PLATFORM_CHANGE_ON_IDLE: - { + if (notify_data->notify_type == NM_L3_CONFIG_NOTIFY_TYPE_PLATFORM_CHANGE_ON_IDLE + && priv->l3cfg_notify.wait_ll_address) { const NMPlatformIP6Address *addr; gs_free_error GError *error = NULL; - if (!priv->l3cfg_notify.wait_ll_address) - return; - addr = ipv6_lladdr_find(self); if (addr) { _LOGD("got IPv6LL address, starting transaction"); @@ -608,11 +604,10 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp })); } } - - break; } - case NM_L3_CONFIG_NOTIFY_TYPE_POST_COMMIT: - { + + if (notify_data->notify_type == NM_L3_CONFIG_NOTIFY_TYPE_POST_COMMIT + && priv->l3cfg_notify.wait_dhcp_commit) { const NML3ConfigData *committed_l3cd; NMDedupMultiIter ipconf_iter; const NMPlatformIPAddress *lease_address; @@ -623,9 +618,6 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp * configured. If the address was added, we can proceed accepting the * lease and notifying NMDevice. */ - if (!priv->l3cfg_notify.wait_dhcp_commit) - return; - nm_l3_config_data_iter_ip_address_for_each (&ipconf_iter, priv->l3cd, priv->config.addr_family, @@ -641,12 +633,12 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp address4->address, address4->plen, address4->peer_address)) - return; + goto wait_dhcp_commit_done; } else { const NMPlatformIP6Address *address6 = (const NMPlatformIP6Address *) lease_address; if (!nm_l3_config_data_lookup_address_6(committed_l3cd, &address6->address)) - return; + goto wait_dhcp_commit_done; } priv->l3cfg_notify.wait_dhcp_commit = FALSE; @@ -662,7 +654,7 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp .notify_type = NM_DHCP_CLIENT_NOTIFY_TYPE_IT_LOOKS_BAD, .it_looks_bad.reason = reason, })); - return; + goto wait_dhcp_commit_done; } _emit_notify( @@ -672,11 +664,8 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp .l3cd = priv->l3cd, .accepted = TRUE, }})); - break; - }; - default: - /* ignore */; } +wait_dhcp_commit_done: } gboolean From 9abcf3a53cce71fb9c1150750b301598c9a7413f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 17 May 2022 11:23:19 +0200 Subject: [PATCH 13/27] dhcp/trivial: rename connect_l3cfg_notify() to l3_cfg_notify_check_connected() The function subscribes a callback l3_cfg_notify_cb(). Rename so that related functions have a clearly related name. --- src/core/dhcp/nm-dhcp-client.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index e5abbfa721..1d92ef06f9 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -183,7 +183,7 @@ _emit_notify(NMDhcpClient *self, const NMDhcpClientNotifyData *notify_data) /*****************************************************************************/ static void -connect_l3cfg_notify(NMDhcpClient *self) +l3_cfg_notify_check_connected(NMDhcpClient *self) { NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); gboolean do_connect; @@ -418,7 +418,7 @@ _nm_dhcp_client_notify(NMDhcpClient *self, } else { priv->l3cfg_notify.wait_dhcp_commit = FALSE; } - connect_l3cfg_notify(self); + l3_cfg_notify_check_connected(self); { const NMDhcpClientNotifyData notify_data = { @@ -591,7 +591,7 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp if (addr) { _LOGD("got IPv6LL address, starting transaction"); priv->l3cfg_notify.wait_ll_address = FALSE; - connect_l3cfg_notify(self); + l3_cfg_notify_check_connected(self); nm_clear_g_source_inst(&priv->v6.lladdr_timeout_source); _no_lease_timeout_schedule(self); @@ -642,7 +642,7 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp } priv->l3cfg_notify.wait_dhcp_commit = FALSE; - connect_l3cfg_notify(self); + l3_cfg_notify_check_connected(self); _LOGD("accept address"); @@ -696,7 +696,7 @@ nm_dhcp_client_start(NMDhcpClient *self, GError **error) if (!addr) { _LOGD("waiting for IPv6LL address"); priv->l3cfg_notify.wait_ll_address = TRUE; - connect_l3cfg_notify(self); + l3_cfg_notify_check_connected(self); priv->v6.lladdr_timeout_source = nm_g_timeout_add_seconds_source(10, ipv6_lladdr_timeout, self); return TRUE; @@ -789,7 +789,7 @@ nm_dhcp_client_stop(NMDhcpClient *self, gboolean release) priv->l3cfg_notify.wait_dhcp_commit = FALSE; priv->l3cfg_notify.wait_ll_address = FALSE; - connect_l3cfg_notify(self); + l3_cfg_notify_check_connected(self); /* Kill the DHCP client */ old_pid = priv->pid; From 479562815c8c226d302af2e627af3e3f67f42d1a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 17 May 2022 13:29:24 +0200 Subject: [PATCH 14/27] dhcp: ensure a valid DHCPv4 lease has an address for dhclient The same check is also for nettools' n-dhcp4 client. It's useful to being able to rely on certain things, like that an DHCPv4 lease always has exactly one address (not equal to 0.0.0.0). --- src/core/dhcp/nm-dhcp-utils.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/core/dhcp/nm-dhcp-utils.c b/src/core/dhcp/nm-dhcp-utils.c index 79310dc5cc..71813594a0 100644 --- a/src/core/dhcp/nm-dhcp-utils.c +++ b/src/core/dhcp/nm-dhcp-utils.c @@ -416,6 +416,8 @@ nm_dhcp_utils_ip4_config_from_options(NMDedupMultiIndex *multi_idx, str = g_hash_table_lookup(options, "ip_address"); if (!str || !nm_utils_parse_inaddr_bin(AF_INET, str, NULL, &addr)) return NULL; + if (addr == INADDR_ANY) + return NULL; _LOG2I(LOGD_DHCP4, iface, " address %s", str); @@ -428,6 +430,7 @@ nm_dhcp_utils_ip4_config_from_options(NMDedupMultiIndex *multi_idx, plen = _nm_utils_ip4_get_default_prefix(addr); _LOG2I(LOGD_DHCP4, iface, " plen %d (default)", plen); } + nm_platform_ip4_address_set_addr(&address, addr, plen); /* Routes: if the server returns classless static routes, we MUST ignore From 1760cea47c5c965cc158f43df3b424ad0d1632aa Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 18 May 2022 15:29:39 +0200 Subject: [PATCH 15/27] dhcp: improve warning logging for dhcp4_event_handle() failure --- src/core/dhcp/nm-dhcp-nettools.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/dhcp/nm-dhcp-nettools.c b/src/core/dhcp/nm-dhcp-nettools.c index b86b889d47..e6549a10c4 100644 --- a/src/core/dhcp/nm-dhcp-nettools.c +++ b/src/core/dhcp/nm-dhcp-nettools.c @@ -910,7 +910,7 @@ dhcp4_event_handle(NMDhcpNettools *self, NDhcp4ClientEvent *event) case N_DHCP4_CLIENT_EVENT_OFFER: r = n_dhcp4_client_lease_get_server_identifier(event->offer.lease, &server_id); if (r) { - _LOGW("selecting lease failed: %d", r); + _LOGW("selecting lease failed: could not get DHCP server identifier (%d)", r); return; } From 31c52545edc2c5759bcc535299e12db934bb11dc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 19 May 2022 12:28:09 +0200 Subject: [PATCH 16/27] dhcp: add and use _NMLOG() macro for "nm-dhcp-manager.c" --- src/core/dhcp/nm-dhcp-manager.c | 59 +++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 18 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-manager.c b/src/core/dhcp/nm-dhcp-manager.c index 9fea166614..cfff23f8d3 100644 --- a/src/core/dhcp/nm-dhcp-manager.c +++ b/src/core/dhcp/nm-dhcp-manager.c @@ -42,6 +42,30 @@ G_DEFINE_TYPE(NMDhcpManager, nm_dhcp_manager, G_TYPE_OBJECT) /*****************************************************************************/ +#undef _NMLOG_ENABLED +#define _NMLOG_ENABLED(level, addr_family) nm_logging_enabled((level), _LOGD_DHCP(addr_family)) + +#define _NMLOG(level, addr_family, ...) \ + G_STMT_START \ + { \ + const int _addr_family = (addr_family); \ + const NMLogLevel _log_level = (level); \ + const NMLogDomain _log_domain = LOGD_DHCP_af(_addr_family); \ + \ + if (nm_logging_enabled(_log_level, _log_domain)) { \ + _nm_log(_log_level, \ + _log_domain, \ + 0, \ + NULL, \ + NULL, \ + "dhcp%s: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ + nm_utils_addr_family_to_str(_addr_family) _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ + } \ + } \ + G_STMT_END + +/*****************************************************************************/ + /* default to installed helper, but can be modified for testing */ const char *nm_dhcp_helper_path = LIBEXECDIR "/nm-dhcp-helper"; @@ -167,11 +191,10 @@ nm_dhcp_manager_start_client(NMDhcpManager *self, NMDhcpClientConfig *config, GE gtype = _client_factory_get_gtype(priv->client_factory, config->addr_family); - nm_log_trace(LOGD_DHCP, - "dhcp%c: creating IPv%c DHCP client of type %s", - nm_utils_addr_family_to_char(config->addr_family), - nm_utils_addr_family_to_char(config->addr_family), - g_type_name(gtype)); + _LOGT(config->addr_family, + "creating IPv%c DHCP client of type %s", + nm_utils_addr_family_to_char(config->addr_family), + g_type_name(gtype)); client = g_object_new(gtype, NM_DHCP_CLIENT_CONFIG, config, NULL); @@ -244,11 +267,11 @@ nm_dhcp_manager_init(NMDhcpManager *self) if (!f) continue; - nm_log_dbg(LOGD_DHCP, - "dhcp-init: enabled DHCP client '%s'%s%s", - f->name, - _client_factory_available(f) ? "" : " (not available)", - f->undocumented ? " (undocumented internal plugin)" : ""); + _LOGD(AF_UNSPEC, + "init: enabled DHCP client '%s'%s%s", + f->name, + _client_factory_available(f) ? "" : " (not available)", + f->undocumented ? " (undocumented internal plugin)" : ""); } /* Client-specific setup */ @@ -261,20 +284,20 @@ nm_dhcp_manager_init(NMDhcpManager *self) if (client) { client_factory = _client_factory_available(_client_factory_find_by_name(client)); if (!client_factory) - nm_log_warn(LOGD_DHCP, "dhcp-init: DHCP client '%s' not available", client); + _LOGW(AF_UNSPEC, "init: DHCP client '%s' not available", client); } if (!client_factory) { client_factory = _client_factory_find_by_name("" NM_CONFIG_DEFAULT_MAIN_DHCP); if (!client_factory) - nm_log_err(LOGD_DHCP, - "dhcp-init: default DHCP client '%s' is not installed", - NM_CONFIG_DEFAULT_MAIN_DHCP); + _LOGE(AF_UNSPEC, + "init: default DHCP client '%s' is not installed", + NM_CONFIG_DEFAULT_MAIN_DHCP); else { client_factory = _client_factory_available(client_factory); if (!client_factory) - nm_log_info(LOGD_DHCP, - "dhcp-init: default DHCP client '%s' is not available", - NM_CONFIG_DEFAULT_MAIN_DHCP); + _LOGI(AF_UNSPEC, + "init: default DHCP client '%s' is not available", + NM_CONFIG_DEFAULT_MAIN_DHCP); } } if (!client_factory) { @@ -287,7 +310,7 @@ nm_dhcp_manager_init(NMDhcpManager *self) g_return_if_fail(client_factory); - nm_log_info(LOGD_DHCP, "dhcp-init: Using DHCP client '%s'", client_factory->name); + _LOGI(AF_UNSPEC, "init: Using DHCP client '%s'", client_factory->name); /* NOTE: currently the DHCP plugin is chosen once at start. It's not * possible to reload that configuration. If that ever becomes possible, From 825bf4943055101de9531cd0b8cdb160d874721d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 May 2022 10:21:54 +0200 Subject: [PATCH 17/27] n-dhcp4: return error when calling accept/decline/select in unexpected state The caller is supposed to call accept/decline/select with the lease that was just announced. Calling it in the wrong state or with the wrong lease is a user error. Return an error when called in the wrong state, so that the user notices they did something wrong. --- src/n-dhcp4/src/n-dhcp4-c-lease.c | 9 +++------ src/n-dhcp4/src/n-dhcp4-c-probe.c | 24 +++++++++--------------- 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/src/n-dhcp4/src/n-dhcp4-c-lease.c b/src/n-dhcp4/src/n-dhcp4-c-lease.c index eaa9627652..a30b455356 100644 --- a/src/n-dhcp4/src/n-dhcp4-c-lease.c +++ b/src/n-dhcp4/src/n-dhcp4-c-lease.c @@ -351,14 +351,13 @@ _c_public_ int n_dhcp4_client_lease_query(NDhcp4ClientLease *lease, uint8_t opti * selected none of the others can be. * * Return: 0 on success, or a negative error code on failure. + * Returns -ENOTRECOVERABLE when called in an unexpected state. */ _c_public_ int n_dhcp4_client_lease_select(NDhcp4ClientLease *lease) { NDhcp4ClientLease *l, *t_l; NDhcp4ClientProbe *probe; int r; - /* XXX error handling, this must be an OFFER */ - if (!lease->probe) return -ENOTRECOVERABLE; if (lease->probe->current_lease) @@ -390,12 +389,11 @@ _c_public_ int n_dhcp4_client_lease_select(NDhcp4ClientLease *lease) { * can be accepted. * * Return: 0 on success, or a negative error code on failure. + * Returns -ENOTRECOVERABLE when called in an unexpected state. */ _c_public_ int n_dhcp4_client_lease_accept(NDhcp4ClientLease *lease) { int r; - /* XXX error handling, this must be an ACK */ - if (!lease->probe) return -ENOTRECOVERABLE; if (lease->probe->current_lease != lease) @@ -421,12 +419,11 @@ _c_public_ int n_dhcp4_client_lease_accept(NDhcp4ClientLease *lease) { * decline. * * Return: 0 on success, or a negative error code on failure. + * Returns -ENOTRECOVERABLE when called in an unexpected state. */ _c_public_ int n_dhcp4_client_lease_decline(NDhcp4ClientLease *lease, const char *error) { int r; - /* XXX: error handling, this must be an ACK */ - if (!lease->probe) return -ENOTRECOVERABLE; if (lease->probe->current_lease != lease) diff --git a/src/n-dhcp4/src/n-dhcp4-c-probe.c b/src/n-dhcp4/src/n-dhcp4-c-probe.c index 283c1693cf..f5d93b394a 100644 --- a/src/n-dhcp4/src/n-dhcp4-c-probe.c +++ b/src/n-dhcp4/src/n-dhcp4-c-probe.c @@ -1046,7 +1046,7 @@ int n_dhcp4_client_probe_transition_select(NDhcp4ClientProbe *probe, NDhcp4Incom probe->state = N_DHCP4_CLIENT_PROBE_STATE_REQUESTING; - break; + return 0; case N_DHCP4_CLIENT_PROBE_STATE_INIT: case N_DHCP4_CLIENT_PROBE_STATE_INIT_REBOOT: case N_DHCP4_CLIENT_PROBE_STATE_REBOOTING: @@ -1057,11 +1057,9 @@ int n_dhcp4_client_probe_transition_select(NDhcp4ClientProbe *probe, NDhcp4Incom case N_DHCP4_CLIENT_PROBE_STATE_REBINDING: case N_DHCP4_CLIENT_PROBE_STATE_EXPIRED: default: - /* ignore */ - break; + /* user called in invalid state. Return error. */ + return -ENOTRECOVERABLE; } - - return 0; } /** @@ -1088,7 +1086,7 @@ int n_dhcp4_client_probe_transition_accept(NDhcp4ClientProbe *probe, NDhcp4Incom n_dhcp4_client_arm_timer(probe->client); - break; + return 0; case N_DHCP4_CLIENT_PROBE_STATE_INIT: case N_DHCP4_CLIENT_PROBE_STATE_INIT_REBOOT: @@ -1100,11 +1098,9 @@ int n_dhcp4_client_probe_transition_accept(NDhcp4ClientProbe *probe, NDhcp4Incom case N_DHCP4_CLIENT_PROBE_STATE_REBINDING: case N_DHCP4_CLIENT_PROBE_STATE_EXPIRED: default: - /* ignore */ - break; + /* user called in invalid state. Return error. */ + return -ENOTRECOVERABLE; } - - return 0; } /** @@ -1128,7 +1124,7 @@ int n_dhcp4_client_probe_transition_decline(NDhcp4ClientProbe *probe, NDhcp4Inco /* XXX: what state to transition to? */ - break; + return 0; case N_DHCP4_CLIENT_PROBE_STATE_INIT: case N_DHCP4_CLIENT_PROBE_STATE_INIT_REBOOT: @@ -1140,11 +1136,9 @@ int n_dhcp4_client_probe_transition_decline(NDhcp4ClientProbe *probe, NDhcp4Inco case N_DHCP4_CLIENT_PROBE_STATE_REBINDING: case N_DHCP4_CLIENT_PROBE_STATE_EXPIRED: default: - /* ignore */ - break; + /* user called in invalid state. Return error. */ + return -ENOTRECOVERABLE; } - - return 0; } /** From f40bbb819f41e536d3af726fadc8c6b705bf7f62 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 May 2022 11:17:15 +0200 Subject: [PATCH 18/27] n-dhcp4: maintain the probe's lease list in "n-dhcp4-c-probe.c" The lease list and the probe's state are strongly related. That is evidenced by the fact that sometimes we check the state and then access probe->current_lease without further checking. The code in "n-dhcp4-c-probe.c" (select_lease, accept, decline) already changes and maintains the state, it should also maintain the lease list. Move the code. --- src/n-dhcp4/src/n-dhcp4-c-lease.c | 39 +++---------------------------- src/n-dhcp4/src/n-dhcp4-c-probe.c | 15 +++++++++--- 2 files changed, 15 insertions(+), 39 deletions(-) diff --git a/src/n-dhcp4/src/n-dhcp4-c-lease.c b/src/n-dhcp4/src/n-dhcp4-c-lease.c index a30b455356..be0d78869b 100644 --- a/src/n-dhcp4/src/n-dhcp4-c-lease.c +++ b/src/n-dhcp4/src/n-dhcp4-c-lease.c @@ -354,28 +354,12 @@ _c_public_ int n_dhcp4_client_lease_query(NDhcp4ClientLease *lease, uint8_t opti * Returns -ENOTRECOVERABLE when called in an unexpected state. */ _c_public_ int n_dhcp4_client_lease_select(NDhcp4ClientLease *lease) { - NDhcp4ClientLease *l, *t_l; - NDhcp4ClientProbe *probe; - int r; - if (!lease->probe) return -ENOTRECOVERABLE; if (lease->probe->current_lease) return -ENOTRECOVERABLE; - r = n_dhcp4_client_probe_transition_select(lease->probe, lease->message, n_dhcp4_gettime(CLOCK_BOOTTIME)); - if (r) - return r; - - /* - * Only one of the offered leases can be selected, so flush the list. - * All offered lease, including this one are now dead. - */ - probe = lease->probe; - c_list_for_each_entry_safe(l, t_l, &probe->lease_list, probe_link) - n_dhcp4_client_lease_unlink(l); - - return 0; + return n_dhcp4_client_probe_transition_select(lease->probe, lease->message, n_dhcp4_gettime(CLOCK_BOOTTIME)); } /** @@ -392,20 +376,12 @@ _c_public_ int n_dhcp4_client_lease_select(NDhcp4ClientLease *lease) { * Returns -ENOTRECOVERABLE when called in an unexpected state. */ _c_public_ int n_dhcp4_client_lease_accept(NDhcp4ClientLease *lease) { - int r; - if (!lease->probe) return -ENOTRECOVERABLE; if (lease->probe->current_lease != lease) return -ENOTRECOVERABLE; - r = n_dhcp4_client_probe_transition_accept(lease->probe, lease->message); - if (r) - return r; - - n_dhcp4_client_lease_unlink(lease); - - return 0; + return n_dhcp4_client_probe_transition_accept(lease->probe, lease->message); } /** @@ -422,19 +398,10 @@ _c_public_ int n_dhcp4_client_lease_accept(NDhcp4ClientLease *lease) { * Returns -ENOTRECOVERABLE when called in an unexpected state. */ _c_public_ int n_dhcp4_client_lease_decline(NDhcp4ClientLease *lease, const char *error) { - int r; - if (!lease->probe) return -ENOTRECOVERABLE; if (lease->probe->current_lease != lease) return -ENOTRECOVERABLE; - r = n_dhcp4_client_probe_transition_decline(lease->probe, lease->message, error, n_dhcp4_gettime(CLOCK_BOOTTIME)); - if (r) - return r; - - lease->probe->current_lease = n_dhcp4_client_lease_unref(lease->probe->current_lease); - n_dhcp4_client_lease_unlink(lease); - - return 0; + return n_dhcp4_client_probe_transition_decline(lease->probe, lease->message, error, n_dhcp4_gettime(CLOCK_BOOTTIME)); } diff --git a/src/n-dhcp4/src/n-dhcp4-c-probe.c b/src/n-dhcp4/src/n-dhcp4-c-probe.c index f5d93b394a..dcd51c6f3e 100644 --- a/src/n-dhcp4/src/n-dhcp4-c-probe.c +++ b/src/n-dhcp4/src/n-dhcp4-c-probe.c @@ -1024,6 +1024,7 @@ static int n_dhcp4_client_probe_transition_nak(NDhcp4ClientProbe *probe) { int n_dhcp4_client_probe_transition_select(NDhcp4ClientProbe *probe, NDhcp4Incoming *offer, uint64_t ns_now) { _c_cleanup_(n_dhcp4_outgoing_freep) NDhcp4Outgoing *request = NULL; + NDhcp4ClientLease *l, *t_l; int r; switch (probe->state) { @@ -1042,10 +1043,15 @@ int n_dhcp4_client_probe_transition_select(NDhcp4ClientProbe *probe, NDhcp4Incom else request = NULL; /* consumed */ - /* XXX: ignore other offers */ - probe->state = N_DHCP4_CLIENT_PROBE_STATE_REQUESTING; + /* + * Only one of the offered leases can be selected, so flush the list. + * All offered lease, including this one are now dead. + */ + c_list_for_each_entry_safe(l, t_l, &probe->lease_list, probe_link) + n_dhcp4_client_lease_unlink(l); + return 0; case N_DHCP4_CLIENT_PROBE_STATE_INIT: case N_DHCP4_CLIENT_PROBE_STATE_INIT_REBOOT: @@ -1083,7 +1089,7 @@ int n_dhcp4_client_probe_transition_accept(NDhcp4ClientProbe *probe, NDhcp4Incom return r; probe->state = N_DHCP4_CLIENT_PROBE_STATE_BOUND; - + n_dhcp4_client_lease_unlink(probe->current_lease); n_dhcp4_client_arm_timer(probe->client); return 0; @@ -1124,6 +1130,9 @@ int n_dhcp4_client_probe_transition_decline(NDhcp4ClientProbe *probe, NDhcp4Inco /* XXX: what state to transition to? */ + n_dhcp4_client_lease_unlink(probe->current_lease); + probe->current_lease = n_dhcp4_client_lease_unref(probe->current_lease); + return 0; case N_DHCP4_CLIENT_PROBE_STATE_INIT: From 65cfece4c5c7f2605c353c70f0973a84af4242f2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 May 2022 11:28:51 +0200 Subject: [PATCH 19/27] n-dhcp4: fix internal state after declining lease Previously, during decline we would clear probe->current_lease, however leave the state at GRANTED. That is a wrong state, and can easily lead to a crash later. For example, on the next timeout we will end up at n_dhcp4_client_dispatch_timer(), then current-lease gets accessed unconditionally: case N_DHCP4_CLIENT_PROBE_STATE_GRANTED: if (ns_now >= probe->current_lease->lifetime) { Instead, return to INIT state and schedule a timer. As suggested by RFC 2131, section 3.1, 5) ([1]). [1] https://datatracker.ietf.org/doc/html/rfc2131#section-3.1 --- src/n-dhcp4/src/n-dhcp4-c-probe.c | 15 +++++++++++++-- src/n-dhcp4/src/n-dhcp4-private.h | 1 + 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/n-dhcp4/src/n-dhcp4-c-probe.c b/src/n-dhcp4/src/n-dhcp4-c-probe.c index dcd51c6f3e..284aa428e9 100644 --- a/src/n-dhcp4/src/n-dhcp4-c-probe.c +++ b/src/n-dhcp4/src/n-dhcp4-c-probe.c @@ -1089,6 +1089,7 @@ int n_dhcp4_client_probe_transition_accept(NDhcp4ClientProbe *probe, NDhcp4Incom return r; probe->state = N_DHCP4_CLIENT_PROBE_STATE_BOUND; + probe->ns_decline_restart_delay = 0; n_dhcp4_client_lease_unlink(probe->current_lease); n_dhcp4_client_arm_timer(probe->client); @@ -1128,11 +1129,21 @@ int n_dhcp4_client_probe_transition_decline(NDhcp4ClientProbe *probe, NDhcp4Inco else request = NULL; /* consumed */ - /* XXX: what state to transition to? */ - n_dhcp4_client_lease_unlink(probe->current_lease); probe->current_lease = n_dhcp4_client_lease_unref(probe->current_lease); + probe->state = N_DHCP4_CLIENT_PROBE_STATE_INIT; + + /* RFC2131, 3.1, 5.) The client SHOULD wait a minimum of ten seconds before restarting + * the configuration process to avoid excessive network traffic in case of looping. + * + * Let's go beyond that, and use an exponential backoff. */ + probe->ns_decline_restart_delay = C_CLAMP(probe->ns_decline_restart_delay * 2u, + UINT64_C(10) * UINT64_C(1000000000), + UINT64_C(300) * UINT64_C(1000000000)); + probe->ns_deferred = n_dhcp4_gettime(CLOCK_BOOTTIME) + probe->ns_decline_restart_delay; + + n_dhcp4_client_arm_timer(probe->client); return 0; case N_DHCP4_CLIENT_PROBE_STATE_INIT: diff --git a/src/n-dhcp4/src/n-dhcp4-private.h b/src/n-dhcp4/src/n-dhcp4-private.h index 858c3d3ab0..6b366884be 100644 --- a/src/n-dhcp4/src/n-dhcp4-private.h +++ b/src/n-dhcp4/src/n-dhcp4-private.h @@ -378,6 +378,7 @@ struct NDhcp4ClientProbe { uint64_t ns_deferred; /* timeout for deferred action */ uint64_t ns_reinit; uint64_t ns_nak_restart_delay; /* restart delay after a nak */ + uint64_t ns_decline_restart_delay; /* restart delay after a decline */ NDhcp4ClientLease *current_lease; /* current lease */ NDhcp4CConnection connection; /* client connection wrapper */ From 4a256092ee5f6c8a00475f040d85df4f1af7fa7e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 17 May 2022 21:46:31 +0200 Subject: [PATCH 20/27] dhcp: move accept/decline function inside "nm-dhcp-client.c" They are no longer used from outside, NMDhcpClient fully handles this. Make them static and internal. Also, decline is currently unused. It will be used soon, with ACD support. --- src/core/dhcp/nm-dhcp-client.c | 24 +++++++++++++++--------- src/core/dhcp/nm-dhcp-client.h | 5 ----- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 1d92ef06f9..0ee71fe961 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -73,6 +73,12 @@ G_DEFINE_ABSTRACT_TYPE(NMDhcpClient, nm_dhcp_client, G_TYPE_OBJECT) /*****************************************************************************/ +static gboolean _dhcp_client_accept(NMDhcpClient *self, GError **error); +static gboolean _dhcp_client_can_accept(NMDhcpClient *self); + +_nm_unused static gboolean +_dhcp_client_decline(NMDhcpClient *self, const char *error_message, GError **error); + static void l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcpClient *self); @@ -397,7 +403,7 @@ _nm_dhcp_client_notify(NMDhcpClient *self, * as a configuration parameter (in NMDhcpClientConfig). When ACD is enabled, * when a new lease gets announced, it must first use NML3Cfg to run ACD on the * interface (the previous lease -- if any -- will still be used at that point). - * If ACD fails, we call nm_dhcp_client_decline() and try to get a different + * If ACD fails, we call _dhcp_client_decline() and try to get a different * lease. * If ACD passes, we need to notify the new lease, and the user (NMDevice) may * then configure the address. We need to watch the configured addresses (in NML3Cfg), @@ -411,7 +417,7 @@ _nm_dhcp_client_notify(NMDhcpClient *self, * as a static address (bypassing ACD), then NML3Cfg is aware of that and signals * immediate success. */ - if (nm_dhcp_client_can_accept(self) && client_event_type == NM_DHCP_CLIENT_EVENT_TYPE_BOUND + if (_dhcp_client_can_accept(self) && client_event_type == NM_DHCP_CLIENT_EVENT_TYPE_BOUND && priv->l3cd && nm_l3_config_data_get_num_addresses(priv->l3cd, priv->config.addr_family) > 0) { priv->l3cfg_notify.wait_dhcp_commit = TRUE; @@ -475,8 +481,8 @@ nm_dhcp_client_stop_watch_child(NMDhcpClient *self, pid_t pid) watch_cleanup(self); } -gboolean -nm_dhcp_client_accept(NMDhcpClient *self, GError **error) +static gboolean +_dhcp_client_accept(NMDhcpClient *self, GError **error) { NMDhcpClientPrivate *priv; @@ -493,8 +499,8 @@ nm_dhcp_client_accept(NMDhcpClient *self, GError **error) return TRUE; } -gboolean -nm_dhcp_client_can_accept(NMDhcpClient *self) +static gboolean +_dhcp_client_can_accept(NMDhcpClient *self) { gboolean can_accept; @@ -507,8 +513,8 @@ nm_dhcp_client_can_accept(NMDhcpClient *self) return can_accept; } -gboolean -nm_dhcp_client_decline(NMDhcpClient *self, const char *error_message, GError **error) +static gboolean +_dhcp_client_decline(NMDhcpClient *self, const char *error_message, GError **error) { NMDhcpClientPrivate *priv; @@ -646,7 +652,7 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp _LOGD("accept address"); - if (!nm_dhcp_client_accept(self, &error)) { + if (!_dhcp_client_accept(self, &error)) { gs_free char *reason = g_strdup_printf("error accepting lease: %s", error->message); _emit_notify(self, diff --git a/src/core/dhcp/nm-dhcp-client.h b/src/core/dhcp/nm-dhcp-client.h index ef735d7df2..e213a9c723 100644 --- a/src/core/dhcp/nm-dhcp-client.h +++ b/src/core/dhcp/nm-dhcp-client.h @@ -241,11 +241,6 @@ nm_dhcp_client_get_lease(NMDhcpClient *self) return NULL; } -gboolean nm_dhcp_client_accept(NMDhcpClient *self, GError **error); -gboolean nm_dhcp_client_can_accept(NMDhcpClient *self); - -gboolean nm_dhcp_client_decline(NMDhcpClient *self, const char *error_message, GError **error); - void nm_dhcp_client_stop(NMDhcpClient *self, gboolean release); /* Backend helpers for subclasses */ From 52a0fe584c3b4115f4ef31ade285f1d18b3af614 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 19 May 2022 18:59:45 +0200 Subject: [PATCH 21/27] dhcp/nettools: better track currently granted lease When we accept/decline a lease, then that only works if we are in state GRANTED. n-dhcp4 API also requires us, to provide the exact lease, that we were announced earlier. As such, we need to make sure that we don't accept/decline in the wrong state. That means, to keep track of what we are doing more carefully. The functions _dhcp_client_accept()/_dhcp_client_decline() now take a l3cd argument, the one that we announced earlier. And we check that it still matches. --- src/core/dhcp/nm-dhcp-client.c | 25 +++++--- src/core/dhcp/nm-dhcp-client.h | 7 ++- src/core/dhcp/nm-dhcp-nettools.c | 98 +++++++++++++++++++++++--------- 3 files changed, 93 insertions(+), 37 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 0ee71fe961..be1a7c8f5b 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -73,11 +73,13 @@ G_DEFINE_ABSTRACT_TYPE(NMDhcpClient, nm_dhcp_client, G_TYPE_OBJECT) /*****************************************************************************/ -static gboolean _dhcp_client_accept(NMDhcpClient *self, GError **error); +static gboolean _dhcp_client_accept(NMDhcpClient *self, const NML3ConfigData *l3cd, GError **error); static gboolean _dhcp_client_can_accept(NMDhcpClient *self); -_nm_unused static gboolean -_dhcp_client_decline(NMDhcpClient *self, const char *error_message, GError **error); +_nm_unused static gboolean _dhcp_client_decline(NMDhcpClient *self, + const NML3ConfigData *l3cd, + const char *error_message, + GError **error); static void l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcpClient *self); @@ -482,18 +484,19 @@ nm_dhcp_client_stop_watch_child(NMDhcpClient *self, pid_t pid) } static gboolean -_dhcp_client_accept(NMDhcpClient *self, GError **error) +_dhcp_client_accept(NMDhcpClient *self, const NML3ConfigData *l3cd, GError **error) { NMDhcpClientPrivate *priv; g_return_val_if_fail(NM_IS_DHCP_CLIENT(self), FALSE); + nm_assert(l3cd); priv = NM_DHCP_CLIENT_GET_PRIVATE(self); g_return_val_if_fail(priv->l3cd, FALSE); if (NM_DHCP_CLIENT_GET_CLASS(self)->accept) { - return NM_DHCP_CLIENT_GET_CLASS(self)->accept(self, error); + return NM_DHCP_CLIENT_GET_CLASS(self)->accept(self, l3cd, error); } return TRUE; @@ -514,18 +517,22 @@ _dhcp_client_can_accept(NMDhcpClient *self) } static gboolean -_dhcp_client_decline(NMDhcpClient *self, const char *error_message, GError **error) +_dhcp_client_decline(NMDhcpClient *self, + const NML3ConfigData *l3cd, + const char *error_message, + GError **error) { NMDhcpClientPrivate *priv; g_return_val_if_fail(NM_IS_DHCP_CLIENT(self), FALSE); + nm_assert(l3cd); priv = NM_DHCP_CLIENT_GET_PRIVATE(self); g_return_val_if_fail(priv->l3cd, FALSE); if (NM_DHCP_CLIENT_GET_CLASS(self)->decline) { - return NM_DHCP_CLIENT_GET_CLASS(self)->decline(self, error_message, error); + return NM_DHCP_CLIENT_GET_CLASS(self)->decline(self, l3cd, error_message, error); } return TRUE; @@ -650,9 +657,9 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp priv->l3cfg_notify.wait_dhcp_commit = FALSE; l3_cfg_notify_check_connected(self); - _LOGD("accept address"); + _LOGD("accept lease"); - if (!_dhcp_client_accept(self, &error)) { + if (!_dhcp_client_accept(self, priv->l3cd, &error)) { gs_free char *reason = g_strdup_printf("error accepting lease: %s", error->message); _emit_notify(self, diff --git a/src/core/dhcp/nm-dhcp-client.h b/src/core/dhcp/nm-dhcp-client.h index e213a9c723..283d3af449 100644 --- a/src/core/dhcp/nm-dhcp-client.h +++ b/src/core/dhcp/nm-dhcp-client.h @@ -200,9 +200,12 @@ typedef struct { gboolean (*ip4_start)(NMDhcpClient *self, GError **error); - gboolean (*accept)(NMDhcpClient *self, GError **error); + gboolean (*accept)(NMDhcpClient *self, const NML3ConfigData *l3cd, GError **error); - gboolean (*decline)(NMDhcpClient *self, const char *error_message, GError **error); + gboolean (*decline)(NMDhcpClient *self, + const NML3ConfigData *l3cd, + const char *error_message, + GError **error); gboolean (*ip6_start)(NMDhcpClient *self, const struct in6_addr *ll_addr, GError **error); diff --git a/src/core/dhcp/nm-dhcp-nettools.c b/src/core/dhcp/nm-dhcp-nettools.c index e6549a10c4..4c44a126be 100644 --- a/src/core/dhcp/nm-dhcp-nettools.c +++ b/src/core/dhcp/nm-dhcp-nettools.c @@ -52,9 +52,14 @@ typedef struct _NMDhcpNettoolsClass NMDhcpNettoolsClass; typedef struct { NDhcp4Client *client; NDhcp4ClientProbe *probe; - NDhcp4ClientLease *lease; - GSource *event_source; - char *lease_file; + + struct { + NDhcp4ClientLease *lease; + const NML3ConfigData *lease_l3cd; + } granted; + + GSource *event_source; + char *lease_file; } NMDhcpNettoolsPrivate; struct _NMDhcpNettools { @@ -864,15 +869,19 @@ lease_save(NMDhcpNettools *self, NDhcp4ClientLease *lease, const char *lease_fil } static void -bound4_handle(NMDhcpNettools *self, NDhcp4ClientLease *lease, gboolean extended) +bound4_handle(NMDhcpNettools *self, guint event, NDhcp4ClientLease *lease) { NMDhcpNettoolsPrivate *priv = NM_DHCP_NETTOOLS_GET_PRIVATE(self); NMDhcpClient *client = NM_DHCP_CLIENT(self); const NMDhcpClientConfig *client_config; nm_auto_unref_l3cd_init NML3ConfigData *l3cd = NULL; - GError *error = NULL; + gs_free_error GError *error = NULL; + + nm_assert(NM_IN_SET(event, N_DHCP4_CLIENT_EVENT_GRANTED, N_DHCP4_CLIENT_EVENT_EXTENDED)); + nm_assert(lease); + + _LOGT("lease available (%s)", (event == N_DHCP4_CLIENT_EVENT_GRANTED) ? "granted" : "extended"); - _LOGT("lease available (%s)", extended ? "extended" : "new"); client_config = nm_dhcp_client_get_config(client); l3cd = lease_to_ip4_config(nm_dhcp_client_get_multi_idx(client), client_config->iface, @@ -881,16 +890,24 @@ bound4_handle(NMDhcpNettools *self, NDhcp4ClientLease *lease, gboolean extended) &error); if (!l3cd) { _LOGW("failure to parse lease: %s", error->message); - g_clear_error(&error); + + if (event == N_DHCP4_CLIENT_EVENT_GRANTED) + n_dhcp4_client_lease_decline(lease, "invalid lease"); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), NM_DHCP_CLIENT_EVENT_TYPE_FAIL, NULL); return; } - lease_save(self, lease, priv->lease_file); + if (event == N_DHCP4_CLIENT_EVENT_GRANTED) { + priv->granted.lease = n_dhcp4_client_lease_ref(lease); + priv->granted.lease_l3cd = nm_l3_config_data_ref(l3cd); + } else + lease_save(self, lease, priv->lease_file); _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), - extended ? NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED - : NM_DHCP_CLIENT_EVENT_TYPE_BOUND, + event == N_DHCP4_CLIENT_EVENT_GRANTED + ? NM_DHCP_CLIENT_EVENT_TYPE_BOUND + : NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED, l3cd); } @@ -906,6 +923,16 @@ dhcp4_event_handle(NMDhcpNettools *self, NDhcp4ClientEvent *event) _LOGT("client event %d", event->event); client_config = nm_dhcp_client_get_config(NM_DHCP_CLIENT(self)); + if (!NM_IN_SET(event->event, N_DHCP4_CLIENT_EVENT_LOG)) { + /* In almost all events (even those that we don't expect below), we clear + * the currently granted lease. That is, because in GRANTED state we + * expect to follow up with accept/decline, and that only works while + * we are still in the same state. Transitioning away to another state + * (on most events) will invalidate that. */ + nm_clear_pointer(&priv->granted.lease, n_dhcp4_client_lease_unref); + nm_clear_l3cd(&priv->granted.lease_l3cd); + } + switch (event->event) { case N_DHCP4_CLIENT_EVENT_OFFER: r = n_dhcp4_client_lease_get_server_identifier(event->offer.lease, &server_id); @@ -934,11 +961,10 @@ dhcp4_event_handle(NMDhcpNettools *self, NDhcp4ClientEvent *event) _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), NM_DHCP_CLIENT_EVENT_TYPE_FAIL, NULL); break; case N_DHCP4_CLIENT_EVENT_GRANTED: - priv->lease = n_dhcp4_client_lease_ref(event->granted.lease); - bound4_handle(self, event->granted.lease, FALSE); + bound4_handle(self, event->event, event->granted.lease); break; case N_DHCP4_CLIENT_EVENT_EXTENDED: - bound4_handle(self, event->extended.lease, TRUE); + bound4_handle(self, event->event, event->extended.lease); break; case N_DHCP4_CLIENT_EVENT_DOWN: /* ignore down events, they are purely informational */ @@ -1098,46 +1124,65 @@ nettools_create(NMDhcpNettools *self, GError **error) } static gboolean -_accept(NMDhcpClient *client, GError **error) +_accept(NMDhcpClient *client, const NML3ConfigData *l3cd, GError **error) { NMDhcpNettools *self = NM_DHCP_NETTOOLS(client); NMDhcpNettoolsPrivate *priv = NM_DHCP_NETTOOLS_GET_PRIVATE(self); int r; - g_return_val_if_fail(priv->lease, FALSE); - _LOGT("accept"); - r = n_dhcp4_client_lease_accept(priv->lease); + g_return_val_if_fail(l3cd, FALSE); + + if (priv->granted.lease_l3cd != l3cd) { + nm_utils_error_set(error, NM_UTILS_ERROR_UNKNOWN, "calling accept in unexpected state"); + return FALSE; + } + + nm_assert(priv->granted.lease); + + r = n_dhcp4_client_lease_accept(priv->granted.lease); + + nm_clear_pointer(&priv->granted.lease, n_dhcp4_client_lease_unref); + nm_clear_l3cd(&priv->granted.lease_l3cd); + if (r) { set_error_nettools(error, r, "failed to accept lease"); return FALSE; } - priv->lease = n_dhcp4_client_lease_unref(priv->lease); - return TRUE; } static gboolean -decline(NMDhcpClient *client, const char *error_message, GError **error) +decline(NMDhcpClient *client, const NML3ConfigData *l3cd, const char *error_message, GError **error) { NMDhcpNettools *self = NM_DHCP_NETTOOLS(client); NMDhcpNettoolsPrivate *priv = NM_DHCP_NETTOOLS_GET_PRIVATE(self); int r; + nm_auto(n_dhcp4_client_lease_unrefp) NDhcp4ClientLease *lease = NULL; - g_return_val_if_fail(priv->lease, FALSE); + _LOGT("decline (%s)", error_message); - _LOGT("dhcp4-client: decline (%s)", error_message); + g_return_val_if_fail(l3cd, FALSE); + + if (priv->granted.lease_l3cd != l3cd) { + nm_utils_error_set(error, NM_UTILS_ERROR_UNKNOWN, "calling decline in unexpected state"); + return FALSE; + } + + nm_assert(priv->granted.lease); + + lease = g_steal_pointer(&priv->granted.lease); + nm_clear_l3cd(&priv->granted.lease_l3cd); + + r = n_dhcp4_client_lease_decline(lease, error_message); - r = n_dhcp4_client_lease_decline(priv->lease, error_message); if (r) { set_error_nettools(error, r, "failed to decline lease"); return FALSE; } - priv->lease = n_dhcp4_client_lease_unref(priv->lease); - return TRUE; } @@ -1348,7 +1393,8 @@ dispose(GObject *object) nm_clear_g_free(&priv->lease_file); nm_clear_g_source_inst(&priv->event_source); - nm_clear_pointer(&priv->lease, n_dhcp4_client_lease_unref); + nm_clear_pointer(&priv->granted.lease, n_dhcp4_client_lease_unref); + nm_clear_l3cd(&priv->granted.lease_l3cd); nm_clear_pointer(&priv->probe, n_dhcp4_client_probe_free); nm_clear_pointer(&priv->client, n_dhcp4_client_unref); From 85b15e02fdd32afbe081266592177b1f86f5dce7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 24 May 2022 12:57:59 +0200 Subject: [PATCH 22/27] dhcp/nettools: cleanup logging for dhcp4_event_handle() It's pretty pointless to log [1653389116.6288] dhcp4 (br0): client event 7 [1653389116.6288] dhcp4 (br0): received OFFER of 192.168.121.110 from 192.168.121.1 where the obscure event #7 is only telling you that we are going to log something. Handle logging events first. In general, drop the "client event %d" message and make sure that all code paths log something (useful), so we can see in the log that the event was reached. --- src/core/dhcp/nm-dhcp-nettools.c | 62 ++++++++++++++++---------------- 1 file changed, 32 insertions(+), 30 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-nettools.c b/src/core/dhcp/nm-dhcp-nettools.c index 4c44a126be..4b580b47ef 100644 --- a/src/core/dhcp/nm-dhcp-nettools.c +++ b/src/core/dhcp/nm-dhcp-nettools.c @@ -914,14 +914,17 @@ bound4_handle(NMDhcpNettools *self, guint event, NDhcp4ClientLease *lease) static void dhcp4_event_handle(NMDhcpNettools *self, NDhcp4ClientEvent *event) { - NMDhcpNettoolsPrivate *priv = NM_DHCP_NETTOOLS_GET_PRIVATE(self); - const NMDhcpClientConfig *client_config; - struct in_addr server_id; - char addr_str[INET_ADDRSTRLEN]; - int r; + NMDhcpNettoolsPrivate *priv = NM_DHCP_NETTOOLS_GET_PRIVATE(self); + struct in_addr server_id; + struct in_addr yiaddr; + char addr_str[INET_ADDRSTRLEN]; + char addr_str2[INET_ADDRSTRLEN]; + int r; - _LOGT("client event %d", event->event); - client_config = nm_dhcp_client_get_config(NM_DHCP_CLIENT(self)); + if (event->event == N_DHCP4_CLIENT_EVENT_LOG) { + _NMLOG(nm_log_level_from_syslog(event->log.level), "event: %s", event->log.message); + return; + } if (!NM_IN_SET(event->event, N_DHCP4_CLIENT_EVENT_LOG)) { /* In almost all events (even those that we don't expect below), we clear @@ -941,52 +944,51 @@ dhcp4_event_handle(NMDhcpNettools *self, NDhcp4ClientEvent *event) return; } + n_dhcp4_client_lease_get_yiaddr(event->offer.lease, &yiaddr); + if (yiaddr.s_addr == INADDR_ANY) { + _LOGD("selecting lease failed: no yiaddr address"); + return; + } + if (nm_dhcp_client_server_id_is_rejected(NM_DHCP_CLIENT(self), &server_id)) { _LOGD("server-id %s is in the reject-list, ignoring", nm_utils_inet_ntop(AF_INET, &server_id, addr_str)); return; } + _LOGT("selecting offered lease from %s for %s", + _nm_utils_inet4_ntop(server_id.s_addr, addr_str), + _nm_utils_inet4_ntop(yiaddr.s_addr, addr_str2)); + r = n_dhcp4_client_lease_select(event->offer.lease); if (r) { _LOGW("selecting lease failed: %d", r); return; } - break; + + return; case N_DHCP4_CLIENT_EVENT_RETRACTED: case N_DHCP4_CLIENT_EVENT_EXPIRED: _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), NM_DHCP_CLIENT_EVENT_TYPE_EXPIRE, NULL); - break; + return; case N_DHCP4_CLIENT_EVENT_CANCELLED: _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), NM_DHCP_CLIENT_EVENT_TYPE_FAIL, NULL); - break; + return; case N_DHCP4_CLIENT_EVENT_GRANTED: bound4_handle(self, event->event, event->granted.lease); - break; + return; case N_DHCP4_CLIENT_EVENT_EXTENDED: bound4_handle(self, event->event, event->extended.lease); - break; + return; case N_DHCP4_CLIENT_EVENT_DOWN: /* ignore down events, they are purely informational */ - break; - case N_DHCP4_CLIENT_EVENT_LOG: - { - NMLogLevel nm_level; - - nm_level = nm_log_level_from_syslog(event->log.level); - if (nm_logging_enabled(nm_level, LOGD_DHCP4)) { - nm_log(nm_level, - LOGD_DHCP4, - NULL, - NULL, - "dhcp4 (%s): %s", - client_config->iface, - event->log.message); - } - } break; + _LOGT("event: down (ignore)"); + return; default: - _LOGW("unhandled DHCP event %d", event->event); - break; + _LOGE("unhandled DHCP event %d", event->event); + nm_assert(event->event != N_DHCP4_CLIENT_EVENT_LOG); + nm_assert_not_reached(); + return; } } From 8f8839dd2abf3789c41abebda71e4577045d3371 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 24 May 2022 15:36:14 +0200 Subject: [PATCH 23/27] dhcp/nettools: add helper function dhcp4_event_pop_all_events() Will be used next. --- src/core/dhcp/nm-dhcp-nettools.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-nettools.c b/src/core/dhcp/nm-dhcp-nettools.c index 4b580b47ef..c012c024d2 100644 --- a/src/core/dhcp/nm-dhcp-nettools.c +++ b/src/core/dhcp/nm-dhcp-nettools.c @@ -992,12 +992,21 @@ dhcp4_event_handle(NMDhcpNettools *self, NDhcp4ClientEvent *event) } } +static void +dhcp4_event_pop_all_events(NMDhcpNettools *self) +{ + NMDhcpNettoolsPrivate *priv = NM_DHCP_NETTOOLS_GET_PRIVATE(self); + NDhcp4ClientEvent *event; + + while (!n_dhcp4_client_pop_event(priv->client, &event) && event) + dhcp4_event_handle(self, event); +} + static gboolean dhcp4_event_cb(int fd, GIOCondition condition, gpointer user_data) { NMDhcpNettools *self = user_data; NMDhcpNettoolsPrivate *priv = NM_DHCP_NETTOOLS_GET_PRIVATE(self); - NDhcp4ClientEvent *event; int r; r = n_dhcp4_client_dispatch(priv->client); @@ -1018,8 +1027,7 @@ dhcp4_event_cb(int fd, GIOCondition condition, gpointer user_data) return G_SOURCE_REMOVE; } - while (!n_dhcp4_client_pop_event(priv->client, &event) && event) - dhcp4_event_handle(self, event); + dhcp4_event_pop_all_events(self); return G_SOURCE_CONTINUE; } From 4f13383460af37dcbbe9b5631ac60a4c63e73a39 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 26 May 2022 16:02:50 +0200 Subject: [PATCH 24/27] dhcp/nettools: pop n-dhcp4 events after select/accept/decline to process logging events --- src/core/dhcp/nm-dhcp-nettools.c | 63 +++++++++++++++++++++++++++++++- 1 file changed, 62 insertions(+), 1 deletion(-) diff --git a/src/core/dhcp/nm-dhcp-nettools.c b/src/core/dhcp/nm-dhcp-nettools.c index c012c024d2..eba4891ef2 100644 --- a/src/core/dhcp/nm-dhcp-nettools.c +++ b/src/core/dhcp/nm-dhcp-nettools.c @@ -58,6 +58,8 @@ typedef struct { const NML3ConfigData *lease_l3cd; } granted; + GSource *pop_all_events_on_idle_source; + GSource *event_source; char *lease_file; } NMDhcpNettoolsPrivate; @@ -78,6 +80,10 @@ G_DEFINE_TYPE(NMDhcpNettools, nm_dhcp_nettools, NM_TYPE_DHCP_CLIENT) /*****************************************************************************/ +static void dhcp4_event_pop_all_events_on_idle(NMDhcpNettools *self); + +/*****************************************************************************/ + static void set_error_nettools(GError **error, int r, const char *message) { @@ -891,8 +897,10 @@ bound4_handle(NMDhcpNettools *self, guint event, NDhcp4ClientLease *lease) if (!l3cd) { _LOGW("failure to parse lease: %s", error->message); - if (event == N_DHCP4_CLIENT_EVENT_GRANTED) + if (event == N_DHCP4_CLIENT_EVENT_GRANTED) { n_dhcp4_client_lease_decline(lease, "invalid lease"); + dhcp4_event_pop_all_events_on_idle(self); + } _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), NM_DHCP_CLIENT_EVENT_TYPE_FAIL, NULL); return; @@ -961,6 +969,9 @@ dhcp4_event_handle(NMDhcpNettools *self, NDhcp4ClientEvent *event) _nm_utils_inet4_ntop(yiaddr.s_addr, addr_str2)); r = n_dhcp4_client_lease_select(event->offer.lease); + + dhcp4_event_pop_all_events_on_idle(self); + if (r) { _LOGW("selecting lease failed: %d", r); return; @@ -1000,6 +1011,51 @@ dhcp4_event_pop_all_events(NMDhcpNettools *self) while (!n_dhcp4_client_pop_event(priv->client, &event) && event) dhcp4_event_handle(self, event); + + nm_clear_g_source_inst(&priv->pop_all_events_on_idle_source); +} + +static gboolean +dhcp4_event_pop_all_events_on_idle_cb(gpointer user_data) +{ + NMDhcpNettools *self = user_data; + NMDhcpNettoolsPrivate *priv = NM_DHCP_NETTOOLS_GET_PRIVATE(self); + + nm_clear_g_source_inst(&priv->pop_all_events_on_idle_source); + dhcp4_event_pop_all_events(self); + return G_SOURCE_CONTINUE; +} + +static void +dhcp4_event_pop_all_events_on_idle(NMDhcpNettools *self) +{ + NMDhcpNettoolsPrivate *priv = NM_DHCP_NETTOOLS_GET_PRIVATE(self); + + /* For the most part, NDhcp4Client gets driven from internal, that is + * by having events ready on the socket or the timerfd. For those + * events, we will poll on the (epoll) FD, then let it be processed + * by n_dhcp4_client_dispatch(), and pop the queued events. + * + * But certain commands (n_dhcp4_client_lease_select(), n_dhcp4_client_lease_accept(), + * n_dhcp4_client_lease_decline()) are initiated by the user. And they tend + * to log events. Logging is done by queuing a message, but that won't be processed, + * unless we pop the event. + * + * To ensure that those logging events get popped, schedule an idle handler to do that. + * + * Yes, this means, that the messages only get logged later, when the idle handler + * runs. The alternative seems even more problematic, because we don't know + * the current call-state, and it seems dangerous to pop unexpected events. + * E.g. we call n_dhcp4_client_lease_select() from inside the event-handler, + * it seems wrong to call dhcp4_event_pop_all_events() in that context again. + * + * See-also: https://github.com/nettools/n-dhcp4/issues/34 + */ + + if (!priv->pop_all_events_on_idle_source) { + priv->pop_all_events_on_idle_source = + nm_g_idle_add_source(dhcp4_event_pop_all_events_on_idle_cb, self); + } } static gboolean @@ -1153,6 +1209,8 @@ _accept(NMDhcpClient *client, const NML3ConfigData *l3cd, GError **error) r = n_dhcp4_client_lease_accept(priv->granted.lease); + dhcp4_event_pop_all_events_on_idle(self); + nm_clear_pointer(&priv->granted.lease, n_dhcp4_client_lease_unref); nm_clear_l3cd(&priv->granted.lease_l3cd); @@ -1188,6 +1246,8 @@ decline(NMDhcpClient *client, const NML3ConfigData *l3cd, const char *error_mess r = n_dhcp4_client_lease_decline(lease, error_message); + dhcp4_event_pop_all_events_on_idle(self); + if (r) { set_error_nettools(error, r, "failed to decline lease"); return FALSE; @@ -1403,6 +1463,7 @@ dispose(GObject *object) nm_clear_g_free(&priv->lease_file); nm_clear_g_source_inst(&priv->event_source); + nm_clear_g_source_inst(&priv->pop_all_events_on_idle_source); nm_clear_pointer(&priv->granted.lease, n_dhcp4_client_lease_unref); nm_clear_l3cd(&priv->granted.lease_l3cd); nm_clear_pointer(&priv->probe, n_dhcp4_client_probe_free); From 0f6df633faf3b1ecd394bda7547b94dfca104bda Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 24 May 2022 09:08:07 +0200 Subject: [PATCH 25/27] dhcp: minor cleanup of accept/decline functions in "nm-dhcp-client.c" - assign the result of NM_DHCP_CLIENT_GET_CLASS() to a local variable. It feels nicer to only call the macro once. Of course, the macro expands to plain pointer dereferences, so there is little difference in terms of executed code. - handle the default case with no virtual function first. --- src/core/dhcp/nm-dhcp-client.c | 35 +++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index be1a7c8f5b..836d40f93f 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -486,32 +486,34 @@ nm_dhcp_client_stop_watch_child(NMDhcpClient *self, pid_t pid) static gboolean _dhcp_client_accept(NMDhcpClient *self, const NML3ConfigData *l3cd, GError **error) { - NMDhcpClientPrivate *priv; + NMDhcpClientClass *klass; g_return_val_if_fail(NM_IS_DHCP_CLIENT(self), FALSE); nm_assert(l3cd); - priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + klass = NM_DHCP_CLIENT_GET_CLASS(self); - g_return_val_if_fail(priv->l3cd, FALSE); + g_return_val_if_fail(NM_DHCP_CLIENT_GET_PRIVATE(self)->l3cd, FALSE); - if (NM_DHCP_CLIENT_GET_CLASS(self)->accept) { - return NM_DHCP_CLIENT_GET_CLASS(self)->accept(self, l3cd, error); - } + if (!klass->accept) + return TRUE; - return TRUE; + return klass->accept(self, l3cd, error); } static gboolean _dhcp_client_can_accept(NMDhcpClient *self) { gboolean can_accept; + NMDhcpClientClass *klass; - g_return_val_if_fail(NM_IS_DHCP_CLIENT(self), FALSE); + nm_assert(NM_IS_DHCP_CLIENT(self)); - can_accept = !!(NM_DHCP_CLIENT_GET_CLASS(self)->accept); + klass = NM_DHCP_CLIENT_GET_CLASS(self); - nm_assert(can_accept == (!!(NM_DHCP_CLIENT_GET_CLASS(self)->decline))); + can_accept = !!klass->accept; + + nm_assert(can_accept == (!!klass->decline)); return can_accept; } @@ -522,20 +524,19 @@ _dhcp_client_decline(NMDhcpClient *self, const char *error_message, GError **error) { - NMDhcpClientPrivate *priv; + NMDhcpClientClass *klass; g_return_val_if_fail(NM_IS_DHCP_CLIENT(self), FALSE); nm_assert(l3cd); - priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + klass = NM_DHCP_CLIENT_GET_CLASS(self); - g_return_val_if_fail(priv->l3cd, FALSE); + g_return_val_if_fail(NM_DHCP_CLIENT_GET_PRIVATE(self)->l3cd, FALSE); - if (NM_DHCP_CLIENT_GET_CLASS(self)->decline) { - return NM_DHCP_CLIENT_GET_CLASS(self)->decline(self, l3cd, error_message, error); - } + if (!klass->decline) + return TRUE; - return TRUE; + return klass->decline(self, l3cd, error_message, error); } static GBytes * From 156d84217cedfbddeb4ce252ddee54ebf0ed2ccf Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 20 May 2022 21:09:24 +0200 Subject: [PATCH 26/27] dhcp/dhclient: implement accept/decline (ACD) for dhclient plugin dhclient itself doesn't do ACD. However, it expects the dhclient-script to exit with non-zero status, which causes dhclient to send a DECLINE. `man dhclient-script`: BOUND: Before actually configuring the address, dhclient-script should somehow ARP for it and exit with a nonzero status if it receives a reply. In this case, the client will send a DHCPDECLINE message to the server and acquire a different address. This may also be done in the RENEW, REBIND, or REBOOT states, but is not required, and indeed may not be desirable. See also Fedora's dhclient-script ([1]). https://gitlab.isc.org/isc-projects/dhcp/-/issues/67#note_97226 https://gitlab.isc.org/isc-projects/dhcp/-/blob/33226f2d76b6b7a06df6b76abbb3526100f5ae2d/client/dhclient.c#L1652 [1] https://src.fedoraproject.org/rpms/dhcp/blob/a8f6fd046fe5ff39631ab8da4ebad00f96fd4f15/f/dhclient-script#_878 https://bugzilla.redhat.com/show_bug.cgi?id=1713380 --- src/core/dhcp/nm-dhcp-client.c | 103 ++++++++++++++++++++++--------- src/core/dhcp/nm-dhcp-client.h | 13 ++-- src/core/dhcp/nm-dhcp-helper.c | 19 +----- src/core/dhcp/nm-dhcp-listener.c | 22 ++++--- 4 files changed, 95 insertions(+), 62 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 836d40f93f..d6cfbf3f71 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -50,7 +50,9 @@ typedef struct _NMDhcpClientPrivate { union { struct { - int _unused; + struct { + GDBusMethodInvocation *invocation; + } bound; } v4; struct { GSource *lladdr_timeout_source; @@ -74,7 +76,6 @@ G_DEFINE_ABSTRACT_TYPE(NMDhcpClient, nm_dhcp_client, G_TYPE_OBJECT) /*****************************************************************************/ static gboolean _dhcp_client_accept(NMDhcpClient *self, const NML3ConfigData *l3cd, GError **error); -static gboolean _dhcp_client_can_accept(NMDhcpClient *self); _nm_unused static gboolean _dhcp_client_decline(NMDhcpClient *self, const NML3ConfigData *l3cd, @@ -419,8 +420,7 @@ _nm_dhcp_client_notify(NMDhcpClient *self, * as a static address (bypassing ACD), then NML3Cfg is aware of that and signals * immediate success. */ - if (_dhcp_client_can_accept(self) && client_event_type == NM_DHCP_CLIENT_EVENT_TYPE_BOUND - && priv->l3cd + if (client_event_type == NM_DHCP_CLIENT_EVENT_TYPE_BOUND && priv->l3cd && nm_l3_config_data_get_num_addresses(priv->l3cd, priv->config.addr_family) > 0) { priv->l3cfg_notify.wait_dhcp_commit = TRUE; } else { @@ -483,6 +483,25 @@ nm_dhcp_client_stop_watch_child(NMDhcpClient *self, pid_t pid) watch_cleanup(self); } +static gboolean +_accept(NMDhcpClient *self, const NML3ConfigData *l3cd, GError **error) +{ + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + + if (!NM_IS_IPv4(priv->config.addr_family)) + return TRUE; + + if (!priv->v4.bound.invocation) { + nm_utils_error_set(error, + NM_UTILS_ERROR_UNKNOWN, + "calling accept in unexpected script state"); + return FALSE; + } + + g_dbus_method_invocation_return_value(g_steal_pointer(&priv->v4.bound.invocation), NULL); + return TRUE; +} + static gboolean _dhcp_client_accept(NMDhcpClient *self, const NML3ConfigData *l3cd, GError **error) { @@ -495,27 +514,29 @@ _dhcp_client_accept(NMDhcpClient *self, const NML3ConfigData *l3cd, GError **err g_return_val_if_fail(NM_DHCP_CLIENT_GET_PRIVATE(self)->l3cd, FALSE); - if (!klass->accept) - return TRUE; - return klass->accept(self, l3cd, error); } static gboolean -_dhcp_client_can_accept(NMDhcpClient *self) +decline(NMDhcpClient *self, const NML3ConfigData *l3cd, const char *error_message, GError **error) { - gboolean can_accept; - NMDhcpClientClass *klass; + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - nm_assert(NM_IS_DHCP_CLIENT(self)); + if (!NM_IS_IPv4(priv->config.addr_family)) + return TRUE; - klass = NM_DHCP_CLIENT_GET_CLASS(self); + if (!priv->v4.bound.invocation) { + nm_utils_error_set(error, + NM_UTILS_ERROR_UNKNOWN, + "calling decline in unexpected script state"); + return FALSE; + } - can_accept = !!klass->accept; - - nm_assert(can_accept == (!!klass->decline)); - - return can_accept; + g_dbus_method_invocation_return_error(g_steal_pointer(&priv->v4.bound.invocation), + NM_DEVICE_ERROR, + NM_DEVICE_ERROR_FAILED, + "acd failed"); + return TRUE; } static gboolean @@ -533,9 +554,6 @@ _dhcp_client_decline(NMDhcpClient *self, g_return_val_if_fail(NM_DHCP_CLIENT_GET_PRIVATE(self)->l3cd, FALSE); - if (!klass->decline) - return TRUE; - return klass->decline(self, l3cd, error_message, error); } @@ -801,6 +819,13 @@ 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), + NM_DEVICE_ERROR, + NM_DEVICE_ERROR_FAILED, + "dhcp stopping"); + } + priv->l3cfg_notify.wait_dhcp_commit = FALSE; priv->l3cfg_notify.wait_ll_address = FALSE; l3_cfg_notify_check_connected(self); @@ -934,12 +959,13 @@ nm_dhcp_client_emit_ipv6_prefix_delegated(NMDhcpClient *self, const NMPlatformIP } gboolean -nm_dhcp_client_handle_event(gpointer unused, - const char *iface, - int pid, - GVariant *options, - const char *reason, - NMDhcpClient *self) +nm_dhcp_client_handle_event(gpointer unused, + const char *iface, + int pid, + GVariant *options, + const char *reason, + GDBusMethodInvocation *invocation, + NMDhcpClient *self) { NMDhcpClientPrivate *priv; nm_auto_unref_l3cd_init NML3ConfigData *l3cd = NULL; @@ -953,6 +979,7 @@ nm_dhcp_client_handle_event(gpointer unused, g_return_val_if_fail(pid > 0, FALSE); g_return_val_if_fail(g_variant_is_of_type(options, G_VARIANT_TYPE_VARDICT), FALSE); g_return_val_if_fail(reason != NULL, FALSE); + g_return_val_if_fail(G_IS_DBUS_METHOD_INVOCATION(invocation), FALSE); priv = NM_DHCP_CLIENT_GET_PRIVATE(self); @@ -964,7 +991,7 @@ nm_dhcp_client_handle_event(gpointer unused, _LOGD("DHCP event (reason: '%s')", reason); if (NM_IN_STRSET_ASCII_CASE(reason, "preinit")) - return TRUE; + goto out_handled; if (NM_IN_STRSET_ASCII_CASE(reason, "bound", "bound6", "static")) client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_BOUND; @@ -1028,7 +1055,7 @@ nm_dhcp_client_handle_event(gpointer unused, * of the DHCP client instance. Instead, we just signal the prefix * to the device. */ nm_dhcp_client_emit_ipv6_prefix_delegated(self, &prefix); - return TRUE; + goto out_handled; } if (NM_IN_SET(client_event_type, @@ -1040,7 +1067,20 @@ nm_dhcp_client_handle_event(gpointer unused, client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_FAIL; } + if (priv->v4.bound.invocation) + g_dbus_method_invocation_return_value(g_steal_pointer(&priv->v4.bound.invocation), NULL); + + if (NM_IS_IPv4(priv->config.addr_family) + && 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); + _nm_dhcp_client_notify(self, client_event_type, l3cd); + +out_handled: + if (invocation) + g_dbus_method_invocation_return_value(invocation, NULL); return TRUE; } @@ -1185,7 +1225,10 @@ 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)){ - ._unused = 0, + .bound = + { + .invocation = NULL, + }, }; } else { priv->v6 = (typeof(priv->v6)){ @@ -1255,6 +1298,8 @@ nm_dhcp_client_class_init(NMDhcpClientClass *client_class) object_class->dispose = dispose; object_class->finalize = finalize; object_class->set_property = set_property; + client_class->accept = _accept; + client_class->decline = decline; client_class->stop = stop; client_class->get_duid = get_duid; diff --git a/src/core/dhcp/nm-dhcp-client.h b/src/core/dhcp/nm-dhcp-client.h index 283d3af449..e4b9992922 100644 --- a/src/core/dhcp/nm-dhcp-client.h +++ b/src/core/dhcp/nm-dhcp-client.h @@ -261,12 +261,13 @@ void _nm_dhcp_client_notify(NMDhcpClient *self, NMDhcpClientEventType client_event_type, const NML3ConfigData *l3cd); -gboolean nm_dhcp_client_handle_event(gpointer unused, - const char *iface, - int pid, - GVariant *options, - const char *reason, - NMDhcpClient *self); +gboolean nm_dhcp_client_handle_event(gpointer unused, + const char *iface, + int pid, + GVariant *options, + const char *reason, + GDBusMethodInvocation *invocation, + NMDhcpClient *self); void nm_dhcp_client_emit_ipv6_prefix_delegated(NMDhcpClient *self, const NMPlatformIP6Address *prefix); diff --git a/src/core/dhcp/nm-dhcp-helper.c b/src/core/dhcp/nm-dhcp-helper.c index 41862f2b68..aab658a2aa 100644 --- a/src/core/dhcp/nm-dhcp-helper.c +++ b/src/core/dhcp/nm-dhcp-helper.c @@ -100,21 +100,6 @@ next:; return g_variant_ref_sink(g_variant_new("(a{sv})", &builder)); } -static void -kill_pid(void) -{ - const char *pid_str; - pid_t pid = 0; - - pid_str = getenv("pid"); - if (pid_str) - pid = strtol(pid_str, NULL, 10); - if (pid) { - _LOGI("a fatal error occurred, kill dhclient instance with pid %d", pid); - kill(pid, SIGTERM); - } -} - int main(int argc, char *argv[]) { @@ -180,7 +165,7 @@ do_notify: parameters, NULL, G_DBUS_CALL_FLAGS_NONE, - 1000, + 60000, NULL, &error); @@ -236,7 +221,5 @@ do_notify: } out: - if (!success) - kill_pid(); return success ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/src/core/dhcp/nm-dhcp-listener.c b/src/core/dhcp/nm-dhcp-listener.c index 2c567593b5..0854c1dcbe 100644 --- a/src/core/dhcp/nm-dhcp-listener.c +++ b/src/core/dhcp/nm-dhcp-listener.c @@ -128,7 +128,7 @@ get_option(GVariant *options, const char *key) } static void -_method_call_handle(NMDhcpListener *self, GVariant *parameters) +_method_call_handle(NMDhcpListener *self, GDBusMethodInvocation *invocation, GVariant *parameters) { gs_free char *iface = NULL; gs_free char *pid_str = NULL; @@ -142,23 +142,23 @@ _method_call_handle(NMDhcpListener *self, GVariant *parameters) iface = get_option(options, "interface"); if (iface == NULL) { _LOGW("dhcp-event: didn't have associated interface."); - return; + goto out; } pid_str = get_option(options, "pid"); pid = _nm_utils_ascii_str_to_int64(pid_str, 10, 0, G_MAXINT32, -1); if (pid == -1) { _LOGW("dhcp-event: couldn't convert PID '%s' to an integer", pid_str ?: "(null)"); - return; + goto out; } reason = get_option(options, "reason"); if (reason == NULL) { _LOGW("dhcp-event: (pid %d) DHCP event didn't have a reason", pid); - return; + goto out; } - g_signal_emit(self, signals[EVENT], 0, iface, pid, options, reason, &handled); + g_signal_emit(self, signals[EVENT], 0, iface, pid, options, reason, invocation, &handled); if (!handled) { if (g_ascii_strcasecmp(reason, "RELEASE") == 0) { /* Ignore event when the dhcp client gets killed and we receive its last message */ @@ -166,6 +166,10 @@ _method_call_handle(NMDhcpListener *self, GVariant *parameters) } else _LOGW("dhcp-event: (pid %d) unhandled DHCP event for interface %s", pid, iface); } + +out: + if (!handled) + g_dbus_method_invocation_return_value(invocation, NULL); } static void @@ -190,8 +194,7 @@ _method_call(GDBusConnection *connection, return; } - _method_call_handle(self, parameters); - g_dbus_method_invocation_return_value(invocation, NULL); + _method_call_handle(self, invocation, parameters); } static GDBusInterfaceInfo *const interface_info = NM_DEFINE_GDBUS_INTERFACE_INFO( @@ -311,9 +314,10 @@ nm_dhcp_listener_class_init(NMDhcpListenerClass *listener_class) NULL, NULL, G_TYPE_BOOLEAN, /* listeners return TRUE if handled */ - 4, + 5, G_TYPE_STRING, /* iface */ G_TYPE_INT, /* pid */ G_TYPE_VARIANT, /* options */ - G_TYPE_STRING); /* reason */ + G_TYPE_STRING, /* reason */ + G_TYPE_DBUS_METHOD_INVOCATION /* invocation*/); } From 240ec7f89119b7929808eb31030ee7fe4d6b24da Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 17 May 2022 13:55:08 +0200 Subject: [PATCH 27/27] dhcp: implement ACD (address collision detection) for DHCPv4 This was working for internal plugin in the past, but broken by l3cfg rework with 1.36. Re-add it. Not it also works with dhclient. For other plugins, it's not really working, because we can't decline. Now NMDhcpClient does ACD (using NML3Cfg) and abstracts that from the caller (NMDevice). It is complicated. Because there is state involved, meaning, we need to remember the current state for ACD and react on and handle a multitude of events. Getting this right, is non-trivial. What we want is that if ACD fails, we decline the lease (and don't use it). https://bugzilla.redhat.com/show_bug.cgi?id=1713380 --- src/core/devices/nm-device.c | 1 + src/core/dhcp/nm-dhcp-client.c | 601 +++++++++++++++++++++++++++---- src/core/dhcp/nm-dhcp-client.h | 6 + src/core/dhcp/nm-dhcp-helper.c | 133 ++++--- src/core/dhcp/nm-dhcp-nettools.c | 5 + 5 files changed, 622 insertions(+), 124 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 7c77c9f39a..a16603320a 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -10299,6 +10299,7 @@ _dev_ipdhcpx_start(NMDevice *self, int addr_family) .v4 = { .request_broadcast = request_broadcast, + .acd_timeout_msec = _prop_get_ipv4_dad_timeout(self), }, }; diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index d6cfbf3f71..64fa0abace 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -32,6 +32,38 @@ /*****************************************************************************/ +/* This is how long we do ACD for each entry and reject new offers for + * the same address. Note that the maximum ACD timeout is limited to 30 seconds + * (NM_ACD_TIMEOUT_MAX_MSEC). + **/ +#define ACD_REGLIST_GRACE_PERIOD_MSEC 300000u + +G_STATIC_ASSERT(ACD_REGLIST_GRACE_PERIOD_MSEC > (NM_ACD_TIMEOUT_MAX_MSEC + 1000)); + +#define ACD_REGLIST_MAX_ENTRIES 30 + +/* To do ACD for an address (new lease), we will register a NML3ConfigData + * with l3cfg. After ACD completes, we still continue having NML3Cfg + * watch that address, for ACD_REGLIST_GRACE_PERIOD_MSEC. The reasons are: + * + * - the caller is supposed to actually configure the address right after + * ACD passed. We would not want to drop the ACD state before the caller + * got a chance to do that. + * - when ACD fails, we decline the address and expect the DHCP client + * to present a new lease. We may want to outright reject the address, + * if ACD is bad. Thus, we want to keep running ACD for the address a bit + * longer, so that future requests for the same address can be rejected. + * + * This data structure is used for tracking the registered ACD address. + */ +typedef struct { + const NML3ConfigData *l3cd; + gint64 expiry_msec; + in_addr_t addr; +} AcdRegListData; + +/*****************************************************************************/ + enum { SIGNAL_NOTIFY, LAST_SIGNAL, @@ -42,14 +74,47 @@ static guint signals[LAST_SIGNAL] = {0}; NM_GOBJECT_PROPERTIES_DEFINE(NMDhcpClient, PROP_CONFIG, ); typedef struct _NMDhcpClientPrivate { - NMDhcpClientConfig config; - const NML3ConfigData *l3cd; - GSource *no_lease_timeout_source; - GSource *watch_source; - GBytes *effective_client_id; + NMDhcpClientConfig config; + + /* This is the "next" data. That is, the one what was received last via + * _nm_dhcp_client_notify(), but which is currently pending on ACD. */ + const NML3ConfigData *l3cd_next; + + /* This is the currently exposed data. It passed ACD (or no ACD was performed), + * and is set from l3cd_next. */ + const NML3ConfigData *l3cd_curr; + + GSource *no_lease_timeout_source; + GSource *watch_source; + GBytes *effective_client_id; union { struct { + struct { + NML3CfgCommitTypeHandle *l3cfg_commit_handle; + GSource *done_source; + + /* When we do ACD for a l3cd lease, we will keep running ACD for + * the grace period ACD_REGLIST_GRACE_PERIOD_MSEC, even if we already + * determined the state. There are two reasons for that: + * + * - after ACD completes we notify the lease to the user, who is supposed + * to configure the address in NML3Cfg. If we were already removing the + * ACD state from NML3Cfg, ACD might need to start over. Instead, when + * the caller tries to configure the address, ACD state is already good. + * + * - if we decline on ACD offer, we may want to keep running and + * select other offers. Offers for which we just failed ACD (within + * ACD_REGLIST_GRACE_PERIOD_MSEC) are rejected. See _nm_dhcp_client_accept_offer(). + * For that, we keep monitoring the ACD state for up to ACD_REGLIST_MAX_ENTRIES + * addresses, to not restart and select the same lease twice in a row. + */ + GArray *reglist; + GSource *reglist_timeout_source; + + in_addr_t addr; + NMOptionBool state; + } acd; struct { GDBusMethodInvocation *invocation; } bound; @@ -75,16 +140,22 @@ G_DEFINE_ABSTRACT_TYPE(NMDhcpClient, nm_dhcp_client, G_TYPE_OBJECT) /*****************************************************************************/ +#define L3CD_ACD_TAG(priv) (&(priv)->v4.acd.addr) + static gboolean _dhcp_client_accept(NMDhcpClient *self, const NML3ConfigData *l3cd, GError **error); -_nm_unused static gboolean _dhcp_client_decline(NMDhcpClient *self, - const NML3ConfigData *l3cd, - const char *error_message, - GError **error); +static gboolean _dhcp_client_decline(NMDhcpClient *self, + const NML3ConfigData *l3cd, + const char *error_message, + GError **error); static void l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcpClient *self); +static void _acd_reglist_timeout_reschedule(NMDhcpClient *self, gint64 now_msec); + +static void _acd_reglist_data_remove(NMDhcpClient *self, guint idx, gboolean do_log); + /*****************************************************************************/ /* we use pid=-1 for invalid PIDs. Ensure that pid_t can hold negative values. */ @@ -197,7 +268,8 @@ l3_cfg_notify_check_connected(NMDhcpClient *self) NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); gboolean do_connect; - do_connect = priv->l3cfg_notify.wait_dhcp_commit | priv->l3cfg_notify.wait_ll_address; + do_connect = priv->l3cfg_notify.wait_dhcp_commit | priv->l3cfg_notify.wait_ll_address + | (NM_IS_IPv4(priv->config.addr_family) && priv->v4.acd.l3cfg_commit_handle); if (!do_connect) { nm_clear_g_signal_handler(priv->config.l3cfg, &priv->l3cfg_notify.id); @@ -303,6 +375,333 @@ _no_lease_timeout_schedule(NMDhcpClient *self) /*****************************************************************************/ +static void +_acd_state_reset(NMDhcpClient *self, gboolean forget_addr, gboolean forget_reglist) +{ + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + + if (!NM_IS_IPv4(priv->config.addr_family)) + return; + + if (priv->v4.acd.addr != INADDR_ANY) { + nm_l3cfg_commit_type_clear(priv->config.l3cfg, &priv->v4.acd.l3cfg_commit_handle); + l3_cfg_notify_check_connected(self); + nm_clear_g_source_inst(&priv->v4.acd.done_source); + if (forget_addr) { + priv->v4.acd.addr = INADDR_ANY; + priv->v4.acd.state = NM_OPTION_BOOL_DEFAULT; + } + } else + nm_assert(priv->v4.acd.state == NM_OPTION_BOOL_DEFAULT); + + if (forget_reglist) { + guint n; + + while ((n = nm_g_array_len(priv->v4.acd.reglist)) > 0) + _acd_reglist_data_remove(self, n - 1, TRUE); + } + + nm_assert(!priv->v4.acd.l3cfg_commit_handle); + nm_assert(!priv->v4.acd.done_source); + nm_assert(!forget_reglist + || !nm_l3cfg_remove_config_all(priv->config.l3cfg, L3CD_ACD_TAG(priv))); +} + +static gboolean +_acd_complete_on_idle_cb(gpointer user_data) +{ + NMDhcpClient *self = user_data; + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + + nm_assert(NM_IS_IPv4(priv->config.addr_family)); + nm_assert(priv->v4.acd.addr != INADDR_ANY); + nm_assert(!priv->v4.acd.l3cfg_commit_handle); + nm_assert(priv->l3cd_next); + + _acd_state_reset(self, FALSE, FALSE); + + _nm_dhcp_client_notify(self, NM_DHCP_CLIENT_EVENT_TYPE_BOUND, priv->l3cd_next); + + return G_SOURCE_CONTINUE; +} + +#define _acd_reglist_data_get(priv, idx) \ + nm_g_array_index_p((priv)->v4.acd.reglist, AcdRegListData, (idx)) + +static guint +_acd_reglist_data_find(NMDhcpClientPrivate *priv, in_addr_t addr_needle) +{ + const guint n = nm_g_array_len(priv->v4.acd.reglist); + guint i; + + nm_assert(addr_needle != INADDR_ANY); + + for (i = 0; i < n; i++) { + AcdRegListData *reglist_data = _acd_reglist_data_get(priv, i); + + if (reglist_data->addr == addr_needle) + return i; + } + return G_MAXUINT; +} + +static void +_acd_reglist_data_remove(NMDhcpClient *self, guint idx, gboolean do_log) +{ + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + AcdRegListData *reglist_data; + + nm_assert(idx < nm_g_array_len(priv->v4.acd.reglist)); + + reglist_data = _acd_reglist_data_get(priv, idx); + + if (do_log) { + char sbuf_addr[NM_UTILS_INET_ADDRSTRLEN]; + + _LOGD("acd: drop check for address %s (l3cd " NM_HASH_OBFUSCATE_PTR_FMT ")", + _nm_utils_inet4_ntop(reglist_data->addr, sbuf_addr), + NM_HASH_OBFUSCATE_PTR(reglist_data->l3cd)); + } + + if (!nm_l3cfg_remove_config(priv->config.l3cfg, L3CD_ACD_TAG(priv), reglist_data->l3cd)) + nm_assert_not_reached(); + + nm_clear_l3cd(®list_data->l3cd); + + nm_l3cfg_commit_on_idle_schedule(priv->config.l3cfg, NM_L3_CFG_COMMIT_TYPE_UPDATE); + + g_array_remove_index(priv->v4.acd.reglist, idx); + + if (priv->v4.acd.reglist->len == 0) { + nm_clear_pointer(&priv->v4.acd.reglist, g_array_unref); + nm_clear_g_source_inst(&priv->v4.acd.reglist_timeout_source); + } +} + +static gboolean +_acd_reglist_timeout_cb(gpointer user_data) +{ + NMDhcpClient *self = user_data; + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + gint64 now_msec; + + nm_clear_g_source_inst(&priv->v4.acd.reglist_timeout_source); + + now_msec = nm_utils_get_monotonic_timestamp_msec(); + + while (nm_g_array_len(priv->v4.acd.reglist) > 0) { + AcdRegListData *reglist_data = _acd_reglist_data_get(priv, 0); + + if (reglist_data->expiry_msec > now_msec) + break; + + _acd_reglist_data_remove(self, 0, TRUE); + } + + _acd_reglist_timeout_reschedule(self, now_msec); + + return G_SOURCE_CONTINUE; +} + +static void +_acd_reglist_timeout_reschedule(NMDhcpClient *self, gint64 now_msec) +{ + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + AcdRegListData *reglist_data; + + if (nm_g_array_len(priv->v4.acd.reglist) == 0) { + nm_assert(!priv->v4.acd.reglist_timeout_source); + return; + } + + if (priv->v4.acd.reglist_timeout_source) { + /* already pending. As we only add new elements with a *later* + * expiry, we don't need to ever cancel a pending timer. Worst + * case, the timer fires, and there is nothing to do and we + * reschedule. */ + return; + } + + now_msec = nm_utils_get_monotonic_timestamp_msec(); + + reglist_data = _acd_reglist_data_get(priv, 0); + + nm_assert(reglist_data->expiry_msec > now_msec); + + priv->v4.acd.reglist_timeout_source = + nm_g_timeout_add_source(reglist_data->expiry_msec - now_msec, + _acd_reglist_timeout_cb, + self); +} + +static void +_acd_check_lease(NMDhcpClient *self, NMOptionBool *out_acd_state) +{ + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + char sbuf_addr[NM_UTILS_INET_ADDRSTRLEN]; + in_addr_t addr; + gboolean addr_changed = FALSE; + guint idx; + gint64 now_msec; + + if (!NM_IS_IPv4(priv->config.addr_family)) + goto handle_no_acd; + + if (!priv->l3cd_next) + goto handle_no_acd; + + /* an IPv4 lease is always expected to have exactly one address. */ + nm_assert(nm_l3_config_data_get_num_addresses(priv->l3cd_next, AF_INET) == 1); + + if (priv->config.v4.acd_timeout_msec == 0) + goto handle_no_acd; + + addr = NMP_OBJECT_CAST_IP4_ADDRESS( + nm_l3_config_data_get_first_obj(priv->l3cd_next, NMP_OBJECT_TYPE_IP4_ADDRESS, NULL)) + ->address; + nm_assert(addr != INADDR_ANY); + + nm_clear_g_source_inst(&priv->v4.acd.done_source); + + if (priv->v4.acd.state != NM_OPTION_BOOL_DEFAULT && priv->v4.acd.addr == addr) { + /* the ACD state is already determined. Return right away. */ + nm_assert(!priv->v4.acd.l3cfg_commit_handle); + *out_acd_state = !!priv->v4.acd.state; + return; + } + + if (priv->v4.acd.addr != addr) { + addr_changed = TRUE; + priv->v4.acd.addr = addr; + } + + _LOGD("acd: %s check for address %s (timeout %u msec, l3cd " NM_HASH_OBFUSCATE_PTR_FMT ")", + addr_changed ? "add" : "update", + _nm_utils_inet4_ntop(addr, sbuf_addr), + priv->config.v4.acd_timeout_msec, + NM_HASH_OBFUSCATE_PTR(priv->l3cd_next)); + + priv->v4.acd.state = NM_OPTION_BOOL_DEFAULT; + + if (nm_l3cfg_add_config(priv->config.l3cfg, + L3CD_ACD_TAG(priv), + FALSE, + priv->l3cd_next, + NM_L3CFG_CONFIG_PRIORITY_IPV4LL, + 0, + 0, + NM_PLATFORM_ROUTE_METRIC_DEFAULT_IP4, + NM_PLATFORM_ROUTE_METRIC_DEFAULT_IP6, + 0, + 0, + NM_DNS_PRIORITY_DEFAULT_NORMAL, + NM_DNS_PRIORITY_DEFAULT_NORMAL, + NM_L3_ACD_DEFEND_TYPE_ONCE, + NM_MIN(priv->config.v4.acd_timeout_msec, NM_ACD_TIMEOUT_MAX_MSEC), + NM_L3CFG_CONFIG_FLAGS_ONLY_FOR_ACD, + NM_L3_CONFIG_MERGE_FLAGS_NONE)) + addr_changed = TRUE; + + if (!priv->v4.acd.reglist) + priv->v4.acd.reglist = g_array_new(FALSE, FALSE, sizeof(AcdRegListData)); + + idx = _acd_reglist_data_find(priv, addr); + + now_msec = nm_utils_get_monotonic_timestamp_msec(); + + g_array_append_val(priv->v4.acd.reglist, + ((AcdRegListData){ + .l3cd = nm_l3_config_data_ref(priv->l3cd_next), + .addr = addr, + .expiry_msec = now_msec + ACD_REGLIST_GRACE_PERIOD_MSEC, + })); + + if (idx != G_MAXUINT) { + /* we already tracked this "addr". We don't need to track it twice, + * forget about this one. This also has the effect, that we will + * always append the new entry to the list (so the list + * stays sorted by the increasing timestamp). */ + _acd_reglist_data_remove(self, idx, FALSE); + } + + if (priv->v4.acd.reglist->len > ACD_REGLIST_MAX_ENTRIES) { + /* rate limit how many addresses we track for ACD. */ + _acd_reglist_data_remove(self, 0, TRUE); + } + + _acd_reglist_timeout_reschedule(self, now_msec); + + if (!priv->v4.acd.l3cfg_commit_handle) { + priv->v4.acd.l3cfg_commit_handle = + nm_l3cfg_commit_type_register(priv->config.l3cfg, + NM_L3_CFG_COMMIT_TYPE_UPDATE, + NULL, + "dhcp4-acd"); + l3_cfg_notify_check_connected(self); + } + + if (addr_changed) + nm_l3cfg_commit_on_idle_schedule(priv->config.l3cfg, NM_L3_CFG_COMMIT_TYPE_AUTO); + + /* ACD is started/pending... */ + nm_assert(priv->v4.acd.addr != INADDR_ANY); + nm_assert(priv->v4.acd.state == NM_OPTION_BOOL_DEFAULT); + nm_assert(priv->v4.acd.l3cfg_commit_handle); + nm_assert(priv->l3cfg_notify.id); + *out_acd_state = NM_OPTION_BOOL_DEFAULT; + return; + +handle_no_acd: + /* Indicate that ACD is good (or disabled) by returning TRUE. */ + _acd_state_reset(self, TRUE, FALSE); + *out_acd_state = NM_OPTION_BOOL_TRUE; + return; +} + +/*****************************************************************************/ + +gboolean +_nm_dhcp_client_accept_offer(NMDhcpClient *self, gconstpointer p_yiaddr) +{ + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + char sbuf_addr[NM_UTILS_INET_ADDRSTRLEN]; + NMIPAddr yiaddr; + const NML3AcdAddrInfo *acd_info; + + if (!NM_IS_IPv4(priv->config.addr_family)) + return nm_assert_unreachable_val(FALSE); + + if (priv->config.v4.acd_timeout_msec == 0) { + /* ACD is disabled. Note that we might track the address for other + * reasons and have information about the ACD state below. But + * with ACD disabled, we always ignore that information. */ + return TRUE; + } + + nm_ip_addr_set(priv->config.addr_family, &yiaddr, p_yiaddr); + + /* Note that once we do ACD for a certain address, even after completing + * it, we keep the l3cd registered in NML3Cfg for ACD_REGLIST_GRACE_PERIOD_MSEC + * The idea is, that we don't yet turn off ACD for a grace period, so that + * we can avoid selecting the same lease again. + * + * Note that we even check whether we have an ACD state if priv->v4.acd.reglist + * is empty. Maybe for odd reasons, we track ACD for the address already. */ + + acd_info = nm_l3cfg_get_acd_addr_info(priv->config.l3cfg, yiaddr.addr4); + + if (!acd_info) + return TRUE; + + if (!NM_IN_SET(acd_info->state, NM_L3_ACD_ADDR_STATE_USED, NM_L3_ACD_ADDR_STATE_CONFLICT)) + return TRUE; + + _LOGD("offered lease rejected: address %s failed ACD check", + _nm_utils_inet4_ntop(yiaddr.addr4, sbuf_addr)); + + return FALSE; +} + void _nm_dhcp_client_notify(NMDhcpClient *self, NMDhcpClientEventType client_event_type, @@ -310,6 +709,8 @@ _nm_dhcp_client_notify(NMDhcpClient *self, { NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); GHashTable *options; + gboolean l3cd_changed; + NMOptionBool acd_state; const int IS_IPv4 = NM_IS_IPv4(priv->config.addr_family); nm_auto_unref_l3cd const NML3ConfigData *l3cd_merged = NULL; char sbuf1[NM_HASH_OBFUSCATE_PTR_STR_BUF_SIZE]; @@ -340,8 +741,7 @@ _nm_dhcp_client_notify(NMDhcpClient *self, nm_dhcp_client_event_type_to_string(client_event_type), NM_PRINT_FMT_QUOTED2(l3cd, ", l3cd=", NM_HASH_OBFUSCATE_PTR_STR(l3cd, sbuf1), "")); - if (l3cd) - nm_l3_config_data_seal(l3cd); + nm_l3_config_data_seal(l3cd); if (client_event_type >= NM_DHCP_CLIENT_EVENT_TYPE_TIMEOUT) watch_cleanup(self); @@ -350,34 +750,40 @@ _nm_dhcp_client_notify(NMDhcpClient *self, /* nm_dhcp_utils_merge_new_dhcp6_lease() relies on "life_starts" option * for merging, which is only set by dhclient. Internal client never sets that, * but it supports multiple IP addresses per lease. */ - if (nm_dhcp_utils_merge_new_dhcp6_lease(priv->l3cd, l3cd, &l3cd_merged)) { + if (nm_dhcp_utils_merge_new_dhcp6_lease(priv->l3cd_next, l3cd, &l3cd_merged)) { _LOGD("lease merged with existing one"); l3cd = nm_l3_config_data_seal(l3cd_merged); } } - if (priv->l3cd == l3cd) - return; - if (l3cd) { nm_clear_g_source_inst(&priv->no_lease_timeout_source); - } else { - if (priv->l3cd) - _no_lease_timeout_schedule(self); + } else + _no_lease_timeout_schedule(self); + + l3cd_changed = nm_l3_config_data_reset(&priv->l3cd_next, l3cd); + + _acd_check_lease(self, &acd_state); + + options = priv->l3cd_next ? nm_dhcp_lease_get_options( + nm_l3_config_data_get_dhcp_lease(priv->l3cd_next, priv->config.addr_family)) + : NULL; + + if (_LOGI_ENABLED()) { + const char *req_str = + IS_IPv4 ? nm_dhcp_option_request_string(AF_INET, NM_DHCP_OPTION_DHCP4_NM_IP_ADDRESS) + : nm_dhcp_option_request_string(AF_INET6, NM_DHCP_OPTION_DHCP6_NM_IP_ADDRESS); + const char *addr = nm_g_hash_table_lookup(options, req_str); + + _LOGI("state changed %s%s%s%s", + priv->l3cd_next ? "new lease" : "no lease", + NM_PRINT_FMT_QUOTED2(addr, ", address=", addr, ""), + acd_state == NM_OPTION_BOOL_DEFAULT ? ", acd pending" + : (acd_state ? "" : ", acd conflict")); } - /* FIXME(l3cfg:dhcp): the API of NMDhcpClient is changing to expose a simpler API. - * The internals like the state should not be exposed (or possibly dropped in large - * parts). */ - - nm_l3_config_data_reset(&priv->l3cd, l3cd); - - options = l3cd ? nm_dhcp_lease_get_options( - nm_l3_config_data_get_dhcp_lease(l3cd, priv->config.addr_family)) - : NULL; - if (_LOGD_ENABLED()) { - if (options) { + if (l3cd_changed && options) { gs_free const char **keys = NULL; guint nkeys; guint i; @@ -391,41 +797,33 @@ _nm_dhcp_client_notify(NMDhcpClient *self, } } - if (_LOGI_ENABLED()) { - const char *req_str = - IS_IPv4 ? nm_dhcp_option_request_string(AF_INET, NM_DHCP_OPTION_DHCP4_NM_IP_ADDRESS) - : nm_dhcp_option_request_string(AF_INET6, NM_DHCP_OPTION_DHCP6_NM_IP_ADDRESS); - const char *addr = nm_g_hash_table_lookup(options, req_str); - - _LOGI("state changed %s%s%s%s", - priv->l3cd ? "new lease" : "no lease", - NM_PRINT_FMT_QUOTED(addr, ", address=", addr, "", "")); + if (acd_state == NM_OPTION_BOOL_DEFAULT) { + /* ACD is in progress... */ + return; } - /* FIXME(l3cfg:dhcp:acd): NMDhcpClient must also do ACD. It needs acd_timeout_msec - * as a configuration parameter (in NMDhcpClientConfig). When ACD is enabled, - * when a new lease gets announced, it must first use NML3Cfg to run ACD on the - * interface (the previous lease -- if any -- will still be used at that point). - * If ACD fails, we call _dhcp_client_decline() and try to get a different - * lease. - * If ACD passes, we need to notify the new lease, and the user (NMDevice) may - * then configure the address. We need to watch the configured addresses (in NML3Cfg), - * and if the address appears there, we need to accept the lease. That is complicated - * but necessary, because we can only accept the lease after we configured the - * address. - * - * As a whole, ACD is transparent for the user (NMDevice). It's entirely managed - * by NMDhcpClient. Note that we do ACD through NML3Cfg, which centralizes IP handling - * for one interface, so for example if the same address happens to be configured - * as a static address (bypassing ACD), then NML3Cfg is aware of that and signals - * immediate success. */ + if (!acd_state) { + gs_free_error GError *error = NULL; - if (client_event_type == NM_DHCP_CLIENT_EVENT_TYPE_BOUND && priv->l3cd - && nm_l3_config_data_get_num_addresses(priv->l3cd, priv->config.addr_family) > 0) { + /* We only decline. We don't actually emit to the caller that + * something is wrong (like NM_DHCP_CLIENT_NOTIFY_TYPE_IT_LOOKS_BAD). + * If we would, NMDevice might decide to tear down the device, when + * we actually should continue trying to get a better lease. There + * is already "ipv4.dhcp-timeout" which will handle the failure if + * we don't get a good lease. */ + if (!_dhcp_client_decline(self, priv->l3cd_next, "acd failed", &error)) + _LOGD("decline failed: %s", error->message); + return; + } + + nm_l3_config_data_reset(&priv->l3cd_curr, priv->l3cd_next); + + if (client_event_type == NM_DHCP_CLIENT_EVENT_TYPE_BOUND && priv->l3cd_curr + && nm_l3_config_data_get_num_addresses(priv->l3cd_curr, priv->config.addr_family) > 0) priv->l3cfg_notify.wait_dhcp_commit = TRUE; - } else { + else priv->l3cfg_notify.wait_dhcp_commit = FALSE; - } + l3_cfg_notify_check_connected(self); { @@ -433,7 +831,7 @@ _nm_dhcp_client_notify(NMDhcpClient *self, .notify_type = NM_DHCP_CLIENT_NOTIFY_TYPE_LEASE_UPDATE, .lease_update = { - .l3cd = priv->l3cd, + .l3cd = priv->l3cd_curr, .accepted = !priv->l3cfg_notify.wait_dhcp_commit, }, }; @@ -512,7 +910,7 @@ _dhcp_client_accept(NMDhcpClient *self, const NML3ConfigData *l3cd, GError **err klass = NM_DHCP_CLIENT_GET_CLASS(self); - g_return_val_if_fail(NM_DHCP_CLIENT_GET_PRIVATE(self)->l3cd, FALSE); + g_return_val_if_fail(NM_DHCP_CLIENT_GET_PRIVATE(self)->l3cd_curr, FALSE); return klass->accept(self, l3cd, error); } @@ -552,7 +950,7 @@ _dhcp_client_decline(NMDhcpClient *self, klass = NM_DHCP_CLIENT_GET_CLASS(self); - g_return_val_if_fail(NM_DHCP_CLIENT_GET_PRIVATE(self)->l3cd, FALSE); + g_return_val_if_fail(NM_DHCP_CLIENT_GET_PRIVATE(self)->l3cd_next, FALSE); return klass->decline(self, l3cd, error_message, error); } @@ -611,6 +1009,7 @@ static void l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcpClient *self) { NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + char sbuf_addr[NM_UTILS_INET_ADDRSTRLEN]; nm_assert(l3cfg == priv->config.l3cfg); @@ -651,7 +1050,7 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp * lease and notifying NMDevice. */ nm_l3_config_data_iter_ip_address_for_each (&ipconf_iter, - priv->l3cd, + priv->l3cd_curr, priv->config.addr_family, &lease_address) break; @@ -678,9 +1077,11 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp _LOGD("accept lease"); - if (!_dhcp_client_accept(self, priv->l3cd, &error)) { + 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, @@ -693,11 +1094,53 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp self, &((NMDhcpClientNotifyData){.notify_type = NM_DHCP_CLIENT_NOTIFY_TYPE_LEASE_UPDATE, .lease_update = { - .l3cd = priv->l3cd, + .l3cd = priv->l3cd_curr, .accepted = TRUE, }})); } wait_dhcp_commit_done: + + if (notify_data->notify_type == NM_L3_CONFIG_NOTIFY_TYPE_ACD_EVENT + && priv->v4.acd.l3cfg_commit_handle) { + nm_assert(priv->v4.acd.addr != INADDR_ANY); + nm_assert(priv->v4.acd.state == NM_OPTION_BOOL_DEFAULT); + nm_assert(!priv->v4.acd.done_source); + + if (priv->v4.acd.addr == notify_data->acd_event.info.addr + && nm_l3_acd_addr_info_find_track_info(¬ify_data->acd_event.info, + L3CD_ACD_TAG(priv), + NULL, + NULL)) { + NMOptionBool acd_state; + + switch (notify_data->acd_event.info.state) { + default: + nm_assert_not_reached(); + /* fall-through */ + case NM_L3_ACD_ADDR_STATE_INIT: + case NM_L3_ACD_ADDR_STATE_PROBING: + acd_state = NM_OPTION_BOOL_DEFAULT; + break; + case NM_L3_ACD_ADDR_STATE_USED: + case NM_L3_ACD_ADDR_STATE_CONFLICT: + case NM_L3_ACD_ADDR_STATE_EXTERNAL_REMOVED: + acd_state = NM_OPTION_BOOL_FALSE; + break; + case NM_L3_ACD_ADDR_STATE_READY: + case NM_L3_ACD_ADDR_STATE_DEFENDING: + acd_state = NM_OPTION_BOOL_TRUE; + break; + } + if (acd_state != NM_OPTION_BOOL_DEFAULT) { + _LOGD("acd: acd %s for %s", + acd_state ? "ready" : "conflict", + _nm_utils_inet4_ntop(priv->v4.acd.addr, sbuf_addr)); + nm_l3cfg_commit_type_clear(priv->config.l3cfg, &priv->v4.acd.l3cfg_commit_handle); + priv->v4.acd.state = acd_state; + priv->v4.acd.done_source = nm_g_idle_add_source(_acd_complete_on_idle_cb, self); + } + } + } } gboolean @@ -826,6 +1269,8 @@ nm_dhcp_client_stop(NMDhcpClient *self, gboolean release) "dhcp stopping"); } + _acd_state_reset(self, TRUE, TRUE); + priv->l3cfg_notify.wait_dhcp_commit = FALSE; priv->l3cfg_notify.wait_ll_address = FALSE; l3_cfg_notify_check_connected(self); @@ -839,6 +1284,9 @@ nm_dhcp_client_stop(NMDhcpClient *self, gboolean release) _LOGI("canceled DHCP transaction"); nm_assert(priv->pid == -1); + nm_clear_l3cd(&priv->l3cd_next); + nm_clear_l3cd(&priv->l3cd_curr); + _nm_dhcp_client_notify(self, NM_DHCP_CLIENT_EVENT_TYPE_TERMINATED, NULL); } @@ -973,6 +1421,7 @@ 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); @@ -983,6 +1432,8 @@ nm_dhcp_client_handle_event(gpointer unused, priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + g_return_val_if_fail(!priv->is_stopped, FALSE); + if (!nm_streq0(priv->config.iface, iface)) return FALSE; if (priv->pid != pid) @@ -1067,10 +1518,12 @@ nm_dhcp_client_handle_event(gpointer unused, client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_FAIL; } - if (priv->v4.bound.invocation) + IS_IPv4 = NM_IS_IPv4(priv->config.addr_family); + + if (IS_IPv4 && priv->v4.bound.invocation) g_dbus_method_invocation_return_value(g_steal_pointer(&priv->v4.bound.invocation), NULL); - if (NM_IS_IPv4(priv->config.addr_family) + if (IS_IPv4 && NM_IN_SET(client_event_type, NM_DHCP_CLIENT_EVENT_TYPE_BOUND, NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED)) @@ -1229,6 +1682,13 @@ set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *ps { .invocation = NULL, }, + .acd = + { + .addr = INADDR_ANY, + .state = NM_OPTION_BOOL_DEFAULT, + .l3cfg_commit_handle = NULL, + .done_source = NULL, + }, }; } else { priv->v6 = (typeof(priv->v6)){ @@ -1266,12 +1726,15 @@ dispose(GObject *object) watch_cleanup(self); nm_clear_g_source_inst(&priv->no_lease_timeout_source); + if (!NM_IS_IPv4(priv->config.addr_family)) nm_clear_g_source_inst(&priv->v6.lladdr_timeout_source); + nm_clear_pointer(&priv->effective_client_id, g_bytes_unref); nm_assert(!priv->watch_source); - nm_assert(!priv->l3cd); + nm_assert(!priv->l3cd_next); + nm_assert(!priv->l3cd_curr); nm_assert(priv->l3cfg_notify.id == 0); G_OBJECT_CLASS(nm_dhcp_client_parent_class)->dispose(object); diff --git a/src/core/dhcp/nm-dhcp-client.h b/src/core/dhcp/nm-dhcp-client.h index e4b9992922..72444c3fc0 100644 --- a/src/core/dhcp/nm-dhcp-client.h +++ b/src/core/dhcp/nm-dhcp-client.h @@ -150,6 +150,10 @@ typedef struct { /* The address from the previous lease */ const char *last_address; + /* Whether to do ACD for the DHCPv4 address. With timeout zero, ACD + * is disabled. */ + guint acd_timeout_msec; + /* Set BOOTP broadcast flag in request packets, so that servers * will always broadcast replies. */ bool request_broadcast : 1; @@ -261,6 +265,8 @@ void _nm_dhcp_client_notify(NMDhcpClient *self, NMDhcpClientEventType client_event_type, const NML3ConfigData *l3cd); +gboolean _nm_dhcp_client_accept_offer(NMDhcpClient *self, gconstpointer p_yiaddr); + gboolean nm_dhcp_client_handle_event(gpointer unused, const char *iface, int pid, diff --git a/src/core/dhcp/nm-dhcp-helper.c b/src/core/dhcp/nm-dhcp-helper.c index aab658a2aa..5a17f4e8ab 100644 --- a/src/core/dhcp/nm-dhcp-helper.c +++ b/src/core/dhcp/nm-dhcp-helper.c @@ -103,14 +103,17 @@ next:; int main(int argc, char *argv[]) { - gs_unref_object GDBusConnection *connection = NULL; - gs_free_error GError *error = NULL; - gs_unref_variant GVariant *parameters = NULL; - gs_unref_variant GVariant *result = NULL; - gboolean success = FALSE; + gs_unref_object GDBusConnection *connection = NULL; + gs_free_error GError *error = NULL; + gs_free_error GError *error_flush = NULL; + gs_unref_variant GVariant *parameters = NULL; + gs_unref_variant GVariant *result = NULL; + gs_free char *s_err = NULL; + gboolean success; guint try_count; gint64 time_start; gint64 time_end; + gint64 remaining_time; /* 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 @@ -121,6 +124,8 @@ main(int argc, char *argv[]) time_end = time_start + (5000 * 1000L); try_count = 0; + _LOGi("nm-dhcp-helper: event called"); + do_connect: try_count++; connection = @@ -131,16 +136,16 @@ do_connect: &error); if (!connection) { if (g_error_matches(error, G_IO_ERROR, G_IO_ERROR_WOULD_BLOCK)) { - gint64 time_remaining = time_end - g_get_monotonic_time(); - gint64 interval; + remaining_time = time_end - g_get_monotonic_time(); + if (remaining_time > 0) { + gint64 interval; - if (time_remaining > 0) { _LOGi("failure to connect: %s (retry %u, waited %lld ms)", error->message, try_count, - (long long) (time_end - time_remaining - time_start) / 1000); + (long long) (time_end - remaining_time - time_start) / 1000); interval = NM_CLAMP((gint64) (100L * (1L << NM_MIN(try_count, 31))), 5000, 100000); - g_usleep(NM_MIN(interval, time_remaining)); + g_usleep(NM_MIN(interval, remaining_time)); g_clear_error(&error); goto do_connect; } @@ -148,6 +153,7 @@ do_connect: g_dbus_error_strip_remote_error(error); _LOGE("could not connect to NetworkManager D-Bus socket: %s", error->message); + success = FALSE; goto out; } @@ -169,57 +175,74 @@ do_notify: NULL, &error); - if (!result) { - gs_free char *s_err = NULL; - - s_err = g_dbus_error_get_remote_error(error); - if (NM_IN_STRSET(s_err, "org.freedesktop.DBus.Error.UnknownMethod")) { - gint64 remaining_time = time_end - g_get_monotonic_time(); - gint64 interval; - - /* I am not sure that a race can actually happen, as we register the object - * on the server side during GDBusServer:new-connection signal. - * - * However, there was also a race for subscribing to an event, so let's just - * do some retry. */ - if (remaining_time > 0) { - _LOGi("failure to call notify: %s (retry %u)", error->message, try_count); - interval = NM_CLAMP((gint64) (100L * (1L << NM_MIN(try_count, 31))), 5000, 25000); - g_usleep(NM_MIN(interval, remaining_time)); - g_clear_error(&error); - goto do_notify; - } - } - _LOGW("failure to call notify: %s (try signal via Event)", error->message); - g_clear_error(&error); - - /* for backward compatibility, try to emit the signal. There is no stable - * API between the dhcp-helper and NetworkManager. However, while upgrading - * the NetworkManager package, a newer helper might want to notify an - * older server, which still uses the "Event". */ - if (!g_dbus_connection_emit_signal(connection, - NULL, - "/", - NM_DHCP_CLIENT_DBUS_IFACE, - "Event", - parameters, - &error)) { - g_dbus_error_strip_remote_error(error); - _LOGE("could not send DHCP Event signal: %s", error->message); - goto out; - } - /* We were able to send the asynchronous Event. Consider that a success. */ - success = TRUE; - } else + if (result) { success = TRUE; + goto out; + } - if (!g_dbus_connection_flush_sync(connection, NULL, &error)) { - g_dbus_error_strip_remote_error(error); - _LOGE("could not flush D-Bus connection: %s", error->message); + s_err = g_dbus_error_get_remote_error(error); + + if (NM_IN_STRSET(s_err, "org.freedesktop.NetworkManager.Device.Failed")) { + _LOGi("notify failed with reason: %s", error->message); success = FALSE; goto out; } + if (!NM_IN_STRSET(s_err, "org.freedesktop.DBus.Error.UnknownMethod")) { + /* Some unexpected error. We treat that as a failure. In particular, + * the daemon will fail the request if ACD fails. This causes nm-dhcp-helper + * to fail, which in turn causes dhclient to send a DECLINE. */ + _LOGW("failure to call notify: %s (try signal via Event)", error->message); + success = FALSE; + goto out; + } + + /* I am not sure that a race can actually happen, as we register the object + * on the server side during GDBusServer:new-connection signal. + * + * However, there was also a race for subscribing to an event, so let's just + * do some retry. */ + remaining_time = time_end - g_get_monotonic_time(); + if (remaining_time > 0) { + gint64 interval; + + _LOGi("failure to call notify: %s (retry %u)", error->message, try_count); + interval = NM_CLAMP((gint64) (100L * (1L << NM_MIN(try_count, 31))), 5000, 25000); + g_usleep(NM_MIN(interval, remaining_time)); + g_clear_error(&error); + goto do_notify; + } + + /* for backward compatibility, try to emit the signal. There is no stable + * API between the dhcp-helper and NetworkManager. However, while upgrading + * the NetworkManager package, a newer helper might want to notify an + * older server, which still uses the "Event". */ + + _LOGW("failure to call notify: %s (try signal via Event)", error->message); + g_clear_error(&error); + + if (g_dbus_connection_emit_signal(connection, + NULL, + "/", + NM_DHCP_CLIENT_DBUS_IFACE, + "Event", + parameters, + &error)) { + /* We were able to send the asynchronous Event. Consider that a success. */ + success = TRUE; + goto out; + } + + g_dbus_error_strip_remote_error(error); + _LOGE("could not send DHCP Event signal: %s", error->message); + success = FALSE; + out: + if (!g_dbus_connection_flush_sync(connection, NULL, &error_flush)) { + _LOGE("could not flush D-Bus connection: %s", error_flush->message); + /* if we considered this a success so far, don't fail because of this. */ + } + + _LOGi("success: %s", success ? "YES" : "NO"); return success ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/src/core/dhcp/nm-dhcp-nettools.c b/src/core/dhcp/nm-dhcp-nettools.c index eba4891ef2..a986dffe92 100644 --- a/src/core/dhcp/nm-dhcp-nettools.c +++ b/src/core/dhcp/nm-dhcp-nettools.c @@ -964,6 +964,11 @@ dhcp4_event_handle(NMDhcpNettools *self, NDhcp4ClientEvent *event) return; } + if (!_nm_dhcp_client_accept_offer(NM_DHCP_CLIENT(self), &yiaddr.s_addr)) { + /* We don't log about this, the parent class is expected to notify about the reasons. */ + return; + } + _LOGT("selecting offered lease from %s for %s", _nm_utils_inet4_ntop(server_id.s_addr, addr_str), _nm_utils_inet4_ntop(yiaddr.s_addr, addr_str2));