From 0ce538c7e0b9bcad3a1bea42e16e8c2911d88655 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 18 Oct 2023 13:25:34 +0200 Subject: [PATCH 1/9] device: return G_SOURCE_* instead of boolean in source callback (cherry picked from commit b88de255fc288b026686d8111924d2932f5710ed) --- src/core/devices/nm-device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 7d9506cb0e..daa75107e8 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -7149,7 +7149,7 @@ device_ip_link_changed(gpointer user_data) ip_iface = pllink->name; if (!ip_iface[0]) - return FALSE; + return G_SOURCE_REMOVE; if (!nm_streq(priv->ip_iface, ip_iface)) { _LOGI(LOGD_DEVICE, From 3ef2da25598bc386e84a8ccc407692eb804cb044 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 20 Oct 2023 17:06:55 +0200 Subject: [PATCH 2/9] ovs-interface: make sure handlers are disconnected on deactivate The deactivation can happen while we are waiting for the ifindex, and it can happen via two code paths, depending on the state. For a regular deactivation, method deactivate_async() is called. Otherwise, if the device goes directly to UNMANAGED or UNAVAILABLE, deactivate() is called. We need to make sure that signal and source handlers are disconnected, so that they are not called at the wrong time. Fixes: 99a6c6eda6e1 ('ovs, dpdk: fix creating ovs-interface when the ovs-bridge is netdev') (cherry picked from commit 164a3435745ac05e8f7251233c749fa0390ddfee) --- src/core/devices/ovs/nm-device-ovs-interface.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/core/devices/ovs/nm-device-ovs-interface.c b/src/core/devices/ovs/nm-device-ovs-interface.c index fd48c2fd0f..c797e86afb 100644 --- a/src/core/devices/ovs/nm-device-ovs-interface.c +++ b/src/core/devices/ovs/nm-device-ovs-interface.c @@ -326,6 +326,7 @@ deactivate(NMDevice *device) NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(self); priv->wait_link_is_waiting = FALSE; + nm_clear_g_signal_handler(nm_device_get_platform(device), &priv->wait_link_signal_id); nm_clear_g_source_inst(&priv->wait_link_idle_source); } @@ -418,6 +419,9 @@ deactivate_async(NMDevice *device, _LOGT(LOGD_CORE, "deactivate: start async"); + nm_clear_g_signal_handler(nm_device_get_platform(device), &priv->wait_link_signal_id); + nm_clear_g_source_inst(&priv->wait_link_idle_source); + /* We want to ensure that the kernel link for this device is * removed upon disconnection so that it will not interfere with * later activations of the same device. Unfortunately there is @@ -442,8 +446,6 @@ deactivate_async(NMDevice *device, return; } - nm_clear_g_source_inst(&priv->wait_link_idle_source); - if (priv->wait_link_is_waiting) { /* At this point we have issued an INSERT and a DELETE * command for the interface to ovsdb. We don't know if @@ -456,6 +458,7 @@ deactivate_async(NMDevice *device, } else _LOGT(LOGD_DEVICE, "deactivate: waiting for link to disappear"); + priv->wait_link_is_waiting = FALSE; data->cancelled_id = g_cancellable_connect(cancellable, G_CALLBACK(deactivate_cancelled_cb), data, NULL); data->link_changed_id = g_signal_connect(nm_device_get_platform(device), @@ -506,6 +509,10 @@ dispose(GObject *object) NMDeviceOvsInterface *self = NM_DEVICE_OVS_INTERFACE(object); NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(self); + nm_assert(!priv->wait_link_is_waiting); + nm_assert(!priv->wait_link_signal_id == 0); + nm_assert(!priv->wait_link_idle_source); + if (priv->ovsdb) { g_signal_handlers_disconnect_by_func(priv->ovsdb, G_CALLBACK(ovsdb_ready), self); g_clear_object(&priv->ovsdb); From d93176df1a5d090f5585abc1e6d303c374d71675 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Sat, 14 Oct 2023 19:29:08 +0200 Subject: [PATCH 3/9] ovs-interface: add ovs_interface_is_netdev_datapath() helper The code to determine if we are using the netdev datapath is logically separated from the code to start IP configuration; move it to its own function to make the code easier to follow. (cherry picked from commit a7a06163bea11bae4aeb166e388f258e2e514475) --- .../devices/ovs/nm-device-ovs-interface.c | 56 ++++++++++++------- 1 file changed, 37 insertions(+), 19 deletions(-) diff --git a/src/core/devices/ovs/nm-device-ovs-interface.c b/src/core/devices/ovs/nm-device-ovs-interface.c index c797e86afb..244656cbf1 100644 --- a/src/core/devices/ovs/nm-device-ovs-interface.c +++ b/src/core/devices/ovs/nm-device-ovs-interface.c @@ -252,13 +252,40 @@ _netdev_tun_link_cb(NMPlatform *platform, } } +static gboolean +ovs_interface_is_netdev_datapath(NMDeviceOvsInterface *self) +{ + NMDevice *device = NM_DEVICE(self); + NMActiveConnection *ac = NULL; + NMSettingOvsBridge *s_ovs_bridge = NULL; + + ac = NM_ACTIVE_CONNECTION(nm_device_get_act_request(device)); + if (!ac) + return FALSE; + + /* get ovs-port active-connection */ + ac = nm_active_connection_get_master(ac); + if (!ac) + return FALSE; + + /* get ovs-bridge active-connection */ + ac = nm_active_connection_get_master(ac); + if (!ac) + return FALSE; + + s_ovs_bridge = + nm_connection_get_setting_ovs_bridge(nm_active_connection_get_applied_connection(ac)); + if (!s_ovs_bridge) + return FALSE; + + return nm_streq0(nm_setting_ovs_bridge_get_datapath_type(s_ovs_bridge), "netdev"); +} + static void act_stage3_ip_config(NMDevice *device, int addr_family) { - NMActiveConnection *controller_act = NULL; - NMSettingOvsBridge *s_ovs_bridge = NULL; - NMDeviceOvsInterface *self = NM_DEVICE_OVS_INTERFACE(device); - NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(self); + NMDeviceOvsInterface *self = NM_DEVICE_OVS_INTERFACE(device); + NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(self); if (!_is_internal_interface(device)) { nm_device_devip_set_state(device, addr_family, NM_DEVICE_IP_STATE_READY, NULL); @@ -269,21 +296,12 @@ act_stage3_ip_config(NMDevice *device, int addr_family) * link created is a tun device instead of a ovs-interface. NetworkManager must * detect the creation of the tun link and attach the ifindex to the * ovs-interface device. */ - controller_act = NM_ACTIVE_CONNECTION(nm_device_get_act_request(device)); - if (controller_act && nm_device_get_ip_ifindex(device) <= 0 && priv->wait_link_signal_id == 0) { - controller_act = nm_active_connection_get_master(controller_act); - if (controller_act) { - controller_act = nm_active_connection_get_master(controller_act); - if (controller_act) - s_ovs_bridge = nm_connection_get_setting_ovs_bridge( - nm_active_connection_get_applied_connection(controller_act)); - if (s_ovs_bridge - && nm_streq0(nm_setting_ovs_bridge_get_datapath_type(s_ovs_bridge), "netdev")) - priv->wait_link_signal_id = g_signal_connect(nm_device_get_platform(device), - NM_PLATFORM_SIGNAL_LINK_CHANGED, - G_CALLBACK(_netdev_tun_link_cb), - self); - } + if (nm_device_get_ip_ifindex(device) <= 0 && priv->wait_link_signal_id == 0 + && ovs_interface_is_netdev_datapath(self)) { + priv->wait_link_signal_id = g_signal_connect(nm_device_get_platform(device), + NM_PLATFORM_SIGNAL_LINK_CHANGED, + G_CALLBACK(_netdev_tun_link_cb), + self); } /* FIXME(l3cfg): we should create the IP ifindex before stage3 start. From 008ad08660946705cc3584a1b454e49afeb3783e Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 18 Oct 2023 16:42:40 +0200 Subject: [PATCH 4/9] ovs-interface: move wait-link members to a sub-struct Group together the members of private struct related to link-waiting, and add comments to them. (cherry picked from commit f1c22699e274465146e52a9432f868bde32569fb) --- .../devices/ovs/nm-device-ovs-interface.c | 72 ++++++++++--------- 1 file changed, 40 insertions(+), 32 deletions(-) diff --git a/src/core/devices/ovs/nm-device-ovs-interface.c b/src/core/devices/ovs/nm-device-ovs-interface.c index 244656cbf1..504dbeff24 100644 --- a/src/core/devices/ovs/nm-device-ovs-interface.c +++ b/src/core/devices/ovs/nm-device-ovs-interface.c @@ -24,10 +24,17 @@ typedef struct { NMOvsdb *ovsdb; - GSource *wait_link_idle_source; - gulong wait_link_signal_id; - int wait_link_ifindex; - bool wait_link_is_waiting : 1; + + struct { + /* The source for the idle handler to set the TUN ifindex */ + GSource *tun_set_ifindex_idle_source; + /* The id for the signal watching the TUN link to appear/change */ + gulong tun_link_signal_id; + /* The TUN ifindex to set in the idle handler */ + int tun_ifindex; + /* Whether we are waiting for the kernel link */ + bool waiting : 1; + } wait_link; } NMDeviceOvsInterfacePrivate; struct _NMDeviceOvsInterface { @@ -122,10 +129,10 @@ link_changed(NMDevice *device, const NMPlatformLink *pllink) { NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(device); - if (!pllink || !priv->wait_link_is_waiting) + if (!pllink || !priv->wait_link.waiting) return; - priv->wait_link_is_waiting = FALSE; + priv->wait_link.waiting = FALSE; if (nm_device_get_state(device) == NM_DEVICE_STATE_IP_CONFIG) { if (!nm_device_hw_addr_set_cloned(device, @@ -215,10 +222,10 @@ _set_ip_ifindex_tun(gpointer user_data) NMDeviceOvsInterface *self = NM_DEVICE_OVS_INTERFACE(device); NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(self); - nm_clear_g_source_inst(&priv->wait_link_idle_source); + nm_clear_g_source_inst(&priv->wait_link.tun_set_ifindex_idle_source); - priv->wait_link_is_waiting = FALSE; - nm_device_set_ip_ifindex(device, priv->wait_link_ifindex); + priv->wait_link.waiting = FALSE; + nm_device_set_ip_ifindex(device, priv->wait_link.tun_ifindex); nm_device_link_properties_set(device, FALSE); nm_device_devip_set_state(device, AF_INET, NM_DEVICE_IP_STATE_PENDING, NULL); @@ -243,11 +250,12 @@ _netdev_tun_link_cb(NMPlatform *platform, if (change_type == NM_PLATFORM_SIGNAL_ADDED) { if (pllink->type == NM_LINK_TYPE_TUN && nm_streq0(pllink->name, nm_device_get_iface(device))) { - nm_clear_g_signal_handler(platform, &priv->wait_link_signal_id); + nm_clear_g_signal_handler(platform, &priv->wait_link.tun_link_signal_id); - priv->wait_link_ifindex = ifindex; + priv->wait_link.tun_ifindex = ifindex; - priv->wait_link_idle_source = nm_g_idle_add_source(_set_ip_ifindex_tun, device); + priv->wait_link.tun_set_ifindex_idle_source = + nm_g_idle_add_source(_set_ip_ifindex_tun, device); } } } @@ -296,12 +304,12 @@ act_stage3_ip_config(NMDevice *device, int addr_family) * link created is a tun device instead of a ovs-interface. NetworkManager must * detect the creation of the tun link and attach the ifindex to the * ovs-interface device. */ - if (nm_device_get_ip_ifindex(device) <= 0 && priv->wait_link_signal_id == 0 + if (nm_device_get_ip_ifindex(device) <= 0 && priv->wait_link.tun_link_signal_id == 0 && ovs_interface_is_netdev_datapath(self)) { - priv->wait_link_signal_id = g_signal_connect(nm_device_get_platform(device), - NM_PLATFORM_SIGNAL_LINK_CHANGED, - G_CALLBACK(_netdev_tun_link_cb), - self); + priv->wait_link.tun_link_signal_id = g_signal_connect(nm_device_get_platform(device), + NM_PLATFORM_SIGNAL_LINK_CHANGED, + G_CALLBACK(_netdev_tun_link_cb), + self); } /* FIXME(l3cfg): we should create the IP ifindex before stage3 start. @@ -313,14 +321,14 @@ act_stage3_ip_config(NMDevice *device, int addr_family) * This should change. */ if (nm_device_get_ip_ifindex(device) <= 0) { _LOGT(LOGD_DEVICE, "waiting for link to appear"); - priv->wait_link_is_waiting = TRUE; + priv->wait_link.waiting = TRUE; nm_device_devip_set_state(device, addr_family, NM_DEVICE_IP_STATE_PENDING, NULL); return; } - priv->wait_link_is_waiting = FALSE; - nm_clear_g_source_inst(&priv->wait_link_idle_source); - nm_clear_g_signal_handler(nm_device_get_platform(device), &priv->wait_link_signal_id); + priv->wait_link.waiting = FALSE; + nm_clear_g_source_inst(&priv->wait_link.tun_set_ifindex_idle_source); + nm_clear_g_signal_handler(nm_device_get_platform(device), &priv->wait_link.tun_link_signal_id); if (!nm_device_hw_addr_set_cloned(device, nm_device_get_applied_connection(device), FALSE)) { nm_device_devip_set_failed(device, addr_family, NM_DEVICE_STATE_REASON_CONFIG_FAILED); @@ -343,9 +351,9 @@ deactivate(NMDevice *device) NMDeviceOvsInterface *self = NM_DEVICE_OVS_INTERFACE(device); NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(self); - priv->wait_link_is_waiting = FALSE; - nm_clear_g_signal_handler(nm_device_get_platform(device), &priv->wait_link_signal_id); - nm_clear_g_source_inst(&priv->wait_link_idle_source); + priv->wait_link.waiting = FALSE; + nm_clear_g_signal_handler(nm_device_get_platform(device), &priv->wait_link.tun_link_signal_id); + nm_clear_g_source_inst(&priv->wait_link.tun_set_ifindex_idle_source); } typedef struct { @@ -437,8 +445,8 @@ deactivate_async(NMDevice *device, _LOGT(LOGD_CORE, "deactivate: start async"); - nm_clear_g_signal_handler(nm_device_get_platform(device), &priv->wait_link_signal_id); - nm_clear_g_source_inst(&priv->wait_link_idle_source); + nm_clear_g_signal_handler(nm_device_get_platform(device), &priv->wait_link.tun_link_signal_id); + nm_clear_g_source_inst(&priv->wait_link.tun_set_ifindex_idle_source); /* We want to ensure that the kernel link for this device is * removed upon disconnection so that it will not interfere with @@ -455,7 +463,7 @@ deactivate_async(NMDevice *device, .callback_user_data = callback_user_data, }; - if (!priv->wait_link_is_waiting + if (!priv->wait_link.waiting && !nm_platform_link_get_by_ifname(nm_device_get_platform(device), nm_device_get_iface(device))) { _LOGT(LOGD_CORE, "deactivate: link not present, proceeding"); @@ -464,7 +472,7 @@ deactivate_async(NMDevice *device, return; } - if (priv->wait_link_is_waiting) { + if (priv->wait_link.waiting) { /* At this point we have issued an INSERT and a DELETE * command for the interface to ovsdb. We don't know if * vswitchd will see the two updates or only one. We @@ -476,7 +484,7 @@ deactivate_async(NMDevice *device, } else _LOGT(LOGD_DEVICE, "deactivate: waiting for link to disappear"); - priv->wait_link_is_waiting = FALSE; + priv->wait_link.waiting = FALSE; data->cancelled_id = g_cancellable_connect(cancellable, G_CALLBACK(deactivate_cancelled_cb), data, NULL); data->link_changed_id = g_signal_connect(nm_device_get_platform(device), @@ -527,9 +535,9 @@ dispose(GObject *object) NMDeviceOvsInterface *self = NM_DEVICE_OVS_INTERFACE(object); NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(self); - nm_assert(!priv->wait_link_is_waiting); - nm_assert(!priv->wait_link_signal_id == 0); - nm_assert(!priv->wait_link_idle_source); + nm_assert(!priv->wait_link.waiting); + nm_assert(priv->wait_link.tun_link_signal_id == 0); + nm_assert(!priv->wait_link.tun_set_ifindex_idle_source); if (priv->ovsdb) { g_signal_handlers_disconnect_by_func(priv->ovsdb, G_CALLBACK(ovsdb_ready), self); From 1f2cf7d1f5cd0809f5328c23923ef25cf81ff2be Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 18 Oct 2023 16:55:31 +0200 Subject: [PATCH 5/9] ovs-interface: add trace messages when waiting for link Also, add prefix "ovs-wait-link" to all messages related to waiting for the ovs link, so that they can be easily spotted in logs. (cherry picked from commit 49a7bd110df74da5cf3866caebe6975460990395) --- .../devices/ovs/nm-device-ovs-interface.c | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/core/devices/ovs/nm-device-ovs-interface.c b/src/core/devices/ovs/nm-device-ovs-interface.c index 504dbeff24..a3f86960a1 100644 --- a/src/core/devices/ovs/nm-device-ovs-interface.c +++ b/src/core/devices/ovs/nm-device-ovs-interface.c @@ -127,7 +127,8 @@ check_connection_compatible(NMDevice *device, static void link_changed(NMDevice *device, const NMPlatformLink *pllink) { - NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(device); + NMDeviceOvsInterface *self = NM_DEVICE_OVS_INTERFACE(device); + NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(self); if (!pllink || !priv->wait_link.waiting) return; @@ -143,6 +144,8 @@ link_changed(NMDevice *device, const NMPlatformLink *pllink) return; } + _LOGT(LOGD_CORE, "ovs-wait-link: link is ready after link changed event"); + nm_device_link_properties_set(device, FALSE); nm_device_bring_up(device); @@ -222,6 +225,10 @@ _set_ip_ifindex_tun(gpointer user_data) NMDeviceOvsInterface *self = NM_DEVICE_OVS_INTERFACE(device); NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(self); + _LOGT(LOGD_CORE, + "ovs-wait-link: setting ip-ifindex %d from tun interface", + priv->wait_link.tun_ifindex); + nm_clear_g_source_inst(&priv->wait_link.tun_set_ifindex_idle_source); priv->wait_link.waiting = FALSE; @@ -250,10 +257,11 @@ _netdev_tun_link_cb(NMPlatform *platform, if (change_type == NM_PLATFORM_SIGNAL_ADDED) { if (pllink->type == NM_LINK_TYPE_TUN && nm_streq0(pllink->name, nm_device_get_iface(device))) { + _LOGT(LOGD_CORE, + "ovs-wait-link: found matching tun interface, schedule set-ip-ifindex(%d)", + ifindex); nm_clear_g_signal_handler(platform, &priv->wait_link.tun_link_signal_id); - priv->wait_link.tun_ifindex = ifindex; - priv->wait_link.tun_set_ifindex_idle_source = nm_g_idle_add_source(_set_ip_ifindex_tun, device); } @@ -320,12 +328,16 @@ act_stage3_ip_config(NMDevice *device, int addr_family) * * This should change. */ if (nm_device_get_ip_ifindex(device) <= 0) { - _LOGT(LOGD_DEVICE, "waiting for link to appear"); + _LOGT(LOGD_DEVICE, "ovs-wait-link: waiting for link to appear"); priv->wait_link.waiting = TRUE; nm_device_devip_set_state(device, addr_family, NM_DEVICE_IP_STATE_PENDING, NULL); return; } + _LOGT(LOGD_DEVICE, + "ovs-wait-link: link is ready, IPv%c can proceed", + nm_utils_addr_family_to_char(addr_family)); + priv->wait_link.waiting = FALSE; nm_clear_g_source_inst(&priv->wait_link.tun_set_ifindex_idle_source); nm_clear_g_signal_handler(nm_device_get_platform(device), &priv->wait_link.tun_link_signal_id); From ac0ae3eada26d9058b0a50573daba8276a9692b9 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 17 Oct 2023 16:34:20 +0200 Subject: [PATCH 6/9] ovs-interface: improve comments (cherry picked from commit c7f1e3719f375418c0be282722dc925be4897b04) --- .../devices/ovs/nm-device-ovs-interface.c | 69 ++++++++++++------- 1 file changed, 46 insertions(+), 23 deletions(-) diff --git a/src/core/devices/ovs/nm-device-ovs-interface.c b/src/core/devices/ovs/nm-device-ovs-interface.c index a3f86960a1..27668d36ee 100644 --- a/src/core/devices/ovs/nm-device-ovs-interface.c +++ b/src/core/devices/ovs/nm-device-ovs-interface.c @@ -254,6 +254,7 @@ _netdev_tun_link_cb(NMPlatform *platform, NMDeviceOvsInterface *self = NM_DEVICE_OVS_INTERFACE(device); NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(self); + /* Monitor all platform events to see when a suitable tun interface appears */ if (change_type == NM_PLATFORM_SIGNAL_ADDED) { if (pllink->type == NM_LINK_TYPE_TUN && nm_streq0(pllink->name, nm_device_get_iface(device))) { @@ -303,15 +304,39 @@ act_stage3_ip_config(NMDevice *device, int addr_family) NMDeviceOvsInterface *self = NM_DEVICE_OVS_INTERFACE(device); NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(self); + /* + * When the ovs-interface device enters stage3, it becomes eligible to be attached to + * its controller (a ovs-port). If also the ovs-bridge is ready, an entry is created + * in the ovsdb in NMDeviceOvsPort->attach_port(). + * FIXME(l3cfg): we should create the IP ifindex before stage3 start. + * + * NMDeviceOvsInterface->act_stage3_ip_config() is supposed to perform device-specific + * IP configuration on the device. An ovs-interface can be of different types, that + * require different handling: + * + * - "patch" and "dpdk" interfaces don't have any kernel link associated and thus + * NetworkManager completely skips any kind of IP configuration on them, by returning + * FALSE to ->ready_for_ip_config(). + * + * - "system" interfaces represent other interface types with kernel link (for + * example, ethernet, bond, etc.) that get attached to a ovs bridge. Once they are + * attached, NetworkManager can start the IP configuration right away. + * + * - "internal" interfaces are virtual interfaces created by openvswitch. Once the + * entry is created in the ovsdb, the kernel will create a link for the + * interface. When using the system datapath (the default), the link is of type + * "openvswitch", while when using the netdev (userspace) datapath, the link is a tun + * (tap) one. For both datapath types, we use this method to delay the IP + * configuration until the link appears. Note that ready_for_ip_config() returns FALSE + * when there is no ifindex, and so all the regular IP methods (static, auto, etc.) + * can't proceed. + */ + if (!_is_internal_interface(device)) { nm_device_devip_set_state(device, addr_family, NM_DEVICE_IP_STATE_READY, NULL); return; } - /* When the ovs-bridge controller is using netdev datapath, the interface - * link created is a tun device instead of a ovs-interface. NetworkManager must - * detect the creation of the tun link and attach the ifindex to the - * ovs-interface device. */ if (nm_device_get_ip_ifindex(device) <= 0 && priv->wait_link.tun_link_signal_id == 0 && ovs_interface_is_netdev_datapath(self)) { priv->wait_link.tun_link_signal_id = g_signal_connect(nm_device_get_platform(device), @@ -320,13 +345,6 @@ act_stage3_ip_config(NMDevice *device, int addr_family) self); } - /* FIXME(l3cfg): we should create the IP ifindex before stage3 start. - * - * For now it's here because when the ovs-interface enters stage3, then it's added to the - * controller (ovs-port) and the entry is create in the ovsdb. Only after that the kernel - * link appears. - * - * This should change. */ if (nm_device_get_ip_ifindex(device) <= 0) { _LOGT(LOGD_DEVICE, "ovs-wait-link: waiting for link to appear"); priv->wait_link.waiting = TRUE; @@ -460,13 +478,22 @@ deactivate_async(NMDevice *device, nm_clear_g_signal_handler(nm_device_get_platform(device), &priv->wait_link.tun_link_signal_id); nm_clear_g_source_inst(&priv->wait_link.tun_set_ifindex_idle_source); - /* We want to ensure that the kernel link for this device is - * removed upon disconnection so that it will not interfere with - * later activations of the same device. Unfortunately there is - * no synchronization mechanism with vswitchd, we only update - * ovsdb and wait that changes are picked up. + /* We want to ensure that the kernel link for this device is removed upon + * disconnection, so that it will not interfere with later activations of the same + * device. + * + * To do so, we need to be very careful, because unfortunately there is no + * synchronization mechanism with vswitchd: we only update ovsdb, wait that changes + * are picked up and we see the effects on the kernel interface (appearing or going + * away). + * + * That means for example that if the ovs interface entered stage3 and the entry was + * added to the ovsdb, we expect a link to appear. If we disconnect at this point, we + * delete the entry from the ovsdb. Now we don't know if ovs-vswitchd will see two + * updates or only one. In other words, we don't know if the interface will appear and + * go away, or if it will not appear ever. In this situation, the solution is to wait + * with a timeout. */ - data = g_slice_new(DeactivateData); *data = (DeactivateData){ .self = g_object_ref(self), @@ -485,12 +512,8 @@ deactivate_async(NMDevice *device, } if (priv->wait_link.waiting) { - /* At this point we have issued an INSERT and a DELETE - * command for the interface to ovsdb. We don't know if - * vswitchd will see the two updates or only one. We - * must add a timeout to avoid waiting forever in case - * the link doesn't appear. - */ + /* Here we have issued an INSERT and a DELETE command for the interface to ovsdb, + * and must wait with a timeout. */ data->link_timeout_id = g_timeout_add(6000, deactivate_link_timeout, data); _LOGT(LOGD_DEVICE, "deactivate: waiting for link to disappear in 6 seconds"); } else From 08ffcf22787fc1b492c100737df238e09baa385b Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 18 Oct 2023 17:12:14 +0200 Subject: [PATCH 7/9] ovs-interface: fix state change in link_changed() The function checks that priv->wait_link.waiting is set. Since the flag is only set in stage3, it is wrong to schedule stage2 again. (cherry picked from commit 01a6a2dc15338f4412e437ec457b33b600129fcc) --- .../devices/ovs/nm-device-ovs-interface.c | 32 ++++++++----------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/src/core/devices/ovs/nm-device-ovs-interface.c b/src/core/devices/ovs/nm-device-ovs-interface.c index 27668d36ee..3891279f88 100644 --- a/src/core/devices/ovs/nm-device-ovs-interface.c +++ b/src/core/devices/ovs/nm-device-ovs-interface.c @@ -133,29 +133,25 @@ link_changed(NMDevice *device, const NMPlatformLink *pllink) if (!pllink || !priv->wait_link.waiting) return; + if (nm_device_get_state(device) != NM_DEVICE_STATE_IP_CONFIG) + return; + priv->wait_link.waiting = FALSE; - if (nm_device_get_state(device) == NM_DEVICE_STATE_IP_CONFIG) { - if (!nm_device_hw_addr_set_cloned(device, - nm_device_get_applied_connection(device), - FALSE)) { - nm_device_devip_set_failed(device, AF_INET, NM_DEVICE_STATE_REASON_CONFIG_FAILED); - nm_device_devip_set_failed(device, AF_INET6, NM_DEVICE_STATE_REASON_CONFIG_FAILED); - return; - } - - _LOGT(LOGD_CORE, "ovs-wait-link: link is ready after link changed event"); - - nm_device_link_properties_set(device, FALSE); - nm_device_bring_up(device); - - nm_device_devip_set_state(device, AF_INET, NM_DEVICE_IP_STATE_PENDING, NULL); - nm_device_devip_set_state(device, AF_INET6, NM_DEVICE_IP_STATE_PENDING, NULL); - nm_device_activate_schedule_stage3_ip_config(device, FALSE); + if (!nm_device_hw_addr_set_cloned(device, nm_device_get_applied_connection(device), FALSE)) { + nm_device_devip_set_failed(device, AF_INET, NM_DEVICE_STATE_REASON_CONFIG_FAILED); + nm_device_devip_set_failed(device, AF_INET6, NM_DEVICE_STATE_REASON_CONFIG_FAILED); return; } - nm_device_activate_schedule_stage2_device_config(device, FALSE); + _LOGT(LOGD_CORE, "ovs-wait-link: link is ready after link changed event"); + + nm_device_link_properties_set(device, FALSE); + nm_device_bring_up(device); + + nm_device_devip_set_state(device, AF_INET, NM_DEVICE_IP_STATE_PENDING, NULL); + nm_device_devip_set_state(device, AF_INET6, NM_DEVICE_IP_STATE_PENDING, NULL); + nm_device_activate_schedule_stage3_ip_config(device, FALSE); } static gboolean From 9f01713824d232addca61b03d73c9929c3d9e727 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 18 Oct 2023 17:26:15 +0200 Subject: [PATCH 8/9] ovs-interface: add check_waiting_for_link() Add a helper function to check whether the ovs link is ready. In the next commit, a new condition will be added to the helper. (cherry picked from commit 3ad82e272641a3a9178bab18003421880ebf9662) --- .../devices/ovs/nm-device-ovs-interface.c | 52 ++++++++++++++----- 1 file changed, 40 insertions(+), 12 deletions(-) diff --git a/src/core/devices/ovs/nm-device-ovs-interface.c b/src/core/devices/ovs/nm-device-ovs-interface.c index 3891279f88..48dea7f992 100644 --- a/src/core/devices/ovs/nm-device-ovs-interface.c +++ b/src/core/devices/ovs/nm-device-ovs-interface.c @@ -124,6 +124,35 @@ check_connection_compatible(NMDevice *device, return TRUE; } +static gboolean +check_waiting_for_link(NMDevice *device, const char *from) +{ + NMDeviceOvsInterface *self = NM_DEVICE_OVS_INTERFACE(device); + NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(self); + NMPlatform *platform = nm_device_get_platform(device); + const NMPlatformLink *pllink; + int ip_ifindex; + const char *reason = NULL; + + if (!priv->wait_link.waiting) + return FALSE; + + ip_ifindex = nm_device_get_ip_ifindex(device); + + if (ip_ifindex <= 0) { + reason = "no ifindex"; + } else if (!(pllink = nm_platform_link_get(platform, ip_ifindex))) { + reason = "platform link not found"; + } else { + priv->wait_link.waiting = FALSE; + } + + if (priv->wait_link.waiting) + _LOGT(LOGD_DEVICE, "ovs-wait-link(%s): not ready: %s", from, reason); + + return priv->wait_link.waiting; +} + static void link_changed(NMDevice *device, const NMPlatformLink *pllink) { @@ -136,7 +165,8 @@ link_changed(NMDevice *device, const NMPlatformLink *pllink) if (nm_device_get_state(device) != NM_DEVICE_STATE_IP_CONFIG) return; - priv->wait_link.waiting = FALSE; + if (check_waiting_for_link(device, "link-changed")) + return; if (!nm_device_hw_addr_set_cloned(device, nm_device_get_applied_connection(device), FALSE)) { nm_device_devip_set_failed(device, AF_INET, NM_DEVICE_STATE_REASON_CONFIG_FAILED); @@ -333,18 +363,16 @@ act_stage3_ip_config(NMDevice *device, int addr_family) return; } - if (nm_device_get_ip_ifindex(device) <= 0 && priv->wait_link.tun_link_signal_id == 0 - && ovs_interface_is_netdev_datapath(self)) { - priv->wait_link.tun_link_signal_id = g_signal_connect(nm_device_get_platform(device), - NM_PLATFORM_SIGNAL_LINK_CHANGED, - G_CALLBACK(_netdev_tun_link_cb), - self); - } - - if (nm_device_get_ip_ifindex(device) <= 0) { - _LOGT(LOGD_DEVICE, "ovs-wait-link: waiting for link to appear"); - priv->wait_link.waiting = TRUE; + priv->wait_link.waiting = TRUE; + if (check_waiting_for_link(device, addr_family == AF_INET ? "stage3-ipv4" : "stage3-ipv6")) { nm_device_devip_set_state(device, addr_family, NM_DEVICE_IP_STATE_PENDING, NULL); + if (nm_device_get_ip_ifindex(device) <= 0 && priv->wait_link.tun_link_signal_id == 0 + && ovs_interface_is_netdev_datapath(self)) { + priv->wait_link.tun_link_signal_id = g_signal_connect(nm_device_get_platform(device), + NM_PLATFORM_SIGNAL_LINK_CHANGED, + G_CALLBACK(_netdev_tun_link_cb), + self); + } return; } From fadabfddb999766b70e2617b72748d2dd232b473 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 18 Oct 2023 13:28:25 +0200 Subject: [PATCH 9/9] ovs-interface: wait that the cloned MAC changes instead of setting it If a ovs interface has the cloned-mac-address property set, we pass the desired MAC to ovsdb when creating the db entry, and openvswitch will eventually assign it to the interface. Note that usually the link will not have the desired MAC when it's created. Therefore, currently we also change the MAC via netlink before proceeding with IP configuration. This is important to make sure that ARP announcements, DHCP client-id, etc. will use the correct MAC address. This doesn't work when using the "netdev" (userspace) datapath, as the attempts to change the MAC of the tun interface via netlink fail, leading to an activation failure. To properly handle both cases in the same way, adopt a different strategy: now we don't set the MAC address explicitly via netlink but we only wait until ovs does that. (cherry picked from commit acf485196c6e73833f6b26db1f69139d0afdcc8c) --- .../devices/ovs/nm-device-ovs-interface.c | 114 ++++++++++++++---- 1 file changed, 90 insertions(+), 24 deletions(-) diff --git a/src/core/devices/ovs/nm-device-ovs-interface.c b/src/core/devices/ovs/nm-device-ovs-interface.c index 48dea7f992..17eb2c2d12 100644 --- a/src/core/devices/ovs/nm-device-ovs-interface.c +++ b/src/core/devices/ovs/nm-device-ovs-interface.c @@ -28,10 +28,14 @@ typedef struct { struct { /* The source for the idle handler to set the TUN ifindex */ GSource *tun_set_ifindex_idle_source; + /* The cloned MAC to set */ + char *cloned_mac; /* The id for the signal watching the TUN link to appear/change */ gulong tun_link_signal_id; /* The TUN ifindex to set in the idle handler */ int tun_ifindex; + /* Whether we have determined the cloned MAC */ + bool cloned_mac_evaluated : 1; /* Whether we are waiting for the kernel link */ bool waiting : 1; } wait_link; @@ -53,6 +57,13 @@ G_DEFINE_TYPE(NMDeviceOvsInterface, nm_device_ovs_interface, NM_TYPE_DEVICE) /*****************************************************************************/ +static void _netdev_tun_link_cb(NMPlatform *platform, + int obj_type_i, + int ifindex, + NMPlatformLink *pllink, + int change_type_i, + NMDevice *device); + static const char * get_type_description(NMDevice *device) { @@ -137,12 +148,19 @@ check_waiting_for_link(NMDevice *device, const char *from) if (!priv->wait_link.waiting) return FALSE; + nm_assert(priv->wait_link.cloned_mac_evaluated); ip_ifindex = nm_device_get_ip_ifindex(device); if (ip_ifindex <= 0) { reason = "no ifindex"; } else if (!(pllink = nm_platform_link_get(platform, ip_ifindex))) { reason = "platform link not found"; + } else if (priv->wait_link.cloned_mac + && !nm_utils_hwaddr_matches(priv->wait_link.cloned_mac, + -1, + pllink->l_address.data, + pllink->l_address.len)) { + reason = "cloned MAC address is not set yet"; } else { priv->wait_link.waiting = FALSE; } @@ -168,12 +186,6 @@ link_changed(NMDevice *device, const NMPlatformLink *pllink) if (check_waiting_for_link(device, "link-changed")) return; - if (!nm_device_hw_addr_set_cloned(device, nm_device_get_applied_connection(device), FALSE)) { - nm_device_devip_set_failed(device, AF_INET, NM_DEVICE_STATE_REASON_CONFIG_FAILED); - nm_device_devip_set_failed(device, AF_INET6, NM_DEVICE_STATE_REASON_CONFIG_FAILED); - return; - } - _LOGT(LOGD_CORE, "ovs-wait-link: link is ready after link changed event"); nm_device_link_properties_set(device, FALSE); @@ -241,7 +253,9 @@ set_platform_mtu(NMDevice *device, guint32 mtu) static gboolean ready_for_ip_config(NMDevice *device, gboolean is_manual) { - return nm_device_get_ip_ifindex(device) > 0; + NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(device); + + return nm_device_get_ip_ifindex(device) > 0 && !priv->wait_link.waiting; } static gboolean @@ -257,8 +271,22 @@ _set_ip_ifindex_tun(gpointer user_data) nm_clear_g_source_inst(&priv->wait_link.tun_set_ifindex_idle_source); - priv->wait_link.waiting = FALSE; nm_device_set_ip_ifindex(device, priv->wait_link.tun_ifindex); + + if (check_waiting_for_link(device, "set-ip-ifindex-tun")) { + /* If the link is not ready, it means the MAC is not set yet. We don't have + * a convenient way to monitor for ip-ifindex changes other than listening + * for platform events again.*/ + nm_assert(!priv->wait_link.tun_link_signal_id); + priv->wait_link.tun_link_signal_id = g_signal_connect(nm_device_get_platform(device), + NM_PLATFORM_SIGNAL_LINK_CHANGED, + G_CALLBACK(_netdev_tun_link_cb), + self); + return G_SOURCE_CONTINUE; + } + + _LOGT(LOGD_CORE, "tun link is ready"); + nm_device_link_properties_set(device, FALSE); nm_device_devip_set_state(device, AF_INET, NM_DEVICE_IP_STATE_PENDING, NULL); @@ -279,20 +307,40 @@ _netdev_tun_link_cb(NMPlatform *platform, const NMPlatformSignalChangeType change_type = change_type_i; NMDeviceOvsInterface *self = NM_DEVICE_OVS_INTERFACE(device); NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(self); + int ip_ifindex; + + if (pllink->type != NM_LINK_TYPE_TUN || !nm_streq0(pllink->name, nm_device_get_iface(device))) + return; + + ip_ifindex = nm_device_get_ip_ifindex(device); + if (ip_ifindex > 0) { + /* When we have an ifindex, we are only waiting for the MAC to settle */ + if (change_type != NM_PLATFORM_SIGNAL_CHANGED) + return; + + if (!check_waiting_for_link(device, "tun-link-changed")) { + _LOGT(LOGD_CORE, "ovs-wait-link: tun link is ready, cloned MAC is set"); - /* Monitor all platform events to see when a suitable tun interface appears */ - if (change_type == NM_PLATFORM_SIGNAL_ADDED) { - if (pllink->type == NM_LINK_TYPE_TUN - && nm_streq0(pllink->name, nm_device_get_iface(device))) { - _LOGT(LOGD_CORE, - "ovs-wait-link: found matching tun interface, schedule set-ip-ifindex(%d)", - ifindex); nm_clear_g_signal_handler(platform, &priv->wait_link.tun_link_signal_id); - priv->wait_link.tun_ifindex = ifindex; - priv->wait_link.tun_set_ifindex_idle_source = - nm_g_idle_add_source(_set_ip_ifindex_tun, device); + nm_device_link_properties_set(device, FALSE); + + nm_device_devip_set_state(device, AF_INET, NM_DEVICE_IP_STATE_PENDING, NULL); + nm_device_devip_set_state(device, AF_INET6, NM_DEVICE_IP_STATE_PENDING, NULL); + nm_device_activate_schedule_stage3_ip_config(device, FALSE); } + return; } + + /* No ip-ifindex on the device, set it when the link appears */ + if (change_type != NM_PLATFORM_SIGNAL_ADDED) + return; + + _LOGT(LOGD_CORE, + "ovs-wait-link: found matching tun interface, schedule set-ip-ifindex(%d)", + ifindex); + nm_clear_g_signal_handler(platform, &priv->wait_link.tun_link_signal_id); + priv->wait_link.tun_ifindex = ifindex; + priv->wait_link.tun_set_ifindex_idle_source = nm_g_idle_add_source(_set_ip_ifindex_tun, device); } static gboolean @@ -363,6 +411,25 @@ act_stage3_ip_config(NMDevice *device, int addr_family) return; } + /* + * If a ovs interface has the cloned-mac-address property set, we pass the desired MAC + * to ovsdb when creating the db entry, and openvswitch will eventually assign it to + * the interface. Note that usually the link will not have the desired MAC when it's + * created, and so we need to also monitor link changes to detect when the MAC is + * ready; only after that we can start IP configuration. Otherwise, the ARP + * announcements, the DHCP client-id, etc will use the wrong MAC. + */ + if (!priv->wait_link.cloned_mac_evaluated) { + nm_assert(!priv->wait_link.cloned_mac); + nm_device_hw_addr_get_cloned(device, + nm_device_get_applied_connection(device), + FALSE, + &priv->wait_link.cloned_mac, + NULL, + NULL); + priv->wait_link.cloned_mac_evaluated = TRUE; + } + priv->wait_link.waiting = TRUE; if (check_waiting_for_link(device, addr_family == AF_INET ? "stage3-ipv4" : "stage3-ipv6")) { nm_device_devip_set_state(device, addr_family, NM_DEVICE_IP_STATE_PENDING, NULL); @@ -384,11 +451,6 @@ act_stage3_ip_config(NMDevice *device, int addr_family) nm_clear_g_source_inst(&priv->wait_link.tun_set_ifindex_idle_source); nm_clear_g_signal_handler(nm_device_get_platform(device), &priv->wait_link.tun_link_signal_id); - if (!nm_device_hw_addr_set_cloned(device, nm_device_get_applied_connection(device), FALSE)) { - nm_device_devip_set_failed(device, addr_family, NM_DEVICE_STATE_REASON_CONFIG_FAILED); - return; - } - nm_device_link_properties_set(device, FALSE); nm_device_devip_set_state(device, addr_family, NM_DEVICE_IP_STATE_READY, NULL); } @@ -405,7 +467,9 @@ deactivate(NMDevice *device) NMDeviceOvsInterface *self = NM_DEVICE_OVS_INTERFACE(device); NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE(self); - priv->wait_link.waiting = FALSE; + priv->wait_link.waiting = FALSE; + priv->wait_link.cloned_mac_evaluated = FALSE; + nm_clear_g_free(&priv->wait_link.cloned_mac); nm_clear_g_signal_handler(nm_device_get_platform(device), &priv->wait_link.tun_link_signal_id); nm_clear_g_source_inst(&priv->wait_link.tun_set_ifindex_idle_source); } @@ -501,6 +565,8 @@ deactivate_async(NMDevice *device, nm_clear_g_signal_handler(nm_device_get_platform(device), &priv->wait_link.tun_link_signal_id); nm_clear_g_source_inst(&priv->wait_link.tun_set_ifindex_idle_source); + priv->wait_link.cloned_mac_evaluated = FALSE; + nm_clear_g_free(&priv->wait_link.cloned_mac); /* We want to ensure that the kernel link for this device is removed upon * disconnection, so that it will not interfere with later activations of the same