device: restart DHCP when the MAC changes and the client-id is fixedbg/dhcp-restart-pt2

If the MAC changes there is the possibility that the DHCP client will
not be able to renew the address because it uses the old MAC as
CHADDR. Depending on the implementation, the DHCP server might use
CHADDR (so, the old address) as the destination MAC for DHCP replies,
and those packets will be lost.

To avoid this problem, we need to restart the DHCP client after a MAC
change; that has the disadvantage that the server will give out a
different address, unless the client is sending a explicit
client-id. So, limit the restart to situations where the client-id is
explicitly set to a fixed value.

https://bugzilla.redhat.com/show_bug.cgi?id=2110000

5 files changed, 27 insertions, 17 deletions

diff --git a/src/core/devices/nm-device-ethernet.c b/src/core/devices/nm-device-ethernet.c
index 9852d659ee..42ef855771 100644
--- a/src/core/devices/nm-device-ethernet.c
+++ b/src/core/devices/nm-device-ethernet.c
@@ -451,7 +451,7 @@ supplicant_auth_state_changed(NMSupplicantInterface *iface,
 
     if (state == NM_SUPPLICANT_AUTH_STATE_SUCCESS) {
         nm_clear_g_signal_handler(priv->supplicant.iface, &priv->supplicant.iface_state_id);
-        nm_device_update_dynamic_ip_setup(NM_DEVICE(self), "supplicant auth state changed");
+        nm_device_update_dynamic_ip_setup(NM_DEVICE(self), FALSE, "supplicant auth state changed");
     }
 }
 
diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c
index 5ad1192b37..58705d0132 100644
--- a/src/core/devices/nm-device.c
+++ b/src/core/devices/nm-device.c
@@ -6373,7 +6373,7 @@ _dev_unmanaged_check_external_down(NMDevice *self, gboolean only_if_unmanaged, g
 }
 
 void
-nm_device_update_dynamic_ip_setup(NMDevice *self, const char *reason)
+nm_device_update_dynamic_ip_setup(NMDevice *self, gboolean preserve_client_id, const char *reason)
 {
     NMDevicePrivate *priv;
 
@@ -6388,10 +6388,14 @@ nm_device_update_dynamic_ip_setup(NMDevice *self, const char *reason)
 
     g_hash_table_remove_all(priv->ip6_saved_properties);
 
-    if (priv->ipdhcp_data_4.state != NM_DEVICE_IP_STATE_NONE)
-        _dev_ipdhcpx_restart(self, AF_INET, FALSE);
-    if (priv->ipdhcp_data_6.state != NM_DEVICE_IP_STATE_NONE)
-        _dev_ipdhcpx_restart(self, AF_INET6, FALSE);
+    if (priv->ipdhcp_data_4.state != NM_DEVICE_IP_STATE_NONE) {
+        if (!preserve_client_id || priv->ipdhcp_data_4.client_id_fixed)
+            _dev_ipdhcpx_restart(self, AF_INET, FALSE);
+    }
+    if (priv->ipdhcp_data_6.state != NM_DEVICE_IP_STATE_NONE) {
+        if (!preserve_client_id || priv->ipdhcp_data_6.client_id_fixed)
+            _dev_ipdhcpx_restart(self, AF_INET6, FALSE);
+    }
 
     if (priv->ipac6_data.ndisc) {
         /* FIXME: todo */
@@ -6696,6 +6700,7 @@ device_link_changed(gpointer user_data)
     NMDeviceClass                  *klass             = NM_DEVICE_GET_CLASS(self);
     NMDevicePrivate                *priv              = NM_DEVICE_GET_PRIVATE(self);
     gboolean                        ip_ifname_changed = FALSE;
+    gboolean                        hw_addr_changed;
     nm_auto_nmpobj const NMPObject *pllink_keep_alive = NULL;
     const NMPlatformLink           *pllink;
     const char                     *str;
@@ -6742,9 +6747,9 @@ device_link_changed(gpointer user_data)
     if (ifindex == nm_device_get_ip_ifindex(self))
         _stats_update_counters_from_pllink(self, pllink);
 
-    had_hw_addr = (priv->hw_addr != NULL);
-    nm_device_update_hw_address(self);
-    got_hw_addr = (!had_hw_addr && priv->hw_addr);
+    had_hw_addr     = (priv->hw_addr != NULL);
+    hw_addr_changed = nm_device_update_hw_address(self);
+    got_hw_addr     = (!had_hw_addr && priv->hw_addr);
     nm_device_update_permanent_hw_address(self, FALSE);
 
     if (pllink->name[0] && !nm_streq(priv->iface, pllink->name)) {
@@ -6794,7 +6799,9 @@ device_link_changed(gpointer user_data)
 
     /* Update DHCP, etc, if needed */
     if (ip_ifname_changed)
-        nm_device_update_dynamic_ip_setup(self, "IP interface changed");
+        nm_device_update_dynamic_ip_setup(self, FALSE, "IP interface changed");
+    else if (hw_addr_changed)
+        nm_device_update_dynamic_ip_setup(self, TRUE, "hw-address changed");
 
     was_up   = priv->up;
     priv->up = NM_FLAGS_HAS(pllink->n_ifi_flags, IFF_UP);
@@ -6854,7 +6861,7 @@ device_link_changed(gpointer user_data)
          * renew DHCP leases and such.
          */
         if (priv->state == NM_DEVICE_STATE_ACTIVATED) {
-            nm_device_update_dynamic_ip_setup(self, "interface got carrier");
+            nm_device_update_dynamic_ip_setup(self, FALSE, "interface got carrier");
         }
     }
 
@@ -6916,7 +6923,7 @@ device_ip_link_changed(gpointer user_data)
         priv->ip_iface_ = g_strdup(ip_iface);
         update_prop_ip_iface(self);
 
-        nm_device_update_dynamic_ip_setup(self, "interface renamed");
+        nm_device_update_dynamic_ip_setup(self, FALSE, "interface renamed");
     }
 
     return G_SOURCE_REMOVE;
diff --git a/src/core/devices/nm-device.h b/src/core/devices/nm-device.h
index 3dfa572064..62628b1fb8 100644
--- a/src/core/devices/nm-device.h
+++ b/src/core/devices/nm-device.h
@@ -767,8 +767,9 @@ void nm_device_update_metered(NMDevice *self);
 gboolean nm_device_update_hw_address(NMDevice *self);
 void     nm_device_update_initial_hw_address(NMDevice *self);
 void     nm_device_update_permanent_hw_address(NMDevice *self, gboolean force_freeze);
-void     nm_device_update_dynamic_ip_setup(NMDevice *self, const char *reason);
-guint    nm_device_get_supplicant_timeout(NMDevice *self);
+void
+nm_device_update_dynamic_ip_setup(NMDevice *self, gboolean preserve_client_id, const char *reason);
+guint nm_device_get_supplicant_timeout(NMDevice *self);
 
 gboolean nm_device_auth_retries_try_next(NMDevice *self);
 
diff --git a/src/core/devices/wifi/nm-device-wifi.c b/src/core/devices/wifi/nm-device-wifi.c
index 171743d075..8beaf05a0d 100644
--- a/src/core/devices/wifi/nm-device-wifi.c
+++ b/src/core/devices/wifi/nm-device-wifi.c
@@ -2525,7 +2525,7 @@ supplicant_iface_state(NMDeviceWifi              *self,
                 _LOGD(LOGD_WIFI,
                       "supplicant state settled after roaming, renew dynamic IP configuration");
                 nm_clear_g_source_inst(&priv->roam_supplicant_wait_source);
-                nm_device_update_dynamic_ip_setup(device, "roamed to a different AP");
+                nm_device_update_dynamic_ip_setup(device, FALSE, "roamed to a different AP");
             }
         }
         break;
@@ -2668,7 +2668,9 @@ supplicant_iface_notify_current_bss(NMSupplicantInterface *iface,
 
             if (nm_supplicant_interface_get_state(priv->sup_iface)
                 == NM_SUPPLICANT_INTERFACE_STATE_COMPLETED) {
-                nm_device_update_dynamic_ip_setup(NM_DEVICE(self), "roamed to a different AP");
+                nm_device_update_dynamic_ip_setup(NM_DEVICE(self),
+                                                  FALSE,
+                                                  "roamed to a different AP");
             } else {
                 /* Wait that the authentication to new the AP completes before
                  * trying to renew, otherwise the DHCP REQUEST could be lost
diff --git a/src/core/nm-manager.c b/src/core/nm-manager.c
index 7c04e8e4e5..f45ab5b64e 100644
--- a/src/core/nm-manager.c
+++ b/src/core/nm-manager.c
@@ -6578,7 +6578,7 @@ do_sleep_wake(NMManager *self, gboolean sleeping_changed)
                 && !nm_device_get_unmanaged_flags(device, NM_UNMANAGED_SLEEPING)) {
                 /* DHCP leases of software devices could have gone stale
                  * so we need to renew them. */
-                nm_device_update_dynamic_ip_setup(device, "wake up");
+                nm_device_update_dynamic_ip_setup(device, FALSE, "wake up");
                 continue;
             }