nic_router: bind link state to remote DNS config

The NIC router README claims that the 'dns_config_from' attribute in a DHCP
server configuration binds the propagated link state of all interfaces at the
domain of the server to the validity of the IP config of the domain that is
given through 'dns_config_from'.

However, this was not true. The router missed to implement this detail which
led to clients of such a DHCP server sending DHCP DISCOVER packets too early.
These early DHCP DISCOVER packets were dropped by the router potentially
causing a big delay until the client started a new attempt. Unnecessary long
network boot-up delays were observed with at least the lwip run script and
Sculpt on the PinePhone and could be tracked down to this former
inconsistency in the router.

This commit fixes the inconsistency.

Fixes #4612

repos/os/src/server/nic_router/README

@@ -485,10 +485,32 @@ sub-tags. The attribute states the domain from whose IP config to take the DNS
 domain name and the list of DNS server addresses that shall be propagated. Note
 that the order of DNS server adresses is not altered thereby. This is useful in
 scenarios where these addresses must be obtained dynamically through the DHCP
-client of another domain. An implication of the 'dns_config_from' attribute is
-that the link state of all interfaces at the domain with the DHCP server
-becomes bound to the validity of the IP config of the domain that is stated in
-the attribute.
+client of another domain.
+
+There is an additional feature the router implements together with the
+'dns_config_from' functionality. If a domain has a DHCP server with a
+'dns_config_from' value that denominates another valid domain, the link state
+of all interfaces, where the router is a NIC session server, at the first
+domain becomes bound to the validity of the IP config of the second domain.
+
+One motivation for having this feature is to optimize the network boot-up time
+of application clients of the NIC router by preventing DHCP re-attempt
+timeouts. Such timeouts would occur because clients attempt DHCP as soon as
+their interface goes up and without the extra feature, the DHCP server would
+potentially not be ready yet.
+
+But this feature is not only an optimization. It also implies that whenever the
+domain denominated by 'dns_config_from' receives a new IP config with a new DNS
+configuration, application clients are poked via a link state "down-up"
+sequence to re-request DHCP and thereby update their DNS configuration. I.e. it
+enables dynamic DNS config propagation.
+
+However, please be aware this feature is limited to applications that connect
+directly and via a NIC session client to the NIC router. This is due to two
+technical limitations. First, the router can't control the link state at
+interfaces where he acts as NIC session client or Uplink session server. And
+second, the router can't ensure that link state changes reach the client if
+there are intermediate network components like a NIC bridge.
 
 The lifetime of an IP address assignment that was yet only offered to the
 client can be configured for all domains in the <config> tag of the router:

repos/os/src/server/nic_router/dhcp_server.cc

@@ -223,14 +223,12 @@ Pointer<Domain> Dhcp_server::_init_dns_config_from(Genode::Xml_node const  node,
 }
 
 
-bool Dhcp_server::ready() const
+bool Dhcp_server::has_invalid_remote_dns_cfg() const
 {
-	if (!_dns_servers.empty()) {
-		return true;
+	if (_dns_config_from.valid()) {
+		return !_dns_config_from().ip_config().valid();
 	}
-	try { return _dns_config_from().ip_config().valid(); }
-	catch (Pointer<Domain>::Invalid) { }
-	return true;
+	return false;
 }
 
 

repos/os/src/server/nic_router/dhcp_server.h

@@ -105,7 +105,7 @@ class Net::Dhcp_server : private Genode::Noncopyable,
 		void free_ip(Domain       const &domain,
 		             Ipv4_address const &ip);
 
-		bool ready() const;
+		bool has_invalid_remote_dns_cfg() const;
 
 		template <typename FUNC>
 		void for_each_dns_server_ip(FUNC && functor) const

repos/os/src/server/nic_router/domain.cc

@@ -49,6 +49,26 @@ void Domain::_log_ip_config() const
 }
 
 
+bool Domain::is_ready() const
+{
+	if (_dhcp_server.valid()) {
+		if (_dhcp_server().has_invalid_remote_dns_cfg()) {
+			return false;
+		}
+	}
+	return true;
+}
+
+
+void Domain::update_ready_state()
+{
+	bool const ready { is_ready() };
+	_interfaces.for_each([&] (Interface &interface) {
+		interface.handle_domain_ready_state(ready);
+	});
+}
+
+
 void Domain::_prepare_reconstructing_ip_config()
 {
 	if (!_ip_config_dynamic) {
@@ -65,9 +85,7 @@ void Domain::_prepare_reconstructing_ip_config()
 			interface.detach_from_ip_config(*this);
 		});
 		_ip_config_dependents.for_each([&] (Domain &domain) {
-			domain._interfaces.for_each([&] (Interface &interface) {
-				interface.detach_from_remote_ip_config();
-			});
+			domain.update_ready_state();
 		});
 		/* dissolve foreign ARP waiters */
 		while (_foreign_arp_waiters.first()) {
@@ -88,9 +106,7 @@ void Domain::_finish_reconstructing_ip_config()
 			interface.attach_to_ip_config(*this, ip_config());
 		});
 		_ip_config_dependents.for_each([&] (Domain &domain) {
-			domain._interfaces.for_each([&] (Interface &interface) {
-				interface.attach_to_remote_ip_config();
-			});
+			domain.update_ready_state();
 		});
 	} else {
 		_interfaces.for_each([&] (Interface &interface) {
@@ -234,7 +250,7 @@ Domain::~Domain()
 Dhcp_server &Domain::dhcp_server()
 {
 	Dhcp_server &dhcp_server = _dhcp_server();
-	if (!dhcp_server.ready()) {
+	if (dhcp_server.has_invalid_remote_dns_cfg()) {
 		throw Pointer<Dhcp_server>::Invalid();
 	}
 	return dhcp_server;

repos/os/src/server/nic_router/domain.h

@@ -223,6 +223,10 @@ class Net::Domain : public Domain_base,
 
 		void add_dropped_fragm_ipv4(unsigned long dropped_fragm_ipv4);
 
+		bool is_ready() const;
+
+		void update_ready_state();
+
 
 		/*********
 		 ** log **

repos/os/src/server/nic_router/interface.cc

@@ -337,7 +337,7 @@ Interface::_transport_rules(Domain &local_domain, L3_protocol const prot) const
 void Interface::_attach_to_domain_raw(Domain &domain)
 {
 	_domain = domain;
-	_policy.interface_ready();
+	_refetch_domain_ready_state();
 	_interfaces.remove(this);
 	domain.attach_interface(*this);
 }
@@ -349,7 +349,7 @@ void Interface::_detach_from_domain_raw()
 	domain.detach_interface(*this);
 	_interfaces.insert(this);
 	_domain = Pointer<Domain>();
-	_policy.interface_unready();
+	_refetch_domain_ready_state();
 
 	domain.add_dropped_fragm_ipv4(_dropped_fragm_ipv4);
 	domain.tcp_stats().dissolve_interface(_tcp_stats);
@@ -367,7 +367,7 @@ void Interface::_update_domain_object(Domain &new_domain) {
 	old_domain.interface_updates_domain_object(*this);
 	_interfaces.insert(this);
 	_domain = Pointer<Domain>();
-	_policy.interface_unready();
+	_refetch_domain_ready_state();
 
 	old_domain.add_dropped_fragm_ipv4(_dropped_fragm_ipv4);
 	old_domain.tcp_stats().dissolve_interface(_tcp_stats);
@@ -378,7 +378,7 @@ void Interface::_update_domain_object(Domain &new_domain) {
 
 	/* attach raw */
 	_domain = new_domain;
-	_policy.interface_ready();
+	_refetch_domain_ready_state();
 	_interfaces.remove(this);
 	new_domain.attach_interface(*this);
 }
@@ -452,18 +452,26 @@ void Interface::detach_from_ip_config(Domain &domain)
 }
 
 
-void Interface::detach_from_remote_ip_config()
+void Interface::handle_domain_ready_state(bool state)
 {
-	/* only the DNS server address of the local DHCP server can be remote */
-	_policy.interface_unready();
-
+	_policy.handle_domain_ready_state(state);
 }
 
 
-void Interface::attach_to_remote_ip_config()
+void Interface::_refetch_domain_ready_state()
 {
-	/* only the DNS server address of the local DHCP server can be remote */
-	_policy.interface_ready();
+	if (_domain.valid()) {
+		handle_domain_ready_state(_domain().is_ready());
+	} else {
+		handle_domain_ready_state(false);
+	}
+}
+
+
+void Interface::_reset_and_refetch_domain_ready_state()
+{
+	_policy.handle_domain_ready_state(false);
+	_refetch_domain_ready_state();
 }
 
 
@@ -2138,8 +2146,7 @@ void Interface::_update_dhcp_allocations(Domain &old_domain,
 		}
 	}
 	if (dhcp_clients_outdated) {
-		_policy.interface_unready();
-		_policy.interface_ready();
+		_reset_and_refetch_domain_ready_state();
 	}
 }
 

repos/os/src/server/nic_router/interface.h

@@ -103,9 +103,7 @@ struct Net::Interface_policy
 
 	virtual Genode::Session_label const &label() const = 0;
 
-	virtual void interface_ready() = 0;
-
-	virtual void interface_unready() = 0;
+	virtual void handle_domain_ready_state(bool) = 0;
 
 	virtual bool interface_link_state() const = 0;
 
@@ -367,6 +365,10 @@ class Net::Interface : private Interface_list::Element
 
 		void _handle_pkt_stream_signal();
 
+		void _reset_and_refetch_domain_ready_state();
+
+		void _refetch_domain_ready_state();
+
 	public:
 
 		struct Free_resources_and_retry_handle_eth : Genode::Exception { L3_protocol prot; Free_resources_and_retry_handle_eth(L3_protocol prot = (L3_protocol)0) : prot(prot) { } };
@@ -456,6 +458,8 @@ class Net::Interface : private Interface_list::Element
 
 		void report(Genode::Xml_generator &xml);
 
+		void handle_domain_ready_state(bool state);
+
 
 		/***************
 		 ** Accessors **

repos/os/src/server/nic_router/nic_client.cc

@@ -127,21 +127,15 @@ Net::Nic_client_interface_base::
 { }
 
 
-void Net::Nic_client_interface_base::interface_unready()
+void Net::Nic_client_interface_base::handle_domain_ready_state(bool state)
 {
-	_interface_ready = false;
-};
-
-
-void Net::Nic_client_interface_base::interface_ready()
-{
-	_interface_ready = true;
-};
+	_domain_ready = state;
+}
 
 
 bool Net::Nic_client_interface_base::interface_link_state() const
 {
-	return _interface_ready && _session_link_state;
+	return _domain_ready && _session_link_state;
 }
 
 

repos/os/src/server/nic_router/nic_client.h

@@ -107,7 +107,7 @@ class Net::Nic_client_interface_base : public Interface_policy
 		Const_reference<Domain_name>  _domain_name;
 		Genode::Session_label  const  _label;
 		bool                   const &_session_link_state;
-		bool                          _interface_ready { false };
+		bool                          _domain_ready { false };
 
 
 		/***************************
@@ -117,8 +117,7 @@ class Net::Nic_client_interface_base : public Interface_policy
 		Domain_name determine_domain_name() const override { return _domain_name(); };
 		void handle_config(Configuration const &) override { }
 		Genode::Session_label const &label() const override { return _label; }
-		void interface_unready() override;
-		void interface_ready() override;
+		void handle_domain_ready_state(bool state) override;
 		bool interface_link_state() const override;
 
 	public:

repos/os/src/server/nic_router/nic_session_root.cc

@@ -52,7 +52,7 @@ Interface_policy::Interface_policy(Genode::Session_label const &label,
 	_config      { config },
 	_session_env { session_env }
 {
-	_session_link_state_transition(UP);
+	_session_link_state_transition(DOWN);
 }
 
 
@@ -86,70 +86,72 @@ Interface_policy::_session_link_state_transition(Transient_link_state tls)
 }
 
 
-void Net::Nic_session_component::Interface_policy::interface_unready()
+void Net::
+Nic_session_component::Interface_policy::handle_domain_ready_state(bool state)
 {
-	switch (_transient_link_state) {
-	case UP_ACKNOWLEDGED:
+	if (state) {
 
-		_session_link_state_transition(DOWN);
-		break;
+		switch (_transient_link_state) {
+		case DOWN_ACKNOWLEDGED:
 
-	case UP:
+			_session_link_state_transition(UP);
+			break;
 
-		_transient_link_state = UP_DOWN;
-		break;
+		case DOWN:
 
-	case DOWN_UP:
+			_transient_link_state = DOWN_UP;
+			break;
 
-		_transient_link_state = DOWN_UP_DOWN;
-		break;
+		case UP_DOWN:
 
-	case UP_DOWN:
+			_transient_link_state = UP_DOWN_UP;
+			break;
 
-		break;
+		case DOWN_UP:
 
-	case UP_DOWN_UP:
+			break;
 
-		_transient_link_state = UP_DOWN;
-		break;
+		case DOWN_UP_DOWN:
 
-	case DOWN_ACKNOWLEDGED: break;
-	case DOWN:              break;
-	case DOWN_UP_DOWN:      break;
-	}
-}
+			_transient_link_state = DOWN_UP;
+			break;
 
+		case UP_ACKNOWLEDGED: break;
+		case UP:              break;
+		case UP_DOWN_UP:      break;
+		}
 
-void Net::Nic_session_component::Interface_policy::interface_ready()
-{
-	switch (_transient_link_state) {
-	case DOWN_ACKNOWLEDGED:
+	} else {
 
-		_session_link_state_transition(UP);
-		break;
+		switch (_transient_link_state) {
+		case UP_ACKNOWLEDGED:
 
-	case DOWN:
+			_session_link_state_transition(DOWN);
+			break;
 
-		_transient_link_state = DOWN_UP;
-		break;
+		case UP:
 
-	case UP_DOWN:
+			_transient_link_state = UP_DOWN;
+			break;
 
-		_transient_link_state = UP_DOWN_UP;
-		break;
+		case DOWN_UP:
 
-	case DOWN_UP:
+			_transient_link_state = DOWN_UP_DOWN;
+			break;
 
-		break;
+		case UP_DOWN:
 
-	case DOWN_UP_DOWN:
+			break;
 
-		_transient_link_state = DOWN_UP;
-		break;
+		case UP_DOWN_UP:
 
-	case UP_ACKNOWLEDGED: break;
-	case UP:              break;
-	case UP_DOWN_UP:      break;
+			_transient_link_state = UP_DOWN;
+			break;
+
+		case DOWN_ACKNOWLEDGED: break;
+		case DOWN:              break;
+		case DOWN_UP_DOWN:      break;
+		}
 	}
 }
 

repos/os/src/server/nic_router/nic_session_root.h

@@ -118,8 +118,7 @@ class Net::Nic_session_component : private Nic_session_component_base,
 				void handle_config(Configuration const &config) override { _config = config; }
 				Genode::Session_label const &label() const override { return _label; }
 				void report(Genode::Xml_generator &xml) const override { _session_env.report(xml); };
-				void interface_unready() override;
-				void interface_ready() override;
+				void handle_domain_ready_state(bool state) override;
 				bool interface_link_state() const override;
 		};
 

repos/os/src/server/nic_router/uplink_session_root.h

@@ -82,8 +82,7 @@ class Net::Uplink_session_component : private Uplink_session_component_base,
 				void handle_config(Configuration const &config) override { _config = config; }
 				Genode::Session_label const &label() const override { return _label; }
 				void report(Genode::Xml_generator &xml) const override { _session_env.report(xml); };
-				void interface_unready() override { }
-				void interface_ready() override { }
+				void handle_domain_ready_state(bool /* state */) override { }
 				bool interface_link_state() const override { return true; }
 		};