From 4cbd379e018c15d2369287adee59d1484eaedcc4 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 26 Apr 2021 23:55:09 +0900 Subject: [PATCH 1/6] network: drop meaningless bitfield specifiers --- src/network/networkd-manager.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/network/networkd-manager.h b/src/network/networkd-manager.h index e02c4ab59e..301b97c1a1 100644 --- a/src/network/networkd-manager.h +++ b/src/network/networkd-manager.h @@ -87,8 +87,8 @@ struct Manager { usec_t speed_meter_usec_new; usec_t speed_meter_usec_old; - bool dhcp4_prefix_root_cannot_set_table:1; - bool bridge_mdb_on_master_not_supported:1; + bool dhcp4_prefix_root_cannot_set_table; + bool bridge_mdb_on_master_not_supported; FirewallContext *fw_ctx; }; From e558d4f47a9c01b007fc193cabcf0dea8370a5f1 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 29 Apr 2021 00:07:09 +0900 Subject: [PATCH 2/6] network: dhcp4: downgrade log level when interface is removed --- src/network/networkd-dhcp4.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/network/networkd-dhcp4.c b/src/network/networkd-dhcp4.c index 1c7f4127eb..f20264be9e 100644 --- a/src/network/networkd-dhcp4.c +++ b/src/network/networkd-dhcp4.c @@ -1274,7 +1274,9 @@ static int dhcp4_handler(sd_dhcp_client *client, int event, void *userdata) { if (link->network->dhcp_send_release) { r = sd_dhcp_client_send_release(client); if (r < 0) - log_link_warning_errno(link, r, "Failed to send DHCP RELEASE, ignoring: %m"); + log_link_full_errno(link, + ERRNO_IS_DISCONNECT(r) ? LOG_DEBUG : LOG_WARNING, + r, "Failed to send DHCP RELEASE, ignoring: %m"); } r = dhcp_lease_lost(link); From fd221544f22ecc01998956ffd5e5c2470e94501b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 29 Apr 2021 01:17:36 +0900 Subject: [PATCH 3/6] network: unify log_link_message_full_errno() and log_message_warning_errno() This also introduces log_message_error_errno() or friends. --- src/network/networkd-link.c | 12 ------------ src/network/networkd-link.h | 7 ------- src/network/networkd-util.c | 15 +++++++++++++++ src/network/networkd-util.h | 20 ++++++++++++++------ 4 files changed, 29 insertions(+), 25 deletions(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index e851e9e51b..d29ba13ad3 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -2967,15 +2967,3 @@ static const char* const link_state_table[_LINK_STATE_MAX] = { }; DEFINE_STRING_TABLE_LOOKUP(link_state, LinkState); - -int log_link_message_full_errno(Link *link, sd_netlink_message *m, int level, int err, const char *msg) { - const char *err_msg = NULL; - - (void) sd_netlink_message_read_string(m, NLMSGERR_ATTR_MSG, &err_msg); - return log_link_full_errno(link, level, err, - "%s: %s%s%s%m", - msg, - strempty(err_msg), - err_msg && !endswith(err_msg, ".") ? "." : "", - err_msg ? " " : ""); -} diff --git a/src/network/networkd-link.h b/src/network/networkd-link.h index a9f6cf61eb..8e91c05f0f 100644 --- a/src/network/networkd-link.h +++ b/src/network/networkd-link.h @@ -237,10 +237,3 @@ int link_reconfigure(Link *link, bool force); int manager_udev_process_link(sd_device_monitor *monitor, sd_device *device, void *userdata); int manager_rtnl_process_link(sd_netlink *rtnl, sd_netlink_message *message, Manager *m); - -int log_link_message_full_errno(Link *link, sd_netlink_message *m, int level, int err, const char *msg); -#define log_link_message_error_errno(link, m, err, msg) log_link_message_full_errno(link, m, LOG_ERR, err, msg) -#define log_link_message_warning_errno(link, m, err, msg) log_link_message_full_errno(link, m, LOG_WARNING, err, msg) -#define log_link_message_notice_errno(link, m, err, msg) log_link_message_full_errno(link, m, LOG_NOTICE, err, msg) -#define log_link_message_info_errno(link, m, err, msg) log_link_message_full_errno(link, m, LOG_INFO, err, msg) -#define log_link_message_debug_errno(link, m, err, msg) log_link_message_full_errno(link, m, LOG_DEBUG, err, msg) diff --git a/src/network/networkd-util.c b/src/network/networkd-util.c index a9dd6d45eb..43ddf124d4 100644 --- a/src/network/networkd-util.c +++ b/src/network/networkd-util.c @@ -2,6 +2,7 @@ #include "condition.h" #include "conf-parser.h" +#include "networkd-link.h" #include "networkd-util.h" #include "parse-util.h" #include "string-table.h" @@ -223,3 +224,17 @@ unsigned hashmap_find_free_section_line(Hashmap *hashmap) { return n + 1; } + +int log_link_message_full_errno(Link *link, sd_netlink_message *m, int level, int err, const char *msg) { + const char *err_msg = NULL; + + /* link may be NULL. */ + + (void) sd_netlink_message_read_string(m, NLMSGERR_ATTR_MSG, &err_msg); + return log_link_full_errno(link, level, err, + "%s: %s%s%s%m", + msg, + strempty(err_msg), + err_msg && !endswith(err_msg, ".") ? "." : "", + err_msg ? " " : ""); +} diff --git a/src/network/networkd-util.h b/src/network/networkd-util.h index 4926fc5eb7..62f38bcdc2 100644 --- a/src/network/networkd-util.h +++ b/src/network/networkd-util.h @@ -11,6 +11,8 @@ #include "network-util.h" #include "string-util.h" +typedef struct Link Link; + typedef struct NetworkConfigSection { unsigned line; bool invalid; @@ -73,9 +75,15 @@ static inline bool section_is_invalid(NetworkConfigSection *section) { DEFINE_TRIVIAL_CLEANUP_FUNC(type*, free_func); \ DEFINE_TRIVIAL_CLEANUP_FUNC(type*, free_func##_or_set_invalid); -static inline int log_message_warning_errno(sd_netlink_message *m, int err, const char *msg) { - const char *err_msg = NULL; - - (void) sd_netlink_message_read_string(m, NLMSGERR_ATTR_MSG, &err_msg); - return log_warning_errno(err, "%s: %s%s%m", msg, strempty(err_msg), err_msg ? " " : ""); -} +int log_link_message_full_errno(Link *link, sd_netlink_message *m, int level, int err, const char *msg); +#define log_link_message_error_errno(link, m, err, msg) log_link_message_full_errno(link, m, LOG_ERR, err, msg) +#define log_link_message_warning_errno(link, m, err, msg) log_link_message_full_errno(link, m, LOG_WARNING, err, msg) +#define log_link_message_notice_errno(link, m, err, msg) log_link_message_full_errno(link, m, LOG_NOTICE, err, msg) +#define log_link_message_info_errno(link, m, err, msg) log_link_message_full_errno(link, m, LOG_INFO, err, msg) +#define log_link_message_debug_errno(link, m, err, msg) log_link_message_full_errno(link, m, LOG_DEBUG, err, msg) +#define log_message_full_errno(m, level, err, msg) log_link_message_full_errno(NULL, m, level, err, msg) +#define log_message_error_errno(m, err, msg) log_message_full_errno(m, LOG_ERR, err, msg) +#define log_message_warning_errno(m, err, msg) log_message_full_errno(m, LOG_WARNING, err, msg) +#define log_message_notice_errno(m, err, msg) log_message_full_errno(m, LOG_NOTICE, err, msg) +#define log_message_info_errno(m, err, msg) log_message_full_errno(m, LOG_INFO, err, msg) +#define log_message_debug_errno(m, err, msg) log_message_full_errno(m, LOG_DEBUG, err, msg) From c3fa1257da36522563c568524ba2a5113b1d90e9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 29 Apr 2021 02:34:17 +0900 Subject: [PATCH 4/6] network: do not set route type on specific route removal --- src/network/networkd-route.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 9ed30d69ed..22e19cc146 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -717,10 +717,6 @@ static int route_set_netlink_message(const Route *route, sd_netlink_message *req } } - r = sd_rtnl_message_route_set_type(req, route->type); - if (r < 0) - return log_link_error_errno(link, r, "Could not set route type: %m"); - if (!route_type_is_reject(route) && route->nexthop_id == 0) { assert(link); /* Those routes must be attached to a specific link */ @@ -769,6 +765,7 @@ int route_remove( link_netlink_message_handler_t callback) { _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *req = NULL; + unsigned char type; int r; assert(link || manager); @@ -786,6 +783,21 @@ int route_remove( if (r < 0) return log_link_error_errno(link, r, "Could not create RTM_DELROUTE message: %m"); + if (route->family == AF_INET && route->nexthop_id > 0 && route->type == RTN_BLACKHOLE) + /* When IPv4 route has nexthop id and the nexthop type is blackhole, even though kernel + * sends RTM_NEWROUTE netlink message with blackhole type, kernel's internal route type + * fib_rt_info::type may not be blackhole. Thus, we cannot know the internal value. + * Moreover, on route removal, the matching is done with the hidden value if we set + * non-zero type in RTM_DELROUTE message. Note, sd_rtnl_message_new_route() sets + * RTN_UNICAST by default. So, we need to clear the type here. */ + type = RTN_UNSPEC; + else + type = route->type; + + r = sd_rtnl_message_route_set_type(req, type); + if (r < 0) + return log_link_error_errno(link, r, "Could not set route type: %m"); + r = route_set_netlink_message(route, req, link); if (r < 0) return r; @@ -1106,6 +1118,10 @@ int route_configure( if (r < 0) return log_link_error_errno(link, r, "Could not create RTM_NEWROUTE message: %m"); + r = sd_rtnl_message_route_set_type(req, route->type); + if (r < 0) + return log_link_error_errno(link, r, "Could not set route type: %m"); + r = route_set_netlink_message(route, req, link); if (r < 0) return r; From 8a9ce525d1d5830c074451dd2b62865958ac7308 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 29 Apr 2021 02:42:42 +0900 Subject: [PATCH 5/6] network: do not remove reject type routes more than once Usually, removing non-existing addresses, routes, and etc, are safe. However, when multiple interfaces lost their carriers simultaneously, then manager_drop_routes() and manager_drop_nexthop() are called multiple times. If a route with a blackhole nexthop is removed in that process, the later removal requests of the route fail with -EINVAL, rathar than -ESRCH, as the corresponding nexthop does not exist anymore. So, let's not remove routes which managed by Manager more than once. --- src/network/networkd-route.c | 5 +++++ src/network/networkd-route.h | 1 + 2 files changed, 6 insertions(+) diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 22e19cc146..c1e5f6b4b8 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -854,6 +854,9 @@ static int manager_drop_routes_internal(Manager *manager, bool foreign, const Li routes = foreign ? manager->routes_foreign : manager->routes; SET_FOREACH(route, routes) { + if (route->removing) + continue; + /* Do not touch routes managed by the kernel. */ if (route->protocol == RTPROT_KERNEL) continue; @@ -867,6 +870,8 @@ static int manager_drop_routes_internal(Manager *manager, bool foreign, const Li k = route_remove(route, manager, NULL, NULL); if (k < 0 && r >= 0) r = k; + + route->removing = true; } return r; diff --git a/src/network/networkd-route.h b/src/network/networkd-route.h index 331f1f9ea2..ddb433817a 100644 --- a/src/network/networkd-route.h +++ b/src/network/networkd-route.h @@ -52,6 +52,7 @@ typedef struct Route { bool protocol_set:1; bool pref_set:1; bool gateway_from_dhcp_or_ra:1; + bool removing:1; union in_addr_union gw; union in_addr_union dst; From 3c7f1c07572481288e7a6fbff22b0c353b1c72bc Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 29 Apr 2021 20:58:10 +0900 Subject: [PATCH 6/6] network: do not set nexthop ID, gateway, and multipath routes simultaneously See kernel's rtm_to_fib_config() in net/ipv4/fib_frontend.c and rtm_to_fib6_config() in net/ipv6/route.c. Note that if both gateway and multipath routes are specified, then kernel ignores gateway. So, strictly speaking, setting both gateway and multipath routes is allowed by kernel. But such situation is mostly user's misconfiguration. Let's refuse it. Note that the conditions newly added in route_configure() are redundant, as all static configurations are already verified in route_section_verify(), and dynamic configurations do not set nexthop_id or multipath routes. Just for safety. --- src/network/networkd-route.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index c1e5f6b4b8..070131f4af 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -645,7 +645,7 @@ static int route_set_netlink_message(const Route *route, sd_netlink_message *req /* link may be NULL */ - if (in_addr_is_set(route->gw_family, &route->gw)) { + if (in_addr_is_set(route->gw_family, &route->gw) && route->nexthop_id == 0) { if (route->gw_family == route->family) { r = netlink_message_append_in_addr_union(req, RTA_GATEWAY, route->gw_family, &route->gw); if (r < 0) @@ -1100,8 +1100,8 @@ int route_configure( Route **ret) { _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *req = NULL; - int r, k = 0; Route *nr; + int r, k; assert(link); assert(link->manager); @@ -1188,19 +1188,22 @@ int route_configure( if (r < 0) return log_link_error_errno(link, r, "Could not append RTA_METRICS attribute: %m"); - r = append_nexthops(route, req); - if (r < 0) - return log_link_error_errno(link, r, "Could not append RTA_MULTIPATH attribute: %m"); - - if (ordered_set_isempty(route->multipath_routes)) { + if (route->nexthop_id != 0 || + in_addr_is_set(route->gw_family, &route->gw) || + ordered_set_isempty(route->multipath_routes)) { k = route_add_and_setup_timer(link, route, NULL, &nr); if (k < 0) return k; } else { MultipathRoute *m; + r = append_nexthops(route, req); + if (r < 0) + return log_link_error_errno(link, r, "Could not append RTA_MULTIPATH attribute: %m"); + assert(!ret); + k = 0; ORDERED_SET_FOREACH(m, route->multipath_routes) { r = route_add_and_setup_timer(link, route, m, NULL); if (r < 0) @@ -2766,8 +2769,17 @@ static int route_section_verify(Route *route, Network *network) { route->section->filename, route->section->line); } + if ((route->gateway_from_dhcp_or_ra || + in_addr_is_set(route->gw_family, &route->gw)) && + !ordered_set_isempty(route->multipath_routes)) + return log_warning_errno(SYNTHETIC_ERRNO(EINVAL), + "%s: Gateway= cannot be specified with MultiPathRoute=. " + "Ignoring [Route] section from line %u.", + route->section->filename, route->section->line); + if (route->nexthop_id > 0 && - (in_addr_is_set(route->gw_family, &route->gw) || + (route->gateway_from_dhcp_or_ra || + in_addr_is_set(route->gw_family, &route->gw) || !ordered_set_isempty(route->multipath_routes))) return log_warning_errno(SYNTHETIC_ERRNO(EINVAL), "%s: NextHopId= cannot be specified with Gateway= or MultiPathRoute=. "