From baa95d2274179e680c4731a74f514e2651722ad2 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 7 Jun 2021 16:26:10 +0900 Subject: [PATCH] network: do not process requests which conditionalized with link flags while the flags are updating E.g. nexthop requires IFF_UP flag, but the currently stored flag may be outdated if we called link_down(). This makes such requests pending if at least one of the flags are updating. --- src/network/networkd-bridge-mdb.c | 3 ++ src/network/networkd-dhcp-common.c | 3 ++ src/network/networkd-dhcp-server.c | 3 ++ src/network/networkd-link.c | 9 ++++-- src/network/networkd-link.h | 1 + src/network/networkd-nexthop.c | 4 ++- src/network/networkd-setlink.c | 50 +++++++++++++++++++++++++----- 7 files changed, 63 insertions(+), 10 deletions(-) diff --git a/src/network/networkd-bridge-mdb.c b/src/network/networkd-bridge-mdb.c index 3bf1e068f00..faf3dbd0b7b 100644 --- a/src/network/networkd-bridge-mdb.c +++ b/src/network/networkd-bridge-mdb.c @@ -258,6 +258,9 @@ static bool bridge_mdb_is_ready_to_configure(Link *link) { if (!IN_SET(master->state, LINK_STATE_CONFIGURING, LINK_STATE_CONFIGURED)) return false; + if (master->set_flags_messages > 0) + return false; + if (!link_has_carrier(master)) return false; diff --git a/src/network/networkd-dhcp-common.c b/src/network/networkd-dhcp-common.c index 8357b2b99df..249d780887e 100644 --- a/src/network/networkd-dhcp-common.c +++ b/src/network/networkd-dhcp-common.c @@ -116,6 +116,9 @@ static int link_configure_and_start_dhcp_delayed(Link *link) { return r; } + if (link->set_flags_messages > 0) + return 0; + if (!link_has_carrier(link)) return 0; diff --git a/src/network/networkd-dhcp-server.c b/src/network/networkd-dhcp-server.c index 055aa82ba24..8c0cd37f3fa 100644 --- a/src/network/networkd-dhcp-server.c +++ b/src/network/networkd-dhcp-server.c @@ -499,6 +499,9 @@ static bool dhcp_server_is_ready_to_configure(Link *link) { if (!IN_SET(link->state, LINK_STATE_CONFIGURING, LINK_STATE_CONFIGURED)) return false; + if (link->set_flags_messages > 0) + return false; + if (!link_has_carrier(link)) return false; diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 66a3740e93d..6933c8890f9 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -148,8 +148,13 @@ bool link_is_ready_to_configure(Link *link, bool allow_unmanaged) { if (!IN_SET(link->state, LINK_STATE_CONFIGURING, LINK_STATE_CONFIGURED)) return false; - if (!link_has_carrier(link) && !link->network->configure_without_carrier) - return false; + if (!link->network->configure_without_carrier) { + if (link->set_flags_messages > 0) + return false; + + if (!link_has_carrier(link)) + return false; + } if (link->set_link_messages > 0) return false; diff --git a/src/network/networkd-link.h b/src/network/networkd-link.h index 647c60e6847..780bdee1852 100644 --- a/src/network/networkd-link.h +++ b/src/network/networkd-link.h @@ -98,6 +98,7 @@ typedef struct Link { unsigned tc_messages; unsigned sr_iov_messages; unsigned set_link_messages; + unsigned set_flags_messages; unsigned create_stacked_netdev_messages; unsigned create_stacked_netdev_after_configured_messages; diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index 954790f832c..4bc29ca0577 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -757,9 +757,11 @@ static bool nexthop_is_ready_to_configure(Link *link, const NextHop *nexthop) { } else { Link *l; - /* TODO: fdb nexthop does not require IFF_UP. The condition below needs to be updated + /* TODO: fdb nexthop does not require IFF_UP. The conditions below needs to be updated * when fdb nexthop support is added. See rtm_to_nh_config() in net/ipv4/nexthop.c of * kernel. */ + if (link->set_flags_messages > 0) + return false; if (!FLAGS_SET(link->flags, IFF_UP)) return false; diff --git a/src/network/networkd-setlink.c b/src/network/networkd-setlink.c index 00fed02403a..2c3803e02c2 100644 --- a/src/network/networkd-setlink.c +++ b/src/network/networkd-setlink.c @@ -38,6 +38,15 @@ static int get_link_master_handler(sd_netlink *rtnl, sd_netlink_message *m, Link return 0; } +static int get_link_update_flag_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) { + assert(link); + assert(link->set_flags_messages > 0); + + link->set_flags_messages--; + + return get_link_default_handler(rtnl, m, link); +} + static int set_link_handler_internal( sd_netlink *rtnl, sd_netlink_message *m, @@ -56,7 +65,7 @@ static int set_link_handler_internal( link->set_link_messages--; if (IN_SET(link->state, LINK_STATE_FAILED, LINK_STATE_LINGER)) - return 0; + goto on_error; r = sd_netlink_message_get_errno(m); if (r < 0) { @@ -67,7 +76,7 @@ static int set_link_handler_internal( if (!ignore) link_enter_failed(link); - return 0; + goto on_error; } log_link_debug(link, "%s set.", set_link_operation_to_string(op)); @@ -76,7 +85,7 @@ static int set_link_handler_internal( r = link_call_getlink(link, get_link_handler); if (r < 0) { link_enter_failed(link); - return 0; + goto on_error; } } @@ -84,6 +93,14 @@ static int set_link_handler_internal( link_check_ready(link); return 1; + +on_error: + if (op == SET_LINK_FLAGS) { + assert(link->set_flags_messages > 0); + link->set_flags_messages--; + } + + return 0; } static int link_set_addrgen_mode_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) { @@ -119,7 +136,7 @@ static int link_set_can_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *l } static int link_set_flags_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) { - return set_link_handler_internal(rtnl, m, link, SET_LINK_FLAGS, false, get_link_default_handler); + return set_link_handler_internal(rtnl, m, link, SET_LINK_FLAGS, false, get_link_update_flag_handler); } static int link_set_group_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) { @@ -455,6 +472,8 @@ static bool link_is_ready_to_call_set_link(Request *req) { return false; break; case SET_LINK_CAN: + /* Do not check link->set_flgas_messages here, as it is ok even if link->flags + * is outdated, and checking the counter causes a deadlock. */ if (FLAGS_SET(link->flags, IFF_UP)) { /* The CAN interface must be down to configure bitrate, etc... */ r = link_down(link); @@ -478,6 +497,8 @@ static bool link_is_ready_to_call_set_link(Request *req) { return false; m = link->network->bond->ifindex; + /* Do not check link->set_flgas_messages here, as it is ok even if link->flags + * is outdated, and checking the counter causes a deadlock. */ if (FLAGS_SET(link->flags, IFF_UP)) { /* link must be down when joining to bond master. */ r = link_down(link); @@ -523,6 +544,9 @@ int request_process_set_link(Request *req) { return log_link_error_errno(req->link, r, "Failed to set %s: %m", set_link_operation_to_string(req->set_link_operation)); + if (req->set_link_operation == SET_LINK_FLAGS) + req->link->set_flags_messages++; + return 1; } @@ -770,7 +794,7 @@ static int link_up_or_down_handler_internal(sd_netlink *rtnl, sd_netlink_message assert(link); if (IN_SET(link->state, LINK_STATE_FAILED, LINK_STATE_LINGER)) - return 0; + goto on_error; r = sd_netlink_message_get_errno(m); if (r < 0) @@ -778,11 +802,23 @@ static int link_up_or_down_handler_internal(sd_netlink *rtnl, sd_netlink_message "Could not bring up interface, ignoring" : "Could not bring down interface, ignoring"); + r = link_call_getlink(link, get_link_update_flag_handler); + if (r < 0) { + link_enter_failed(link); + goto on_error; + } + if (check_ready) { link->activated = true; link_check_ready(link); } + return 1; + +on_error: + assert(link->set_flags_messages > 0); + link->set_flags_messages--; + return 0; } @@ -906,7 +942,7 @@ int link_request_to_activate(Link *link) { link->activated = false; - r = link_queue_request(link, REQUEST_TYPE_ACTIVATE_LINK, NULL, false, NULL, + r = link_queue_request(link, REQUEST_TYPE_ACTIVATE_LINK, NULL, false, &link->set_flags_messages, up ? link_activate_up_handler : link_activate_down_handler, &req); if (r < 0) return log_link_error_errno(link, r, "Failed to request to activate link: %m"); @@ -963,7 +999,7 @@ int link_request_to_bring_up_or_down(Link *link, bool up) { assert(link); - r = link_queue_request(link, REQUEST_TYPE_UP_DOWN, NULL, false, NULL, + r = link_queue_request(link, REQUEST_TYPE_UP_DOWN, NULL, false, &link->set_flags_messages, up ? link_up_handler : link_down_handler, &req); if (r < 0) return log_link_error_errno(link, r, "Failed to request to bring %s link: %m", -- 2.47.3