fdo-mirrors/NetworkManager
1
0
Fork 0
mirror of https://gitlab.freedesktop.org/NetworkManager/NetworkManager.git synced 2026-02-28 05:10:32 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions

platform: fix handling "weight" for IPv4 routes

Browse source 
The hash/cmp functions were wrong with respect to IPv4 routes. Fix that.

- the weight was cast to a guint8, which is too small to hold all
  values.

- NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID comparison would normalize a zero
  weight to one

    NM_CMP_DIRECT(NM_MAX(a->weight, 1u), NM_MAX(b->weight, 1u));

  That was very wrong.

Clean this up. How the weight is handled, depends on the n_nexthops
parameter.

The remarkable thing is that upper layers find it useful to track IPv4
single-hop routes with a non-zero weight. Consequently, this is honored
by NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID which treats single-hop routes
different, if their weight differs. However, adding such a route in
kernel will not work. In kernel, single-hop routes have no weight. While
the route exists as distinct according to the implemented identity, it never
exists in kernel. Well, you can call nm_platform_ip_route_add() with such
a route, but the result will have a weight of zero (being a different
route). See also nm_platform_ip_route_normalize().

This works all mostly fine. The only thing to take care is that you
cannot look into the NMPlatform cache and ever find a IPv4 single-hop
route with a non-zero weight. If you preform such a lookup, realize that
such routes don't exist in platform. You can however normalize the
weight to zero first (nm_platform_ip_route_normalize()) and look for a
similar route with a zero weight.

Fixes: 1bbdecf5e1 ('platform: manage ECMP routes')
This commit is contained in:
Thomas Haller 2023-11-28 12:37:01 +01:00
parent 02371a18b0
commit 335f62fe52
No known key found for this signature in database
GPG key ID: 29C2366E4DFC5728
5 changed files with 130 additions and 50 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
src/core/nm-l3-config-data.c
View file

@ -2512,7 +2512,7 @@ nm_l3_config_data_get_blacklisted_ip4_routes(const NML3ConfigData *self, gboolea
.metric = NM_PLATFORM_ROUTE_METRIC_IP4_DEVICE_ROUTE,
.scope_inv = nm_platform_route_scope_inv(NM_RT_SCOPE_LINK),
};
nm_platform_ip_route_normalize(AF_INET, &rx.rx);
nm_platform_ip_route_normalize(AF_INET, &rx.rx, NMP_IP_ROUTE_NORMALIZE_FLAGS_NONE);
if (nm_l3_config_data_lookup_route(self, AF_INET, &rx.rx)) {
/* we track such a route explicitly. Don't blacklist it. */
@ -2693,7 +2693,9 @@ nm_l3_config_data_add_dependent_device_routes(NML3ConfigData *self,
.plen = plen,
};
nm_platform_ip_route_normalize(addr_family, &rx.rx);
nm_platform_ip_route_normalize(addr_family,
&rx.rx,
NMP_IP_ROUTE_NORMALIZE_FLAGS_NONE);
nm_l3_config_data_add_route(self, addr_family, NULL, &rx.rx);
}
}

2
src/core/nm-l3cfg.c
View file

@ -3911,7 +3911,7 @@ _l3cfg_update_combined_config(NML3Cfg *self,
.type_coerced = nm_platform_route_type_coerce(RTN_LOCAL),
.pref_src = NM_IPV4LO_ADDR1,
};
nm_platform_ip_route_normalize(AF_INET, &rx.rx);
nm_platform_ip_route_normalize(AF_INET, &rx.rx, NMP_IP_ROUTE_NORMALIZE_FLAGS_NONE);
if (!nm_l3_config_data_lookup_route(l3cd, AF_INET, &rx.rx)) {
nm_l3_config_data_add_route_4(l3cd, &rx.r4);
}

10
src/core/platform/tests/test-route.c
View file

@ -719,7 +719,9 @@ test_ip4_route_options(gconstpointer test_data)
for (i = 0; i < rts_n; i++) {
rts_cmp[i] = rts_add[i];
nm_platform_ip_route_normalize(AF_INET, NM_PLATFORM_IP_ROUTE_CAST(&rts_cmp[i]));
nm_platform_ip_route_normalize(AF_INET,
NM_PLATFORM_IP_ROUTE_CAST(&rts_cmp[i]),
NMP_IP_ROUTE_NORMALIZE_FLAGS_NONE);
}
routes = nmtstp_ip4_route_get_all(NM_PLATFORM_GET, IFINDEX);
@ -891,7 +893,9 @@ test_ip6_route_options(gconstpointer test_data)
for (i = 0; i < rts_n; i++) {
rts_cmp[i] = rts_add[i];
nm_platform_ip_route_normalize(AF_INET6, NM_PLATFORM_IP_ROUTE_CAST(&rts_cmp[i]));
nm_platform_ip_route_normalize(AF_INET6,
NM_PLATFORM_IP_ROUTE_CAST(&rts_cmp[i]),
NMP_IP_ROUTE_NORMALIZE_FLAGS_NONE);
}
routes = nmtstp_ip6_route_get_all(NM_PLATFORM_GET, IFINDEX);
@ -1936,7 +1940,7 @@ test_blackhole(gconstpointer test_data)
};
}
nm_platform_ip_route_normalize(addr_family, &rr.rx);
nm_platform_ip_route_normalize(addr_family, &rr.rx, NMP_IP_ROUTE_NORMALIZE_FLAGS_NONE);
if (IS_IPv4)
r = nm_platform_ip4_route_add(NM_PLATFORM_GET, NMP_NLM_FLAG_APPEND, &rr.r4, NULL);

102
src/libnm-platform/nm-platform.c
View file

@ -5274,10 +5274,38 @@ _route_pref_normalize(guint8 pref)
: NM_ICMPV6_ROUTER_PREF_MEDIUM;
}
static guint16
_ip4_route_weight_normalize(guint n_nexthops, guint16 weight, gboolean keep_ecmp_weight)
{
if (n_nexthops > 1u) {
/* This is a multihop-route. The weight is relevant.
*
* We only normalize a zero to one (because in kernel such weights
* don't exist. */
return NM_MAX(weight, 1u);
}
if (n_nexthops == 0) {
/* This route has no next-hop (e.g. blackhole type). The weight is
* always irrelevant. Normalize to zero. */
return 0;
}
/* We have a IPv4 single-hop route. In kernel, the weight does not exist.
* It's always zero.
*
* For upper layers, we find it useful to track such routes with a positive
* weight. They are candidates to be merged into a multi-hop ECMP route.
*
* Depending on what the caller requests, we normalize it (or leave it
* unchanged). */
return keep_ecmp_weight ? weight : 0u;
}
/**
* nm_platform_ip_route_normalize:
* @addr_family: AF_INET or AF_INET6
* @route: an NMPlatformIP4Route or NMPlatformIP6Route instance, depending on @addr_family.
* @flags: flag affecting normalization.
*
* Adding a route to kernel via nm_platform_ip_route_add() will normalize/coerce some
* properties of the route. This function modifies (normalizes) the route like it
@ -5286,9 +5314,18 @@ _route_pref_normalize(guint8 pref)
* Note that this function is related to NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY
* in that if two routes compare semantically equal, after normalizing they also shall
* compare equal with NM_PLATFORM_IP_ROUTE_CMP_TYPE_FULL.
*
* Note that a positive "weight" of IPv4 single hop routes is not meaningful in
* kernel. While we track such routes at upper layers, they don't exist in
* kernel (well, they exist, with their weight set to zero, which makes them a
* different route according to NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID. The
* NMP_IP_ROUTE_NORMALIZE_FLAGS_KEEP_ECMP_WEIGHT flag can be used to prevent
* such normalization.
*/
void
nm_platform_ip_route_normalize(int addr_family, NMPlatformIPRoute *route)
nm_platform_ip_route_normalize(int addr_family,
NMPlatformIPRoute *route,
NMPIPRouteNormalizeFlags flags)
{
NMPlatformIP4Route *r4;
NMPlatformIP6Route *r6;
@ -5306,6 +5343,11 @@ nm_platform_ip_route_normalize(int addr_family, NMPlatformIPRoute *route)
route->metric_any = FALSE;
r4->network = nm_ip4_addr_clear_host_address(r4->network, r4->plen);
r4->scope_inv = _ip_route_scope_inv_get_normalized(r4);
r4->n_nexthops = nm_platform_ip4_route_get_n_nexthops(r4);
r4->weight = _ip4_route_weight_normalize(
r4->n_nexthops,
r4->weight,
NM_FLAGS_HAS(flags, NMP_IP_ROUTE_NORMALIZE_FLAGS_KEEP_ECMP_WEIGHT));
break;
case AF_INET6:
r6 = (NMPlatformIP6Route *) route;
@ -5343,7 +5385,8 @@ _ip_route_add(NMPlatform *self, NMPNlmFlags flags, NMPObject *obj_stack, char **
|| obj_stack->ip4_route.n_nexthops <= 1u || obj_stack->_ip4_route.extra_nexthops);
nm_platform_ip_route_normalize(NMP_OBJECT_GET_ADDR_FAMILY((obj_stack)),
NMP_OBJECT_CAST_IP_ROUTE(obj_stack));
NMP_OBJECT_CAST_IP_ROUTE(obj_stack),
NMP_IP_ROUTE_NORMALIZE_FLAGS_NONE);
ifindex = obj_stack->ip_route.ifindex;
@ -7039,7 +7082,7 @@ nm_platform_ip4_route_to_string_full(const NMPlatformIP4Route *route,
route->plen,
n_nexthops <= 1 && s_gateway[0] ? " via " : "",
n_nexthops <= 1 ? s_gateway : "",
NM_PRINT_FMT_QUOTED2(n_nexthops <= 1 && route->weight != 0,
NM_PRINT_FMT_QUOTED2(n_nexthops <= 1 && route->weight != 0u,
" weight ",
nm_sprintf_buf(weight_str, "%u", route->weight),
""),
@ -8596,12 +8639,11 @@ nm_platform_ip4_rt_nexthop_hash_update(const NMPlatformIP4RtNextHop *obj,
gboolean for_id,
NMHashState *h)
{
guint8 w;
guint16 w;
nm_assert(obj);
w = for_id ? NM_MAX(obj->weight, 1u) : obj->weight;
nm_hash_update_vals(h, obj->ifindex, obj->gateway, w);
}
@ -8610,6 +8652,8 @@ nm_platform_ip4_route_hash_update(const NMPlatformIP4Route *obj,
NMPlatformIPRouteCmpType cmp_type,
NMHashState *h)
{
guint n_nexthops;
switch (cmp_type) {
case NM_PLATFORM_IP_ROUTE_CMP_TYPE_WEAK_ID:
case NM_PLATFORM_IP_ROUTE_CMP_TYPE_ECMP_ID:
@ -8647,15 +8691,17 @@ nm_platform_ip4_route_hash_update(const NMPlatformIP4Route *obj,
obj->lock_mtu,
obj->lock_mss));
if (cmp_type == NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID) {
n_nexthops = nm_platform_ip4_route_get_n_nexthops(obj);
nm_hash_update_vals(h,
obj->ifindex,
nm_platform_ip4_route_get_n_nexthops(obj),
n_nexthops,
obj->gateway,
(guint8) NM_MAX(obj->weight, 1u));
_ip4_route_weight_normalize(n_nexthops, obj->weight, TRUE));
}
}
break;
case NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY:
n_nexthops = nm_platform_ip4_route_get_n_nexthops(obj);
nm_hash_update_vals(
h,
obj->type_coerced,
@ -8664,9 +8710,9 @@ nm_platform_ip4_route_hash_update(const NMPlatformIP4Route *obj,
nm_ip4_addr_clear_host_address(obj->network, obj->plen),
obj->plen,
obj->metric,
nm_platform_ip4_route_get_n_nexthops(obj),
n_nexthops,
obj->gateway,
(guint8) NM_MAX(obj->weight, 1u),
_ip4_route_weight_normalize(n_nexthops, obj->weight, TRUE),
nmp_utils_ip_config_source_round_trip_rtprot(obj->rt_source),
_ip_route_scope_inv_get_normalized(obj),
obj->tos,
@ -8732,14 +8778,9 @@ nm_platform_ip4_rt_nexthop_cmp(const NMPlatformIP4RtNextHop *a,
const NMPlatformIP4RtNextHop *b,
gboolean for_id)
{
guint8 w_a;
guint8 w_b;
guint16 w_a;
guint16 w_b;
/* Note that weight zero is not valid (in kernel). We thus treat
* weight zero usually the same as 1.
*
* Not here for cmp/hash_update functions. These functions check for the exact
* bit-pattern, and not the it means at other places. */
NM_CMP_SELF(a, b);
NM_CMP_FIELD(a, b, ifindex);
NM_CMP_FIELD(a, b, gateway);
@ -8756,6 +8797,8 @@ nm_platform_ip4_route_cmp(const NMPlatformIP4Route *a,
const NMPlatformIP4Route *b,
NMPlatformIPRouteCmpType cmp_type)
{
guint n_nexthops;
NM_CMP_SELF(a, b);
switch (cmp_type) {
case NM_PLATFORM_IP_ROUTE_CMP_TYPE_ECMP_ID:
@ -8802,9 +8845,10 @@ nm_platform_ip4_route_cmp(const NMPlatformIP4Route *a,
if (cmp_type == NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID) {
NM_CMP_FIELD(a, b, ifindex);
NM_CMP_FIELD(a, b, gateway);
NM_CMP_DIRECT(NM_MAX(a->weight, 1u), NM_MAX(b->weight, 1u));
NM_CMP_DIRECT(nm_platform_ip4_route_get_n_nexthops(a),
nm_platform_ip4_route_get_n_nexthops(b));
n_nexthops = nm_platform_ip4_route_get_n_nexthops(a);
NM_CMP_DIRECT(n_nexthops, nm_platform_ip4_route_get_n_nexthops(b));
NM_CMP_DIRECT(_ip4_route_weight_normalize(n_nexthops, a->weight, TRUE),
_ip4_route_weight_normalize(n_nexthops, b->weight, TRUE));
}
}
break;
@ -8825,16 +8869,16 @@ nm_platform_ip4_route_cmp(const NMPlatformIP4Route *a,
NM_CMP_FIELD(a, b, plen);
NM_CMP_FIELD_UNSAFE(a, b, metric_any);
NM_CMP_FIELD(a, b, metric);
if (cmp_type == NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY) {
NM_CMP_DIRECT(nm_platform_ip4_route_get_n_nexthops(a),
nm_platform_ip4_route_get_n_nexthops(b));
} else
NM_CMP_FIELD(a, b, n_nexthops);
NM_CMP_FIELD(a, b, gateway);
if (cmp_type == NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY)
NM_CMP_DIRECT(NM_MAX(a->weight, 1u), NM_MAX(b->weight, 1u));
else
if (cmp_type == NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY) {
n_nexthops = nm_platform_ip4_route_get_n_nexthops(a);
NM_CMP_DIRECT(n_nexthops, nm_platform_ip4_route_get_n_nexthops(b));
NM_CMP_DIRECT(_ip4_route_weight_normalize(n_nexthops, a->weight, TRUE),
_ip4_route_weight_normalize(n_nexthops, b->weight, TRUE));
} else {
NM_CMP_FIELD(a, b, n_nexthops);
NM_CMP_FIELD(a, b, weight);
}
if (cmp_type == NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY) {
NM_CMP_DIRECT(nmp_utils_ip_config_source_round_trip_rtprot(a->rt_source),
nmp_utils_ip_config_source_round_trip_rtprot(b->rt_source));
@ -9414,7 +9458,9 @@ nm_platform_ip4_address_generate_device_route(const NMPlatformIP4Address *addr,
.scope_inv = nm_platform_route_scope_inv(NM_RT_SCOPE_LINK),
};
nm_platform_ip_route_normalize(AF_INET, (NMPlatformIPRoute *) dst);
nm_platform_ip_route_normalize(AF_INET,
(NMPlatformIPRoute *) dst,
NMP_IP_ROUTE_NORMALIZE_FLAGS_NONE);
return dst;
}

60
src/libnm-platform/nm-platform.h
View file

@ -444,18 +444,27 @@ 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.
/* This is the weight of for the first next-hop.
*
* 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.
* For multi-hop routes (n_nexthops > 1) this is the weight of the first
* hop. Note that the valid range is from 1-256. Zero is treated the same
* as 1 (for NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID comparison).
*
* 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.
* For routes without next-hop (e.g. blackhole type), the weight is
* meaningless. It should be set to zero. NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID
* will treat it as zero.
*
* The valid range for weight in kernel is 1-256. */
* For single-hop routes, in kernel they don't have a weight. That means,
* all routes in the platform cache have a weight of zero. For tracking
* purposes, we find it useful that upper layers have single-hop routes
* with a positive weight. Such routes can never exist in kernel. Trying
* to add such a route will somewhat work, because
* nm_platform_ip_route_normalize() normalizes the weight to zero
* (effectively adding another route, according to
* NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID). A lookup in the platform cache with
* such a route will not yield a result. It does not exist there. If you
* want to find such a route, normalize it first.
*/
guint16 weight;
/* rtm_tos (iproute2: tos)
@ -704,13 +713,11 @@ typedef struct {
typedef struct {
int ifindex;
in_addr_t gateway;
/* 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).
/* The weight of the next hop. The valid range for weight is 1-256.
*
* Upper layers (nm_utils_ip_route_attribute_to_platform()) care about
* the distinction of unset weight (no-ECMP). They express no-ECMP as
* zero.
* Zero is allowed too, but treated as 1 (by
* NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID comparison).
*/
guint16 weight;
@ -2263,7 +2270,28 @@ nm_platform_ip_address_get_prune_list(NMPlatform *self,
gboolean nm_platform_ip_address_flush(NMPlatform *self, int addr_family, int ifindex);
void nm_platform_ip_route_normalize(int addr_family, NMPlatformIPRoute *route);
typedef enum {
/* No flags. */
NMP_IP_ROUTE_NORMALIZE_FLAGS_NONE = 0,
/* Don't normalize the "weight" for IPv4 single hop routes.
* nm_platform_ip_route_normalize() aims to normalize a route as it
* can exist in kernel. But in kernel, a IPv4 single hop route cannot
* have a (non-zero) weight. Normalization would usually loose that
* information.
*
* However, it can be useful to normalize routes, but not the "weight".
* Upper layers want to track single hop routes with positive weight for
* internal purposes, while normalizing all other fields. This flag
* allows to opt-out from that normalization. The result may not ever
* exist in kernel (as far as NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID is concerned).
*/
NMP_IP_ROUTE_NORMALIZE_FLAGS_KEEP_ECMP_WEIGHT = 0x1,
} NMPIPRouteNormalizeFlags;
void nm_platform_ip_route_normalize(int addr_family,
NMPlatformIPRoute *route,
NMPIPRouteNormalizeFlags flags);
static inline guint32
nm_platform_ip4_route_get_effective_metric(const NMPlatformIP4Route *r)