Skip to content

Commit

Permalink
odp: ND: Follow Open Flow spec converting from OF to DP.
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@apconole @igsilya
apconole authored and igsilya committed Feb 8, 2024
1 parent 027ae2b commit 61003d0
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 18 deletions.
31 changes: 13 additions & 18 deletions lib/odp-util.c
View file
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
15 changes: 15 additions & 0 deletions tests/ofproto-macros.at
View file
Expand Up @@ -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.
Expand Down
51 changes: 51 additions & 0 deletions tests/system-traffic.at
View file
Expand Up @@ -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])
Expand Down

0 comments on commit 61003d0

Please sign in to comment.