diff mbox series

[ovs-dev] odp: ND: Follow Open Flow spec converting from OF to DP.

Message ID 20240208164940.949779-1-i.maximets@ovn.org
State Accepted
Commit 61003d0280625e15796f18d14ce1b5f46e560f3e
Headers show
Series [ovs-dev] odp: ND: Follow Open Flow spec converting from OF to DP. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Ilya Maximets Feb. 8, 2024, 4:49 p.m. UTC 
From: Aaron Conole <aconole@redhat.com>

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 <aconole@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---

The patch is already applied.  Posting for completeness.

 lib/odp-util.c          | 31 +++++++++++--------------
 tests/ofproto-macros.at | 15 ++++++++++++
 tests/system-traffic.at | 51 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 18 deletions(-)
diff mbox series

Patch

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 3eb2c3cb9..9306c9b4d 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 c870cf819..c22fb3c79 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(<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 f363a778c..62d00376c 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(<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])