From 85a6f300c14d75d161cbfdb3eaf5af9594400ecd Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 3 Jan 2024 04:41:34 +0900 Subject: [PATCH 1/4] network/queue: introduce RemoveRequest and relevant functions This is similar to Request, but will be used on removing configuration (e.g. address, route, and so on). By using another queue for removing configuration, then we can avoid to fill the reply callback buffer in sd-netlink by remove message calls. Follow-up for 4e6a35e2b2fad0f167a71b63525f4210bc858bc6. --- src/network/networkd-manager.c | 2 + src/network/networkd-manager.h | 1 + src/network/networkd-queue.c | 111 +++++++++++++++++++++++++++++++++ src/network/networkd-queue.h | 49 +++++++++++++++ 4 files changed, 163 insertions(+) diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 8933fc4977..6f3d90d64c 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -424,6 +424,7 @@ static int manager_connect_rtnl(Manager *m, int fd) { static int manager_post_handler(sd_event_source *s, void *userdata) { Manager *manager = ASSERT_PTR(userdata); + (void) manager_process_remove_requests(manager); (void) manager_process_requests(manager); (void) manager_clean_all(manager); return 0; @@ -594,6 +595,7 @@ Manager* manager_free(Manager *m) { (void) link_stop_engines(link, true); m->request_queue = ordered_set_free(m->request_queue); + m->remove_request_queue = ordered_set_free(m->remove_request_queue); m->dirty_links = set_free_with_destructor(m->dirty_links, link_unref); m->new_wlan_ifindices = set_free(m->new_wlan_ifindices); diff --git a/src/network/networkd-manager.h b/src/network/networkd-manager.h index a2edfd0e79..a97ae8ea21 100644 --- a/src/network/networkd-manager.h +++ b/src/network/networkd-manager.h @@ -102,6 +102,7 @@ struct Manager { bool request_queued; OrderedSet *request_queue; + OrderedSet *remove_request_queue; Hashmap *tuntap_fds_by_name; }; diff --git a/src/network/networkd-queue.c b/src/network/networkd-queue.c index 1678510d52..0b89ec9736 100644 --- a/src/network/networkd-queue.c +++ b/src/network/networkd-queue.c @@ -224,6 +224,10 @@ int manager_process_requests(Manager *manager) { assert(manager); + /* Process only when no remove request is queued. */ + if (!ordered_set_isempty(manager->remove_request_queue)) + return 0; + manager->request_queued = false; ORDERED_SET_FOREACH(req, manager->request_queue) { @@ -339,3 +343,110 @@ static const char *const request_type_table[_REQUEST_TYPE_MAX] = { }; DEFINE_STRING_TABLE_LOOKUP_TO_STRING(request_type, RequestType); + +static RemoveRequest* remove_request_free(RemoveRequest *req) { + if (!req) + return NULL; + + if (req->manager) + ordered_set_remove(req->manager->remove_request_queue, req); + + if (req->unref_func) + req->unref_func(req->userdata); + + link_unref(req->link); + sd_netlink_unref(req->netlink); + sd_netlink_message_unref(req->message); + + return mfree(req); +} + +DEFINE_TRIVIAL_CLEANUP_FUNC(RemoveRequest*, remove_request_free); +DEFINE_TRIVIAL_DESTRUCTOR(remove_request_destroy_callback, RemoveRequest, remove_request_free); +DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR( + remove_request_hash_ops, + void, + trivial_hash_func, + trivial_compare_func, + remove_request_free); + +int remove_request_add( + Manager *manager, + Link *link, + void *userdata, + mfree_func_t unref_func, + sd_netlink *netlink, + sd_netlink_message *message, + remove_request_netlink_handler_t netlink_handler) { + + _cleanup_(remove_request_freep) RemoveRequest *req = NULL; + int r; + + assert(manager); + assert(userdata); + assert(netlink); + assert(message); + + req = new(RemoveRequest, 1); + if (!req) + return -ENOMEM; + + *req = (RemoveRequest) { + .link = link_ref(link), /* link may be NULL, but link_ref() handles it gracefully. */ + .userdata = userdata, + .netlink = sd_netlink_ref(netlink), + .message = sd_netlink_message_ref(message), + .netlink_handler = netlink_handler, + }; + + r = ordered_set_ensure_put(&manager->remove_request_queue, &remove_request_hash_ops, req); + if (r < 0) + return r; + assert(r > 0); + + req->manager = manager; + req->unref_func = unref_func; + + TAKE_PTR(req); + return 0; +} + +int manager_process_remove_requests(Manager *manager) { + RemoveRequest *req; + int r; + + assert(manager); + + while ((req = ordered_set_first(manager->remove_request_queue))) { + + /* Do not make the reply callback queue in sd-netlink full. */ + if (netlink_get_reply_callback_count(req->netlink) >= REPLY_CALLBACK_COUNT_THRESHOLD) + return 0; + + r = netlink_call_async( + req->netlink, NULL, req->message, + req->netlink_handler, + remove_request_destroy_callback, + req); + if (r < 0) { + _cleanup_(link_unrefp) Link *link = link_ref(req->link); + + log_link_warning_errno(link, r, "Failed to call netlink message: %m"); + + /* First free the request. */ + remove_request_free(req); + + /* Then, make the link enter the failed state. */ + if (link) + link_enter_failed(link); + + } else { + /* On success, netlink needs to be unref()ed. Otherwise, the netlink and remove + * request may not freed on shutting down. */ + req->netlink = sd_netlink_unref(req->netlink); + ordered_set_remove(manager->remove_request_queue, req); + } + } + + return 0; +} diff --git a/src/network/networkd-queue.h b/src/network/networkd-queue.h index 21fa7d9453..e911fdd33a 100644 --- a/src/network/networkd-queue.h +++ b/src/network/networkd-queue.h @@ -139,3 +139,52 @@ int manager_process_requests(Manager *manager); int request_call_netlink_async(sd_netlink *nl, sd_netlink_message *m, Request *req); const char* request_type_to_string(RequestType t) _const_; + +typedef struct RemoveRequest RemoveRequest; +typedef int (*remove_request_netlink_handler_t)(sd_netlink *nl, sd_netlink_message *m, RemoveRequest *req); + +struct RemoveRequest { + Manager *manager; + Link *link; + void *userdata; /* e.g. Address */ + mfree_func_t unref_func; /* e.g. address_unref() */ + sd_netlink *netlink; + sd_netlink_message *message; + remove_request_netlink_handler_t netlink_handler; +}; + +int remove_request_add( + Manager *manager, + Link *link, + void *userdata, /* This is unref()ed when the call failed. */ + mfree_func_t unref_func, + sd_netlink *netlink, + sd_netlink_message *message, + remove_request_netlink_handler_t netlink_handler); + +#define _remove_request_add(manager, link, data, name, nl, m, handler) \ + ({ \ + typeof(*data) *_data = (data); \ + int _r; \ + \ + _r = remove_request_add(manager, link, _data, \ + (mfree_func_t) name##_unref, \ + nl, m, handler); \ + if (_r >= 0) \ + name##_ref(_data); \ + _r; \ + }) + + +#define link_remove_request_add(link, data, name, nl, m, handler) \ + ({ \ + Link *_link = (link); \ + \ + _remove_request_add(_link->manager, _link, data, name, \ + nl, m, handler); \ + }) + +#define manager_remove_request_add(manager, data, name, nl, m, handler) \ + _remove_request_add(manager, NULL, data, name, nl, m, handler) + +int manager_process_remove_requests(Manager *manager); From 56a995fe8e50b2432ff930ed0431cc70adbe492d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 3 Jan 2024 04:41:42 +0900 Subject: [PATCH 2/4] network/address: forget address even if we could not remove it If we could not remove an address, then previously the corresponding Address object was never removed, as it was freed only when we receive remove notification from the kernel. So, we might confused that the address still exists and being removed, and might block reconfiguring the address. With this change, even if we fail to remove an address, the corresponding Address object will be freed. --- src/network/networkd-address.c | 35 +++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index e54c122361..4c3393645e 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -1111,18 +1111,35 @@ static int address_set_netlink_message(const Address *address, sd_netlink_messag return 0; } -static int address_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) { +static int address_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, RemoveRequest *rreq) { int r; assert(m); - assert(link); + assert(rreq); - if (IN_SET(link->state, LINK_STATE_FAILED, LINK_STATE_LINGER)) + Link *link = ASSERT_PTR(rreq->link); + Address *address = ASSERT_PTR(rreq->userdata); + + if (link->state == LINK_STATE_LINGER) return 0; r = sd_netlink_message_get_errno(m); - if (r < 0 && r != -EADDRNOTAVAIL) - log_link_message_warning_errno(link, m, r, "Could not drop address"); + if (r < 0) { + log_link_message_full_errno(link, m, + (r == -EADDRNOTAVAIL || !address->link) ? LOG_DEBUG : LOG_WARNING, + r, "Could not drop address"); + + if (address->link) { + /* If the address cannot be removed, then assume the address is already removed. */ + log_address_debug(address, "Forgetting", link); + + Request *req; + if (address_get_request(link, address, &req) >= 0) + address_enter_removed(req->userdata); + + (void) address_drop(address); + } + } return 1; } @@ -1149,13 +1166,9 @@ int address_remove(Address *address, Link *link) { if (r < 0) return log_link_warning_errno(link, r, "Could not set netlink attributes: %m"); - r = netlink_call_async(link->manager->rtnl, NULL, m, - address_remove_handler, - link_netlink_destroy_callback, link); + r = link_remove_request_add(link, address, address, link->manager->rtnl, m, address_remove_handler); if (r < 0) - return log_link_warning_errno(link, r, "Could not send rtnetlink message: %m"); - - link_ref(link); + return log_link_warning_errno(link, r, "Could not queue rtnetlink message: %m"); address_enter_removing(address); From 84c86ae19483f29ae49ec987ac3af4140ec03ead Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 3 Jan 2024 04:41:50 +0900 Subject: [PATCH 3/4] network/neighbor: drop Neighbor object even if we fail to remove the neighbor --- src/network/networkd-neighbor.c | 36 +++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/src/network/networkd-neighbor.c b/src/network/networkd-neighbor.c index b9c97841b0..d30282fe1e 100644 --- a/src/network/networkd-neighbor.c +++ b/src/network/networkd-neighbor.c @@ -399,19 +399,36 @@ int link_request_static_neighbors(Link *link) { return 0; } -static int neighbor_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) { +static int neighbor_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, RemoveRequest *rreq) { int r; assert(m); - assert(link); + assert(rreq); - if (IN_SET(link->state, LINK_STATE_FAILED, LINK_STATE_LINGER)) - return 1; + Link *link = ASSERT_PTR(rreq->link); + Neighbor *neighbor = ASSERT_PTR(rreq->userdata); + + if (link->state == LINK_STATE_LINGER) + return 0; r = sd_netlink_message_get_errno(m); - if (r < 0 && r != -ESRCH) + if (r < 0) { /* Neighbor may not exist because it already got deleted, ignore that. */ - log_link_message_warning_errno(link, m, r, "Could not remove neighbor"); + log_link_message_full_errno(link, m, + (r == -ESRCH || !neighbor->link) ? LOG_DEBUG : LOG_WARNING, + r, "Could not remove neighbor"); + + if (neighbor->link) { + /* If the neighbor cannot be removed, then assume the neighbor is already removed. */ + log_neighbor_debug(neighbor, "Forgetting", link); + + Request *req; + if (neighbor_get_request(link, neighbor, &req) >= 0) + neighbor_enter_removed(req->userdata); + + neighbor_detach(neighbor); + } + } return 1; } @@ -436,12 +453,9 @@ int neighbor_remove(Neighbor *neighbor, Link *link) { if (r < 0) return log_link_error_errno(link, r, "Could not append NDA_DST attribute: %m"); - r = netlink_call_async(link->manager->rtnl, NULL, m, neighbor_remove_handler, - link_netlink_destroy_callback, link); + r = link_remove_request_add(link, neighbor, neighbor, link->manager->rtnl, m, neighbor_remove_handler); if (r < 0) - return log_link_error_errno(link, r, "Could not send rtnetlink message: %m"); - - link_ref(link); + return log_link_error_errno(link, r, "Could not queue rtnetlink message: %m"); neighbor_enter_removing(neighbor); return 0; From 62c6f9f7acbe1267740b2840a392e469cf9f89fc Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 3 Jan 2024 04:41:58 +0900 Subject: [PATCH 4/4] network/nexthop: drop NextHop object even if we fail to remove the nexthop --- src/network/networkd-nexthop.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index bc2eb7be40..4de1e09999 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -436,19 +436,32 @@ static void log_nexthop_debug(const NextHop *nexthop, const char *str, Manager * yes_no(nexthop->blackhole), strna(group), strna(flags)); } -static int nexthop_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) { +static int nexthop_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, RemoveRequest *rreq) { int r; assert(m); + assert(rreq); - /* link may be NULL. */ - - if (link && IN_SET(link->state, LINK_STATE_FAILED, LINK_STATE_LINGER)) - return 1; + Manager *manager = ASSERT_PTR(rreq->manager); + NextHop *nexthop = ASSERT_PTR(rreq->userdata); r = sd_netlink_message_get_errno(m); - if (r < 0 && r != -ENOENT) - log_link_message_warning_errno(link, m, r, "Could not drop nexthop, ignoring"); + if (r < 0) { + log_message_full_errno(m, + (r == -ENOENT || !nexthop->manager) ? LOG_DEBUG : LOG_WARNING, + r, "Could not drop nexthop, ignoring"); + + if (nexthop->manager) { + /* If the nexthop cannot be removed, then assume the nexthop is already removed. */ + log_nexthop_debug(nexthop, "Forgetting", manager); + + Request *req; + if (nexthop_get_request_by_id(manager, nexthop->id, &req) >= 0) + nexthop_enter_removed(req->userdata); + + nexthop_detach(nexthop); + } + } return 1; } @@ -475,12 +488,9 @@ int nexthop_remove(NextHop *nexthop, Manager *manager) { if (r < 0) return log_link_error_errno(link, r, "Could not append NHA_ID attribute: %m"); - r = netlink_call_async(manager->rtnl, NULL, m, nexthop_remove_handler, - link ? link_netlink_destroy_callback : NULL, link); + r = manager_remove_request_add(manager, nexthop, nexthop, manager->rtnl, m, nexthop_remove_handler); if (r < 0) - return log_link_error_errno(link, r, "Could not send rtnetlink message: %m"); - - link_ref(link); /* link may be NULL, link_ref() is OK with that */ + return log_link_error_errno(link, r, "Could not queue rtnetlink message: %m"); nexthop_enter_removing(nexthop); return 0;