From 3b29cd7eee7ba07f0f4750382819a9a53502124c Mon Sep 17 00:00:00 2001

From: Beniamino Galvani <bgalvani@redhat.com>

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)

---

 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 e1865f9de..0b3fa3fdb 100644

--- a/src/devices/ovs/nm-ovsdb.c

+++ b/src/devices/ovs/nm-ovsdb.c

@@ -310,6 +310,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:

  *

@@ -353,15 +365,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;

@@ -377,18 +390,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,

@@ -490,7 +491,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;

@@ -499,23 +504,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) {

@@ -586,6 +577,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;

@@ -596,6 +590,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;

@@ -606,11 +604,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;

@@ -624,7 +662,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;

 			}

@@ -638,7 +676,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;

 				}

@@ -661,12 +699,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"));

@@ -675,11 +715,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"));

 	}

 }

-- 

2.26.2

From 3df555a42166f0f1ec50e6bdb38f1719d1fe2eef Mon Sep 17 00:00:00 2001

From: Beniamino Galvani <bgalvani@redhat.com>

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)

---

 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 10f9fa943..83954cf07 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);

 }

-- 

2.26.2

From 73a99046f02f82705b0cf7da8c06ce53ddcbedba Mon Sep 17 00:00:00 2001

From: Beniamino Galvani <bgalvani@redhat.com>

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)

---

 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 a6c3e6680..980cd3a06 100644

--- a/src/devices/nm-device.c

+++ b/src/devices/nm-device.c

@@ -15606,17 +15606,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;

-- 

2.26.2