From ac7a5c074c72310d8328fb448824d29bbec932f3 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 5 Jun 2017 13:51:18 +0200 Subject: libnm-core: remove unsupported bond options during normalization In an ideal world, we should not validate connections containing options not valid for the current bond mode. However adding such restriction now means that during an upgrade to the new NM version some connections that were valid before become invalid, possibly disrupting connectivity. Instead, consider invalid options as a normalizable error and remove them during normalization. Converting the setting to a "canonical" form without invalid options is important for the connection matching logic, where such invalid options can cause false mismatches. (cherry picked from commit f25e008e2fe655bbddbb8a66612a9d141e982049) --- libnm-core/nm-connection.c | 31 +++++++++++++++++++++++ libnm-core/nm-setting-bond.c | 18 +++++++++++++ libnm-core/tests/test-setting-bond.c | 49 ++++++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+) diff --git a/libnm-core/nm-connection.c b/libnm-core/nm-connection.c index ecfb9780be..c1b75068cf 100644 --- a/libnm-core/nm-connection.c +++ b/libnm-core/nm-connection.c @@ -912,6 +912,36 @@ _normalize_bond_mode (NMConnection *self, GHashTable *parameters) return FALSE; } +static gboolean +_normalize_bond_options (NMConnection *self, GHashTable *parameters) +{ + NMSettingBond *s_bond = nm_connection_get_setting_bond (self); + gboolean changed = FALSE; + const char *name, *mode_str; + NMBondMode mode; + guint32 num, i; + + /* Strip away unsupported options for current mode */ + if (s_bond) { + mode_str = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_MODE); + mode = _nm_setting_bond_mode_from_string (mode_str); + if (mode == NM_BOND_MODE_UNKNOWN) + return FALSE; +again: + num = nm_setting_bond_get_num_options (s_bond); + for (i = 0; i < num; i++) { + if ( nm_setting_bond_get_option (s_bond, i, &name, NULL) + && !_nm_setting_bond_option_supported (name, mode)) { + nm_setting_bond_remove_option (s_bond, name); + changed = TRUE; + goto again; + } + } + } + + return changed; +} + static gboolean _normalize_wireless_mac_address_randomization (NMConnection *self, GHashTable *parameters) { @@ -1275,6 +1305,7 @@ nm_connection_normalize (NMConnection *connection, was_modified |= _normalize_ethernet_link_neg (connection); was_modified |= _normalize_infiniband_mtu (connection, parameters); was_modified |= _normalize_bond_mode (connection, parameters); + was_modified |= _normalize_bond_options (connection, parameters); was_modified |= _normalize_wireless_mac_address_randomization (connection, parameters); was_modified |= _normalize_team_config (connection, parameters); was_modified |= _normalize_team_port_config (connection, parameters); diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c index 9a8bdc3702..b62964c721 100644 --- a/libnm-core/nm-setting-bond.c +++ b/libnm-core/nm-setting-bond.c @@ -542,6 +542,7 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) const char *arp_ip_target = NULL; const char *lacp_rate; const char *primary; + NMBondMode bond_mode; g_hash_table_iter_init (&iter, priv->options); while (g_hash_table_iter_next (&iter, (gpointer) &key, (gpointer) &value)) { @@ -776,6 +777,23 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) return NM_SETTING_VERIFY_NORMALIZABLE; } + /* normalize unsupported options for the current mode */ + bond_mode = _nm_setting_bond_mode_from_string (mode_new); + g_hash_table_iter_init (&iter, priv->options); + while (g_hash_table_iter_next (&iter, (gpointer) &key, NULL)) { + if (nm_streq (key, "mode")) + continue; + if (!_nm_setting_bond_option_supported (key, bond_mode)) { + g_set_error (error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("'%s' option is not valid with mode '%s'"), + key, mode_new); + g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS); + return NM_SETTING_VERIFY_NORMALIZABLE; + } + } + return TRUE; } diff --git a/libnm-core/tests/test-setting-bond.c b/libnm-core/tests/test-setting-bond.c index 91a8199719..e6a65bba64 100644 --- a/libnm-core/tests/test-setting-bond.c +++ b/libnm-core/tests/test-setting-bond.c @@ -182,6 +182,54 @@ test_compare (void) ((const char *[]){ "num_unsol_na", "4", "num_grat_arp", "4", NULL })); } +static void +test_normalize_options (const char **opts1, const char **opts2) +{ + gs_unref_object NMConnection *con = NULL; + NMSettingBond *s_bond; + GError *error = NULL; + gboolean success; + const char **p; + int num = 0; + + create_bond_connection (&con, &s_bond); + + for (p = opts1; p[0] && p[1]; p += 2) + g_assert (nm_setting_bond_add_option (s_bond, p[0], p[1])); + + nmtst_assert_connection_verifies_and_normalizable (con); + nmtst_connection_normalize (con); + success = nm_setting_verify ((NMSetting *) s_bond, con, &error); + nmtst_assert_success (success, error); + + for (p = opts2; p[0] && p[1]; p += 2) { + g_assert_cmpstr (nm_setting_bond_get_option_by_name (s_bond, p[0]), ==, p[1]); + num++; + } + + g_assert_cmpint (num, ==, nm_setting_bond_get_num_options (s_bond)); +} + +static void +test_normalize (void) +{ + test_normalize_options ( + ((const char *[]){ "mode", "802.3ad", "ad_actor_system", "00:02:03:04:05:06", NULL }), + ((const char *[]){ "mode", "802.3ad", "ad_actor_system", "00:02:03:04:05:06", NULL })); + test_normalize_options ( + ((const char *[]){ "mode", "1", "miimon", "1", NULL }), + ((const char *[]){ "mode", "active-backup", "miimon", "1", NULL })); + test_normalize_options ( + ((const char *[]){ "mode", "balance-alb", "tlb_dynamic_lb", "1", NULL }), + ((const char *[]){ "mode", "balance-alb", NULL })); + test_normalize_options ( + ((const char *[]){ "mode", "balance-tlb", "tlb_dynamic_lb", "1", NULL }), + ((const char *[]){ "mode", "balance-tlb", "tlb_dynamic_lb", "1", NULL })); + test_normalize_options ( + ((const char *[]){ "mode", "balance-rr", "ad_actor_sys_prio", "4", "packets_per_slave", "3", NULL }), + ((const char *[]){ "mode", "balance-rr", "packets_per_slave", "3", NULL })); +} + #define TPATH "/libnm/settings/bond/" NMTST_DEFINE (); @@ -193,6 +241,7 @@ main (int argc, char **argv) g_test_add_func (TPATH "verify", test_verify); g_test_add_func (TPATH "compare", test_compare); + g_test_add_func (TPATH "normalize", test_normalize); return g_test_run (); } -- cgit v1.2.1 From 61817661c899844ddb364ebd529716f574146588 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 5 Jun 2017 14:48:08 +0200 Subject: bond: add only supported options to the generated connection Upstream commit [1] changed in the kernel the default value of tlb_dynamic_lb bond from 1 to 0 when the mode is not tlb. This is not wrong, as the option value doesn't really matter for other modes, but it breaks the connection matching because we read back a 0 value when we expect a default of 1. Fix this in a generic way by ignoring altogether options that are not relevant for the current bond mode, because they are removed from the connection during normalization. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8b426dc54cf4056984bab7dfa48c92ee79a46434 https://bugzilla.redhat.com/show_bug.cgi?id=1457909 (cherry picked from commit 056a973a4fdb68abe8bc7bfc5f31250345d71f21) --- src/devices/nm-device-bond.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c index 3325c9485e..c8748fe8f2 100644 --- a/src/devices/nm-device-bond.c +++ b/src/devices/nm-device-bond.c @@ -155,6 +155,7 @@ update_connection (NMDevice *device, NMConnection *connection) { NMSettingBond *s_bond = nm_connection_get_setting_bond (connection); int ifindex = nm_device_get_ifindex (device); + NMBondMode mode = NM_BOND_MODE_UNKNOWN; const char **options; if (!s_bond) { @@ -164,7 +165,7 @@ update_connection (NMDevice *device, NMConnection *connection) /* Read bond options from sysfs and update the Bond setting to match */ options = nm_setting_bond_get_valid_options (s_bond); - while (options && *options) { + for (; *options; options++) { gs_free char *value = nm_platform_sysctl_master_get_option (nm_device_get_platform (device), ifindex, *options); const char *defvalue = nm_setting_bond_get_option_default (s_bond, *options); char *p; @@ -176,6 +177,12 @@ update_connection (NMDevice *device, NMConnection *connection) *p = '\0'; } + if (nm_streq (*options, NM_SETTING_BOND_OPTION_MODE)) + mode = _nm_setting_bond_mode_from_string (value); + + if (!_nm_setting_bond_option_supported (*options, mode)) + continue; + if ( value && value[0] && !ignore_if_zero (*options, value) @@ -190,7 +197,6 @@ update_connection (NMDevice *device, NMConnection *connection) nm_setting_bond_add_option (s_bond, *options, value); } - options++; } } -- cgit v1.2.1