From d3f9eab1abbb6a11c2a166472c184f54fd740bf1 Mon Sep 17 00:00:00 2001
From: Aaron Conole <[email protected]>
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 <[email protected]>
Signed-off-by: Ilya Maximets <[email protected]>
---
 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 5fc312f8c00..fa353eab5c8 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -6405,12 +6405,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);
@@ -7126,20 +7124,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 676d55aa956..fcffe210756 100644
--- a/tests/ofproto-macros.at
+++ b/tests/ofproto-macros.at
@@ -141,6 +141,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(<recirc_id>)' and
 # 'recirc=<recirc_id>' respectively.  This should make output easier to
 # compare.
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index cac60bb0a53..5d12fd41b4f 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2064,6 +2064,57 @@ recirc_id(<recirc>),in_port(3),eth_type(0x0800),ipv4(frag=no), packets:29, bytes
 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(<recirc>),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(<recirc>),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(<recirc>),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(<recirc>),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])