From 3141c8173632c100654144faa3d78b84bdffd72f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 30 Dec 2020 02:03:48 +0900 Subject: [PATCH 1/7] network: constify several arguments --- src/network/networkd-routing-policy-rule.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/network/networkd-routing-policy-rule.c b/src/network/networkd-routing-policy-rule.c index 1f1e3e5b76..bb1c77ad26 100644 --- a/src/network/networkd-routing-policy-rule.c +++ b/src/network/networkd-routing-policy-rule.c @@ -114,7 +114,7 @@ static int routing_policy_rule_new_static(Network *network, const char *filename return 0; } -static int routing_policy_rule_copy(RoutingPolicyRule *dest, RoutingPolicyRule *src) { +static int routing_policy_rule_copy(RoutingPolicyRule *dest, const RoutingPolicyRule *src) { _cleanup_free_ char *iif = NULL, *oif = NULL; assert(dest); @@ -306,7 +306,7 @@ DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR( routing_policy_rule_compare_func, routing_policy_rule_free); -static int routing_policy_rule_get(Manager *m, RoutingPolicyRule *rule, RoutingPolicyRule **ret) { +static int routing_policy_rule_get(Manager *m, const RoutingPolicyRule *rule, RoutingPolicyRule **ret) { RoutingPolicyRule *existing; @@ -329,7 +329,7 @@ static int routing_policy_rule_get(Manager *m, RoutingPolicyRule *rule, RoutingP return -ENOENT; } -static int routing_policy_rule_add_internal(Manager *m, Set **rules, RoutingPolicyRule *in, int family, RoutingPolicyRule **ret) { +static int routing_policy_rule_add_internal(Manager *m, Set **rules, const RoutingPolicyRule *in, int family, RoutingPolicyRule **ret) { _cleanup_(routing_policy_rule_freep) RoutingPolicyRule *rule = NULL; int r; @@ -364,11 +364,11 @@ static int routing_policy_rule_add_internal(Manager *m, Set **rules, RoutingPoli return 0; } -static int routing_policy_rule_add(Manager *m, RoutingPolicyRule *rule, int family, RoutingPolicyRule **ret) { +static int routing_policy_rule_add(Manager *m, const RoutingPolicyRule *rule, int family, RoutingPolicyRule **ret) { return routing_policy_rule_add_internal(m, &m->rules, rule, family, ret); } -static int routing_policy_rule_add_foreign(Manager *m, RoutingPolicyRule *rule, RoutingPolicyRule **ret) { +static int routing_policy_rule_add_foreign(Manager *m, const RoutingPolicyRule *rule, RoutingPolicyRule **ret) { return routing_policy_rule_add_internal(m, &m->rules_foreign, rule, rule->family, ret); } @@ -392,7 +392,7 @@ static void log_routing_policy_rule_debug(const RoutingPolicyRule *rule, int fam } } -static int routing_policy_rule_set_netlink_message(RoutingPolicyRule *rule, sd_netlink_message *m, Link *link) { +static int routing_policy_rule_set_netlink_message(const RoutingPolicyRule *rule, sd_netlink_message *m, Link *link) { int r; assert(rule); @@ -525,7 +525,7 @@ static int routing_policy_rule_remove_handler(sd_netlink *rtnl, sd_netlink_messa return 1; } -static int routing_policy_rule_remove(RoutingPolicyRule *rule, Manager *manager) { +static int routing_policy_rule_remove(const RoutingPolicyRule *rule, Manager *manager) { _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *m = NULL; int r; @@ -583,7 +583,7 @@ static int routing_policy_rule_handler(sd_netlink *rtnl, sd_netlink_message *m, return 1; } -static int routing_policy_rule_configure_internal(RoutingPolicyRule *rule, int family, Link *link) { +static int routing_policy_rule_configure_internal(const RoutingPolicyRule *rule, int family, Link *link) { _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *m = NULL; int r; @@ -619,7 +619,7 @@ static int routing_policy_rule_configure_internal(RoutingPolicyRule *rule, int f return 1; } -static int routing_policy_rule_configure(RoutingPolicyRule *rule, Link *link) { +static int routing_policy_rule_configure(const RoutingPolicyRule *rule, Link *link) { int r; if (IN_SET(rule->family, AF_INET, AF_INET6)) From fdce9324c7132a9a6d3b2fe9f8449dcfc8309114 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 30 Dec 2020 02:11:32 +0900 Subject: [PATCH 2/7] network: fix possible memory leak When set_put() returns 0, then already stored rule will be unref()ed from Manager. --- src/network/networkd-routing-policy-rule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/network/networkd-routing-policy-rule.c b/src/network/networkd-routing-policy-rule.c index bb1c77ad26..723a44cb6d 100644 --- a/src/network/networkd-routing-policy-rule.c +++ b/src/network/networkd-routing-policy-rule.c @@ -343,8 +343,6 @@ static int routing_policy_rule_add_internal(Manager *m, Set **rules, const Routi if (r < 0) return r; - rule->manager = m; - r = routing_policy_rule_copy(rule, in); if (r < 0) return r; @@ -357,6 +355,8 @@ static int routing_policy_rule_add_internal(Manager *m, Set **rules, const Routi if (r == 0) return -EEXIST; + rule->manager = m; + if (ret) *ret = rule; From 57fe5a42f0975ca12ff856ca221c081a1ad3e87e Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 30 Dec 2020 03:17:35 +0900 Subject: [PATCH 3/7] network: drop unnecessary checks By the previous commit, the checks are not necessary any more. --- src/network/networkd-routing-policy-rule.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/network/networkd-routing-policy-rule.c b/src/network/networkd-routing-policy-rule.c index 723a44cb6d..dd155caefb 100644 --- a/src/network/networkd-routing-policy-rule.c +++ b/src/network/networkd-routing-policy-rule.c @@ -39,10 +39,8 @@ RoutingPolicyRule *routing_policy_rule_free(RoutingPolicyRule *rule) { } if (rule->manager) { - if (set_get(rule->manager->rules, rule) == rule) - set_remove(rule->manager->rules, rule); - if (set_get(rule->manager->rules_foreign, rule) == rule) - set_remove(rule->manager->rules_foreign, rule); + set_remove(rule->manager->rules, rule); + set_remove(rule->manager->rules_foreign, rule); } network_config_section_free(rule->section); From c1934a8f2f37b8e64d4b65f465d823c2a96abbc7 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 30 Dec 2020 02:45:25 +0900 Subject: [PATCH 4/7] network: always re-configure rules even if already exist routing_policy_rule_get() in link_set_routing_policy_rules() does not work when [RoutingPolicyRule] section does not have From= or To=. --- src/network/networkd-routing-policy-rule.c | 74 ++++++++++++---------- 1 file changed, 40 insertions(+), 34 deletions(-) diff --git a/src/network/networkd-routing-policy-rule.c b/src/network/networkd-routing-policy-rule.c index dd155caefb..79c4b9adcd 100644 --- a/src/network/networkd-routing-policy-rule.c +++ b/src/network/networkd-routing-policy-rule.c @@ -305,7 +305,6 @@ DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR( routing_policy_rule_free); static int routing_policy_rule_get(Manager *m, const RoutingPolicyRule *rule, RoutingPolicyRule **ret) { - RoutingPolicyRule *existing; assert(m); @@ -327,12 +326,12 @@ static int routing_policy_rule_get(Manager *m, const RoutingPolicyRule *rule, Ro return -ENOENT; } -static int routing_policy_rule_add_internal(Manager *m, Set **rules, const RoutingPolicyRule *in, int family, RoutingPolicyRule **ret) { +static int routing_policy_rule_add(Manager *m, const RoutingPolicyRule *in, int family, RoutingPolicyRule **ret) { _cleanup_(routing_policy_rule_freep) RoutingPolicyRule *rule = NULL; + RoutingPolicyRule *existing; int r; assert(m); - assert(rules); assert(in); assert(IN_SET(family, AF_INET, AF_INET6)); assert(in->family == AF_UNSPEC || in->family == family); @@ -347,27 +346,50 @@ static int routing_policy_rule_add_internal(Manager *m, Set **rules, const Routi rule->family = family; - r = set_ensure_put(rules, &routing_policy_rule_hash_ops, rule); - if (r < 0) - return r; - if (r == 0) - return -EEXIST; + r = routing_policy_rule_get(m, rule, &existing); + if (r == -ENOENT) { + /* Rule does not exist, use a new one. */ + r = set_ensure_put(&m->rules, &routing_policy_rule_hash_ops, rule); + if (r < 0) + return r; + assert(r > 0); - rule->manager = m; + rule->manager = m; + existing = TAKE_PTR(rule); + } else if (r == 0) { + /* Take over a foreign rule. */ + r = set_ensure_put(&m->rules, &routing_policy_rule_hash_ops, existing); + if (r < 0) + return r; + assert(r > 0); + + set_remove(m->rules_foreign, existing); + } else if (r == 1) { + /* Already exists, do nothing. */ + ; + } else + return r; if (ret) - *ret = rule; + *ret = existing; - TAKE_PTR(rule); return 0; } -static int routing_policy_rule_add(Manager *m, const RoutingPolicyRule *rule, int family, RoutingPolicyRule **ret) { - return routing_policy_rule_add_internal(m, &m->rules, rule, family, ret); -} +static int routing_policy_rule_consume_foreign(Manager *m, RoutingPolicyRule *rule) { + int r; -static int routing_policy_rule_add_foreign(Manager *m, const RoutingPolicyRule *rule, RoutingPolicyRule **ret) { - return routing_policy_rule_add_internal(m, &m->rules_foreign, rule, rule->family, ret); + assert(m); + assert(rule); + assert(IN_SET(rule->family, AF_INET, AF_INET6)); + + r = set_ensure_consume(&m->rules_foreign, &routing_policy_rule_hash_ops, rule); + if (r <= 0) + return r; + + rule->manager = m; + + return 1; } static void log_routing_policy_rule_debug(const RoutingPolicyRule *rule, int family, const char *str, const Link *link) { @@ -701,20 +723,6 @@ int link_set_routing_policy_rules(Link *link) { link->routing_policy_rules_configured = false; HASHMAP_FOREACH(rule, link->network->rules_by_section) { - RoutingPolicyRule *existing; - - r = routing_policy_rule_get(link->manager, rule, &existing); - if (r > 0) - continue; - if (r == 0) { - r = set_ensure_put(&link->manager->rules, &routing_policy_rule_hash_ops, existing); - if (r < 0) - return log_link_warning_errno(link, r, "Could not store existing routing policy rule: %m"); - - set_remove(link->manager->rules_foreign, existing); - continue; - } - r = routing_policy_rule_configure(rule, link); if (r < 0) return log_link_warning_errno(link, r, "Could not set routing policy rule: %m"); @@ -945,11 +953,9 @@ int manager_rtnl_process_rule(sd_netlink *rtnl, sd_netlink_message *message, Man log_routing_policy_rule_debug(tmp, tmp->family, "Received remembered", NULL); else { log_routing_policy_rule_debug(tmp, tmp->family, "Remembering foreign", NULL); - r = routing_policy_rule_add_foreign(m, tmp, &rule); - if (r < 0) { + r = routing_policy_rule_consume_foreign(m, TAKE_PTR(tmp)); + if (r < 0) log_warning_errno(r, "Could not remember foreign rule, ignoring: %m"); - return 0; - } } break; case RTM_DELRULE: From 49de8d5cedc9f381a8b61cc9dcd40781cacf7e68 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 30 Dec 2020 03:07:33 +0900 Subject: [PATCH 5/7] network: set RoutingPolicyRule::family based on Family= setting --- src/network/networkd-routing-policy-rule.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/network/networkd-routing-policy-rule.c b/src/network/networkd-routing-policy-rule.c index 79c4b9adcd..0012a4bfa9 100644 --- a/src/network/networkd-routing-policy-rule.c +++ b/src/network/networkd-routing-policy-rule.c @@ -1553,8 +1553,13 @@ static int routing_policy_rule_section_verify(RoutingPolicyRule *rule) { "specified by To= or From=. Ignoring [RoutingPolicyRule] section from line %u.", rule->section->filename, rule->section->line); - if (rule->family == AF_UNSPEC && rule->address_family == ADDRESS_FAMILY_NO) - rule->family = AF_INET; + if (rule->family == AF_UNSPEC) { + if (IN_SET(rule->address_family, ADDRESS_FAMILY_IPV4, ADDRESS_FAMILY_NO)) + rule->family = AF_INET; + else if (rule->address_family == ADDRESS_FAMILY_IPV6) + rule->family = AF_INET6; + /* rule->family can be AF_UNSPEC only when Family=both. */ + } /* Currently, [RoutingPolicyRule] does not have a setting to set FRA_L3MDEV flag. Please also * update routing_policy_rule_is_created_by_kernel() when a new setting which sets the flag is From a75466ed198fad0f50054b4715cfc55c17ffba09 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 30 Dec 2020 03:15:44 +0900 Subject: [PATCH 6/7] network: drop fib rules configured with Family=both --- src/network/networkd-routing-policy-rule.c | 34 +++++++++++++++++++--- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/src/network/networkd-routing-policy-rule.c b/src/network/networkd-routing-policy-rule.c index 0012a4bfa9..722e415830 100644 --- a/src/network/networkd-routing-policy-rule.c +++ b/src/network/networkd-routing-policy-rule.c @@ -660,8 +660,9 @@ static int routing_policy_rule_configure(const RoutingPolicyRule *rule, Link *li return 0; } -static bool links_have_routing_policy_rule(const Manager *m, const RoutingPolicyRule *rule, const Link *except) { +static int links_have_routing_policy_rule(const Manager *m, const RoutingPolicyRule *rule, const Link *except) { Link *link; + int r; assert(m); assert(rule); @@ -676,8 +677,29 @@ static bool links_have_routing_policy_rule(const Manager *m, const RoutingPolicy continue; HASHMAP_FOREACH(link_rule, link->network->rules_by_section) - if (routing_policy_rule_equal(link_rule, rule)) - return true; + if (IN_SET(link_rule->family, AF_INET, AF_INET6)) { + if (routing_policy_rule_equal(link_rule, rule)) + return true; + } else { + /* The case Family=both. */ + _cleanup_(routing_policy_rule_freep) RoutingPolicyRule *tmp = NULL; + + r = routing_policy_rule_new(&tmp); + if (r < 0) + return r; + + r = routing_policy_rule_copy(tmp, link_rule); + if (r < 0) + return r; + + tmp->family = AF_INET; + if (routing_policy_rule_equal(tmp, rule)) + return true; + + tmp->family = AF_INET6; + if (routing_policy_rule_equal(tmp, rule)) + return true; + } } return false; @@ -697,8 +719,12 @@ int manager_drop_routing_policy_rules_internal(Manager *m, bool foreign, const L continue; /* The rule will be configured later, or already configured by a link. */ - if (links_have_routing_policy_rule(m, rule, except)) + k = links_have_routing_policy_rule(m, rule, except); + if (k != 0) { + if (k < 0 && r >= 0) + r = k; continue; + } k = routing_policy_rule_remove(rule, m); if (k < 0 && r >= 0) From 49ff3f34d549ad214924548f46a71315b5cd23c6 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 30 Dec 2020 03:32:07 +0900 Subject: [PATCH 7/7] test-network: add tests for fib rules with Family=both vs networkctl reload or reconfigure --- ... routing-policy-rule-reconfigure1.network} | 2 +- .../routing-policy-rule-reconfigure2.network | 33 ++++++++++++ test/test-network/systemd-networkd-tests.py | 53 ++++++++++++++----- 3 files changed, 75 insertions(+), 13 deletions(-) rename test/test-network/conf/{routing-policy-rule-reconfigure.network => routing-policy-rule-reconfigure1.network} (96%) create mode 100644 test/test-network/conf/routing-policy-rule-reconfigure2.network diff --git a/test/test-network/conf/routing-policy-rule-reconfigure.network b/test/test-network/conf/routing-policy-rule-reconfigure1.network similarity index 96% rename from test/test-network/conf/routing-policy-rule-reconfigure.network rename to test/test-network/conf/routing-policy-rule-reconfigure1.network index ca38b78f13..96650c2551 100644 --- a/test/test-network/conf/routing-policy-rule-reconfigure.network +++ b/test/test-network/conf/routing-policy-rule-reconfigure1.network @@ -21,7 +21,7 @@ OutgoingInterface=test1 # iif [RoutingPolicyRule] Table=1011 -Family=ipv4 +Family=both Priority=10113 IncomingInterface=test1 diff --git a/test/test-network/conf/routing-policy-rule-reconfigure2.network b/test/test-network/conf/routing-policy-rule-reconfigure2.network new file mode 100644 index 0000000000..d12fe04020 --- /dev/null +++ b/test/test-network/conf/routing-policy-rule-reconfigure2.network @@ -0,0 +1,33 @@ +[Match] +Name=test1 + +[Network] +IPv6AcceptRA=no + +# fwmark +[RoutingPolicyRule] +Table=1011 +Family=ipv4 +Priority=10111 +FirewallMark=1011 + +# oif +[RoutingPolicyRule] +Table=1011 +Family=both +Priority=10112 +OutgoingInterface=test1 + +# iif +[RoutingPolicyRule] +Table=1011 +Family=ipv4 +Priority=10113 +IncomingInterface=test1 + +# source +[RoutingPolicyRule] +Table=1011 +Family=ipv4 +Priority=10114 +From=192.168.8.254 diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 41d3e7036b..0d0923ea7b 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -1800,7 +1800,8 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): '26-link-local-addressing-ipv6.network', 'routing-policy-rule-dummy98.network', 'routing-policy-rule-test1.network', - 'routing-policy-rule-reconfigure.network', + 'routing-policy-rule-reconfigure1.network', + 'routing-policy-rule-reconfigure2.network', ] routing_policy_rule_tables = ['7', '8', '9', '1011'] @@ -2084,37 +2085,65 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): stop_networkd(remove_state_files=False) def test_routing_policy_rule_reconfigure(self): - copy_unit_to_networkd_unit_path('routing-policy-rule-reconfigure.network', '11-dummy.netdev') + copy_unit_to_networkd_unit_path('routing-policy-rule-reconfigure2.network', '11-dummy.netdev') start_networkd() self.wait_online(['test1:degraded']) output = check_output('ip rule list table 1011') print(output) - self.assertRegex(output, '10111: from all fwmark 0x3f3 lookup 1011') - self.assertRegex(output, '10112: from all oif test1 lookup 1011') - self.assertRegex(output, '10113: from all iif test1 lookup 1011') - self.assertRegex(output, '10114: from 192.168.8.254 lookup 1011') + self.assertIn('10111: from all fwmark 0x3f3 lookup 1011', output) + self.assertIn('10112: from all oif test1 lookup 1011', output) + self.assertIn('10113: from all iif test1 lookup 1011', output) + self.assertIn('10114: from 192.168.8.254 lookup 1011', output) + + output = check_output('ip -6 rule list table 1011') + print(output) + self.assertIn('10112: from all oif test1 lookup 1011', output) + + copy_unit_to_networkd_unit_path('routing-policy-rule-reconfigure1.network', '11-dummy.netdev') + run(*networkctl_cmd, 'reload', env=env) + time.sleep(1) + self.wait_online(['test1:degraded']) + + output = check_output('ip rule list table 1011') + print(output) + self.assertIn('10111: from all fwmark 0x3f3 lookup 1011', output) + self.assertIn('10112: from all oif test1 lookup 1011', output) + self.assertIn('10113: from all iif test1 lookup 1011', output) + self.assertIn('10114: from 192.168.8.254 lookup 1011', output) + + output = check_output('ip -6 rule list table 1011') + print(output) + self.assertNotIn('10112: from all oif test1 lookup 1011', output) + self.assertIn('10113: from all iif test1 lookup 1011', output) run('ip rule delete priority 10111') run('ip rule delete priority 10112') run('ip rule delete priority 10113') run('ip rule delete priority 10114') - run('ip rule delete priority 10115') + run('ip -6 rule delete priority 10113') output = check_output('ip rule list table 1011') print(output) self.assertEqual(output, '') - run(*networkctl_cmd, 'reconfigure', 'test1', env=env) + output = check_output('ip -6 rule list table 1011') + print(output) + self.assertEqual(output, '') + run(*networkctl_cmd, 'reconfigure', 'test1', env=env) self.wait_online(['test1:degraded']) output = check_output('ip rule list table 1011') print(output) - self.assertRegex(output, '10111: from all fwmark 0x3f3 lookup 1011') - self.assertRegex(output, '10112: from all oif test1 lookup 1011') - self.assertRegex(output, '10113: from all iif test1 lookup 1011') - self.assertRegex(output, '10114: from 192.168.8.254 lookup 1011') + self.assertIn('10111: from all fwmark 0x3f3 lookup 1011', output) + self.assertIn('10112: from all oif test1 lookup 1011', output) + self.assertIn('10113: from all iif test1 lookup 1011', output) + self.assertIn('10114: from 192.168.8.254 lookup 1011', output) + + output = check_output('ip -6 rule list table 1011') + print(output) + self.assertIn('10113: from all iif test1 lookup 1011', output) @expectedFailureIfRoutingPolicyPortRangeIsNotAvailable() def test_routing_policy_rule_port_range(self):