From c6607719d2b43de63e2010eae47585e11ec334a8 Mon Sep 17 00:00:00 2001 From: AnotherTest Date: Mon, 5 Apr 2021 22:33:41 +0430 Subject: [PATCH] DHCPClient: Retry DISCOVER for interfaces that fail Attempts are spaced out with exponential backoff, cut at 10 minutes per attempt. Also avoid trying to acquire an IP on interfaces that aren't up. Fixes #6126. Fixes #6125. --- Userland/Services/DHCPClient/DHCPv4Client.cpp | 118 ++++++++++++++++-- Userland/Services/DHCPClient/DHCPv4Client.h | 16 ++- Userland/Services/DHCPClient/main.cpp | 49 +------- 3 files changed, 129 insertions(+), 54 deletions(-) diff --git a/Userland/Services/DHCPClient/DHCPv4Client.cpp b/Userland/Services/DHCPClient/DHCPv4Client.cpp index 8cefa1e66c..4b19a48d52 100644 --- a/Userland/Services/DHCPClient/DHCPv4Client.cpp +++ b/Userland/Services/DHCPClient/DHCPv4Client.cpp @@ -29,22 +29,43 @@ #include #include #include +#include +#include +#include #include +#include #include #include #include -static void send(const InterfaceDescriptor& iface, const DHCPv4Packet& packet, Core::Object*) +static u8 mac_part(const Vector& parts, size_t index) +{ + auto result = AK::StringUtils::convert_to_uint_from_hex(parts.at(index)); + VERIFY(result.has_value()); + return result.value(); +} + +static MACAddress mac_from_string(const String& str) +{ + auto chunks = str.split(':'); + VERIFY(chunks.size() == 6); // should we...worry about this? + return { + mac_part(chunks, 0), mac_part(chunks, 1), mac_part(chunks, 2), + mac_part(chunks, 3), mac_part(chunks, 4), mac_part(chunks, 5) + }; +} + +static bool send(const InterfaceDescriptor& iface, const DHCPv4Packet& packet, Core::Object*) { int fd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP); if (fd < 0) { dbgln("ERROR: socket :: {}", strerror(errno)); - return; + return false; } if (setsockopt(fd, SOL_SOCKET, SO_BINDTODEVICE, iface.m_ifname.characters(), IFNAMSIZ) < 0) { dbgln("ERROR: setsockopt(SO_BINDTODEVICE) :: {}", strerror(errno)); - return; + return false; } sockaddr_in dst; @@ -59,8 +80,10 @@ static void send(const InterfaceDescriptor& iface, const DHCPv4Packet& packet, C dbgln_if(DHCPV4CLIENT_DEBUG, "sendto({}) = {}", fd, rc); if (rc < 0) { dbgln("sendto failed with {}", strerror(errno)); - // FIXME: what do we do here? + return false; } + + return true; } static void set_params(const InterfaceDescriptor& iface, const IPv4Address& ipv4_addr, const IPv4Address& netmask, const IPv4Address& gateway) @@ -109,8 +132,9 @@ static void set_params(const InterfaceDescriptor& iface, const IPv4Address& ipv4 } } -DHCPv4Client::DHCPv4Client(Vector ifnames) - : m_ifnames(ifnames) +DHCPv4Client::DHCPv4Client(Vector ifnames_for_immediate_discover, Vector ifnames_for_later) + : m_ifnames_for_immediate_discover(move(ifnames_for_immediate_discover)) + , m_ifnames_for_later(move(ifnames_for_later)) { m_server = Core::UDPServer::construct(this); m_server->on_ready_to_receive = [this] { @@ -129,8 +153,78 @@ DHCPv4Client::DHCPv4Client(Vector ifnames) VERIFY_NOT_REACHED(); } - for (auto& iface : m_ifnames) + for (auto& iface : m_ifnames_for_immediate_discover) dhcp_discover(iface); + + m_fail_check_timer = Core::Timer::create_repeating( + 1000, [this] { try_discover_deferred_ifs(); }, this); + + m_fail_check_timer->start(); +} + +void DHCPv4Client::try_discover_deferred_ifs() +{ + auto current_interval = m_fail_check_timer->interval(); + if (current_interval < m_max_timer_backoff_interval) + current_interval *= 1.9f; + m_fail_check_timer->set_interval(current_interval); + + if (m_ifnames_for_later.is_empty()) + return; + + // See if any of them are now up. + auto ifs_result = get_discoverable_interfaces(); + if (ifs_result.is_error()) + return; + + Interfaces& ifs = ifs_result.value(); + for (auto& iface : ifs.ready) { + if (!m_ifnames_for_later.remove_first_matching([&](auto& if_) { return if_.m_ifname == iface.m_ifname; })) + continue; + + deferred_invoke([this, iface](auto&) { dhcp_discover(iface); }); + } +} + +Result DHCPv4Client::get_discoverable_interfaces() +{ + auto file = Core::File::construct("/proc/net/adapters"); + if (!file->open(Core::IODevice::ReadOnly)) { + dbgln("Error: Failed to open /proc/net/adapters: {}", file->error_string()); + return String { file->error_string() }; + } + + auto file_contents = file->read_all(); + auto json = JsonValue::from_string(file_contents); + + if (!json.has_value() || !json.value().is_array()) { + dbgln("Error: No network adapters available"); + return String { "No network adapters available" }; + } + + Vector ifnames_to_immediately_discover, ifnames_to_attempt_later; + json.value().as_array().for_each([&ifnames_to_immediately_discover, &ifnames_to_attempt_later](auto& value) { + auto if_object = value.as_object(); + + if (if_object.get("class_name").to_string() == "LoopbackAdapter") + return; + + auto name = if_object.get("name").to_string(); + auto mac = if_object.get("mac_address").to_string(); + auto is_up = if_object.get("link_up").to_bool(); + if (is_up) { + dbgln_if(DHCPV4_DEBUG, "Found adapter '{}' with mac {}, and it was up!", name, mac); + ifnames_to_immediately_discover.empend(name, mac_from_string(mac)); + } else { + dbgln_if(DHCPV4_DEBUG, "Found adapter '{}' with mac {}, but it was down", name, mac); + ifnames_to_attempt_later.empend(name, mac_from_string(mac)); + } + }); + + return Interfaces { + move(ifnames_to_immediately_discover), + move(ifnames_to_attempt_later) + }; } DHCPv4Client::~DHCPv4Client() @@ -266,7 +360,10 @@ void DHCPv4Client::dhcp_discover(const InterfaceDescriptor& iface) auto& dhcp_packet = builder.build(); // broadcast the discover request - send(iface, dhcp_packet, this); + if (!send(iface, dhcp_packet, this)) { + m_ifnames_for_later.append(iface); + return; + } m_ongoing_transactions.set(transaction_id, make(iface)); } @@ -292,6 +389,9 @@ void DHCPv4Client::dhcp_request(DHCPv4Transaction& transaction, const DHCPv4Pack auto& dhcp_packet = builder.build(); // broadcast the "request" request - send(iface, dhcp_packet, this); + if (!send(iface, dhcp_packet, this)) { + m_ifnames_for_later.append(iface); + return; + } transaction.accepted_offer = true; } diff --git a/Userland/Services/DHCPClient/DHCPv4Client.h b/Userland/Services/DHCPClient/DHCPv4Client.h index 1c4a9e260e..518332bd87 100644 --- a/Userland/Services/DHCPClient/DHCPv4Client.h +++ b/Userland/Services/DHCPClient/DHCPv4Client.h @@ -29,6 +29,7 @@ #include "DHCPv4.h" #include #include +#include #include #include #include @@ -59,7 +60,7 @@ class DHCPv4Client final : public Core::Object { C_OBJECT(DHCPv4Client) public: - explicit DHCPv4Client(Vector ifnames); + explicit DHCPv4Client(Vector ifnames_for_immediate_discover, Vector ifnames_for_later); virtual ~DHCPv4Client() override; void dhcp_discover(const InterfaceDescriptor& ifname); @@ -69,10 +70,21 @@ public: bool id_is_registered(u32 id) { return m_ongoing_transactions.contains(id); } + struct Interfaces { + Vector ready; + Vector not_ready; + }; + static Result get_discoverable_interfaces(); + private: + void try_discover_deferred_ifs(); + HashMap> m_ongoing_transactions; - Vector m_ifnames; + Vector m_ifnames_for_immediate_discover; + Vector m_ifnames_for_later; RefPtr m_server; + RefPtr m_fail_check_timer; + int m_max_timer_backoff_interval { 600000 }; // 10 minutes void handle_offer(const DHCPv4Packet&, const ParsedDHCPv4Options&); void handle_ack(const DHCPv4Packet&, const ParsedDHCPv4Options&); diff --git a/Userland/Services/DHCPClient/main.cpp b/Userland/Services/DHCPClient/main.cpp index a70a9fc045..03c676b387 100644 --- a/Userland/Services/DHCPClient/main.cpp +++ b/Userland/Services/DHCPClient/main.cpp @@ -38,23 +38,6 @@ #include #include -static u8 mac_part(const Vector& parts, size_t index) -{ - auto result = AK::StringUtils::convert_to_uint_from_hex(parts.at(index)); - VERIFY(result.has_value()); - return result.value(); -} - -static MACAddress mac_from_string(const String& str) -{ - auto chunks = str.split(':'); - VERIFY(chunks.size() == 6); // should we...worry about this? - return { - mac_part(chunks, 0), mac_part(chunks, 1), mac_part(chunks, 2), - mac_part(chunks, 3), mac_part(chunks, 4), mac_part(chunks, 5) - }; -} - int main([[maybe_unused]] int argc, [[maybe_unused]] char** argv) { if (pledge("stdio unix inet cpath rpath fattr", nullptr) < 0) { @@ -71,36 +54,16 @@ int main([[maybe_unused]] int argc, [[maybe_unused]] char** argv) unveil(nullptr, nullptr); - auto file = Core::File::construct("/proc/net/adapters"); - if (!file->open(Core::IODevice::ReadOnly)) { - dbgln("Error: {}", file->error_string()); + auto ifs_result = DHCPv4Client::get_discoverable_interfaces(); + if (ifs_result.is_error()) { + warnln("Error: {}", ifs_result.error()); return 1; } - auto file_contents = file->read_all(); - auto json = JsonValue::from_string(file_contents); + auto ifs = ifs_result.release_value(); + auto client = DHCPv4Client::construct(move(ifs.ready), move(ifs.not_ready)); - if (!json.has_value() || !json.value().is_array()) { - dbgln("Error: No network adapters available"); - return 1; - } - - Vector ifnames; - json.value().as_array().for_each([&ifnames](auto& value) { - auto if_object = value.as_object(); - - if (if_object.get("class_name").to_string() == "LoopbackAdapter") - return; - - auto name = if_object.get("name").to_string(); - auto mac = if_object.get("mac_address").to_string(); - dbgln_if(DHCPV4_DEBUG, "Found adapter '{}' with mac {}", name, mac); - ifnames.append({ name, mac_from_string(mac) }); - }); - - auto client = DHCPv4Client::construct(move(ifnames)); - - if (pledge("stdio inet", nullptr) < 0) { + if (pledge("stdio inet cpath rpath fattr", nullptr) < 0) { perror("pledge"); return 1; }