From 3fcb1a072f230b53c6fdf6e106e0972293a2f742 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Thu, 11 May 2023 13:32:13 +0200 Subject: [PATCH] ipv6ll: don't regenerate the address when it's removed externally Currently if the IPv6 link-local address is removed after it passed DAD, NetworkManager tries to generate a new link-local address. If this fails, which is always the case for EUI64, ipv6ll is considered as failed and the connection can go down (depending on may-fail). This is particularly bad for virtual interfaces because if somebody removes the link-local address, the activation can fail and destroy the interface, breaking all services that require it. Also, it's a change in behavior introduced in 1.36.0. It seems that a better approach here is to re-add the address that was removed externally. [bgalvani@redhat.com: since the branch is missing commit 7ca95cee15b3 ('platform: always reconfigure IP routes even if removed externally'), we need to set flag NM_L3CFG_CONFIG_FLAGS_FORCE_ONCE when committing the address, otherwise it's not re-added] Fixes: aa070fb82190 ('core: add NML3IPv6LL helper') https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1622 (cherry picked from commit 53ba9f4701f30b12637df2c7215a0b7da845b34c) (cherry picked from commit 2976e4c3b7fcee06051ce83c9a7fa911ad192dc4) (cherry picked from commit 4a13b5f52217c81ddf2329ba343796bfa4ed5ef9) --- src/core/nm-l3-ipv6ll.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/src/core/nm-l3-ipv6ll.c b/src/core/nm-l3-ipv6ll.c index 2640c07554..6e5e460258 100644 --- a/src/core/nm-l3-ipv6ll.c +++ b/src/core/nm-l3-ipv6ll.c @@ -391,7 +391,7 @@ _pladdr_find_ll(NML3IPv6LL *self, gboolean *out_cur_addr_failed) /*****************************************************************************/ static void -_lladdr_handle_changed(NML3IPv6LL *self) +_lladdr_handle_changed(NML3IPv6LL *self, gboolean force_commit) { const NML3ConfigData *l3cd; gboolean changed = FALSE; @@ -420,7 +420,9 @@ _lladdr_handle_changed(NML3IPv6LL *self) NM_DNS_PRIORITY_DEFAULT_NORMAL, NM_L3_ACD_DEFEND_TYPE_ALWAYS, 0, - NM_L3CFG_CONFIG_FLAGS_NONE, + /* Even if the address was removed from platform, it must + * be re-added, hence FORCE_ONCE. */ + NM_L3CFG_CONFIG_FLAGS_FORCE_ONCE, NM_L3_CONFIG_MERGE_FLAGS_NONE)) changed = TRUE; } else { @@ -434,7 +436,7 @@ _lladdr_handle_changed(NML3IPv6LL *self) self->l3cfg_commit_handle, "ipv6ll"); - if (changed) + if (changed || force_commit) nm_l3cfg_commit_on_idle_schedule(self->l3cfg, NM_L3_CFG_COMMIT_TYPE_AUTO); if (!self->emit_changed_idle_source) { @@ -515,6 +517,7 @@ _check(NML3IPv6LL *self) const NMPlatformIP6Address *pladdr; char sbuf[INET6_ADDRSTRLEN]; gboolean cur_addr_failed; + gboolean restarted = FALSE; struct in6_addr lladdr; pladdr = _pladdr_find_ll(self, &cur_addr_failed); @@ -526,14 +529,14 @@ _check(NML3IPv6LL *self) if (_set_cur_lladdr_obj(self, NM_L3_IPV6LL_STATE_DAD_IN_PROGRESS, pladdr)) { _LOGT("changed: waiting for address %s to complete DAD", _nm_utils_inet6_ntop(&self->cur_lladdr, sbuf)); - _lladdr_handle_changed(self); + _lladdr_handle_changed(self, FALSE); } return; } if (_set_cur_lladdr_obj(self, NM_L3_IPV6LL_STATE_READY, pladdr)) { _LOGT("changed: address %s is ready", _nm_utils_inet6_ntop(&self->cur_lladdr, sbuf)); - _lladdr_handle_changed(self); + _lladdr_handle_changed(self, FALSE); } return; } @@ -543,11 +546,17 @@ _check(NML3IPv6LL *self) * Prematurely abort DAD to generate a new address below. */ nm_assert( NM_IN_SET(self->state, NM_L3_IPV6LL_STATE_DAD_IN_PROGRESS, NM_L3_IPV6LL_STATE_READY)); - if (self->state == NM_L3_IPV6LL_STATE_DAD_IN_PROGRESS) - _LOGT("changed: address %s did not complete DAD", - _nm_utils_inet6_ntop(&self->cur_lladdr, sbuf)); - else { + + if (cur_addr_failed) { + /* On DAD failure, we always try to regenerate a new address. */ + _LOGT("changed: address %s failed", _nm_utils_inet6_ntop(&self->cur_lladdr, sbuf)); + } else { _LOGT("changed: address %s is gone", _nm_utils_inet6_ntop(&self->cur_lladdr, sbuf)); + /* When the address is removed, we always try to re-add it. */ + nm_clear_g_source_inst(&self->wait_for_addr_source); + lladdr = self->cur_lladdr; + restarted = TRUE; + goto commit; } /* reset the state here, so that we are sure that the following @@ -569,19 +578,20 @@ _check(NML3IPv6LL *self) if (_set_cur_lladdr_bin(self, NM_L3_IPV6LL_STATE_DAD_FAILED, NULL)) { _LOGW("changed: no IPv6 link local address to retry after Duplicate Address Detection " "failures (back off)"); - _lladdr_handle_changed(self); + _lladdr_handle_changed(self, FALSE); } return; } +commit: /* we give NML3Cfg 2 seconds to configure the address on the interface. We * thus very soon expect to see this address configured (and kernel started DAD). * If that does not happen within timeout, we assume that this address failed DAD. */ self->wait_for_addr_source = nm_g_timeout_add_source(2000, _wait_for_addr_timeout_cb, self); - if (_set_cur_lladdr_bin(self, NM_L3_IPV6LL_STATE_DAD_IN_PROGRESS, &lladdr)) { + if (_set_cur_lladdr_bin(self, NM_L3_IPV6LL_STATE_DAD_IN_PROGRESS, &lladdr) || restarted) { _LOGT("changed: starting DAD for address %s", _nm_utils_inet6_ntop(&self->cur_lladdr, sbuf)); - _lladdr_handle_changed(self); + _lladdr_handle_changed(self, restarted); } return; } -- 2.39.2