From f54c5400c8e43e71fb7c4e1e705866862af69ccd Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 1 Jul 2020 10:55:01 +0200 Subject: [PATCH 1/3] ovs: set MAC address on the bridge for local interfaces When a user creates a ovs-interface with the same name of the parent ovs-bridge, openvswitch considers the interface as the "local interface" [1] and assigns the MAC address of the bridge to the interface [2]. This is confusing for users, as the cloned MAC property is ignored in some cases, depending on the ovs-interface name. Instead, detect when the interface is local and set the MAC from the ovs-interface connection in the bridge table. [1] https://github.com/openvswitch/ovs/blob/v2.13.0/vswitchd/vswitch.xml#L2546 [2] https://github.com/openvswitch/ovs/blob/v2.13.0/vswitchd/bridge.c#L4744 (cherry picked from commit 5d4c8521a38c166a9a0aafe5be2bd0545084e154) (cherry picked from commit 7548c29a89e12349fdfe196e96d65a6abae74cb5) (cherry picked from commit 127294babc23782781c17192c52a2d57f7916170) --- src/devices/ovs/nm-ovsdb.c | 114 +++++++++++++++++++++++++------------ 1 file changed, 77 insertions(+), 37 deletions(-) diff --git a/src/devices/ovs/nm-ovsdb.c b/src/devices/ovs/nm-ovsdb.c index e005ea9330..03c8ccad37 100644 --- a/src/devices/ovs/nm-ovsdb.c +++ b/src/devices/ovs/nm-ovsdb.c @@ -309,6 +309,18 @@ _set_bridge_ports (json_t *params, const char *ifname, json_t *new_ports) ); } +static void +_set_bridge_mac (json_t *params, const char *ifname, const char *mac) +{ + json_array_append_new (params, + json_pack ("{s:s, s:s, s:{s:[s, [[s, s]]]}, s:[[s, s, s]]}", + "op", "update", "table", "Bridge", + "row", "other_config", "map", + "hwaddr", mac, + "where", "name", "==", ifname) + ); +} + /** * _expect_port_interfaces: * @@ -352,15 +364,16 @@ _set_port_interfaces (json_t *params, const char *ifname, json_t *new_interfaces * Returns an commands that adds new interface from a given connection. */ static void -_insert_interface (json_t *params, NMConnection *interface, NMDevice *interface_device) +_insert_interface (json_t *params, + NMConnection *interface, + NMDevice *interface_device, + const char *cloned_mac) { const char *type = NULL; NMSettingOvsInterface *s_ovs_iface; NMSettingOvsDpdk *s_ovs_dpdk; NMSettingOvsPatch *s_ovs_patch; json_t *options = json_array (); - gs_free char *cloned_mac = NULL; - gs_free_error GError *error = NULL; json_t *row; guint32 mtu = 0; @@ -376,18 +389,6 @@ _insert_interface (json_t *params, NMConnection *interface, NMDevice *interface_ mtu = nm_setting_wired_get_mtu (s_wired); } - if (!nm_device_hw_addr_get_cloned (interface_device, - interface, - FALSE, - &cloned_mac, - NULL, - &error)) { - _LOGW ("Cannot determine cloned mac for OVS %s '%s': %s", - "interface", - nm_connection_get_interface_name (interface), - error->message); - } - json_array_append_new (options, json_string ("map")); s_ovs_dpdk = (NMSettingOvsDpdk *) nm_connection_get_setting (interface, @@ -489,7 +490,11 @@ _insert_port (json_t *params, NMConnection *port, json_t *new_interfaces) * Returns an commands that adds new bridge from a given connection. */ static void -_insert_bridge (json_t *params, NMConnection *bridge, NMDevice *bridge_device, json_t *new_ports) +_insert_bridge (json_t *params, + NMConnection *bridge, + NMDevice *bridge_device, + json_t *new_ports, + const char *cloned_mac) { NMSettingOvsBridge *s_ovs_bridge; const char *fail_mode = NULL; @@ -498,23 +503,9 @@ _insert_bridge (json_t *params, NMConnection *bridge, NMDevice *bridge_device, j gboolean stp_enable = FALSE; const char *datapath_type = NULL; json_t *row; - gs_free_error GError *error = NULL; - gs_free char *cloned_mac = NULL; s_ovs_bridge = nm_connection_get_setting_ovs_bridge (bridge); - if (!nm_device_hw_addr_get_cloned (bridge_device, - bridge, - FALSE, - &cloned_mac, - NULL, - &error)) { - _LOGW ("Cannot determine cloned mac for OVS %s '%s': %s", - "bridge", - nm_connection_get_interface_name (bridge), - error->message); - } - row = json_object (); if (s_ovs_bridge) { @@ -585,6 +576,9 @@ _add_interface (NMOvsdb *self, json_t *params, const char *bridge_uuid; const char *port_uuid; const char *interface_uuid; + const char *bridge_name; + const char *port_name; + const char *interface_name; OpenvswitchBridge *ovs_bridge = NULL; OpenvswitchPort *ovs_port = NULL; OpenvswitchInterface *ovs_interface = NULL; @@ -595,6 +589,10 @@ _add_interface (NMOvsdb *self, json_t *params, nm_auto_decref_json json_t *interfaces = NULL; nm_auto_decref_json json_t *new_interfaces = NULL; gboolean has_interface = FALSE; + gboolean interface_is_internal; + gs_free char *bridge_cloned_mac = NULL; + gs_free char *interface_cloned_mac = NULL; + GError *error = NULL; int pi; int ii; @@ -605,11 +603,51 @@ _add_interface (NMOvsdb *self, json_t *params, new_ports = json_array (); new_interfaces = json_array (); + bridge_name = nm_connection_get_interface_name (bridge); + port_name = nm_connection_get_interface_name (port); + interface_name = nm_connection_get_interface_name (interface); + interface_is_internal = nm_streq0 (bridge_name, interface_name); + + /* Determine cloned MAC addresses */ + if (!nm_device_hw_addr_get_cloned (bridge_device, + bridge, + FALSE, + &bridge_cloned_mac, + NULL, + &error)) { + _LOGW ("Cannot determine cloned mac for OVS %s '%s': %s", + "bridge", + bridge_name, + error->message); + g_clear_error (&error); + } + + if (!nm_device_hw_addr_get_cloned (interface_device, + interface, + FALSE, + &interface_cloned_mac, + NULL, + &error)) { + _LOGW ("Cannot determine cloned mac for OVS %s '%s': %s", + "interface", + interface_name, + error->message); + g_clear_error (&error); + } + + if ( interface_is_internal + && !bridge_cloned_mac + && interface_cloned_mac) { + _LOGT ("'%s' is a local ovs-interface, the MAC will be set on ovs-bridge '%s'", + interface_name, bridge_name); + bridge_cloned_mac = g_steal_pointer (&interface_cloned_mac); + } + g_hash_table_iter_init (&iter, priv->bridges); while (g_hash_table_iter_next (&iter, (gpointer) &bridge_uuid, (gpointer) &ovs_bridge)) { json_array_append_new (bridges, json_pack ("[s, s]", "uuid", bridge_uuid)); - if ( g_strcmp0 (ovs_bridge->name, nm_connection_get_interface_name (bridge)) != 0 + if ( g_strcmp0 (ovs_bridge->name, bridge_name) != 0 || g_strcmp0 (ovs_bridge->connection_uuid, nm_connection_get_uuid (bridge)) != 0) continue; @@ -623,7 +661,7 @@ _add_interface (NMOvsdb *self, json_t *params, /* This would be a violation of ovsdb's reference integrity (a bug). */ _LOGW ("Unknown port '%s' in bridge '%s'", port_uuid, bridge_uuid); continue; - } else if ( strcmp (ovs_port->name, nm_connection_get_interface_name (port)) != 0 + } else if ( strcmp (ovs_port->name, port_name) != 0 || g_strcmp0 (ovs_port->connection_uuid, nm_connection_get_uuid (port)) != 0) { continue; } @@ -637,7 +675,7 @@ _add_interface (NMOvsdb *self, json_t *params, if (!ovs_interface) { /* This would be a violation of ovsdb's reference integrity (a bug). */ _LOGW ("Unknown interface '%s' in port '%s'", interface_uuid, port_uuid); - } else if ( strcmp (ovs_interface->name, nm_connection_get_interface_name (interface)) == 0 + } else if ( strcmp (ovs_interface->name, interface_name) == 0 && g_strcmp0 (ovs_interface->connection_uuid, nm_connection_get_uuid (interface)) == 0) { has_interface = TRUE; } @@ -660,12 +698,14 @@ _add_interface (NMOvsdb *self, json_t *params, _expect_ovs_bridges (params, priv->db_uuid, bridges); json_array_append_new (new_bridges, json_pack ("[s, s]", "named-uuid", "rowBridge")); _set_ovs_bridges (params, priv->db_uuid, new_bridges); - _insert_bridge (params, bridge, bridge_device, new_ports); + _insert_bridge (params, bridge, bridge_device, new_ports, bridge_cloned_mac); } else { /* Bridge already exists. */ g_return_if_fail (ovs_bridge); _expect_bridge_ports (params, ovs_bridge->name, ports); - _set_bridge_ports (params, nm_connection_get_interface_name (bridge), new_ports); + _set_bridge_ports (params, bridge_name, new_ports); + if (bridge_cloned_mac && interface_is_internal) + _set_bridge_mac (params, bridge_name, bridge_cloned_mac); } json_array_append_new (new_ports, json_pack ("[s, s]", "named-uuid", "rowPort")); @@ -674,11 +714,11 @@ _add_interface (NMOvsdb *self, json_t *params, /* Port already exists */ g_return_if_fail (ovs_port); _expect_port_interfaces (params, ovs_port->name, interfaces); - _set_port_interfaces (params, nm_connection_get_interface_name (port), new_interfaces); + _set_port_interfaces (params, port_name, new_interfaces); } if (!has_interface) { - _insert_interface (params, interface, interface_device); + _insert_interface (params, interface, interface_device, interface_cloned_mac); json_array_append_new (new_interfaces, json_pack ("[s, s]", "named-uuid", "rowInterface")); } } From 69c5c5e7670e8199829df308316d055be049c978 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 1 Jul 2020 09:01:10 +0200 Subject: [PATCH 2/3] ovs: also set cloned MAC address via netlink We already set the MAC of OVS interfaces in the ovsdb. Unfortunately, vswitchd doesn't create the interface with the given MAC from the beginning, but first creates it with a random MAC and then changes it. This causes a race condition: as soon as NM sees the new link, it starts IP configuration on it and (possibly later) vswitchd will change the MAC. To avoid this, also set the desired MAC via netlink before starting IP configuration. https://bugzilla.redhat.com/show_bug.cgi?id=1852106 https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/483 (cherry picked from commit 47ec3d14d49eb9e9db956b3efd495d5d696da996) (cherry picked from commit 60d10b146d57290f146536705d744e67925de90e) (cherry picked from commit 01399955906aa1b1e52998e8daf487b30fa6eb81) --- src/devices/ovs/nm-device-ovs-interface.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/devices/ovs/nm-device-ovs-interface.c b/src/devices/ovs/nm-device-ovs-interface.c index 951b578892..4d3210d0f9 100644 --- a/src/devices/ovs/nm-device-ovs-interface.c +++ b/src/devices/ovs/nm-device-ovs-interface.c @@ -104,6 +104,14 @@ link_changed (NMDevice *device, priv->waiting_for_interface = 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_state_changed (device, + NM_DEVICE_STATE_FAILED, + NM_DEVICE_STATE_REASON_CONFIG_FAILED); + return; + } nm_device_bring_up (device, TRUE, NULL); nm_device_activate_schedule_stage3_ip_config_start (device); } @@ -176,6 +184,13 @@ act_stage3_ip_config_start (NMDevice *device, return NM_ACT_STAGE_RETURN_POSTPONE; } + if (!nm_device_hw_addr_set_cloned (device, + nm_device_get_applied_connection (device), + FALSE)) { + *out_failure_reason = NM_DEVICE_STATE_REASON_CONFIG_FAILED; + return NM_ACT_STAGE_RETURN_FAILURE; + } + return NM_DEVICE_CLASS (nm_device_ovs_interface_parent_class)->act_stage3_ip_config_start (device, addr_family, out_config, out_failure_reason); } From 5f22c06c53d9ae9003cd17603d6d6ff3e16f195b Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Thu, 2 Jul 2020 13:47:22 +0200 Subject: [PATCH 3/3] device: don't reset the MAC without ifindex nm_device_cleanup() can be called when the device no longer has an ifindex. In such case, don't try to reset the MAC address as that would lead to an assertion failure. (cherry picked from commit 77b6ce7d04f6c88e78fb7f1972549956e00e1f4b) (cherry picked from commit 791a888cad3d260675781c0ed30acf13cc1194f7) (cherry picked from commit e1f76e70447049487b2d8240ce12007624ab3c29) --- src/devices/nm-device.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 8638ce4c40..c84d908659 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -15307,17 +15307,19 @@ nm_device_cleanup (NMDevice *self, NMDeviceStateReason reason, CleanupType clean nm_device_update_metered (self); - /* during device cleanup, we want to reset the MAC address of the device - * to the initial state. - * - * We certainly want to do that when reaching the UNMANAGED state... */ - if (nm_device_get_state (self) <= NM_DEVICE_STATE_UNMANAGED) - nm_device_hw_addr_reset (self, "unmanage"); - else { - /* for other device states (UNAVAILABLE, DISCONNECTED), allow the - * device to overwrite the reset behavior, so that Wi-Fi can set - * a randomized MAC address used during scanning. */ - NM_DEVICE_GET_CLASS (self)->deactivate_reset_hw_addr (self); + if (ifindex > 0) { + /* during device cleanup, we want to reset the MAC address of the device + * to the initial state. + * + * We certainly want to do that when reaching the UNMANAGED state... */ + if (nm_device_get_state (self) <= NM_DEVICE_STATE_UNMANAGED) + nm_device_hw_addr_reset (self, "unmanage"); + else { + /* for other device states (UNAVAILABLE, DISCONNECTED), allow the + * device to overwrite the reset behavior, so that Wi-Fi can set + * a randomized MAC address used during scanning. */ + NM_DEVICE_GET_CLASS (self)->deactivate_reset_hw_addr (self); + } } priv->mtu_source = NM_DEVICE_MTU_SOURCE_NONE;