From 65955601f0ac34243f882710003f3512a9e6fb98 Mon Sep 17 00:00:00 2001 From: Martin Stein Date: Mon, 27 Jun 2022 16:45:05 +0200 Subject: [PATCH] nic_router: find permit rules w/o exceptions Replaces the former implementation of the 'find_by_port' method at the data structure for permit rules. This method used to return a reference to the found object and threw an exception if no matching object was found. The new implementation doesn't return anything and doesn't throw exceptions. It takes two lambda arguments instead. One for handling the case that a match was found with a reference to the matching object as argument and another for handling the case that no object matches. This way, expensive exception handling can be avoided and object references stay in a local scope. Furthermore, the commit introduces a convenience wrapper for finding the best matching pair of transport rule and corresponding permit rule for a given destination IP and port. This method as well follows the above mentioned concept. Ref #4536 --- repos/os/src/server/nic_router/interface.cc | 63 +++++++++---------- repos/os/src/server/nic_router/permit_rule.cc | 31 --------- repos/os/src/server/nic_router/permit_rule.h | 47 ++++++++++++-- .../src/server/nic_router/transport_rule.cc | 8 --- .../os/src/server/nic_router/transport_rule.h | 54 ++++++++++++++-- 5 files changed, 121 insertions(+), 82 deletions(-) diff --git a/repos/os/src/server/nic_router/interface.cc b/repos/os/src/server/nic_router/interface.cc index 019dbb7a74..781bbe239d 100644 --- a/repos/os/src/server/nic_router/interface.cc +++ b/repos/os/src/server/nic_router/interface.cc @@ -1288,34 +1288,30 @@ void Interface::_handle_ip(Ethernet_frame ð, } } /* try to route via transport and permit rules */ - try { - _transport_rules(local_domain, prot).find_longest_prefix_match( - local_id.dst_ip, - [&] /* handle_match */ (Transport_rule const &transport_rule) - { - Permit_rule const &permit_rule = - transport_rule.permit_rule(local_id.dst_port); + _transport_rules(local_domain, prot).find_best_match( + local_id.dst_ip, + local_id.dst_port, + [&] /* handle_match */ (Transport_rule const &transport_rule, + Permit_rule const &permit_rule) + { + if(_config().verbose()) { + log("[", local_domain, "] using ", + l3_protocol_name(prot), " rule: ", + transport_rule, " ", permit_rule); + } + Domain &remote_domain = permit_rule.domain(); + _adapt_eth(eth, local_id.dst_ip, pkt, remote_domain); + _nat_link_and_pass( + eth, size_guard, ip, prot, prot_base, prot_size, + local_id, local_domain, remote_domain); - if(_config().verbose()) { - log("[", local_domain, "] using ", - l3_protocol_name(prot), " rule: ", transport_rule, - " ", permit_rule); - } - Domain &remote_domain = permit_rule.domain(); - _adapt_eth(eth, local_id.dst_ip, pkt, remote_domain); - _nat_link_and_pass( - eth, size_guard, ip, prot, prot_base, prot_size, - local_id, local_domain, remote_domain); - - done = true; - }, - [&] /* handle_no_match */ () { } - ); - if (done) { - return; - } + done = true; + }, + [&] /* handle_no_match */ () { } + ); + if (done) { + return; } - catch (Permit_single_rule_tree::No_match) { } } catch (Interface::Bad_transport_protocol) { } @@ -1932,28 +1928,25 @@ void Interface::_update_udp_tcp_links(L3_protocol prot, try { /* try to find transport rule that matches the server IP */ bool done { false }; - _transport_rules(cln_dom, prot).find_longest_prefix_match( + _transport_rules(cln_dom, prot).find_best_match( link.client().dst_ip(), - [&] /* handle_match */ (Transport_rule const &transport_rule) + link.client().dst_port(), + [&] /* handle_match */ (Transport_rule const &, + Permit_rule const &permit_rule) { - /* try to find permit rule that matches the server port */ - Permit_rule const &permit_rule = - transport_rule.permit_rule(link.client().dst_port()); - _update_link_check_nat(link, permit_rule.domain(), prot, cln_dom); done = true; }, [&] /* handle_no_match */ () { - _dismiss_link_log(link, "no transport/forward rule"); + _dismiss_link_log(link, "no matching transport/permit/forward rule"); } ); if (done) { return; } } - catch (Permit_single_rule_tree::No_match) { _dismiss_link_log(link, "no permit rule"); } - catch (Dismiss_link) { } + catch (Dismiss_link) { } } ); } diff --git a/repos/os/src/server/nic_router/permit_rule.cc b/repos/os/src/server/nic_router/permit_rule.cc index 4bfc88779c..dadba1db9b 100644 --- a/repos/os/src/server/nic_router/permit_rule.cc +++ b/repos/os/src/server/nic_router/permit_rule.cc @@ -85,34 +85,3 @@ Permit_single_rule::Permit_single_rule(Domain_tree &domains, if (_port == Port(0) || dynamic_port(_port)) { throw Invalid(); } } - - -Permit_single_rule const & -Permit_single_rule::find_by_port(Port const port) const -{ - if (port == _port) { - return *this; } - - bool const side = port.value > _port.value; - Permit_single_rule *const rule = Avl_node::child(side); - if (!rule) { - throw Permit_single_rule_tree::No_match(); } - - return rule->find_by_port(port); -} - - - -/***************************** - ** Permit_single_rule_tree ** - *****************************/ - -Permit_single_rule const & -Permit_single_rule_tree::find_by_port(Port const port) const -{ - Permit_single_rule *const rule = first(); - if (!rule) { - throw No_match(); } - - return rule->find_by_port(port); -} diff --git a/repos/os/src/server/nic_router/permit_rule.h b/repos/os/src/server/nic_router/permit_rule.h index e75f41c91f..594a0c64c3 100644 --- a/repos/os/src/server/nic_router/permit_rule.h +++ b/repos/os/src/server/nic_router/permit_rule.h @@ -112,7 +112,33 @@ class Net::Permit_single_rule : public Permit_rule, Permit_single_rule(Domain_tree &domains, Genode::Xml_node const node); - Permit_single_rule const &find_by_port(Port const port) const; + template + + void find_by_port(Port const port, + HANDLE_MATCH_FN && handle_match, + HANDLE_NO_MATCH_FN && handle_no_match) const + { + if (port.value != _port.value) { + + Permit_single_rule *const rule_ptr { + Avl_node::child( + port.value > _port.value) }; + + if (rule_ptr != nullptr) { + + rule_ptr->find_by_port( + port, handle_match, handle_no_match); + + } else { + + handle_no_match(); + } + } else { + + handle_match(*this); + } + } /********* @@ -141,8 +167,6 @@ struct Net::Permit_single_rule_tree : private Avl_tree { friend class Transport_rule; - struct No_match : Genode::Exception { }; - void insert(Permit_single_rule *rule) { Genode::Avl_tree::insert(rule); @@ -150,7 +174,22 @@ struct Net::Permit_single_rule_tree : private Avl_tree using Genode::Avl_tree::first; - Permit_single_rule const &find_by_port(Port const port) const; + template + + void find_by_port(Port const port, + HANDLE_MATCH_FN && handle_match, + HANDLE_NO_MATCH_FN && handle_no_match) const + { + if (first() != nullptr) { + + first()->find_by_port(port, handle_match, handle_no_match); + + } else { + + handle_no_match(); + } + } }; #endif /* _PERMIT_RULE_H_ */ diff --git a/repos/os/src/server/nic_router/transport_rule.cc b/repos/os/src/server/nic_router/transport_rule.cc index 3baf4ea863..c61c7c4dad 100644 --- a/repos/os/src/server/nic_router/transport_rule.cc +++ b/repos/os/src/server/nic_router/transport_rule.cc @@ -84,11 +84,3 @@ Transport_rule::~Transport_rule() try { destroy(_alloc, &_permit_any_rule()); } catch (Pointer::Invalid) { } } - - -Permit_rule const &Transport_rule::permit_rule(Port const port) const -{ - try { return _permit_any_rule(); } - catch (Pointer::Invalid) { } - return _permit_single_rules.find_by_port(port); -} diff --git a/repos/os/src/server/nic_router/transport_rule.h b/repos/os/src/server/nic_router/transport_rule.h index 061885628d..e1fae0e646 100644 --- a/repos/os/src/server/nic_router/transport_rule.h +++ b/repos/os/src/server/nic_router/transport_rule.h @@ -23,9 +23,9 @@ namespace Genode { class Allocator; } namespace Net { - class Configuration; - class Transport_rule; - struct Transport_rule_list : Direct_rule_list { }; + class Configuration; + class Transport_rule; + class Transport_rule_list; } @@ -53,7 +53,53 @@ class Net::Transport_rule : public Direct_rule ~Transport_rule(); - Permit_rule const &permit_rule(Port const port) const; + template + void + find_permit_rule_by_port(Port const port, + HANDLE_MATCH_FN && handle_match, + HANDLE_NO_MATCH_FN && handle_no_match) const + { + if (_permit_any_rule.valid()) { + + handle_match(_permit_any_rule()); + + } else { + + _permit_single_rules.find_by_port( + port, handle_match, handle_no_match); + } + } +}; + + +class Net::Transport_rule_list : public Direct_rule_list +{ + public: + + template + + void find_best_match(Ipv4_address const &ip, + Port const port, + HANDLE_MATCH_FN && handle_match, + HANDLE_NO_MATCH_FN && handle_no_match) const + { + find_longest_prefix_match( + ip, + [&] /* handle_match */ (Transport_rule const &transport_rule) + { + transport_rule.find_permit_rule_by_port( + port, + [&] /* handle_match */ (Permit_rule const &permit_rule) + { + handle_match(transport_rule, permit_rule); + }, + handle_no_match); + }, + handle_no_match + ); + } }; #endif /* _TRANSPORT_RULE_H_ */