From 8c212f76c246d9c3780b1de9b12dc21b8775561e Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 20 Oct 2020 14:20:19 +0900 Subject: [PATCH 1/4] network: make route_configure() return 0 on success Previously, route_configure() always returns 1 on success, and never returns 0. It is not necessary to return positive value. --- src/network/networkd-route.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 707660df3c..4ad9adc920 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -1130,7 +1130,7 @@ int route_configure( } } - return 1; + return 0; } static int route_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) { @@ -1201,8 +1201,8 @@ int link_set_routes(Link *link) { r = route_configure(rt, link, route_handler, NULL); if (r < 0) return log_link_warning_errno(link, r, "Could not set routes: %m"); - if (r > 0) - link->route_messages++; + + link->route_messages++; } if (link->route_messages == 0) { From cc17f75f66a2f42a143b56cc238c69b7e3a66acf Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 20 Oct 2020 15:26:51 +0900 Subject: [PATCH 2/4] network: copy multipath route element earlier `route_get()` compares input with existing routes, however previously, the input may did not have information about gateway. So, the comparison result might be incorrect, and the foregoing set_put() might return -EEXIST. --- src/network/networkd-route.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 4ad9adc920..34ede60405 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -487,20 +487,19 @@ static void route_copy(Route *dest, const Route *src, const MultipathRoute *m) { } } -static int route_add_internal(Manager *manager, Link *link, Set **routes, const Route *in, const MultipathRoute *m, Route **ret) { +static int route_add_internal(Manager *manager, Link *link, Set **routes, const Route *in, Route **ret) { _cleanup_(route_freep) Route *route = NULL; int r; assert(manager || link); assert(routes); assert(in); - assert(!m || (link && (m->ifindex == 0 || m->ifindex == link->ifindex))); r = route_new(&route); if (r < 0) return r; - route_copy(route, in, m); + route_copy(route, in, NULL); r = set_ensure_put(routes, &route_hash_ops, route); if (r < 0) @@ -521,20 +520,32 @@ static int route_add_internal(Manager *manager, Link *link, Set **routes, const static int route_add_foreign(Manager *manager, Link *link, const Route *in, Route **ret) { assert(manager || link); - return route_add_internal(manager, link, link ? &link->routes_foreign : &manager->routes_foreign, in, NULL, ret); + return route_add_internal(manager, link, link ? &link->routes_foreign : &manager->routes_foreign, in, ret); } static int route_add(Manager *manager, Link *link, const Route *in, const MultipathRoute *m, Route **ret) { + _cleanup_(route_freep) Route *tmp = NULL; Route *route; int r; assert(manager || link); assert(in); + if (m) { + assert(link && (m->ifindex == 0 || m->ifindex == link->ifindex)); + + r = route_new(&tmp); + if (r < 0) + return r; + + route_copy(tmp, in, m); + in = tmp; + } + r = route_get(manager, link, in, &route); if (r == -ENOENT) { /* Route does not exist, create a new one */ - r = route_add_internal(manager, link, link ? &link->routes : &manager->routes, in, m, &route); + r = route_add_internal(manager, link, link ? &link->routes : &manager->routes, in, &route); if (r < 0) return r; } else if (r == 0) { From 40075951dc437b3f7ea9a56085e7efa78598e33f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 20 Oct 2020 15:32:36 +0900 Subject: [PATCH 3/4] network: also compare and hash weight of the gateway --- src/network/networkd-route.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 34ede60405..85d540c32a 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -299,9 +299,10 @@ void route_hash_func(const Route *route, struct siphash *state) { siphash24_compress(&route->src, FAMILY_ADDRESS_SIZE(route->family), state); siphash24_compress(&route->gw_family, sizeof(route->gw_family), state); - if (IN_SET(route->gw_family, AF_INET, AF_INET6)) + if (IN_SET(route->gw_family, AF_INET, AF_INET6)) { siphash24_compress(&route->gw, FAMILY_ADDRESS_SIZE(route->gw_family), state); - + siphash24_compress(&route->gw_weight, sizeof(route->gw_weight), state); + } siphash24_compress(&route->prefsrc, FAMILY_ADDRESS_SIZE(route->family), state); @@ -356,6 +357,10 @@ int route_compare_func(const Route *a, const Route *b) { r = memcmp(&a->gw, &b->gw, FAMILY_ADDRESS_SIZE(a->family)); if (r != 0) return r; + + r = CMP(a->gw_weight, b->gw_weight); + if (r != 0) + return r; } r = memcmp(&a->prefsrc, &b->prefsrc, FAMILY_ADDRESS_SIZE(a->family)); @@ -484,6 +489,7 @@ static void route_copy(Route *dest, const Route *src, const MultipathRoute *m) { } else { dest->gw_family = src->gw_family; dest->gw = src->gw; + dest->gw_weight = src->gw_weight; } } From 0c54bfd698925c0449a85399bbf8d30b10295f7b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 20 Oct 2020 15:44:21 +0900 Subject: [PATCH 4/4] network: call netlink in the last of route_configure() Otherwise, assertion will be hit when route_add() fails. --- src/network/networkd-route.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 85d540c32a..dd05e11067 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -1119,13 +1119,6 @@ int route_configure( if (r < 0) return log_link_error_errno(link, r, "Could not append RTA_MULTIPATH attribute: %m"); - r = netlink_call_async(link->manager->rtnl, NULL, req, callback, - link_netlink_destroy_callback, link); - if (r < 0) - return log_link_error_errno(link, r, "Could not send rtnetlink message: %m"); - - link_ref(link); - if (ordered_set_isempty(route->multipath_routes)) { Route *nr; @@ -1147,6 +1140,13 @@ int route_configure( } } + r = netlink_call_async(link->manager->rtnl, NULL, req, callback, + link_netlink_destroy_callback, link); + if (r < 0) + return log_link_error_errno(link, r, "Could not send rtnetlink message: %m"); + + link_ref(link); + return 0; }