From 8c6f8c65955d18ca9b43ad2bcd1bccf2cd85e7ed Mon Sep 17 00:00:00 2001

From: Beniamino Galvani <bgalvani@redhat.com>

Date: Mon, 5 Jun 2017 13:51:18 +0200

Subject: [PATCH 1/2] 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)

(cherry picked from commit ac7a5c074c72310d8328fb448824d29bbec932f3)

---

 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 ecfb978..c1b7506 100644

--- a/libnm-core/nm-connection.c

+++ b/libnm-core/nm-connection.c

@@ -913,6 +913,36 @@ _normalize_bond_mode (NMConnection *self, GHashTable *parameters)

 }

 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)

 {

 	NMSettingWireless *s_wifi = nm_connection_get_setting_wireless (self);

@@ -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 9a8bdc3..b62964c 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 91a8199..e6a65bb 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 ();

 }

-- 

2.9.3

From 0d96d249ffe95e0232b8247e6bb9c1385a2b4940 Mon Sep 17 00:00:00 2001

From: Beniamino Galvani <bgalvani@redhat.com>

Date: Mon, 5 Jun 2017 14:48:08 +0200

Subject: [PATCH 2/2] 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)

(cherry picked from commit 61817661c899844ddb364ebd529716f574146588)

---

 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 3325c94..c8748fe 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++;

 	}

 }

-- 

2.9.3