From 61003d0280625e15796f18d14ce1b5f46e560f3e Mon Sep 17 00:00:00 2001 From: Aaron Conole Date: Mon, 11 Dec 2023 12:04:19 -0500 Subject: [PATCH] odp: ND: Follow Open Flow spec converting from OF to DP. The OpenFlow spec doesn't require that a user specify icmp_code when specifying a type. However, the conversion for a DP flow asks that the user explicitly specified an icmp_code field to match and forces this via a mask check. This means that valid matches for icmp_type=136,... (for example) won't properly generate a full flow and there will be a much broader match installed in the kernel datapath. This can be worked around by explicitly including icmp_code, but for users that want to write flows which are installed in the kernel, it is not possible to omit icmp_code in the openflow message and still have a neighbor discovery match field included. An alternative way to fix up the flow and mask would be to modify the output of the translation in the xlate_wc_finish() to set the mask when detecting a neighbor discovery related packet. This would require additional matching logic in the xlate_wc_finish() path to validate the ICMP type/code details, and set the masks correctly. The approach taken here is to relax the requirements from the ODP side. This follows the OpenFlow specification and only require that the user include an 'icmp_type=' match, rather than both 'icmp_type=..,icmp_code=0' when matching on neighbor discovery. Signed-off-by: Aaron Conole Signed-off-by: Ilya Maximets --- lib/odp-util.c | 31 +++++++++++-------------- tests/ofproto-macros.at | 15 ++++++++++++ tests/system-traffic.at | 51 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 18 deletions(-) diff --git a/lib/odp-util.c b/lib/odp-util.c index 3eb2c3cb98c..9306c9b4d47 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -6464,12 +6464,10 @@ odp_flow_key_from_flow__(const struct odp_flow_key_parms *parms, icmpv6_key->icmpv6_code = ntohs(data->tp_dst); if (is_nd(flow, NULL) - /* Even though 'tp_src' and 'tp_dst' are 16 bits wide, ICMP - * type and code are 8 bits wide. Therefore, an exact match - * looks like htons(0xff), not htons(0xffff). See - * xlate_wc_finish() for details. */ - && (!export_mask || (data->tp_src == htons(0xff) - && data->tp_dst == htons(0xff)))) { + /* Even though 'tp_src' is 16 bits wide, ICMP type is 8 bits + * wide. Therefore, an exact match looks like htons(0xff), + * not htons(0xffff). See xlate_wc_finish() for details. */ + && (!export_mask || data->tp_src == htons(0xff))) { struct ovs_key_nd *nd_key; nd_key = nl_msg_put_unspec_uninit(buf, OVS_KEY_ATTR_ND, sizeof *nd_key); @@ -7185,20 +7183,17 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], flow->arp_sha = nd_key->nd_sll; flow->arp_tha = nd_key->nd_tll; if (is_mask) { - /* Even though 'tp_src' and 'tp_dst' are 16 bits wide, - * ICMP type and code are 8 bits wide. Therefore, an - * exact match looks like htons(0xff), not - * htons(0xffff). See xlate_wc_finish() for details. - * */ + /* Even though 'tp_src' is 16 bits wide, ICMP type + * is 8 bits wide. Therefore, an exact match looks + * like htons(0xff), not htons(0xffff). See + * xlate_wc_finish() for details. */ if (!is_all_zeros(nd_key, sizeof *nd_key) && - (flow->tp_src != htons(0xff) || - flow->tp_dst != htons(0xff))) { + flow->tp_src != htons(0xff)) { odp_parse_error(&rl, errorp, - "ICMP (src,dst) masks should be " - "(0xff,0xff) but are actually " - "(%#"PRIx16",%#"PRIx16")", - ntohs(flow->tp_src), - ntohs(flow->tp_dst)); + "ICMP src mask should be " + "(0xff) but is actually " + "(%#"PRIx16")", + ntohs(flow->tp_src)); return ODP_FIT_ERROR; } else { *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ND; diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at index c870cf8197c..c22fb3c79c3 100644 --- a/tests/ofproto-macros.at +++ b/tests/ofproto-macros.at @@ -146,6 +146,21 @@ strip_stats () { s/bytes:[[0-9]]*/bytes:0/' } +# Strips key32 field from output. +strip_key32 () { + sed 's/key32([[0-9 \/]]*),//' +} + +# Strips packet-type from output. +strip_ptype () { + sed 's/packet_type(ns=[[0-9]]*,id=[[0-9]]*),//' +} + +# Strips bare eth from output. +strip_eth () { + sed 's/eth(),//' +} + # Changes all 'recirc(...)' and 'recirc=...' to say 'recirc()' and # 'recirc=' respectively. This should make output easier to # compare. diff --git a/tests/system-traffic.at b/tests/system-traffic.at index f363a778cc7..62d00376c8c 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -2247,6 +2247,57 @@ AT_CHECK([diff -q payload.bin data_1], [0]) OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([datapath - Neighbor Discovery with loose match]) +OVS_TRAFFIC_VSWITCHD_START() + +ADD_NAMESPACES(at_ns0, at_ns1) + +ADD_VETH(p0, at_ns0, br0, "2001::1:0:392/64", 36:b1:ee:7c:01:03) +ADD_VETH(p1, at_ns1, br0, "2001::1:0:9/64", 36:b1:ee:7c:01:02) + +dnl Set up flows for moving icmp ND Solicit around. This should be the +dnl same for the other ND types. +AT_DATA([flows.txt], [dnl +table=0 priority=95 icmp6,icmp_type=136,nd_target=2001::1:0:9 actions=resubmit(,10) +table=0 priority=95 icmp6,icmp_type=136,nd_target=2001::1:0:392 actions=resubmit(,10) +table=0 priority=65 actions=resubmit(,20) +table=10 actions=NORMAL +table=20 actions=drop +]) +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt]) + +dnl Send a mismatching neighbor discovery. +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 36 b1 ee 7c 01 02 36 b1 ee 7c 01 03 86 dd 60 00 00 00 00 20 3a ff fe 80 00 00 00 00 00 00 f8 16 3e ff fe 04 66 04 fe 80 00 00 00 00 00 00 f8 16 3e ff fe a7 dd 0e 88 00 f1 f2 20 00 00 00 30 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 02 01 36 b1 ee 7c 01 03 > /dev/null]) + +dnl Send a matching neighbor discovery. +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 36 b1 ee 7c 01 02 36 b1 ee 7c 01 03 86 dd 60 00 00 00 00 20 3a ff fe 80 00 00 00 00 00 00 f8 16 3e ff fe 04 66 04 fe 80 00 00 00 00 00 00 f8 16 3e ff fe a7 dd 0e 88 00 fe 5f 20 00 00 00 20 01 00 00 00 00 00 00 00 00 00 01 00 00 03 92 02 01 36 b1 ee 7c 01 03 > /dev/null]) + +AT_CHECK([ovs-appctl dpctl/dump-flows | strip_stats | strip_used | dnl + strip_key32 | strip_ptype | strip_eth | strip_recirc | dnl + grep ",nd" | sort], [0], [dnl +recirc_id(),in_port(2),eth(src=36:b1:ee:7c:01:03,dst=36:b1:ee:7c:01:02),eth_type(0x86dd),ipv6(proto=58,frag=no),icmpv6(type=136),nd(target=2001::1:0:392), packets:0, bytes:0, used:never, actions:1,3 +recirc_id(),in_port(2),eth_type(0x86dd),ipv6(proto=58,frag=no),icmpv6(type=136),nd(target=3000::1), packets:0, bytes:0, used:never, actions:drop +]) + +OVS_WAIT_UNTIL([ovs-appctl dpctl/dump-flows | grep ",nd" | wc -l | grep -E ^0]) + +dnl Send a matching neighbor discovery. +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 36 b1 ee 7c 01 02 36 b1 ee 7c 01 03 86 dd 60 00 00 00 00 20 3a ff fe 80 00 00 00 00 00 00 f8 16 3e ff fe 04 66 04 fe 80 00 00 00 00 00 00 f8 16 3e ff fe a7 dd 0e 88 00 fe 5f 20 00 00 00 20 01 00 00 00 00 00 00 00 00 00 01 00 00 03 92 02 01 36 b1 ee 7c 01 03 > /dev/null]) + +dnl Send a mismatching neighbor discovery. +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 36 b1 ee 7c 01 02 36 b1 ee 7c 01 03 86 dd 60 00 00 00 00 20 3a ff fe 80 00 00 00 00 00 00 f8 16 3e ff fe 04 66 04 fe 80 00 00 00 00 00 00 f8 16 3e ff fe a7 dd 0e 88 00 f1 f2 20 00 00 00 30 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 02 01 36 b1 ee 7c 01 03 > /dev/null]) + +AT_CHECK([ovs-appctl dpctl/dump-flows | strip_stats | strip_used | dnl + strip_key32 | strip_ptype | strip_eth | strip_recirc | dnl + grep ",nd" | sort], [0], [dnl +recirc_id(),in_port(2),eth(src=36:b1:ee:7c:01:03,dst=36:b1:ee:7c:01:02),eth_type(0x86dd),ipv6(proto=58,frag=no),icmpv6(type=136),nd(target=2001::1:0:392), packets:0, bytes:0, used:never, actions:1,3 +recirc_id(),in_port(2),eth_type(0x86dd),ipv6(proto=58,frag=no),icmpv6(type=136),nd(target=3000::1), packets:0, bytes:0, used:never, actions:drop +]) + +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP + AT_BANNER([MPLS]) AT_SETUP([mpls - encap header dp-support])