From e773559d9d92b6a59295629e41541345cf97f615 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 21 Mar 2023 15:27:54 +0100 Subject: [PATCH 1/2] device: schedule an idle commit when setting device's sys-iface-state When assuming a device, the NMActiveConnection switches the sys-iface-state from "assume" to "managed" when the device reaches the activated state. [1679353062.8884] active-connection[000055bd310b92e0]: set state activated (was activating) [1679353062.8885] active-connection[000055bd310b92e0]: update activation type from assume to managed Note that the "assume" state is probably a misfeature, and should be dropped in favor of more appropriate flags. Meaning, "assume" state for the most part is very similar to sys-iface-state "managed", and the cases where (during activation) we need to be graceful, may be better covered with other (more specialized) state flags. Regardless, for most practical purposes, sys-iface-state "assume" should be treated similar to "managed" state. When we fully activated, we should be sure to do yet another idle commit. Note that scheduling an idle-commit is something that must always be allowed to any users of NML3Cfg. The users have no knowledge about each other and coordinate by registering their commit type handles. Issuing an idle "auto" commit must be therefore allowed to them at any time. If that were not the case, then there would be a bug to fix. The only reason to maybe not do it, is when we are sure there is nothing to commit and we would want to avoid unnecessary work. You can easily reproduce this and see that we don't in fact schedule a commit after becoming managed. A commit usually only happens later, for example when we receive an autoconf6 update. This affects for example setting the MTU. Currently, _commit_mtu() bails out for nm_device_sys_iface_state_is_external_or_assume() and thus during activation the MTU will not be set. Later, once we reach activated state, due to this it still is not set right away. This patch fixes that, although we should also change _commit_mtu() to not bail out for sys-iface-state "assume". --- src/core/devices/nm-device.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 497108b1c1..f305ddeb7d 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -3043,6 +3043,7 @@ nm_device_sys_iface_state_set(NMDevice *self, NMDeviceSysIfaceState sys_iface_st nm_device_sys_iface_state_to_string(sys_iface_state)); priv->sys_iface_state_ = sys_iface_state; _dev_l3_cfg_commit_type_reset(self); + nm_device_l3cfg_commit(self, NM_L3_CFG_COMMIT_TYPE_AUTO, FALSE); } /* this function only sets a flag, no immediate actions are initiated. From 15101447c327044d8657ef961245fa9b703880aa Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 21 Mar 2023 09:08:20 +0100 Subject: [PATCH 2/2] device: also configure MTU while assuming devices The sys-iface-state "assume" means to gracefully take over a device (for example, after a restart). The end result is a fully managed interface. The flag only has meaning while activating, and for most practical purposes, such devices should be treated the same as fully activated ones. Without this, the MTU is not reset until the device reaches fully activated state, at which point the sys-iface-state switches from "assume" to "managed". With the previous commit, at that point we also schedule an idle commit, which ends up also setting the MTU. Before that, the MTU was only reset some undefined time later, when we happened to do another NML3Cfg commit. Nonetheless, even waiting until we reach fully activated state is wrong. Also during activation, commit the MTU. I guess, what theoretically could happen is that we get our MTU via ip-config (like DHCP). Then, during assuming we hit _commit_mtu() without having the DHCP lease yet. This happens after a restart, so it would be wrong to first reset the MTU, before we re-receive the DHCP lease. However, if the MTU is really to be set due via NM_DEVICE_MTU_SOURCE_IP_CONFIG, then all other MTU sources are also not in effect (because ip-config has a low priority). In that case, we would not have an MTU to reset and the code would not commit a new MTU. Thus this should still be fine, also during activation when we didn't yet get the DHCP lease (or other information to dynamically set the MTU). --- src/core/devices/nm-device.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index f305ddeb7d..6723d1415a 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -11327,10 +11327,8 @@ _commit_mtu(NMDevice *self) if (ifindex <= 0) return; - if (!nm_device_get_applied_connection(self) - || nm_device_sys_iface_state_is_external_or_assume(self)) { - /* we don't tamper with the MTU of disconnected and - * external/assumed devices. */ + if (!nm_device_get_applied_connection(self) || nm_device_sys_iface_state_is_external(self)) { + /* we don't tamper with the MTU of disconnected and external devices. */ return; }