From aeaf8ca23c49cd0c90a692771697abf1a62796d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Fri, 25 Apr 2025 14:35:02 +0200 Subject: [PATCH] nm-initrd-generator: fix IPv6 with square brackets in bond options If any bond option contains an IPv6 address it needs to be enclosed with []. Otherwise the ':' separators from the IP address can be confused with the ':' separators from the 'bond=' cmdline arguments. However, the square brackets were ignored: $ nm-initrd-generator -s "bond=bond0:eth0,eth1:ns_ip6_target=[FC08::789:1:0:0:3]" NetworkManager-Message: 08:46:55.114: [1745498815.1146] cmdline-reader: Ignoring invalid bond option: "ns_ip6_target" = "[FC08": '[FC08' is not a valid IPv6 address for 'ns_ip6_target' option NetworkManager-Message: 08:46:55.114: [1745498815.1148] cmdline-reader: Ignoring extra: '789:1:0:0:3]'. The opening '[' was only being considered if it was the first character in `get_word`. Fix it and consider it if it's in the middle too. If the brackets are used first and last, directly remove them as it is what most callers expect. However, if it's in the middle there is no reasonable way to remove them, so don't do it. Instead, the caller will have to consider this possibility when processing the content. Fixes: ecc074b2f8a6 ('initrd: add command line parser') Fixes https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/1755 --- src/nm-initrd-generator/nmi-cmdline-reader.c | 60 ++++++--- .../tests/test-cmdline-reader.c | 122 +++++++++++++++++- 2 files changed, 159 insertions(+), 23 deletions(-) diff --git a/src/nm-initrd-generator/nmi-cmdline-reader.c b/src/nm-initrd-generator/nmi-cmdline-reader.c index a6cd30f10..59b9f04b1 100644 --- a/src/nm-initrd-generator/nmi-cmdline-reader.c +++ b/src/nm-initrd-generator/nmi-cmdline-reader.c @@ -299,33 +299,44 @@ get_word(char **argument, const char separator) { char *word; int nest = 0; + char *last_ch; + char *first_close = NULL; if (*argument == NULL) return NULL; - if (**argument == '[') { - nest++; - (*argument)++; - } - - word = *argument; + word = last_ch = *argument; while (**argument != '\0') { - if (nest && **argument == ']') { - **argument = '\0'; - (*argument)++; - nest--; - continue; - } - if (nest == 0 && **argument == separator) { **argument = '\0'; (*argument)++; break; } + if (**argument == '[') { + nest++; + } else if (nest && **argument == ']') { + nest--; + if (!first_close && nest == 0) + first_close = *argument; + } + + last_ch = *argument; (*argument)++; } + /* If the word is surrounded with the nesting symbols [], strip them so we return + * the inner content only. + * If there were nesting symbols but embracing only part of the inner content, don't + * remove them. Example: + * Remove [] in get_word("[fc08::1]:other_token", ":") + * Don't remove [] in get_word("ip6=[fc08::1]:other_token", ":") + */ + if (*word == '[' && *last_ch == ']' && last_ch == first_close) { + word++; + *last_ch = '\0'; + } + return *word ? word : NULL; } @@ -905,14 +916,25 @@ reader_parse_controller(Reader *reader, opts = get_word(&argument, ':'); while (opts && *opts) { - gs_free_error GError *error = NULL; - char *opt; - const char *opt_name; + gs_free_error GError *error = NULL; + char *tmp; + const char *opt_name; + char *opt; + const char *opt_value; + nm_auto_unref_ptrarray GPtrArray *opt_values = g_ptr_array_new(); + gs_free char *opt_normalized = NULL; + opt_name = get_word(&opts, '='); opt = get_word(&opts, ','); - opt_name = get_word(&opt, '='); - if (!_nm_setting_bond_validate_option(opt_name, opt, &error)) { + /* Normalize: convert ';' to ',' and remove '[]' from IPv6 addresses */ + tmp = opt; + while ((opt_value = get_word(&tmp, ';'))) + g_ptr_array_add(opt_values, (gpointer) opt_value); + g_ptr_array_add(opt_values, NULL); + opt_normalized = g_strjoinv(",", (char **) opt_values->pdata); + + if (!_nm_setting_bond_validate_option(opt_name, opt_normalized, &error)) { _LOGW(LOGD_CORE, "Ignoring invalid bond option: %s%s%s = %s%s%s: %s", NM_PRINT_FMT_QUOTE_STRING(opt_name), @@ -920,7 +942,7 @@ reader_parse_controller(Reader *reader, error->message); continue; } - nm_setting_bond_add_option(s_bond, opt_name, opt); + nm_setting_bond_add_option(s_bond, opt_name, opt_normalized); } mtu = get_word(&argument, ':'); diff --git a/src/nm-initrd-generator/tests/test-cmdline-reader.c b/src/nm-initrd-generator/tests/test-cmdline-reader.c index a0100764c..af13849c8 100644 --- a/src/nm-initrd-generator/tests/test-cmdline-reader.c +++ b/src/nm-initrd-generator/tests/test-cmdline-reader.c @@ -975,8 +975,8 @@ static void test_bond(void) { gs_unref_hashtable GHashTable *connections = NULL; - const char *const *ARGV = NM_MAKE_STRV("rd.route=192.0.2.53::bong0", - "bond=bong0:eth0,eth1:mode=balance-rr:9000", + const char *const *ARGV = NM_MAKE_STRV("rd.route=192.0.2.53::bond0", + "bond=bond0:eth0,eth1:mode=balance-rr:9000", "nameserver=203.0.113.53"); NMConnection *connection; NMSettingConnection *s_con; @@ -990,12 +990,12 @@ test_bond(void) connections = _parse_cons(ARGV); g_assert_cmpint(g_hash_table_size(connections), ==, 3); - connection = g_hash_table_lookup(connections, "bong0"); + connection = g_hash_table_lookup(connections, "bond0"); nmtst_assert_connection_verifies_without_normalization(connection); g_assert_cmpstr(nm_connection_get_connection_type(connection), ==, NM_SETTING_BOND_SETTING_NAME); - g_assert_cmpstr(nm_connection_get_id(connection), ==, "bong0"); + g_assert_cmpstr(nm_connection_get_id(connection), ==, "bond0"); controller_uuid = nm_connection_get_uuid(connection); g_assert(controller_uuid); @@ -1162,6 +1162,118 @@ test_bond_ip(void) NM_CONNECTION_MULTI_CONNECT_SINGLE); } +static void +test_bond_ip6_option(void) +{ + /* Test that IPv6 addresses within [] are parsed fine in different positions */ + + gs_unref_hashtable GHashTable *connections = NULL; + const char *const *ARGV = + NM_MAKE_STRV("bond=bond0:eth0,eth1:arp_interval=100,ns_ip6_target=[fc08::1]", + "bond=bond1:eth2,eth3:arp_interval=100,ns_ip6_target=[fc08::1]:9000", + "bond=bond2:eth4,eth5:ns_ip6_target=[fc08::1],arp_interval=100"); + NMConnection *connection; + NMSettingBond *s_bond; + + connections = _parse_cons(ARGV); + g_assert_cmpint(g_hash_table_size(connections), ==, 9); + + connection = g_hash_table_lookup(connections, "bond0"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "ns_ip6_target"), ==, "fc08::1"); + + connection = g_hash_table_lookup(connections, "bond1"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "ns_ip6_target"), ==, "fc08::1"); + + connection = g_hash_table_lookup(connections, "bond2"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "ns_ip6_target"), ==, "fc08::1"); +} + +static void +test_bond_multi_values_option(void) +{ + /* Test that semicolon-separated multi-valued options are parsed fine in different positions */ + + gs_unref_hashtable GHashTable *connections = NULL; + const char *const *ARGV = + NM_MAKE_STRV("bond=bond0:eth0,eth1:arp_interval=100,ns_ip6_target=[fc08::1];[fc08::2]", + "bond=bond1:eth2,eth3:arp_interval=100,ns_ip6_target=[fc08::1];[fc08::2]:9000", + "bond=bond2:eth4,eth5:ns_ip6_target=[fc08::1];[fc08::2],arp_interval=100", + "bond=bond3:eth6,eth7:arp_interval=100,arp_ip_target=10.0.0.1;10.0.0.2", + "bond=bond4:eth8,eth9:arp_interval=100,arp_ip_target=10.0.0.1;10.0.0.2:9000", + "bond=bond5:eth10,eth11:arp_ip_target=10.0.0.1;10.0.0.2,arp_interval=100"); + NMConnection *connection; + NMSettingBond *s_bond; + + connections = _parse_cons(ARGV); + g_assert_cmpint(g_hash_table_size(connections), ==, 18); + + connection = g_hash_table_lookup(connections, "bond0"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "ns_ip6_target"), + ==, + "fc08::1,fc08::2"); + + connection = g_hash_table_lookup(connections, "bond1"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "ns_ip6_target"), + ==, + "fc08::1,fc08::2"); + + connection = g_hash_table_lookup(connections, "bond2"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "ns_ip6_target"), + ==, + "fc08::1,fc08::2"); + + connection = g_hash_table_lookup(connections, "bond3"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "arp_ip_target"), + ==, + "10.0.0.1,10.0.0.2"); + + connection = g_hash_table_lookup(connections, "bond4"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "arp_ip_target"), + ==, + "10.0.0.1,10.0.0.2"); + + connection = g_hash_table_lookup(connections, "bond5"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "arp_ip_target"), + ==, + "10.0.0.1,10.0.0.2"); +} + static void test_bond_default(void) { @@ -2701,6 +2813,8 @@ main(int argc, char **argv) g_test_add_func("/initrd/cmdline/bootdev", test_bootdev); g_test_add_func("/initrd/cmdline/bond", test_bond); g_test_add_func("/initrd/cmdline/bond/ip", test_bond_ip); + g_test_add_func("/initrd/cmdline/bond/ip6-option", test_bond_ip6_option); + g_test_add_func("/initrd/cmdline/bond/multi-values-option", test_bond_multi_values_option); g_test_add_func("/initrd/cmdline/bond/default", test_bond_default); g_test_add_func("/initrd/cmdline/team", test_team); g_test_add_func("/initrd/cmdline/vlan", test_vlan);