From 99c7adc1e12862f22026bb8f806c9223db9a78ec 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. --- 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 5b50f84069..a08a3647f5 100644 --- a/src/devices/ovs/nm-ovsdb.c +++ b/src/devices/ovs/nm-ovsdb.c @@ -534,9 +534,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); @@ -544,9 +549,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; @@ -642,16 +651,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)); @@ -878,7 +898,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 b1feebc43aafd5e40c371c2a971cdefd5f8acae6 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. --- 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 a08a3647f5..a93aa70a1f 100644 --- a/src/devices/ovs/nm-ovsdb.c +++ b/src/devices/ovs/nm-ovsdb.c @@ -58,7 +58,6 @@ typedef struct { enum { DEVICE_ADDED, DEVICE_REMOVED, - DEVICE_CHANGED, LAST_SIGNAL }; @@ -925,8 +924,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, @@ -980,8 +977,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 ? ", " : "", @@ -1030,8 +1025,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 ? ", " : "", @@ -1592,11 +1585,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 cf9fe2a21b..c7f391fe31 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 dedc0cba23b5d51773f88d8bc06424eb589083cd 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 --- 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 a93aa70a1f..42e6b132fa 100644 --- a/src/devices/ovs/nm-ovsdb.c +++ b/src/devices/ovs/nm-ovsdb.c @@ -1577,12 +1577,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 f2c066e1046cc8da5a277b1528a59bf2653e17c8 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. --- 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 42e6b132fa..c11abe51c5 100644 --- a/src/devices/ovs/nm-ovsdb.c +++ b/src/devices/ovs/nm-ovsdb.c @@ -58,6 +58,7 @@ typedef struct { enum { DEVICE_ADDED, DEVICE_REMOVED, + INTERFACE_FAILED, LAST_SIGNAL }; @@ -737,14 +738,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: @@ -883,15 +884,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; @@ -936,6 +939,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); } } @@ -1585,4 +1596,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 c7f391fe31..279155a498 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 e948ce7debb4f2da2db9c19ab4f980eac5415b9d 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. --- 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 c11abe51c5..4bb9928758 100644 --- a/src/devices/ovs/nm-ovsdb.c +++ b/src/devices/ovs/nm-ovsdb.c @@ -923,6 +923,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 ? ", " : "", @@ -947,7 +948,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); } } @@ -984,6 +984,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 ? ", " : "", @@ -995,7 +996,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); } } @@ -1032,6 +1032,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 ? ", " : "", @@ -1043,7 +1044,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 02950ec600d07647dadabdf08b151d5c4f5f8985 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 --- 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 *