From ce9388d72a710ffc4840572fecdfe7d9c0be0956 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 17 May 2021 15:23:57 +0900 Subject: [PATCH 1/4] network: fix possible message counter double decrement When after_configure() for a request fails, then the request is not removed from the queue at that time, and the link enters to failed state. After that, if the link is reconfigured or its carrier is lost, then the request is dropped from the queue, and the message_counter is decreased. However, the counter is already or will be also decreased when the corresponding netlink reply is received. So, the counter is decreased twice. --- src/network/networkd-address.c | 3 +++ src/network/networkd-neighbor.c | 3 +++ src/network/networkd-nexthop.c | 3 +++ src/network/networkd-route.c | 3 +++ src/network/networkd-routing-policy-rule.c | 3 +++ 5 files changed, 15 insertions(+) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index 5a3388c3c4..daf959ec5f 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -1149,6 +1149,9 @@ int request_process_address(Request *req) { if (r < 0) return r; + /* To prevent a double decrement on failure in after_configure(). */ + req->message_counter = NULL; + if (req->after_configure) { r = req->after_configure(req, ret); if (r < 0) diff --git a/src/network/networkd-neighbor.c b/src/network/networkd-neighbor.c index 2afe71c5f8..88620579ac 100644 --- a/src/network/networkd-neighbor.c +++ b/src/network/networkd-neighbor.c @@ -479,6 +479,9 @@ int request_process_neighbor(Request *req) { if (r < 0) return r; + /* To prevent a double decrement on failure in after_configure(). */ + req->message_counter = NULL; + if (req->after_configure) { r = req->after_configure(req, ret); if (r < 0) diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index 5c6c58356c..013bddf30c 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -751,6 +751,9 @@ int request_process_nexthop(Request *req) { if (r < 0) return r; + /* To prevent a double decrement on failure in after_configure(). */ + req->message_counter = NULL; + if (req->after_configure) { r = req->after_configure(req, ret); if (r < 0) diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 8707e5c089..e6c1d71265 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -1622,6 +1622,9 @@ int request_process_route(Request *req) { if (r < 0) return r; + /* To prevent a double decrement on failure in after_configure(). */ + req->message_counter = NULL; + if (req->after_configure) { assert(n_routes > 0); diff --git a/src/network/networkd-routing-policy-rule.c b/src/network/networkd-routing-policy-rule.c index 9b9880d1da..20512d2477 100644 --- a/src/network/networkd-routing-policy-rule.c +++ b/src/network/networkd-routing-policy-rule.c @@ -814,6 +814,9 @@ int request_process_routing_policy_rule(Request *req) { if (r < 0) return r; + /* To prevent a double decrement on failure in after_configure(). */ + req->message_counter = NULL; + if (req->after_configure) { r = req->after_configure(req, ret); if (r < 0) From 63130eb36dc51e4fd50716c585f98ebe456ca7cf Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 17 May 2021 15:40:15 +0900 Subject: [PATCH 2/4] network: merge link_drop() and link_detach_from_manager() link_detach_from_manager() is only called by link_drop(). It is not necessary to split such tiny function. --- src/network/networkd-link.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 9d30e16b0a..67d01ac44d 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -1771,24 +1771,17 @@ static void link_drop_from_master(Link *link, NetDev *netdev) { link_unref(set_remove(master->slaves, link)); } -static void link_detach_from_manager(Link *link) { - if (!link || !link->manager) - return; - - link_unref(set_remove(link->manager->links_requesting_uuid, link)); - link_clean(link); - - /* The following must be called at last. */ - assert_se(hashmap_remove(link->manager->links, INT_TO_PTR(link->ifindex)) == link); - link_unref(link); -} - static void link_drop(Link *link) { - if (!link || link->state == LINK_STATE_LINGER) + if (!link) return; + assert(link->manager); + link_set_state(link, LINK_STATE_LINGER); + /* Drop all references from other links and manager. Note that async netlink calls may have + * references to the link, and they will be dropped when we receive replies. */ + link_free_carrier_maps(link); if (link->network) { @@ -1797,10 +1790,14 @@ static void link_drop(Link *link) { link_drop_from_master(link, link->network->bond); } - log_link_debug(link, "Link removed"); + link_unref(set_remove(link->manager->links_requesting_uuid, link)); (void) unlink(link->state_file); - link_detach_from_manager(link); + link_clean(link); + + /* The following must be called at last. */ + assert_se(hashmap_remove(link->manager->links, INT_TO_PTR(link->ifindex)) == link); + link_unref(link); } int link_activate(Link *link) { From 56001f023305ea99329e27141d6e6067596491a9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 17 May 2021 15:32:57 +0900 Subject: [PATCH 3/4] network: also drop requests when link enters linger state Otherwise, if link is removed, several references to the link in remain exist in requests. --- src/network/networkd-link.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 67d01ac44d..b56c232eca 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -1771,6 +1771,17 @@ static void link_drop_from_master(Link *link, NetDev *netdev) { link_unref(set_remove(master->slaves, link)); } +static void link_drop_requests(Link *link) { + Request *req; + + assert(link); + assert(link->manager); + + ORDERED_SET_FOREACH(req, link->manager->request_queue) + if (req->link == link) + request_drop(req); +} + static void link_drop(Link *link) { if (!link) return; @@ -1782,6 +1793,8 @@ static void link_drop(Link *link) { /* Drop all references from other links and manager. Note that async netlink calls may have * references to the link, and they will be dropped when we receive replies. */ + link_drop_requests(link); + link_free_carrier_maps(link); if (link->network) { @@ -2098,17 +2111,6 @@ static int link_drop_config(Link *link) { return r; } -static void link_drop_requests(Link *link) { - Request *req; - - assert(link); - assert(link->manager); - - ORDERED_SET_FOREACH(req, link->manager->request_queue) - if (req->link == link) - request_drop(req); -} - int link_configure(Link *link) { int r; From cc2d7efc5ca09a7de4bec55e80476986839a655c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 14 May 2021 15:58:15 +0900 Subject: [PATCH 4/4] network: fix Link reference counter issue Previously, when link_new() fails, `link_unref()` was called, so, `Manager::links` may become dirty. This introduces `link_drop_or_unref()` and it will be called on failure. --- src/network/networkd-link.c | 240 ++++++++++++++++++------------------ 1 file changed, 122 insertions(+), 118 deletions(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index b56c232eca..d493afda4c 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -401,121 +401,6 @@ static int link_update_flags(Link *link, sd_netlink_message *m, bool force_updat return 0; } -static int link_new(Manager *manager, sd_netlink_message *message, Link **ret) { - _cleanup_(link_unrefp) Link *link = NULL; - const char *ifname, *kind = NULL; - unsigned short iftype; - int r, ifindex; - uint16_t type; - - assert(manager); - assert(message); - assert(ret); - - /* check for link kind */ - r = sd_netlink_message_enter_container(message, IFLA_LINKINFO); - if (r == 0) { - (void) sd_netlink_message_read_string(message, IFLA_INFO_KIND, &kind); - r = sd_netlink_message_exit_container(message); - if (r < 0) - return r; - } - - r = sd_netlink_message_get_type(message, &type); - if (r < 0) - return r; - else if (type != RTM_NEWLINK) - return -EINVAL; - - r = sd_rtnl_message_link_get_ifindex(message, &ifindex); - if (r < 0) - return r; - else if (ifindex <= 0) - return -EINVAL; - - r = sd_rtnl_message_link_get_type(message, &iftype); - if (r < 0) - return r; - - r = sd_netlink_message_read_string(message, IFLA_IFNAME, &ifname); - if (r < 0) - return r; - - link = new(Link, 1); - if (!link) - return -ENOMEM; - - *link = (Link) { - .n_ref = 1, - .manager = manager, - .state = LINK_STATE_PENDING, - .ifindex = ifindex, - .iftype = iftype, - - .n_dns = UINT_MAX, - .dns_default_route = -1, - .llmnr = _RESOLVE_SUPPORT_INVALID, - .mdns = _RESOLVE_SUPPORT_INVALID, - .dnssec_mode = _DNSSEC_MODE_INVALID, - .dns_over_tls_mode = _DNS_OVER_TLS_MODE_INVALID, - }; - - link->ifname = strdup(ifname); - if (!link->ifname) - return -ENOMEM; - - if (kind) { - link->kind = strdup(kind); - if (!link->kind) - return -ENOMEM; - } - - r = sd_netlink_message_read_u32(message, IFLA_MASTER, (uint32_t *)&link->master_ifindex); - if (r < 0) - log_link_debug_errno(link, r, "New device has no master, continuing without"); - - r = netlink_message_read_hw_addr(message, IFLA_ADDRESS, &link->hw_addr); - if (r < 0) - log_link_debug_errno(link, r, "Hardware address not found for new device, continuing without"); - - r = netlink_message_read_hw_addr(message, IFLA_BROADCAST, &link->bcast_addr); - if (r < 0) - log_link_debug_errno(link, r, "Broadcast address not found for new device, continuing without"); - - r = ethtool_get_permanent_macaddr(&manager->ethtool_fd, link->ifname, &link->permanent_mac); - if (r < 0) - log_link_debug_errno(link, r, "Permanent MAC address not found for new device, continuing without: %m"); - - r = ethtool_get_driver(&manager->ethtool_fd, link->ifname, &link->driver); - if (r < 0) - log_link_debug_errno(link, r, "Failed to get driver, continuing without: %m"); - - r = sd_netlink_message_read_strv(message, IFLA_PROP_LIST, IFLA_ALT_IFNAME, &link->alternative_names); - if (r < 0 && r != -ENODATA) - return r; - - if (asprintf(&link->state_file, "/run/systemd/netif/links/%d", link->ifindex) < 0) - return -ENOMEM; - - if (asprintf(&link->lease_file, "/run/systemd/netif/leases/%d", link->ifindex) < 0) - return -ENOMEM; - - if (asprintf(&link->lldp_file, "/run/systemd/netif/lldp/%d", link->ifindex) < 0) - return -ENOMEM; - - r = hashmap_ensure_put(&manager->links, NULL, INT_TO_PTR(link->ifindex), link); - if (r < 0) - return r; - - r = link_update_flags(link, message, false); - if (r < 0) - return r; - - *ret = TAKE_PTR(link); - - return 0; -} - void link_ntp_settings_clear(Link *link) { link->ntp = strv_free(link->ntp); } @@ -1782,9 +1667,10 @@ static void link_drop_requests(Link *link) { request_drop(req); } -static void link_drop(Link *link) { + +static Link *link_drop(Link *link) { if (!link) - return; + return NULL; assert(link->manager); @@ -1810,7 +1696,7 @@ static void link_drop(Link *link) { /* The following must be called at last. */ assert_se(hashmap_remove(link->manager->links, INT_TO_PTR(link->ifindex)) == link); - link_unref(link); + return link_unref(link); } int link_activate(Link *link) { @@ -2523,6 +2409,124 @@ static int link_initialized(Link *link, sd_device *device) { return 0; } +static Link *link_drop_or_unref(Link *link) { + if (!link) + return NULL; + if (!link->manager) + return link_unref(link); + return link_drop(link); +} + +DEFINE_TRIVIAL_CLEANUP_FUNC(Link*, link_drop_or_unref); + +static int link_new(Manager *manager, sd_netlink_message *message, Link **ret) { + _cleanup_(link_drop_or_unrefp) Link *link = NULL; + _cleanup_free_ char *ifname = NULL, *kind = NULL; + unsigned short iftype; + int r, ifindex; + uint16_t type; + + assert(manager); + assert(message); + assert(ret); + + r = sd_netlink_message_get_type(message, &type); + if (r < 0) + return r; + else if (type != RTM_NEWLINK) + return -EINVAL; + + r = sd_rtnl_message_link_get_ifindex(message, &ifindex); + if (r < 0) + return r; + else if (ifindex <= 0) + return -EINVAL; + + r = sd_rtnl_message_link_get_type(message, &iftype); + if (r < 0) + return r; + + r = sd_netlink_message_read_string_strdup(message, IFLA_IFNAME, &ifname); + if (r < 0) + return r; + + /* check for link kind */ + r = sd_netlink_message_enter_container(message, IFLA_LINKINFO); + if (r >= 0) { + (void) sd_netlink_message_read_string_strdup(message, IFLA_INFO_KIND, &kind); + r = sd_netlink_message_exit_container(message); + if (r < 0) + return r; + } + + link = new(Link, 1); + if (!link) + return -ENOMEM; + + *link = (Link) { + .n_ref = 1, + .state = LINK_STATE_PENDING, + .ifindex = ifindex, + .iftype = iftype, + .ifname = TAKE_PTR(ifname), + .kind = TAKE_PTR(kind), + + .n_dns = UINT_MAX, + .dns_default_route = -1, + .llmnr = _RESOLVE_SUPPORT_INVALID, + .mdns = _RESOLVE_SUPPORT_INVALID, + .dnssec_mode = _DNSSEC_MODE_INVALID, + .dns_over_tls_mode = _DNS_OVER_TLS_MODE_INVALID, + }; + + r = hashmap_ensure_put(&manager->links, NULL, INT_TO_PTR(link->ifindex), link); + if (r < 0) + return r; + + link->manager = manager; + + r = sd_netlink_message_read_u32(message, IFLA_MASTER, (uint32_t*) &link->master_ifindex); + if (r < 0) + log_link_debug_errno(link, r, "New device has no master, continuing without"); + + r = netlink_message_read_hw_addr(message, IFLA_ADDRESS, &link->hw_addr); + if (r < 0) + log_link_debug_errno(link, r, "Hardware address not found for new device, continuing without"); + + r = netlink_message_read_hw_addr(message, IFLA_BROADCAST, &link->bcast_addr); + if (r < 0) + log_link_debug_errno(link, r, "Broadcast address not found for new device, continuing without"); + + r = ethtool_get_permanent_macaddr(&manager->ethtool_fd, link->ifname, &link->permanent_mac); + if (r < 0) + log_link_debug_errno(link, r, "Permanent MAC address not found for new device, continuing without: %m"); + + r = ethtool_get_driver(&manager->ethtool_fd, link->ifname, &link->driver); + if (r < 0) + log_link_debug_errno(link, r, "Failed to get driver, continuing without: %m"); + + r = sd_netlink_message_read_strv(message, IFLA_PROP_LIST, IFLA_ALT_IFNAME, &link->alternative_names); + if (r < 0 && r != -ENODATA) + return r; + + if (asprintf(&link->state_file, "/run/systemd/netif/links/%d", link->ifindex) < 0) + return -ENOMEM; + + if (asprintf(&link->lease_file, "/run/systemd/netif/leases/%d", link->ifindex) < 0) + return -ENOMEM; + + if (asprintf(&link->lldp_file, "/run/systemd/netif/lldp/%d", link->ifindex) < 0) + return -ENOMEM; + + r = link_update_flags(link, message, false); + if (r < 0) + return r; + + *ret = TAKE_PTR(link); + + return 0; +} + static int link_add(Manager *m, sd_netlink_message *message, Link **ret) { _cleanup_(sd_device_unrefp) sd_device *device = NULL; char ifindex_str[2 + DECIMAL_STR_MAX(int)];