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