From fae833799f0c3398a18d1c0823e395345cdb9aa1 Mon Sep 17 00:00:00 2001 From: Andrew Yourtchenko Date: Wed, 26 Sep 2018 21:03:06 +0200 Subject: [PATCH] acl-plugin: fix the stateful ICMP handling and add testcases The stateful ICMP/ICMPv6 handling got broken. Fix that and introduce testcases to catch in the future. Change-Id: Ie602e72d6ac613d64ab0bf6693b6d75afb1a9552 Signed-off-by: Andrew Yourtchenko --- src/plugins/acl/session_inlines.h | 79 +++++++++++----------- test/test_acl_plugin_l2l3.py | 134 ++++++++++++++++++++++++++++++-------- 2 files changed, 147 insertions(+), 66 deletions(-) diff --git a/src/plugins/acl/session_inlines.h b/src/plugins/acl/session_inlines.h index 5e975300142..6ac5983e086 100644 --- a/src/plugins/acl/session_inlines.h +++ b/src/plugins/acl/session_inlines.h @@ -16,10 +16,10 @@ /* ICMPv4 invert type for stateful ACL */ static const u8 icmp4_invmap[] = { - [ICMP4_echo_reply] = ICMP4_echo_request + 1, - [ICMP4_timestamp_reply] = ICMP4_timestamp_request + 1, - [ICMP4_information_reply] = ICMP4_information_request + 1, - [ICMP4_address_mask_reply] = ICMP4_address_mask_request + 1 + [ICMP4_echo_request] = ICMP4_echo_reply + 1, + [ICMP4_timestamp_request] = ICMP4_timestamp_reply + 1, + [ICMP4_information_request] = ICMP4_information_reply + 1, + [ICMP4_address_mask_request] = ICMP4_address_mask_reply + 1 }; /* Supported ICMPv4 messages for session creation */ @@ -32,8 +32,8 @@ static const u8 icmp4_valid_new[] = { /* ICMPv6 invert type for stateful ACL */ static const u8 icmp6_invmap[] = { - [ICMP6_echo_reply - 128] = ICMP6_echo_request + 1, - [ICMP6_node_information_response - 128] = ICMP6_node_information_request + 1 + [ICMP6_echo_request - 128] = ICMP6_echo_reply + 1, + [ICMP6_node_information_request - 128] = ICMP6_node_information_response + 1 }; /* Supported ICMPv6 messages for session creation */ @@ -294,8 +294,8 @@ reverse_l4_u64_fastpath (u64 l4, int is_ip6) return l4o.as_u64; } -always_inline u64 -reverse_l4_u64_slowpath (u64 l4, int is_ip6) +always_inline int +reverse_l4_u64_slowpath_valid (u64 l4, int is_ip6, u64 * out) { fa_session_l4_key_t l4i = {.as_u64 = l4 }; fa_session_l4_key_t l4o; @@ -324,39 +324,24 @@ reverse_l4_u64_slowpath (u64 l4, int is_ip6) * The other messages will be forwarded without creating a reverse session. */ - if (type >= 0 && (type <= icmp_valid_new_size[is_ip6]) - && (icmp_valid_new[is_ip6][type]) - && (type <= icmp_invmap_size[is_ip6]) && icmp_invmap[is_ip6][type]) + int valid_reverse_sess = (type >= 0 + && (type <= icmp_valid_new_size[is_ip6]) + && (icmp_valid_new[is_ip6][type]) + && (type <= icmp_invmap_size[is_ip6]) + && icmp_invmap[is_ip6][type]); + if (valid_reverse_sess) { - /* - * we set the inverse direction and correct the port, - * if it is okay to add the reverse session. - * If not, then the same session will be added twice - * to bihash, which is the same as adding just one session. - */ l4o.is_input = 1 - l4i.is_input; l4o.port[0] = icmp_invmap[is_ip6][type] - 1; } - return l4o.as_u64; + *out = l4o.as_u64; + return valid_reverse_sess; } else - return reverse_l4_u64_fastpath (l4, is_ip6); -} - -always_inline u64 -reverse_l4_u64 (u64 l4, int is_ip6) -{ - fa_session_l4_key_t l4i = {.as_u64 = l4 }; + *out = reverse_l4_u64_fastpath (l4, is_ip6); - if (PREDICT_FALSE (l4i.is_slowpath)) - { - return reverse_l4_u64_slowpath (l4, is_ip6); - } - else - { - return reverse_l4_u64_fastpath (l4, is_ip6); - } + return 1; } always_inline void @@ -368,10 +353,18 @@ reverse_session_add_del_ip6 (acl_main_t * am, kv2.key[1] = pkv->key[3]; kv2.key[2] = pkv->key[0]; kv2.key[3] = pkv->key[1]; - /* the last u64 needs special treatment (ports, etc.) */ - kv2.key[4] = reverse_l4_u64 (pkv->key[4], 1); + /* the last u64 needs special treatment (ports, etc.) so we do it last */ kv2.value = pkv->value; - clib_bihash_add_del_40_8 (&am->fa_ip6_sessions_hash, &kv2, is_add); + if (PREDICT_FALSE (((fa_session_l4_key_t) pkv->key[4]).is_slowpath)) + { + if (reverse_l4_u64_slowpath_valid (pkv->key[4], 1, &kv2.key[4])) + clib_bihash_add_del_40_8 (&am->fa_ip6_sessions_hash, &kv2, is_add); + } + else + { + kv2.key[4] = reverse_l4_u64_fastpath (pkv->key[4], 1); + clib_bihash_add_del_40_8 (&am->fa_ip6_sessions_hash, &kv2, is_add); + } } always_inline void @@ -381,10 +374,18 @@ reverse_session_add_del_ip4 (acl_main_t * am, clib_bihash_kv_16_8_t kv2; kv2.key[0] = ((pkv->key[0] & 0xffffffff) << 32) | ((pkv->key[0] >> 32) & 0xffffffff); - /* the last u64 needs special treatment (ports, etc.) */ - kv2.key[1] = reverse_l4_u64 (pkv->key[1], 0); + /* the last u64 needs special treatment (ports, etc.) so we do it last */ kv2.value = pkv->value; - clib_bihash_add_del_16_8 (&am->fa_ip4_sessions_hash, &kv2, is_add); + if (PREDICT_FALSE (((fa_session_l4_key_t) pkv->key[1]).is_slowpath)) + { + if (reverse_l4_u64_slowpath_valid (pkv->key[1], 0, &kv2.key[1])) + clib_bihash_add_del_16_8 (&am->fa_ip4_sessions_hash, &kv2, is_add); + } + else + { + kv2.key[1] = reverse_l4_u64_fastpath (pkv->key[1], 0); + clib_bihash_add_del_16_8 (&am->fa_ip4_sessions_hash, &kv2, is_add); + } } always_inline void diff --git a/test/test_acl_plugin_l2l3.py b/test/test_acl_plugin_l2l3.py index 66d053ff584..8e6b9e30650 100644 --- a/test/test_acl_plugin_l2l3.py +++ b/test/test_acl_plugin_l2l3.py @@ -40,7 +40,7 @@ from vpp_papi_provider import L2_PORT_TYPE import time -class TestIpIrb(VppTestCase): +class TestAclIpIrb(VppTestCase): """IRB Test Case""" @classmethod @@ -53,7 +53,7 @@ class TestIpIrb(VppTestCase): #. Loopback BVI interface has remote hosts, one half of hosts are behind pg0 second behind pg1. """ - super(TestIpIrb, cls).setUpClass() + super(TestAclIpIrb, cls).setUpClass() cls.pg_if_packet_sizes = [64, 512, 1518, 9018] # packet sizes cls.bd_id = 10 @@ -94,6 +94,8 @@ class TestIpIrb(VppTestCase): cls.WITHOUT_EH = False cls.WITH_EH = True + cls.STATELESS_ICMP = False + cls.STATEFUL_ICMP = True # Loopback BVI interface has remote hosts, one half of hosts are behind # pg0 second behind pg1 @@ -106,24 +108,24 @@ class TestIpIrb(VppTestCase): ``show l2fib verbose``,``show bridge-domain detail``, ``show ip arp``. """ - super(TestIpIrb, self).tearDown() + super(TestAclIpIrb, self).tearDown() if not self.vpp_dead: self.logger.info(self.vapi.cli("show l2patch")) self.logger.info(self.vapi.cli("show classify tables")) - self.logger.info(self.vapi.cli("show vlib graph")) self.logger.info(self.vapi.cli("show l2fib verbose")) self.logger.info(self.vapi.cli("show bridge-domain %s detail" % self.bd_id)) self.logger.info(self.vapi.cli("show ip arp")) self.logger.info(self.vapi.cli("show ip6 neighbors")) - self.logger.info(self.vapi.cli("show acl-plugin sessions")) + cmd = "show acl-plugin sessions verbose 1" + self.logger.info(self.vapi.cli(cmd)) self.logger.info(self.vapi.cli("show acl-plugin acl")) self.logger.info(self.vapi.cli("show acl-plugin interface")) self.logger.info(self.vapi.cli("show acl-plugin tables")) def create_stream(self, src_ip_if, dst_ip_if, reverse, packet_sizes, is_ip6, expect_blocked, expect_established, - add_extension_header): + add_extension_header, icmp_stateful=False): pkts = [] rules = [] permit_rules = [] @@ -131,7 +133,15 @@ class TestIpIrb(VppTestCase): total_packet_count = 8 for i in range(0, total_packet_count): modulo = (i//2) % 2 - can_reflect_this_packet = (modulo == 0) + icmp_type_delta = i % 2 + icmp_code = i + is_udp_packet = (modulo == 0) + if is_udp_packet and icmp_stateful: + continue + is_reflectable_icmp = (icmp_stateful and icmp_type_delta == 0 and + not is_udp_packet) + is_reflected_icmp = is_reflectable_icmp and expect_established + can_reflect_this_packet = is_udp_packet or is_reflectable_icmp is_permit = i % 2 remote_dst_index = i % len(dst_ip_if.remote_hosts) remote_dst_host = dst_ip_if.remote_hosts[remote_dst_index] @@ -167,13 +177,15 @@ class TestIpIrb(VppTestCase): dst_ip4 = remote_dst_host.ip4 src_l4 = 1234 + i dst_l4 = 4321 + i + if is_reflected_icmp: + icmp_type_delta = 1 # default ULP should be something we do not use in tests ulp_l4 = TCP(sport=src_l4, dport=dst_l4) # potentially a chain of protocols leading to ULP ulp = ulp_l4 - if can_reflect_this_packet: + if is_udp_packet: if is_ip6: ulp_l4 = UDP(sport=src_l4, dport=dst_l4) if add_extension_header: @@ -200,14 +212,15 @@ class TestIpIrb(VppTestCase): Raw(payload)) elif modulo == 1: if is_ip6: - ulp_l4 = ICMPv6Unknown(type=128 + (i % 2), code=i % 2) + ulp_l4 = ICMPv6Unknown(type=128 + icmp_type_delta, + code=icmp_code) ulp = ulp_l4 p = (Ether(dst=dst_mac, src=src_mac) / IPv6(src=src_ip6, dst=dst_ip6) / ulp / Raw(payload)) else: - ulp_l4 = ICMP(type=8 + (i % 2), code=i % 2) + ulp_l4 = ICMP(type=8 - 8*icmp_type_delta, code=icmp_code) ulp = ulp_l4 p = (Ether(dst=dst_mac, src=src_mac) / IP(src=src_ip4, dst=dst_ip4) / @@ -264,7 +277,9 @@ class TestIpIrb(VppTestCase): new_rule_permit_and_reflect['is_permit'] = 2 else: new_rule_permit_and_reflect['is_permit'] = is_permit + permit_and_reflect_rules.append(new_rule_permit_and_reflect) + self.logger.info("create_stream pkt#%d: %s" % (i, payload)) return {'stream': pkts, 'rules': rules, @@ -452,20 +467,22 @@ class TestIpIrb(VppTestCase): def apply_acl_ip46_both_directions_reflect(self, primary_is_bridged_to_routed, - reflect_on_l2, is_ip6, add_eh): + reflect_on_l2, is_ip6, add_eh, + stateful_icmp): primary_is_routed_to_bridged = not primary_is_bridged_to_routed self.reset_packet_infos() stream_dict_fwd = self.create_stream(self.pg2, self.loop0, primary_is_bridged_to_routed, self.pg_if_packet_sizes, is_ip6, - False, False, add_eh) + False, False, add_eh, + stateful_icmp) acl_idx_fwd = self.create_acls_for_a_stream(stream_dict_fwd, reflect_on_l2, True) stream_dict_rev = self.create_stream(self.pg2, self.loop0, not primary_is_bridged_to_routed, self.pg_if_packet_sizes, is_ip6, - True, True, add_eh) + True, True, add_eh, stateful_icmp) # We want the primary action to be "deny" rather than reflect acl_idx_rev = self.create_acls_for_a_stream(stream_dict_rev, reflect_on_l2, False) @@ -517,13 +534,14 @@ class TestIpIrb(VppTestCase): def run_traffic_ip46_x_to_y(self, bridged_to_routed, test_l2_deny, is_ip6, - is_reflect, is_established, add_eh): + is_reflect, is_established, add_eh, + stateful_icmp=False): self.reset_packet_infos() stream_dict = self.create_stream(self.pg2, self.loop0, bridged_to_routed, self.pg_if_packet_sizes, is_ip6, not is_reflect, is_established, - add_eh) + add_eh, stateful_icmp) stream = stream_dict['stream'] tx_if = self.pg0 if bridged_to_routed else self.pg2 @@ -537,14 +555,18 @@ class TestIpIrb(VppTestCase): self.verify_capture(self.loop0, self.pg2, rcvd1, bridged_to_routed) def run_traffic_ip46_routed_to_bridged(self, test_l2_deny, is_ip6, - is_reflect, is_established, add_eh): + is_reflect, is_established, add_eh, + stateful_icmp=False): self.run_traffic_ip46_x_to_y(False, test_l2_deny, is_ip6, - is_reflect, is_established, add_eh) + is_reflect, is_established, add_eh, + stateful_icmp) def run_traffic_ip46_bridged_to_routed(self, test_l2_deny, is_ip6, - is_reflect, is_established, add_eh): + is_reflect, is_established, add_eh, + stateful_icmp=False): self.run_traffic_ip46_x_to_y(True, test_l2_deny, is_ip6, - is_reflect, is_established, add_eh) + is_reflect, is_established, add_eh, + stateful_icmp) def run_test_ip46_routed_to_bridged(self, test_l2_deny, is_ip6, is_reflect, add_eh): @@ -561,22 +583,30 @@ class TestIpIrb(VppTestCase): is_reflect, False, add_eh) def run_test_ip46_routed_to_bridged_and_back(self, test_l2_action, - is_ip6, add_eh): + is_ip6, add_eh, + stateful_icmp=False): self.apply_acl_ip46_both_directions_reflect(False, test_l2_action, - is_ip6, add_eh) + is_ip6, add_eh, + stateful_icmp) self.run_traffic_ip46_routed_to_bridged(test_l2_action, is_ip6, - True, False, add_eh) + True, False, add_eh, + stateful_icmp) self.run_traffic_ip46_bridged_to_routed(test_l2_action, is_ip6, - False, True, add_eh) + False, True, add_eh, + stateful_icmp) def run_test_ip46_bridged_to_routed_and_back(self, test_l2_action, - is_ip6, add_eh): + is_ip6, add_eh, + stateful_icmp=False): self.apply_acl_ip46_both_directions_reflect(True, test_l2_action, - is_ip6, add_eh) + is_ip6, add_eh, + stateful_icmp) self.run_traffic_ip46_bridged_to_routed(test_l2_action, is_ip6, - True, False, add_eh) + True, False, add_eh, + stateful_icmp) self.run_traffic_ip46_routed_to_bridged(test_l2_action, is_ip6, - False, True, add_eh) + False, True, add_eh, + stateful_icmp) def test_0000_ip6_irb_1(self): """ ACL plugin prepare""" @@ -761,6 +791,56 @@ class TestIpIrb(VppTestCase): """ ACL IPv4+MF bridged -> routed, L3 ACL permit+reflect""" self.run_test_ip46_bridged_to_routed_and_back(False, False, self.WITH_EH) + # Stateful ACL tests with stateful ICMP + + def test_1401_ip6_irb_1(self): + """ IPv6 routed -> bridged, L2 ACL permit+reflect, ICMP reflect""" + self.run_test_ip46_routed_to_bridged_and_back(True, True, + self.WITHOUT_EH, + self.STATEFUL_ICMP) + + def test_1402_ip6_irb_1(self): + """ IPv6 bridged -> routed, L2 ACL permit+reflect, ICMP reflect""" + self.run_test_ip46_bridged_to_routed_and_back(True, True, + self.WITHOUT_EH, + self.STATEFUL_ICMP) + + def test_1403_ip4_irb_1(self): + """ IPv4 routed -> bridged, L2 ACL permit+reflect, ICMP reflect""" + self.run_test_ip46_routed_to_bridged_and_back(True, False, + self.WITHOUT_EH, + self.STATEFUL_ICMP) + + def test_1404_ip4_irb_1(self): + """ IPv4 bridged -> routed, L2 ACL permit+reflect, ICMP reflect""" + self.run_test_ip46_bridged_to_routed_and_back(True, False, + self.WITHOUT_EH, + self.STATEFUL_ICMP) + + def test_1411_ip6_irb_1(self): + """ IPv6 routed -> bridged, L3 ACL permit+reflect, ICMP reflect""" + self.run_test_ip46_routed_to_bridged_and_back(False, True, + self.WITHOUT_EH, + self.STATEFUL_ICMP) + + def test_1412_ip6_irb_1(self): + """ IPv6 bridged -> routed, L3 ACL permit+reflect, ICMP reflect""" + self.run_test_ip46_bridged_to_routed_and_back(False, True, + self.WITHOUT_EH, + self.STATEFUL_ICMP) + + def test_1413_ip4_irb_1(self): + """ IPv4 routed -> bridged, L3 ACL permit+reflect, ICMP reflect""" + self.run_test_ip46_routed_to_bridged_and_back(False, False, + self.WITHOUT_EH, + self.STATEFUL_ICMP) + + def test_1414_ip4_irb_1(self): + """ IPv4 bridged -> routed, L3 ACL permit+reflect, ICMP reflect""" + self.run_test_ip46_bridged_to_routed_and_back(False, False, + self.WITHOUT_EH, + self.STATEFUL_ICMP) + if __name__ == '__main__': unittest.main(testRunner=VppTestRunner) -- 2.16.6