From 3cd02b6ed6e75319a5abee1a6716e90836ef30b8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 24 Dec 2022 21:50:38 +0100 Subject: [PATCH] libnm,platform: fix range for "weight" property of next hops for routes In kernel, the valid range for the weight is 1-256 (on netlink this is expressed as u8 in rtnh_hops, ranging 0-255). We need an additional value, to represent - unset weight, for non-ECMP routes in kernel. - in libnm API, to express routes that should not be merged as ECMP routes (the default). Extend the type in NMPlatformIP4Route.weight to u16, and fix the code for the special handling of the numeric range. Also the libnm API needs to change. Modify the type of the attribute on D-Bus from "b" to "u", to use a 32 bit integer. We use 32 bit, because we already have common code to handle 32 bit unsigned integers, despite only requiring 257 values. It seems better to stick to a few data types (u32) instead of introducing more, only because the range is limited. Co-Authored-By: Fernando Fernandez Mancera Fixes: 1bbdecf5e125 ('platform: manage ECMP routes') --- src/core/NetworkManagerUtils.c | 2 +- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 2 +- .../plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 7 ++-- .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 4 +- .../keyfile/tests/test-keyfile-settings.c | 4 +- src/libnm-core-impl/nm-setting-ip-config.c | 16 +++++++- src/libnm-core-impl/tests/test-general.c | 6 +-- src/libnm-platform/nm-linux-platform.c | 11 +++-- src/libnm-platform/nm-platform.c | 22 ++++------ src/libnm-platform/nm-platform.h | 41 +++++++++++-------- 10 files changed, 64 insertions(+), 51 deletions(-) diff --git a/src/core/NetworkManagerUtils.c b/src/core/NetworkManagerUtils.c index 21f8d19d44..f3c032ce9f 100644 --- a/src/core/NetworkManagerUtils.c +++ b/src/core/NetworkManagerUtils.c @@ -1382,7 +1382,7 @@ nm_utils_ip_route_attribute_to_platform(int addr_family, GET_ATTR(NM_IP_ROUTE_ATTRIBUTE_TOS, r4->tos, BYTE, byte, 0); GET_ATTR(NM_IP_ROUTE_ATTRIBUTE_SCOPE, scope, BYTE, byte, RT_SCOPE_NOWHERE); - GET_ATTR(NM_IP_ROUTE_ATTRIBUTE_WEIGHT, r4->weight, BYTE, byte, 0); + GET_ATTR(NM_IP_ROUTE_ATTRIBUTE_WEIGHT, r4->weight, UINT32, uint32, 0); r4->scope_inv = nm_platform_route_scope_inv(scope); } diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 2655cd9247..0eeead8af5 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -970,7 +970,7 @@ parse_route_line(const char *line, [PARSE_LINE_ATTR_ROUTE_WEIGHT] = { .key = NM_IP_ROUTE_ATTRIBUTE_WEIGHT, - .type = PARSE_LINE_TYPE_UINT8, + .type = PARSE_LINE_TYPE_UINT32, .disabled = PARSE_LINE_AF_FLAG_FOR_IPV6, }, [PARSE_LINE_ATTR_ROUTE_SCOPE] = diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index 5f0b8a6f7a..2ea097fd98 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -2360,15 +2360,14 @@ get_route_attributes_string(NMIPRoute *route, int family) /* we also have a corresponding attribute with the numeric value. The * lock setting is handled above. */ } - } else if (NM_IN_STRSET(names[i], - NM_IP_ROUTE_ATTRIBUTE_SCOPE, - NM_IP_ROUTE_ATTRIBUTE_WEIGHT)) { + } else if (NM_IN_STRSET(names[i], NM_IP_ROUTE_ATTRIBUTE_SCOPE)) { g_string_append_printf(str, "%s %u", names[i], (unsigned) g_variant_get_byte(attr)); } else if (nm_streq(names[i], NM_IP_ROUTE_ATTRIBUTE_TOS)) { g_string_append_printf(str, "%s 0x%02x", names[i], (unsigned) g_variant_get_byte(attr)); } else if (NM_IN_STRSET(names[i], + NM_IP_ROUTE_ATTRIBUTE_RTO_MIN, NM_IP_ROUTE_ATTRIBUTE_TABLE, - NM_IP_ROUTE_ATTRIBUTE_RTO_MIN)) { + NM_IP_ROUTE_ATTRIBUTE_WEIGHT)) { g_string_append_printf(str, "%s %u", names[i], (unsigned) g_variant_get_uint32(attr)); } else if (nm_streq(names[i], NM_IP_ROUTE_ATTRIBUTE_QUICKACK)) { g_string_append_printf(str, "%s %u", names[i], (unsigned) g_variant_get_boolean(attr)); diff --git a/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c index 7d8cf0ebad..40ff7c670e 100644 --- a/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -1365,7 +1365,7 @@ test_read_wired_static_routes(void) nmtst_assert_route_attribute_byte(ip4_route, NM_IP_ROUTE_ATTRIBUTE_TOS, 0x28); nmtst_assert_route_attribute_uint32(ip4_route, NM_IP_ROUTE_ATTRIBUTE_WINDOW, 30000); nmtst_assert_route_attribute_uint32(ip4_route, NM_IP_ROUTE_ATTRIBUTE_CWND, 12); - nmtst_assert_route_attribute_byte(ip4_route, NM_IP_ROUTE_ATTRIBUTE_WEIGHT, 5); + nmtst_assert_route_attribute_uint32(ip4_route, NM_IP_ROUTE_ATTRIBUTE_WEIGHT, 5); nmtst_assert_route_attribute_uint32(ip4_route, NM_IP_ROUTE_ATTRIBUTE_INITCWND, 13); nmtst_assert_route_attribute_uint32(ip4_route, NM_IP_ROUTE_ATTRIBUTE_INITRWND, 14); nmtst_assert_route_attribute_uint32(ip4_route, NM_IP_ROUTE_ATTRIBUTE_MTU, 9000); @@ -4398,7 +4398,7 @@ test_write_wired_static(void) route4 = nm_ip_route_new(AF_INET, "1.1.1.1", 24, "1.2.3.4", 99, &error); g_assert_no_error(error); nm_ip_route_set_attribute(route4, NM_IP_ROUTE_ATTRIBUTE_CWND, g_variant_new_uint32(100)); - nm_ip_route_set_attribute(route4, NM_IP_ROUTE_ATTRIBUTE_WEIGHT, g_variant_new_byte(5)); + nm_ip_route_set_attribute(route4, NM_IP_ROUTE_ATTRIBUTE_WEIGHT, g_variant_new_uint32(5)); nm_setting_ip_config_add_route(s_ip4, route4); nm_ip_route_unref(route4); diff --git a/src/core/settings/plugins/keyfile/tests/test-keyfile-settings.c b/src/core/settings/plugins/keyfile/tests/test-keyfile-settings.c index 2d4adfbcfe..83019babb1 100644 --- a/src/core/settings/plugins/keyfile/tests/test-keyfile-settings.c +++ b/src/core/settings/plugins/keyfile/tests/test-keyfile-settings.c @@ -311,7 +311,7 @@ test_read_valid_wired_connection(void) nmtst_assert_route_attribute_uint32(route, NM_IP_ROUTE_ATTRIBUTE_CWND, 10); nmtst_assert_route_attribute_uint32(route, NM_IP_ROUTE_ATTRIBUTE_MTU, 1430); - nmtst_assert_route_attribute_byte(route, NM_IP_ROUTE_ATTRIBUTE_WEIGHT, 5); + nmtst_assert_route_attribute_uint32(route, NM_IP_ROUTE_ATTRIBUTE_WEIGHT, 5); nmtst_assert_route_attribute_boolean(route, NM_IP_ROUTE_ATTRIBUTE_LOCK_CWND, TRUE); nmtst_assert_route_attribute_string(route, NM_IP_ROUTE_ATTRIBUTE_SRC, "7.7.7.7"); nmtst_assert_route_attribute_string(route, NM_IP_ROUTE_ATTRIBUTE_TYPE, "unicast"); @@ -496,7 +496,7 @@ test_write_wired_connection(void) g_assert_no_error(error); nm_ip_route_set_attribute(rt, NM_IP_ROUTE_ATTRIBUTE_CWND, g_variant_new_uint32(10)); nm_ip_route_set_attribute(rt, NM_IP_ROUTE_ATTRIBUTE_MTU, g_variant_new_uint32(1492)); - nm_ip_route_set_attribute(rt, NM_IP_ROUTE_ATTRIBUTE_WEIGHT, g_variant_new_byte(5)); + nm_ip_route_set_attribute(rt, NM_IP_ROUTE_ATTRIBUTE_WEIGHT, g_variant_new_uint32(5)); nm_ip_route_set_attribute(rt, NM_IP_ROUTE_ATTRIBUTE_SRC, g_variant_new_string("1.2.3.4")); g_assert(nm_setting_ip_config_add_route(s_ip4, rt)); nm_ip_route_unref(rt); diff --git a/src/libnm-core-impl/nm-setting-ip-config.c b/src/libnm-core-impl/nm-setting-ip-config.c index 7d38e16d95..bea0ee7f11 100644 --- a/src/libnm-core-impl/nm-setting-ip-config.c +++ b/src/libnm-core-impl/nm-setting-ip-config.c @@ -1279,8 +1279,9 @@ static const NMVariantAttributeSpec *const ip_route_attribute_spec[] = { .v6 = TRUE, .type_detail = 'T', ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_IP_ROUTE_ATTRIBUTE_WEIGHT, - G_VARIANT_TYPE_BYTE, - .v4 = TRUE, ), + G_VARIANT_TYPE_UINT32, + .v4 = TRUE, + .type_detail = 'w'), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_IP_ROUTE_ATTRIBUTE_WINDOW, G_VARIANT_TYPE_UINT32, .v4 = TRUE, @@ -1316,6 +1317,7 @@ _ip_route_attribute_validate(const char *name, { const NMVariantAttributeSpec *spec; const char *string; + guint32 u32; nm_assert(name); nm_assert(value); @@ -1429,6 +1431,16 @@ _ip_route_attribute_validate(const char *name, if (parse_data) parse_data->scope = g_variant_get_byte(value); break; + case 'w': /* weight */ + u32 = g_variant_get_uint32(value); + if (u32 > 256) { + g_set_error_literal(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_FAILED, + _("route weight cannot be larger than 256")); + return FALSE; + } + break; case '\0': break; default: diff --git a/src/libnm-core-impl/tests/test-general.c b/src/libnm-core-impl/tests/test-general.c index 35882f7ed4..f6684bfbc8 100644 --- a/src/libnm-core-impl/tests/test-general.c +++ b/src/libnm-core-impl/tests/test-general.c @@ -2501,7 +2501,7 @@ test_setting_ip_route_attributes(void) TEST_ATTR("tos", byte, 127, AF_INET, TRUE, TRUE); TEST_ATTR("tos", string, "0x28", AF_INET, FALSE, TRUE); - TEST_ATTR("weight", byte, 100, AF_INET, TRUE, TRUE); + TEST_ATTR("weight", uint32, 100, AF_INET, TRUE, TRUE); TEST_ATTR("weight", string, "100", AF_INET, FALSE, TRUE); TEST_ATTR("advmss", uint32, 1400, AF_INET, TRUE, TRUE); @@ -9990,8 +9990,8 @@ test_route_attributes_parse(void) variant = g_hash_table_lookup(ht, NM_IP_ROUTE_ATTRIBUTE_WEIGHT); g_assert(variant); - g_assert(g_variant_is_of_type(variant, G_VARIANT_TYPE_BYTE)); - g_assert_cmpuint(g_variant_get_byte(variant), ==, 5); + g_assert(g_variant_is_of_type(variant, G_VARIANT_TYPE_UINT32)); + g_assert_cmpuint(g_variant_get_uint32(variant), ==, 5); variant = g_hash_table_lookup(ht, NM_IP_ROUTE_ATTRIBUTE_SRC); g_assert(variant); diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index ae074dac51..6d53d99849 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -3847,7 +3847,7 @@ _new_from_nl_route(const struct nlmsghdr *nlh, gboolean id_only, ParseNlmsgIter nm_assert(v4_n_nexthops - 1u < v4_nh_extra_alloc); new_nexthop = &v4_nh_extra_nexthops[v4_n_nexthops - 1u]; new_nexthop->ifindex = rtnh->rtnh_ifindex; - new_nexthop->weight = NM_MAX(rtnh->rtnh_hops, 1u); + new_nexthop->weight = NM_MAX(((guint) rtnh->rtnh_hops) + 1u, 1u); if (rtnh->rtnh_len > sizeof(*rtnh)) { struct nlattr *ntb[RTA_MAX + 1]; @@ -3864,7 +3864,7 @@ _new_from_nl_route(const struct nlmsghdr *nlh, gboolean id_only, ParseNlmsgIter } else if (IS_IPv4 || idx == multihop_idx) { nh.found = TRUE; nh.ifindex = rtnh->rtnh_ifindex; - nh.weight = NM_MAX(rtnh->rtnh_hops, 1u); + nh.weight = NM_MAX(((guint) rtnh->rtnh_hops) + 1u, 1u); if (rtnh->rtnh_len > sizeof(*rtnh)) { struct nlattr *ntb[RTA_MAX + 1]; @@ -5398,14 +5398,17 @@ _nl_msg_new_route(uint16_t nlmsg_type, uint16_t nlmsg_flags, const NMPObject *ob if (!rtnh) goto nla_put_failure; + /* For multihop routes, a valid weight must be in range 1-256 (on netlink's + * "rtnh_hops" this is 0-255). We allow that the caller leaves the value unset + * at zero. */ if (i == 0u) { - rtnh->rtnh_hops = NM_MAX(obj->ip4_route.weight, 1u); + rtnh->rtnh_hops = NM_MAX(obj->ip4_route.weight, 1u) - 1u; rtnh->rtnh_ifindex = obj->ip4_route.ifindex; NLA_PUT_U32(msg, RTA_GATEWAY, obj->ip4_route.gateway); } else { const NMPlatformIP4RtNextHop *n = &obj->_ip4_route.extra_nexthops[i - 1u]; - rtnh->rtnh_hops = NM_MAX(n->weight, 1u); + rtnh->rtnh_hops = NM_MAX(n->weight, 1u) - 1u; rtnh->rtnh_ifindex = n->ifindex; NLA_PUT_U32(msg, RTA_GATEWAY, n->gateway); } diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index f201f2f9c7..85bd785ac8 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -6988,16 +6988,13 @@ nm_platform_ip4_route_to_string_full(const NMPlatformIP4Route *route, nm_strbuf_append(&buf, &len, " nexthop" - "%s%s" /* gateway */ - "%s%s" /* weight */ - "%s" /* dev/ifindex */ + "%s%s" /* gateway */ + " weight %s" /* weight */ + "%s" /* dev/ifindex */ "", s_gateway[0] ? " via " : "", s_gateway, - NM_PRINT_FMT_QUOTED2(route->weight != 1, - " weight ", - nm_sprintf_buf(weight_str, "%u", route->weight), - ""), + nm_sprintf_buf(weight_str, "%u", route->weight), _to_string_dev(str_dev, route->ifindex)); if (!extra_nexthops) nm_strbuf_append_str(&buf, &len, " nexthops [...]"); @@ -7011,19 +7008,16 @@ nm_platform_ip4_route_to_string_full(const NMPlatformIP4Route *route, &buf, &len, " nexthop" - "%s" /* ifindex */ - "%s%s" /* gateway */ - "%s%s" /* weight */ + "%s" /* ifindex */ + "%s%s" /* gateway */ + " weight %s" /* weight */ "", NM_PRINT_FMT_QUOTED2(nexthop->gateway != 0 || nexthop->ifindex <= 0, " via ", nm_inet4_ntop(nexthop->gateway, s_gateway), ""), _to_string_dev(str_dev, nexthop->ifindex), - NM_PRINT_FMT_QUOTED2(nexthop->weight != 1, - " weight ", - nm_sprintf_buf(weight_str, "%u", nexthop->weight), - "")); + nm_sprintf_buf(weight_str, "%u", nexthop->weight)); } } } diff --git a/src/libnm-platform/nm-platform.h b/src/libnm-platform/nm-platform.h index 71ac6ccc3b..ea46321534 100644 --- a/src/libnm-platform/nm-platform.h +++ b/src/libnm-platform/nm-platform.h @@ -410,6 +410,20 @@ struct _NMPlatformIP4Route { * pref_src must match, unless set to 0.0.0.0 to match any. */ in_addr_t pref_src; + /* This is the weight of for the first next-hop, in case of n_nexthops > 1. + * + * If n_nexthops is zero, this value is undefined (should be zero). + * If n_nexthops is 1, this also doesn't matter, but it's usually set to + * zero. + * If n_nexthops is greater or equal to one, this is the weight of + * the first hop. + * + * Note that upper layers (nm_utils_ip_route_attribute_to_platform()) use this flag to indicate + * whether this is a multihop route. Single-hop, non-ECMP routes will have a weight of zero. + * + * The valid range for weight in kernel is 1-256. */ + guint16 weight; + /* rtm_tos (iproute2: tos) * * For IPv4, tos is part of the weak-id (like metric). @@ -429,21 +443,6 @@ struct _NMPlatformIP4Route { * For IPv6 routes, the scope is ignored and kernel always assumes global scope. * Hence, this field is only in NMPlatformIP4Route. */ guint8 scope_inv; - - /* This is the weight of for the first next-hop, in case of n_nexthops > 1. - * - * If n_nexthops is zero, this value is undefined (should be zero). - * If n_nexthops is 1, this also doesn't matter, but it's usually set to - * zero. - * If n_nexthops is greater or equal to one, this is the weight of - * the first hop. - * - * Note that upper layers use this flag to indicate whether this is a multihop route. - * Single-hop, non-ECMP routes will have a weight of zero. - * - * The valid range for weight is 1-255. For convenience, we treat 0 the same - * as 1 for multihop routes. */ - guint8 weight; } _nm_alignas(NMPlatformObject); struct _NMPlatformIP6Route { @@ -671,9 +670,15 @@ typedef struct { typedef struct { int ifindex; in_addr_t gateway; - /* The valid range for weight is 1-255. For convenience, we treat 0 the same - * as 1 for multihop routes. */ - guint8 weight; + /* The valid range for weight is 1-256. Single hop routes in kernel + * don't have a weight, we assign them weight zero (to indicate the + * weight is missing). + * + * Upper layers (nm_utils_ip_route_attribute_to_platform()) care about + * the distinction of unset weight (no-ECMP). They express no-ECMP as + * zero. + */ + guint16 weight; } NMPlatformIP4RtNextHop; typedef struct {