From 3455c30a58ff14952acf86f8ea068ef8f3b6d889 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 12 Jun 2019 17:58:49 +0200 Subject: [PATCH 1/6] ovs/ovsdb: guard against OVSDB integrity issues Don't crash in situations, where the bridge or a port has a child with UUID we don't know. This could happen if we mess up the parsing of messages from OVSDB, but could also theoretically happen in OVSDB sends us bad data. (cherry picked from commit 99c7adc1e12862f22026bb8f806c9223db9a78ec) --- src/devices/ovs/nm-ovsdb.c | 40 +++++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/src/devices/ovs/nm-ovsdb.c b/src/devices/ovs/nm-ovsdb.c index 82e25f396c..5f46c23a26 100644 --- a/src/devices/ovs/nm-ovsdb.c +++ b/src/devices/ovs/nm-ovsdb.c @@ -584,9 +584,14 @@ _add_interface (NMOvsdb *self, json_t *params, json_array_append_new (ports, json_pack ("[s, s]", "uuid", port_uuid)); - if ( g_strcmp0 (ovs_port->name, nm_connection_get_interface_name (port)) != 0 - || g_strcmp0 (ovs_port->connection_uuid, nm_connection_get_uuid (port)) != 0) + if (!ovs_port) { + /* 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 + || g_strcmp0 (ovs_port->connection_uuid, nm_connection_get_uuid (port)) != 0) { + continue; + } for (ii = 0; ii < ovs_port->interfaces->len; ii++) { interface_uuid = g_ptr_array_index (ovs_port->interfaces, ii); @@ -594,9 +599,13 @@ _add_interface (NMOvsdb *self, json_t *params, json_array_append_new (interfaces, json_pack ("[s, s]", "uuid", interface_uuid)); - if ( g_strcmp0 (ovs_interface->name, nm_connection_get_interface_name (interface)) == 0 - && g_strcmp0 (ovs_interface->connection_uuid, nm_connection_get_uuid (interface)) == 0) + 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 + && g_strcmp0 (ovs_interface->connection_uuid, nm_connection_get_uuid (interface)) == 0) { has_interface = TRUE; + } } break; @@ -692,16 +701,27 @@ _delete_interface (NMOvsdb *self, json_t *params, const char *ifname) interfaces_changed = FALSE; + if (!ovs_port) { + /* This would be a violation of ovsdb's reference integrity (a bug). */ + _LOGW ("Unknown port '%s' in bridge '%s'", port_uuid, bridge_uuid); + continue; + } + for (ii = 0; ii < ovs_port->interfaces->len; ii++) { interface_uuid = g_ptr_array_index (ovs_port->interfaces, ii); ovs_interface = g_hash_table_lookup (priv->interfaces, interface_uuid); json_array_append_new (interfaces, json_pack ("[s,s]", "uuid", interface_uuid)); - if (strcmp (ovs_interface->name, ifname) == 0) { - /* skip the interface */ - interfaces_changed = TRUE; - continue; + if (ovs_interface) { + if (strcmp (ovs_interface->name, ifname) == 0) { + /* skip the interface */ + interfaces_changed = TRUE; + continue; + } + } else { + /* This would be a violation of ovsdb's reference integrity (a bug). */ + _LOGW ("Unknown interface '%s' in port '%s'", interface_uuid, port_uuid); } json_array_append_new (new_interfaces, json_pack ("[s,s]", "uuid", interface_uuid)); @@ -929,7 +949,9 @@ ovsdb_got_update (NMOvsdb *self, json_t *msg) if (old) { ovs_interface = g_hash_table_lookup (priv->interfaces, key); - if (!new || g_strcmp0 (ovs_interface->name, name) != 0) { + if (!ovs_interface) { + _LOGW ("Interface '%s' was not seen", key); + } else if (!new || strcmp (ovs_interface->name, name) != 0) { old = FALSE; _LOGT ("removed an '%s' interface: %s%s%s", ovs_interface->type, ovs_interface->name, From 7448ad84679188157eda4ad3ba0289af4918d384 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 12 Jun 2019 17:10:09 +0200 Subject: [PATCH 2/6] ovs/ovsdb: remove the device-changes signal It doesn't communicate anything about the nature of the change and indeed nothing uses it. (cherry picked from commit b1feebc43aafd5e40c371c2a971cdefd5f8acae6) --- src/devices/ovs/nm-ovsdb.c | 14 -------------- src/devices/ovs/nm-ovsdb.h | 1 - 2 files changed, 15 deletions(-) diff --git a/src/devices/ovs/nm-ovsdb.c b/src/devices/ovs/nm-ovsdb.c index 5f46c23a26..9ea78a39b9 100644 --- a/src/devices/ovs/nm-ovsdb.c +++ b/src/devices/ovs/nm-ovsdb.c @@ -59,7 +59,6 @@ typedef struct { enum { DEVICE_ADDED, DEVICE_REMOVED, - DEVICE_CHANGED, LAST_SIGNAL }; @@ -976,8 +975,6 @@ ovsdb_got_update (NMOvsdb *self, json_t *msg) _LOGT ("changed an '%s' interface: %s%s%s", type, ovs_interface->name, ovs_interface->connection_uuid ? ", " : "", ovs_interface->connection_uuid ?: ""); - g_signal_emit (self, signals[DEVICE_CHANGED], 0, - "ovs-interface", ovs_interface->name); } else { _LOGT ("added an '%s' interface: %s%s%s", ovs_interface->type, ovs_interface->name, @@ -1031,8 +1028,6 @@ ovsdb_got_update (NMOvsdb *self, json_t *msg) _LOGT ("changed a port: %s%s%s", ovs_port->name, ovs_port->connection_uuid ? ", " : "", ovs_port->connection_uuid ?: ""); - g_signal_emit (self, signals[DEVICE_CHANGED], 0, - NM_SETTING_OVS_PORT_SETTING_NAME, ovs_port->name); } else { _LOGT ("added a port: %s%s%s", ovs_port->name, ovs_port->connection_uuid ? ", " : "", @@ -1081,8 +1076,6 @@ ovsdb_got_update (NMOvsdb *self, json_t *msg) _LOGT ("changed a bridge: %s%s%s", ovs_bridge->name, ovs_bridge->connection_uuid ? ", " : "", ovs_bridge->connection_uuid ?: ""); - g_signal_emit (self, signals[DEVICE_CHANGED], 0, - NM_SETTING_OVS_BRIDGE_SETTING_NAME, ovs_bridge->name); } else { _LOGT ("added a bridge: %s%s%s", ovs_bridge->name, ovs_bridge->connection_uuid ? ", " : "", @@ -1648,11 +1641,4 @@ nm_ovsdb_class_init (NMOvsdbClass *klass) G_SIGNAL_RUN_LAST, 0, NULL, NULL, NULL, G_TYPE_NONE, 2, G_TYPE_POINTER, G_TYPE_UINT); - - signals[DEVICE_CHANGED] = - g_signal_new (NM_OVSDB_DEVICE_CHANGED, - G_OBJECT_CLASS_TYPE (object_class), - G_SIGNAL_RUN_LAST, - 0, NULL, NULL, NULL, - G_TYPE_NONE, 2, G_TYPE_POINTER, G_TYPE_UINT); } diff --git a/src/devices/ovs/nm-ovsdb.h b/src/devices/ovs/nm-ovsdb.h index f9dbabbb7a..e403b0e447 100644 --- a/src/devices/ovs/nm-ovsdb.h +++ b/src/devices/ovs/nm-ovsdb.h @@ -29,7 +29,6 @@ #define NM_OVSDB_DEVICE_ADDED "device-added" #define NM_OVSDB_DEVICE_REMOVED "device-removed" -#define NM_OVSDB_DEVICE_CHANGED "device-changed" typedef struct _NMOvsdb NMOvsdb; typedef struct _NMOvsdbClass NMOvsdbClass; From 4107490c892ee90e87d6231557a01501cf340bd1 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Fri, 14 Jun 2019 10:31:15 +0200 Subject: [PATCH 3/6] ovs/ovsdb: fix signal handler argument types (cherry picked from commit dedc0cba23b5d51773f88d8bc06424eb589083cd) --- src/devices/ovs/nm-ovsdb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/devices/ovs/nm-ovsdb.c b/src/devices/ovs/nm-ovsdb.c index 9ea78a39b9..a9c209a2f4 100644 --- a/src/devices/ovs/nm-ovsdb.c +++ b/src/devices/ovs/nm-ovsdb.c @@ -1633,12 +1633,12 @@ nm_ovsdb_class_init (NMOvsdbClass *klass) G_OBJECT_CLASS_TYPE (object_class), G_SIGNAL_RUN_LAST, 0, NULL, NULL, NULL, - G_TYPE_NONE, 2, G_TYPE_POINTER, G_TYPE_UINT); + G_TYPE_NONE, 2, G_TYPE_STRING, G_TYPE_UINT); signals[DEVICE_REMOVED] = g_signal_new (NM_OVSDB_DEVICE_REMOVED, G_OBJECT_CLASS_TYPE (object_class), G_SIGNAL_RUN_LAST, 0, NULL, NULL, NULL, - G_TYPE_NONE, 2, G_TYPE_POINTER, G_TYPE_UINT); + G_TYPE_NONE, 2, G_TYPE_STRING, G_TYPE_UINT); } From cb19680d342d30043b6c7534cab4c8a12d3e797f Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 12 Jun 2019 17:15:13 +0200 Subject: [PATCH 4/6] ovs/ovsdb: signal a failure when an error column is set When an interface (other OVS device types can not fail) encounters an error it indicates it by changing the error column. Watch for those changes so that we can eventually communicate them to the OVS factory to deal with them. (cherry picked from commit f2c066e1046cc8da5a277b1528a59bf2653e17c8) --- src/devices/ovs/nm-ovsdb.c | 24 +++++++++++++++++++++--- src/devices/ovs/nm-ovsdb.h | 5 +++-- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/devices/ovs/nm-ovsdb.c b/src/devices/ovs/nm-ovsdb.c index a9c209a2f4..a9644ce9e1 100644 --- a/src/devices/ovs/nm-ovsdb.c +++ b/src/devices/ovs/nm-ovsdb.c @@ -59,6 +59,7 @@ typedef struct { enum { DEVICE_ADDED, DEVICE_REMOVED, + INTERFACE_FAILED, LAST_SIGNAL }; @@ -787,14 +788,14 @@ ovsdb_next_command (NMOvsdb *self) msg = json_pack ("{s:i, s:s, s:[s, n, {" " s:[{s:[s, s, s]}]," " s:[{s:[s, s, s]}]," - " s:[{s:[s, s, s]}]," + " s:[{s:[s, s, s, s]}]," " s:[{s:[]}]" "}]}", "id", call->id, "method", "monitor", "params", "Open_vSwitch", "Bridge", "columns", "name", "ports", "external_ids", "Port", "columns", "name", "interfaces", "external_ids", - "Interface", "columns", "name", "type", "external_ids", + "Interface", "columns", "name", "type", "external_ids", "error", "Open_vSwitch", "columns"); break; case OVSDB_ADD_INTERFACE: @@ -934,15 +935,17 @@ ovsdb_got_update (NMOvsdb *self, json_t *msg) /* Interfaces */ json_object_foreach (interface, key, value) { + json_t *error = NULL; gboolean old = FALSE; gboolean new = FALSE; if (json_unpack (value, "{s:{}}", "old") == 0) old = TRUE; - if (json_unpack (value, "{s:{s:s, s:s, s:o}}", "new", + if (json_unpack (value, "{s:{s:s, s:s, s?:o, s:o}}", "new", "name", &name, "type", &type, + "error", &error, "external_ids", &external_ids) == 0) new = TRUE; @@ -987,6 +990,14 @@ ovsdb_got_update (NMOvsdb *self, json_t *msg) ovs_interface->name, NM_DEVICE_TYPE_OVS_INTERFACE); } } + /* The error is a string. No error is indicated by an empty set, + * because why the fuck not: [ "set": [] ] */ + if (error && json_is_string (error)) { + g_signal_emit (self, signals[INTERFACE_FAILED], 0, + ovs_interface->name, + ovs_interface->connection_uuid, + json_string_value (error)); + } g_hash_table_insert (priv->interfaces, g_strdup (key), ovs_interface); } } @@ -1641,4 +1652,11 @@ nm_ovsdb_class_init (NMOvsdbClass *klass) G_SIGNAL_RUN_LAST, 0, NULL, NULL, NULL, G_TYPE_NONE, 2, G_TYPE_STRING, G_TYPE_UINT); + + signals[INTERFACE_FAILED] = + g_signal_new (NM_OVSDB_INTERFACE_FAILED, + G_OBJECT_CLASS_TYPE (object_class), + G_SIGNAL_RUN_LAST, + 0, NULL, NULL, NULL, + G_TYPE_NONE, 3, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING); } diff --git a/src/devices/ovs/nm-ovsdb.h b/src/devices/ovs/nm-ovsdb.h index e403b0e447..e7f9d71984 100644 --- a/src/devices/ovs/nm-ovsdb.h +++ b/src/devices/ovs/nm-ovsdb.h @@ -27,8 +27,9 @@ #define NM_IS_OVSDB_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), NM_TYPE_OVSDB)) #define NM_OVSDB_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), NM_TYPE_OVSDB, NMOvsdbClass)) -#define NM_OVSDB_DEVICE_ADDED "device-added" -#define NM_OVSDB_DEVICE_REMOVED "device-removed" +#define NM_OVSDB_DEVICE_ADDED "device-added" +#define NM_OVSDB_DEVICE_REMOVED "device-removed" +#define NM_OVSDB_INTERFACE_FAILED "interface-failed" typedef struct _NMOvsdb NMOvsdb; typedef struct _NMOvsdbClass NMOvsdbClass; From 83a42ef8ad8a3edd95eceff1a3ea69989d78baa4 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 12 Jun 2019 18:30:36 +0200 Subject: [PATCH 5/6] ovs/ovsdb: track the devices before we signal addition This doesn't make any difference in practice, but it seems more correct. It would cause issues if we decided to remove an interface from the signal handler. (cherry picked from commit e948ce7debb4f2da2db9c19ab4f980eac5415b9d) --- src/devices/ovs/nm-ovsdb.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/devices/ovs/nm-ovsdb.c b/src/devices/ovs/nm-ovsdb.c index a9644ce9e1..c85115201e 100644 --- a/src/devices/ovs/nm-ovsdb.c +++ b/src/devices/ovs/nm-ovsdb.c @@ -974,6 +974,7 @@ ovsdb_got_update (NMOvsdb *self, json_t *msg) ovs_interface->name = g_strdup (name); ovs_interface->type = g_strdup (type); ovs_interface->connection_uuid = _connection_uuid_from_external_ids (external_ids); + g_hash_table_insert (priv->interfaces, g_strdup (key), ovs_interface); if (old) { _LOGT ("changed an '%s' interface: %s%s%s", type, ovs_interface->name, ovs_interface->connection_uuid ? ", " : "", @@ -998,7 +999,6 @@ ovsdb_got_update (NMOvsdb *self, json_t *msg) ovs_interface->connection_uuid, json_string_value (error)); } - g_hash_table_insert (priv->interfaces, g_strdup (key), ovs_interface); } } @@ -1035,6 +1035,7 @@ ovsdb_got_update (NMOvsdb *self, json_t *msg) ovs_port->connection_uuid = _connection_uuid_from_external_ids (external_ids); ovs_port->interfaces = g_ptr_array_new_with_free_func (g_free); _uuids_to_array (ovs_port->interfaces, items); + g_hash_table_insert (priv->ports, g_strdup (key), ovs_port); if (old) { _LOGT ("changed a port: %s%s%s", ovs_port->name, ovs_port->connection_uuid ? ", " : "", @@ -1046,7 +1047,6 @@ ovsdb_got_update (NMOvsdb *self, json_t *msg) g_signal_emit (self, signals[DEVICE_ADDED], 0, ovs_port->name, NM_DEVICE_TYPE_OVS_PORT); } - g_hash_table_insert (priv->ports, g_strdup (key), ovs_port); } } @@ -1083,6 +1083,7 @@ ovsdb_got_update (NMOvsdb *self, json_t *msg) ovs_bridge->connection_uuid = _connection_uuid_from_external_ids (external_ids); ovs_bridge->ports = g_ptr_array_new_with_free_func (g_free); _uuids_to_array (ovs_bridge->ports, items); + g_hash_table_insert (priv->bridges, g_strdup (key), ovs_bridge); if (old) { _LOGT ("changed a bridge: %s%s%s", ovs_bridge->name, ovs_bridge->connection_uuid ? ", " : "", @@ -1094,7 +1095,6 @@ ovsdb_got_update (NMOvsdb *self, json_t *msg) g_signal_emit (self, signals[DEVICE_ADDED], 0, ovs_bridge->name, NM_DEVICE_TYPE_OVS_BRIDGE); } - g_hash_table_insert (priv->bridges, g_strdup (key), ovs_bridge); } } From ba2c71a01db83a786ec4d1fe10b42574cd45e2c2 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 12 Jun 2019 18:38:58 +0200 Subject: [PATCH 6/6] ovs/factory: fail the NMDevice if there's an error in OVSDB (cherry picked from commit 02950ec600d07647dadabdf08b151d5c4f5f8985) --- src/devices/ovs/nm-ovs-factory.c | 41 +++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/src/devices/ovs/nm-ovs-factory.c b/src/devices/ovs/nm-ovs-factory.c index 2124b2a0b1..fdf07bd375 100644 --- a/src/devices/ovs/nm-ovs-factory.c +++ b/src/devices/ovs/nm-ovs-factory.c @@ -26,7 +26,9 @@ #include "nm-device-ovs-bridge.h" #include "platform/nm-platform.h" #include "nm-core-internal.h" +#include "settings/nm-settings.h" #include "devices/nm-device-factory.h" +#include "devices/nm-device-private.h" /*****************************************************************************/ @@ -51,7 +53,13 @@ G_DEFINE_TYPE (NMOvsFactory, nm_ovs_factory, NM_TYPE_DEVICE_FACTORY) /*****************************************************************************/ #define _NMLOG_DOMAIN LOGD_DEVICE -#define _NMLOG(level, ...) __NMLOG_DEFAULT (level, _NMLOG_DOMAIN, "ovs", __VA_ARGS__) +#define _NMLOG(level, ifname, con_uuid, ...) \ + G_STMT_START { \ + nm_log ((level), _NMLOG_DOMAIN, (ifname), (con_uuid), \ + "ovs: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__) \ + _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ + } G_STMT_END + /*****************************************************************************/ @@ -138,6 +146,36 @@ ovsdb_device_removed (NMOvsdb *ovsdb, const char *name, NMDeviceType device_type } } +static void +ovsdb_interface_failed (NMOvsdb *ovsdb, + const char *name, + const char *connection_uuid, + const char *error, + NMDeviceFactory *self) +{ + NMDevice *device = NULL; + NMSettingsConnection *connection = NULL; + + _LOGI (name, connection_uuid, "ovs interface \"%s\" (%s) failed: %s", name, connection_uuid, error); + + device = nm_manager_get_device (nm_manager_get (), name, NM_DEVICE_TYPE_OVS_INTERFACE); + if (!device) + return; + + if (connection_uuid) + connection = nm_settings_get_connection_by_uuid (nm_device_get_settings (device), connection_uuid); + + if (connection) { + nm_settings_connection_autoconnect_blocked_reason_set (connection, + NM_SETTINGS_AUTO_CONNECT_BLOCKED_REASON_FAILED, + TRUE); + } + + nm_device_state_changed (device, + NM_DEVICE_STATE_FAILED, + NM_DEVICE_STATE_REASON_OVSDB_FAILED); +} + static void start (NMDeviceFactory *self) { @@ -147,6 +185,7 @@ start (NMDeviceFactory *self) g_signal_connect_object (ovsdb, NM_OVSDB_DEVICE_ADDED, G_CALLBACK (ovsdb_device_added), self, (GConnectFlags) 0); g_signal_connect_object (ovsdb, NM_OVSDB_DEVICE_REMOVED, G_CALLBACK (ovsdb_device_removed), self, (GConnectFlags) 0); + g_signal_connect_object (ovsdb, NM_OVSDB_INTERFACE_FAILED, G_CALLBACK (ovsdb_interface_failed), self, (GConnectFlags) 0); } static NMDevice *