From patchwork Sat Dec 15 02:16:53 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 1013799 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 43Grdx5HmQz9s3Z for ; Sat, 15 Dec 2018 13:17:09 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 5C11CC7D; Sat, 15 Dec 2018 02:17:06 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 74A109E2 for ; Sat, 15 Dec 2018 02:17:04 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net [217.70.183.195]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id AD901E7 for ; Sat, 15 Dec 2018 02:17:02 +0000 (UTC) X-Originating-IP: 75.54.222.30 Received: from sigabrt.attlocal.net (75-54-222-30.lightspeed.rdcyca.sbcglobal.net [75.54.222.30]) (Authenticated sender: blp@ovn.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 3E71E60005; Sat, 15 Dec 2018 02:16:58 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Fri, 14 Dec 2018 18:16:53 -0800 Message-Id: <20181215021655.29228-1-blp@ovn.org> X-Mailer: git-send-email 2.16.1 X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff Subject: [ovs-dev] [PATCH 1/3] Fix bugs in L3 protocol support. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org Test 854 "tunnel_push_pop - action" showed problems in revalidation for L3 protocol support in its L3 GRE test. L3 packets (that is, packets without an Ethernet header but only some L3 protocol such as IPv4 or IPv6) have an Ethernet type that is kept in the dl_type member of the flow, and the flows that they pass through can cause L3 and L4 fields to be matched. However, the translation process incorrectly forced the dl_type to be wildcarded, which caused a contradiction since it's not possible to match on L3 and L4 fields if the dl_type is not known, and the code in odp_flow_key_to_flow() and related functions therefore rejected these flows at revalidation time. This commit fixes the problem by treating dl_type the same for L2 and L3 flows in translation. It also makes odp_flow_key_to_flow__() copy the Ethernet type that comes from a packet_type field into dl_type, which is the expected behavior. The actual error that this fixes is only visible after applying an upcoming commit that improves logging for bad datapath flows. Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- lib/odp-util.c | 3 +++ ofproto/ofproto-dpif-xlate.c | 12 ++++++------ tests/nsh.at | 14 +++++++------- tests/packet-type-aware.at | 8 ++++---- 4 files changed, 20 insertions(+), 17 deletions(-) diff --git a/lib/odp-util.c b/lib/odp-util.c index 1e8c5f194793..6726869729e7 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -6774,6 +6774,9 @@ odp_flow_key_to_flow__(const struct nlattr *key, size_t key_len, flow->packet_type = nl_attr_get_be32(attrs[OVS_KEY_ATTR_PACKET_TYPE]); expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_PACKET_TYPE; + if (pt_ns(src_flow->packet_type) == OFPHTN_ETHERTYPE) { + flow->dl_type = pt_ns_type_be(flow->packet_type); + } } else if (!is_mask) { flow->packet_type = htonl(PT_ETH); } diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 839fddd99fbe..8c13d45d385b 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -7039,9 +7039,7 @@ xlate_wc_init(struct xlate_ctx *ctx) /* Some fields we consider to always be examined. */ WC_MASK_FIELD(ctx->wc, packet_type); WC_MASK_FIELD(ctx->wc, in_port); - if (is_ethernet(&ctx->xin->flow, NULL)) { - WC_MASK_FIELD(ctx->wc, dl_type); - } + WC_MASK_FIELD(ctx->wc, dl_type); if (is_ip_any(&ctx->xin->flow)) { WC_MASK_FIELD_MASK(ctx->wc, nw_frag, FLOW_NW_FRAG_MASK); } @@ -7068,12 +7066,14 @@ xlate_wc_finish(struct xlate_ctx *ctx) * use non-header fields as part of the cache. */ flow_wildcards_clear_non_packet_fields(ctx->wc); - /* Wildcard ethernet fields if the original packet type was not - * Ethernet. */ + /* Wildcard Ethernet address fields if the original packet type was not + * Ethernet. + * + * (The Ethertype field is used even when the original packet type is not + * Ethernet.) */ if (ctx->xin->upcall_flow->packet_type != htonl(PT_ETH)) { ctx->wc->masks.dl_dst = eth_addr_zero; ctx->wc->masks.dl_src = eth_addr_zero; - ctx->wc->masks.dl_type = 0; } /* ICMPv4 and ICMPv6 have 8-bit "type" and "code" fields. struct flow diff --git a/tests/nsh.at b/tests/nsh.at index 7729d1bf21d1..87f74c5f7ee7 100644 --- a/tests/nsh.at +++ b/tests/nsh.at @@ -374,7 +374,7 @@ bridge("br0") decap() Final flow: recirc_id=0x1,eth,in_port=4,vlan_tci=0x0000,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,dl_type=0x0000 -Megaflow: recirc_id=0x1,packet_type=(1,0x894f),in_port=4,nsh_mdtype=1,nsh_np=3,nsh_spi=0x1234,nsh_c1=0x11223344 +Megaflow: recirc_id=0x1,packet_type=(1,0x894f),in_port=4,dl_type=0x894f,nsh_mdtype=1,nsh_np=3,nsh_spi=0x1234,nsh_c1=0x11223344 Datapath actions: pop_nsh(),recirc(0x2) ]) @@ -413,7 +413,7 @@ AT_CHECK([ ovs-appctl dpctl/dump-flows dummy@ovs-dummy | strip_used | grep -v ipv6 | sort ], [0], [flow-dump from non-dpdk interfaces: recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth(dst=1e:2c:e9:2a:66:9e),eth_type(0x0800),ipv4(frag=no), packets:1, bytes:98, used:0.0s, actions:push_nsh(flags=0,ttl=63,mdtype=1,np=3,spi=0x1234,si=255,c1=0x11223344,c2=0x0,c3=0x0,c4=0x0),push_nsh(flags=0,ttl=63,mdtype=1,np=4,spi=0x5678,si=255,c1=0x55667788,c2=0x0,c3=0x0,c4=0x0),push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55:66),pop_eth,pop_nsh(),recirc(0x3) -recirc_id(0x3),in_port(1),packet_type(ns=1,id=0x894f),nsh(mdtype=1,np=3,spi=0x1234,c1=0x11223344), packets:1, bytes:122, used:0.0s, actions:pop_nsh(),recirc(0x4) +recirc_id(0x3),in_port(1),packet_type(ns=1,id=0x894f),eth_type(0x894f),nsh(mdtype=1,np=3,spi=0x1234,c1=0x11223344), packets:1, bytes:122, used:0.0s, actions:pop_nsh(),recirc(0x4) recirc_id(0x4),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:1, bytes:98, used:0.0s, actions:2 ]) @@ -725,8 +725,8 @@ AT_CHECK([ ovs-appctl dpctl/dump-flows dummy@ovs-dummy | strip_used | grep -v ipv6 | sort ], [0], [flow-dump from non-dpdk interfaces: recirc_id(0),in_port(4),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=192.168.10.30,frag=no), packets:1, bytes:98, used:0.0s, actions:pop_eth,push_nsh(flags=0,ttl=63,mdtype=1,np=1,spi=0x3000,si=255,c1=0x0,c2=0x0,c3=0x0,c4=0x0),clone(tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=aa:55:00:00:00:03,src=aa:55:00:00:00:01,dl_type=0x0800),ipv4(src=10.0.0.1,dst=10.0.0.3,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0xc000004,vni=0x0)),out_port(1)),set(ipv4(src=30.0.0.1,dst=30.0.0.3)),tnl_pop(4789)) -tunnel(tun_id=0x0,src=30.0.0.1,dst=30.0.0.3,flags(-df-csum+key)),recirc_id(0),in_port(4789),packet_type(ns=1,id=0x894f),nsh(np=1,spi=0x3000,si=255), packets:1, bytes:108, used:0.0s, actions:pop_nsh(),recirc(0x1) -tunnel(tun_id=0x0,src=30.0.0.1,dst=30.0.0.3,flags(-df-csum+key)),recirc_id(0x1),in_port(4789),packet_type(ns=1,id=0x800),ipv4(frag=no), packets:1, bytes:84, used:0.0s, actions:push_eth(src=00:00:00:00:00:00,dst=aa:55:aa:55:00:03),6 +tunnel(tun_id=0x0,src=30.0.0.1,dst=30.0.0.3,flags(-df-csum+key)),recirc_id(0),in_port(4789),packet_type(ns=1,id=0x894f),eth_type(0x894f),nsh(np=1,spi=0x3000,si=255), packets:1, bytes:108, used:0.0s, actions:pop_nsh(),recirc(0x1) +tunnel(tun_id=0x0,src=30.0.0.1,dst=30.0.0.3,flags(-df-csum+key)),recirc_id(0x1),in_port(4789),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4(frag=no), packets:1, bytes:84, used:0.0s, actions:push_eth(src=00:00:00:00:00:00,dst=aa:55:aa:55:00:03),6 ]) AT_CHECK([ @@ -779,9 +779,9 @@ AT_CHECK([ ovs-appctl dpctl/dump-flows dummy@ovs-dummy | strip_used | grep -v ipv6 | sort ], [0], [flow-dump from non-dpdk interfaces: recirc_id(0),in_port(4),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=192.168.10.20/255.255.255.248,frag=no), packets:1, bytes:98, used:0.0s, actions:pop_eth,push_nsh(flags=0,ttl=63,mdtype=1,np=1,spi=0x3020,si=255,c1=0x0,c2=0x0,c3=0x0,c4=0x0),clone(tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=aa:55:00:00:00:02,src=aa:55:00:00:00:01,dl_type=0x0800),ipv4(src=10.0.0.1,dst=10.0.0.2,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0xc000004,vni=0x0)),out_port(1)),set(ipv4(src=20.0.0.1,dst=20.0.0.2)),tnl_pop(4789)) -tunnel(tun_id=0x0,src=20.0.0.1,dst=20.0.0.2,flags(-df-csum+key)),recirc_id(0),in_port(4789),packet_type(ns=1,id=0x894f),nsh(spi=0x3020,si=255), packets:1, bytes:108, used:0.0s, actions:push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55:66),set(nsh(spi=0x3020,si=254)),pop_eth,clone(tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=aa:55:00:00:00:03,src=aa:55:00:00:00:02,dl_type=0x0800),ipv4(src=20.0.0.2,dst=20.0.0.3,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0xc000004,vni=0x0)),out_port(2)),set(ipv4(src=30.0.0.2,dst=30.0.0.3)),tnl_pop(4789)) -tunnel(tun_id=0x0,src=30.0.0.2,dst=30.0.0.3,flags(-df-csum+key)),recirc_id(0),in_port(4789),packet_type(ns=1,id=0x894f),nsh(np=1,spi=0x3020,si=254), packets:1, bytes:108, used:0.0s, actions:pop_nsh(),recirc(0x2) -tunnel(tun_id=0x0,src=30.0.0.2,dst=30.0.0.3,flags(-df-csum+key)),recirc_id(0x2),in_port(4789),packet_type(ns=1,id=0x800),ipv4(frag=no), packets:1, bytes:84, used:0.0s, actions:push_eth(src=00:00:00:00:00:00,dst=aa:55:aa:55:00:03),6 +tunnel(tun_id=0x0,src=20.0.0.1,dst=20.0.0.2,flags(-df-csum+key)),recirc_id(0),in_port(4789),packet_type(ns=1,id=0x894f),eth_type(0x894f),nsh(spi=0x3020,si=255), packets:1, bytes:108, used:0.0s, actions:push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55:66),set(nsh(spi=0x3020,si=254)),pop_eth,clone(tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=aa:55:00:00:00:03,src=aa:55:00:00:00:02,dl_type=0x0800),ipv4(src=20.0.0.2,dst=20.0.0.3,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0xc000004,vni=0x0)),out_port(2)),set(ipv4(src=30.0.0.2,dst=30.0.0.3)),tnl_pop(4789)) +tunnel(tun_id=0x0,src=30.0.0.2,dst=30.0.0.3,flags(-df-csum+key)),recirc_id(0),in_port(4789),packet_type(ns=1,id=0x894f),eth_type(0x894f),nsh(np=1,spi=0x3020,si=254), packets:1, bytes:108, used:0.0s, actions:pop_nsh(),recirc(0x2) +tunnel(tun_id=0x0,src=30.0.0.2,dst=30.0.0.3,flags(-df-csum+key)),recirc_id(0x2),in_port(4789),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4(frag=no), packets:1, bytes:84, used:0.0s, actions:push_eth(src=00:00:00:00:00:00,dst=aa:55:aa:55:00:03),6 ]) AT_CHECK([ diff --git a/tests/packet-type-aware.at b/tests/packet-type-aware.at index bfb47b421dbb..e169709a906b 100644 --- a/tests/packet-type-aware.at +++ b/tests/packet-type-aware.at @@ -400,8 +400,8 @@ AT_CHECK([ ovs-appctl dpctl/dump-flows --names dummy@ovs-dummy | strip_used | grep -v ipv6 | sort ], [0], [flow-dump from non-dpdk interfaces: recirc_id(0),in_port(n3),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=192.168.10.10,tos=0/0x3,frag=no), packets:1, bytes:98, used:0.0s, actions:pop_eth,clone(tnl_push(tnl_port(gre_sys),header(size=38,type=3,eth(dst=aa:55:00:00:00:02,src=aa:55:00:00:00:03,dl_type=0x0800),ipv4(src=30.0.0.3,dst=30.0.0.2,proto=47,tos=0,ttl=64,frag=0x4000),gre((flags=0x0,proto=0x800))),out_port(br-p3)),set(ipv4(src=20.0.0.3,dst=20.0.0.2)),tnl_pop(gre_sys)) -tunnel(src=10.0.0.2,dst=10.0.0.1,flags(-df-csum)),recirc_id(0),in_port(gre_sys),packet_type(ns=1,id=0x800),ipv4(dst=192.168.10.10,frag=no), packets:1, bytes:84, used:0.0s, actions:push_eth(src=00:00:00:00:00:00,dst=aa:55:aa:55:00:01),n1 -tunnel(src=20.0.0.3,dst=20.0.0.2,flags(-df-csum)),recirc_id(0),in_port(gre_sys),packet_type(ns=1,id=0x800),ipv4(dst=192.168.10.10,tos=0/0x3,frag=no), packets:1, bytes:84, used:0.0s, actions:clone(tnl_push(tnl_port(gre_sys),header(size=38,type=3,eth(dst=aa:55:00:00:00:01,src=aa:55:00:00:00:02,dl_type=0x0800),ipv4(src=20.0.0.2,dst=20.0.0.1,proto=47,tos=0,ttl=64,frag=0x4000),gre((flags=0x0,proto=0x800))),out_port(br-p2)),set(ipv4(src=10.0.0.2,dst=10.0.0.1)),tnl_pop(gre_sys)) +tunnel(src=10.0.0.2,dst=10.0.0.1,flags(-df-csum)),recirc_id(0),in_port(gre_sys),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4(dst=192.168.10.10,frag=no), packets:1, bytes:84, used:0.0s, actions:push_eth(src=00:00:00:00:00:00,dst=aa:55:aa:55:00:01),n1 +tunnel(src=20.0.0.3,dst=20.0.0.2,flags(-df-csum)),recirc_id(0),in_port(gre_sys),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4(dst=192.168.10.10,tos=0/0x3,frag=no), packets:1, bytes:84, used:0.0s, actions:clone(tnl_push(tnl_port(gre_sys),header(size=38,type=3,eth(dst=aa:55:00:00:00:01,src=aa:55:00:00:00:02,dl_type=0x0800),ipv4(src=20.0.0.2,dst=20.0.0.1,proto=47,tos=0,ttl=64,frag=0x4000),gre((flags=0x0,proto=0x800))),out_port(br-p2)),set(ipv4(src=10.0.0.2,dst=10.0.0.1)),tnl_pop(gre_sys)) ]) # Clear up megaflow cache @@ -505,7 +505,7 @@ AT_CHECK([ ovs-appctl dpctl/dump-flows --names dummy@ovs-dummy | strip_used | grep -v ipv6 | sort ], [0], [flow-dump from non-dpdk interfaces: recirc_id(0),in_port(n3),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=192.168.10.20,tos=0/0x3,frag=no), packets:1, bytes:98, used:0.0s, actions:pop_eth,clone(tnl_push(tnl_port(gre_sys),header(size=38,type=3,eth(dst=aa:55:00:00:00:02,src=aa:55:00:00:00:03,dl_type=0x0800),ipv4(src=30.0.0.3,dst=30.0.0.2,proto=47,tos=0,ttl=64,frag=0x4000),gre((flags=0x0,proto=0x800))),out_port(br-p3)),set(ipv4(src=20.0.0.3,dst=20.0.0.2)),tnl_pop(gre_sys)) -tunnel(src=20.0.0.3,dst=20.0.0.2,flags(-df-csum)),recirc_id(0),in_port(gre_sys),packet_type(ns=1,id=0x800),ipv4(dst=192.168.10.20,frag=no), packets:1, bytes:84, used:0.0s, actions:drop +tunnel(src=20.0.0.3,dst=20.0.0.2,flags(-df-csum)),recirc_id(0),in_port(gre_sys),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4(dst=192.168.10.20,frag=no), packets:1, bytes:84, used:0.0s, actions:drop ]) OVS_VSWITCHD_STOP(["/The Open vSwitch kernel module is probably not loaded/d"]) @@ -1020,7 +1020,7 @@ AT_CHECK([ ovs-appctl dpctl/dump-flows --names dummy@ovs-dummy | strip_used | grep -v ipv6 | sort ], [0], [flow-dump from non-dpdk interfaces: recirc_id(0),in_port(p0),packet_type(ns=0,id=0),eth(src=aa:bb:cc:00:00:02,dst=aa:bb:cc:00:00:01),eth_type(0x0800),ipv4(dst=20.0.0.1,proto=47,frag=no), packets:3, bytes:378, used:0.0s, actions:tnl_pop(gre_sys) -tunnel(src=20.0.0.2,dst=20.0.0.1,flags(-df-csum)),recirc_id(0),in_port(gre_sys),packet_type(ns=1,id=0x8847),mpls(label=999/0x0,tc=0/0,ttl=64/0x0,bos=1/1), packets:3, bytes:264, used:0.0s, actions:push_eth(src=00:00:00:00:00:00,dst=00:00:00:00:00:00),pop_mpls(eth_type=0x800),recirc(0x1) +tunnel(src=20.0.0.2,dst=20.0.0.1,flags(-df-csum)),recirc_id(0),in_port(gre_sys),packet_type(ns=1,id=0x8847),eth_type(0x8847),mpls(label=999/0x0,tc=0/0,ttl=64/0x0,bos=1/1), packets:3, bytes:264, used:0.0s, actions:push_eth(src=00:00:00:00:00:00,dst=00:00:00:00:00:00),pop_mpls(eth_type=0x800),recirc(0x1) tunnel(src=20.0.0.2,dst=20.0.0.1,flags(-df-csum)),recirc_id(0x1),in_port(gre_sys),packet_type(ns=0,id=0),eth(dst=00:00:00:00:00:00),eth_type(0x0800),ipv4(ttl=64,frag=no), packets:3, bytes:294, used:0.0s, actions:set(ipv4(ttl=63)),int-br ]) From patchwork Sat Dec 15 02:16:54 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 1013800 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 43Grff52HLz9s47 for ; Sat, 15 Dec 2018 13:17:46 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 0F751ACD; Sat, 15 Dec 2018 02:17:07 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id ABC4D9E2 for ; Sat, 15 Dec 2018 02:17:04 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net [217.70.183.195]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 2C7A1177 for ; Sat, 15 Dec 2018 02:17:03 +0000 (UTC) X-Originating-IP: 75.54.222.30 Received: from sigabrt.attlocal.net (75-54-222-30.lightspeed.rdcyca.sbcglobal.net [75.54.222.30]) (Authenticated sender: blp@ovn.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id AFBB060006; Sat, 15 Dec 2018 02:17:01 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Fri, 14 Dec 2018 18:16:54 -0800 Message-Id: <20181215021655.29228-2-blp@ovn.org> X-Mailer: git-send-email 2.16.1 In-Reply-To: <20181215021655.29228-1-blp@ovn.org> References: <20181215021655.29228-1-blp@ovn.org> X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff Subject: [ovs-dev] [PATCH 2/3] odp-util: Avoid revalidation error for masked NSH set action. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org A masked NSH set action has mdtype 0 because the mdtype is not being changed, but odp_nsh_key_from_attr() rejects this because mdtype 0 does not match up with the OVS_NSH_KEY_ATTR_MD1 attribute being present. This fixes the problem. The kernel datapath in flow_netlink function nsh_key_put_from_nlattr() has a similar exception. Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- lib/odp-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/odp-util.c b/lib/odp-util.c index 6726869729e7..a3d0ab9362c1 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -2712,7 +2712,7 @@ odp_nsh_key_from_attr(const struct nlattr *attr, struct ovs_key_nsh *nsh, return ODP_FIT_TOO_MUCH; } - if (has_md1 && nsh->mdtype != NSH_M_TYPE1) { + if (has_md1 && nsh->mdtype != NSH_M_TYPE1 && !nsh_mask) { return ODP_FIT_ERROR; } From patchwork Sat Dec 15 02:16:55 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 1013801 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 43GrgQ1Tpkz9s3Z for ; Sat, 15 Dec 2018 13:18:26 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id C5EE4E9E; Sat, 15 Dec 2018 02:17:10 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 82D38D33 for ; Sat, 15 Dec 2018 02:17:09 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net [217.70.183.195]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 3DCDBE7 for ; Sat, 15 Dec 2018 02:17:06 +0000 (UTC) X-Originating-IP: 75.54.222.30 Received: from sigabrt.attlocal.net (75-54-222-30.lightspeed.rdcyca.sbcglobal.net [75.54.222.30]) (Authenticated sender: blp@ovn.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 356C660008; Sat, 15 Dec 2018 02:17:02 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Fri, 14 Dec 2018 18:16:55 -0800 Message-Id: <20181215021655.29228-3-blp@ovn.org> X-Mailer: git-send-email 2.16.1 In-Reply-To: <20181215021655.29228-1-blp@ovn.org> References: <20181215021655.29228-1-blp@ovn.org> X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff Subject: [ovs-dev] [PATCH 3/3] odp-util: Improve log messages and error reporting for Netlink parsing. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- lib/dpctl.c | 28 +++-- lib/dpif-netdev.c | 4 +- lib/netdev-dummy.c | 15 ++- lib/odp-execute.c | 6 +- lib/odp-util.c | 255 +++++++++++++++++++++++++++++++----------- lib/odp-util.h | 15 +-- ofproto/ofproto-dpif-sflow.c | 2 +- ofproto/ofproto-dpif-trace.c | 22 ++-- ofproto/ofproto-dpif-upcall.c | 15 ++- ofproto/ofproto-dpif-xlate.c | 25 ++++- ofproto/ofproto-dpif-xlate.h | 3 +- ofproto/ofproto-dpif.c | 6 +- tests/odp.at | 2 +- tests/ofproto-dpif.at | 2 +- tests/test-odp.c | 23 ++-- tests/tunnel.at | 4 +- 16 files changed, 294 insertions(+), 133 deletions(-) diff --git a/lib/dpctl.c b/lib/dpctl.c index 59071cdba83d..1632fad89a57 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -1022,8 +1022,8 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p) struct match match, match_filter; struct minimatch minimatch; - odp_flow_key_to_flow(f.key, f.key_len, &flow); - odp_flow_key_to_mask(f.mask, f.mask_len, &wc, &flow); + odp_flow_key_to_flow(f.key, f.key_len, &flow, NULL); + odp_flow_key_to_mask(f.mask, f.mask_len, &wc, &flow, NULL); match_init(&match, &flow, &wc); match_init(&match_filter, &flow_filter, &wc); @@ -1111,10 +1111,12 @@ dpctl_put_flow(int argc, const char *argv[], enum dpif_flow_put_flags flags, ofpbuf_init(&key, 0); ofpbuf_init(&mask, 0); - error = odp_flow_from_string(key_s, &port_names, &key, &mask); + char *error_s; + error = odp_flow_from_string(key_s, &port_names, &key, &mask, &error_s); simap_destroy(&port_names); if (error) { - dpctl_error(dpctl_p, error, "parsing flow key"); + dpctl_error(dpctl_p, error, "parsing flow key (%s)", error_s); + free(error_s); goto out_freekeymask; } @@ -1263,9 +1265,11 @@ dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p) ofpbuf_init(&key, 0); ofpbuf_init(&mask, 0); - error = odp_flow_from_string(key_s, &port_names, &key, &mask); + char *error_s; + error = odp_flow_from_string(key_s, &port_names, &key, &mask, &error_s); if (error) { - dpctl_error(dpctl_p, error, "parsing flow key"); + dpctl_error(dpctl_p, error, "%s", error_s); + free(error_s); goto out; } @@ -2076,9 +2080,11 @@ dpctl_normalize_actions(int argc, const char *argv[], /* Parse flow key. */ ofpbuf_init(&keybuf, 0); - error = odp_flow_from_string(argv[1], &port_names, &keybuf, NULL); + char *error_s; + error = odp_flow_from_string(argv[1], &port_names, &keybuf, NULL, + &error_s); if (error) { - dpctl_error(dpctl_p, error, "odp_flow_key_from_string"); + dpctl_error(dpctl_p, error, "odp_flow_key_from_string (%s)", error_s); goto out_freekeybuf; } @@ -2087,9 +2093,11 @@ dpctl_normalize_actions(int argc, const char *argv[], &s, dpctl_p->verbosity); dpctl_print(dpctl_p, "input flow: %s\n", ds_cstr(&s)); - error = odp_flow_key_to_flow(keybuf.data, keybuf.size, &flow); + error = odp_flow_key_to_flow(keybuf.data, keybuf.size, &flow, &error_s); if (error) { - dpctl_error(dpctl_p, error, "odp_flow_key_to_flow"); + dpctl_error(dpctl_p, error, "odp_flow_key_to_flow failed (%s)", + error_s ? error_s : "reason unknown"); + free(error_s); goto out_freekeybuf; } diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 1564db9c6e44..d94ac7951fe0 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -3048,7 +3048,7 @@ dpif_netdev_mask_from_nlattrs(const struct nlattr *key, uint32_t key_len, { enum odp_key_fitness fitness; - fitness = odp_flow_key_to_mask(mask_key, mask_key_len, wc, flow); + fitness = odp_flow_key_to_mask(mask_key, mask_key_len, wc, flow, NULL); if (fitness) { if (!probe) { /* This should not happen: it indicates that @@ -3079,7 +3079,7 @@ static int dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len, struct flow *flow, bool probe) { - if (odp_flow_key_to_flow(key, key_len, flow)) { + if (odp_flow_key_to_flow(key, key_len, flow, NULL)) { if (!probe) { /* This should not happen: it indicates that * odp_flow_key_from_flow() and odp_flow_key_to_flow() disagree on diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 72b4f7adc62f..78aa34957a20 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c @@ -1462,8 +1462,10 @@ eth_from_packet(const char *s) } static struct dp_packet * -eth_from_flow(const char *s, size_t packet_size) +eth_from_flow(const char *s, size_t packet_size, char **errorp) { + *errorp = NULL; + enum odp_key_fitness fitness; struct dp_packet *packet; struct ofpbuf odp_key; @@ -1477,14 +1479,14 @@ eth_from_flow(const char *s, size_t packet_size) * settle for parsing a datapath key for now. */ ofpbuf_init(&odp_key, 0); - error = odp_flow_from_string(s, NULL, &odp_key, NULL); + error = odp_flow_from_string(s, NULL, &odp_key, NULL, errorp); if (error) { ofpbuf_uninit(&odp_key); return NULL; } /* Convert odp_key to flow. */ - fitness = odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow); + fitness = odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow, errorp); if (fitness == ODP_FIT_ERROR) { ofpbuf_uninit(&odp_key); return NULL; @@ -1592,10 +1594,11 @@ netdev_dummy_receive(struct unixctl_conn *conn, i += 2; } /* Try parse 'argv[i]' as odp flow. */ - packet = eth_from_flow(flow_str, packet_size); - + char *error_s; + packet = eth_from_flow(flow_str, packet_size, &error_s); if (!packet) { - unixctl_command_reply_error(conn, "bad packet or flow syntax"); + unixctl_command_reply_error(conn, error_s); + free(error_s); goto exit; } } diff --git a/lib/odp-execute.c b/lib/odp-execute.c index 3b6890e952c2..ddd041e82522 100644 --- a/lib/odp-execute.c +++ b/lib/odp-execute.c @@ -198,7 +198,7 @@ odp_set_sctp(struct dp_packet *packet, const struct ovs_key_sctp *key, static void odp_set_tunnel_action(const struct nlattr *a, struct flow_tnl *tun_key) { - ovs_assert(odp_tun_key_from_attr(a, tun_key) != ODP_FIT_ERROR); + ovs_assert(odp_tun_key_from_attr(a, tun_key, NULL) != ODP_FIT_ERROR); } static void @@ -280,9 +280,9 @@ odp_set_nsh(struct dp_packet *packet, const struct nlattr *a, bool has_mask) ovs_be32 path_hdr; if (has_mask) { - odp_nsh_key_from_attr(a, &key, &mask); + odp_nsh_key_from_attr(a, &key, &mask, NULL); } else { - odp_nsh_key_from_attr(a, &key, NULL); + odp_nsh_key_from_attr(a, &key, NULL, NULL); } if (!has_mask) { diff --git a/lib/odp-util.c b/lib/odp-util.c index a3d0ab9362c1..2f4e8b4b418d 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -2641,10 +2641,25 @@ odp_nsh_hdr_from_attr(const struct nlattr *attr, return ODP_FIT_PERFECT; } +#define ODP_PARSE_ERROR(RL, ERRORP, ...) \ + do { \ + if (OVS_UNLIKELY(ERRORP)) { \ + free(*(ERRORP)); \ + *(ERRORP) = xasprintf(__VA_ARGS__); \ + } else { \ + VLOG_WARN_RL(RL, __VA_ARGS__); \ + } \ + } while (0) + enum odp_key_fitness odp_nsh_key_from_attr(const struct nlattr *attr, struct ovs_key_nsh *nsh, - struct ovs_key_nsh *nsh_mask) + struct ovs_key_nsh *nsh_mask, char **errorp) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + if (errorp) { + *errorp = NULL; + } + unsigned int left; const struct nlattr *a; bool unknown = false; @@ -2655,17 +2670,18 @@ odp_nsh_key_from_attr(const struct nlattr *attr, struct ovs_key_nsh *nsh, size_t len = nl_attr_get_size(a); int expected_len = odp_key_attr_len(ovs_nsh_key_attr_lens, OVS_NSH_KEY_ATTR_MAX, type); - - /* the attribute can have mask, len is 2 * expected_len for that case. - */ - if ((len != expected_len) && (len != 2 * expected_len) && - (expected_len >= 0)) { - return ODP_FIT_ERROR; - } - - if ((nsh_mask && (expected_len >= 0) && (len != 2 * expected_len)) || - (!nsh_mask && (expected_len >= 0) && (len == 2 * expected_len))) { - return ODP_FIT_ERROR; + if (expected_len) { + if (nsh_mask) { + expected_len *= 2; + } + if (len != expected_len) { + ODP_PARSE_ERROR(&rl, errorp, "NSH %s attribute %"PRIu16" " + "should have length %d but actually has " + "%"PRIuSIZE, + nsh_mask ? "mask" : "key", + type, expected_len, len); + return ODP_FIT_ERROR; + } } switch (type) { @@ -2713,6 +2729,9 @@ odp_nsh_key_from_attr(const struct nlattr *attr, struct ovs_key_nsh *nsh, } if (has_md1 && nsh->mdtype != NSH_M_TYPE1 && !nsh_mask) { + ODP_PARSE_ERROR(&rl, errorp, "OVS_NSH_KEY_ATTR_MD1 present but " + "declared mdtype %"PRIu8" is not %d (NSH_M_TYPE1)", + nsh->mdtype, NSH_M_TYPE1); return ODP_FIT_ERROR; } @@ -2721,8 +2740,9 @@ odp_nsh_key_from_attr(const struct nlattr *attr, struct ovs_key_nsh *nsh, static enum odp_key_fitness odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask, - struct flow_tnl *tun) + struct flow_tnl *tun, char **errorp) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); unsigned int left; const struct nlattr *a; bool ttl = false; @@ -2735,6 +2755,9 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask, OVS_TUNNEL_ATTR_MAX, type); if (len != expected_len && expected_len >= 0) { + ODP_PARSE_ERROR(&rl, errorp, "tunnel key attribute %"PRIu16" " + "should have length %d but actually has %"PRIuSIZE, + type, expected_len, len); return ODP_FIT_ERROR; } @@ -2784,6 +2807,7 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask, struct nlattr *ext[ARRAY_SIZE(vxlan_opts_policy)]; if (!nl_parse_nested(a, vxlan_opts_policy, ext, ARRAY_SIZE(ext))) { + ODP_PARSE_ERROR(&rl, errorp, "error parsing VXLAN options"); return ODP_FIT_ERROR; } @@ -2823,6 +2847,7 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask, } if (!ttl) { + ODP_PARSE_ERROR(&rl, errorp, "tunnel options missing TTL"); return ODP_FIT_ERROR; } if (unknown) { @@ -2832,10 +2857,14 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask, } enum odp_key_fitness -odp_tun_key_from_attr(const struct nlattr *attr, struct flow_tnl *tun) +odp_tun_key_from_attr(const struct nlattr *attr, struct flow_tnl *tun, + char **errorp) { + if (errorp) { + *errorp = NULL; + } memset(tun, 0, sizeof *tun); - return odp_tun_key_from_attr__(attr, false, tun); + return odp_tun_key_from_attr__(attr, false, tun, errorp); } static void @@ -5631,8 +5660,11 @@ parse_odp_key_mask_attr(struct parse_odp_context *context, const char *s, * have duplicated keys. odp_flow_key_to_flow() will detect those errors. */ int odp_flow_from_string(const char *s, const struct simap *port_names, - struct ofpbuf *key, struct ofpbuf *mask) + struct ofpbuf *key, struct ofpbuf *mask, + char **errorp) { + *errorp = NULL; + const size_t old_size = key->size; struct parse_odp_context context = (struct parse_odp_context) { .port_names = port_names, @@ -5649,6 +5681,7 @@ odp_flow_from_string(const char *s, const struct simap *port_names, ovs_u128 ufid; retval = odp_ufid_from_string(s, &ufid); if (retval < 0) { + *errorp = xasprintf("syntax error at %s", s); key->size = old_size; return -retval; } else if (retval > 0) { @@ -5658,6 +5691,7 @@ odp_flow_from_string(const char *s, const struct simap *port_names, retval = parse_odp_key_mask_attr(&context, s, key, mask); if (retval < 0) { + *errorp = xasprintf("syntax error at %s", s); key->size = old_size; return -retval; } @@ -6091,7 +6125,7 @@ odp_key_to_dp_packet(const struct nlattr *key, size_t key_len, case OVS_KEY_ATTR_TUNNEL: { enum odp_key_fitness res; - res = odp_tun_key_from_attr(nla, &md->tunnel); + res = odp_tun_key_from_attr(nla, &md->tunnel, NULL); if (res == ODP_FIT_ERROR) { memset(&md->tunnel, 0, sizeof md->tunnel); } @@ -6200,7 +6234,7 @@ odp_to_ovs_frag(uint8_t odp_frag, bool is_mask) static bool parse_flow_nlattrs(const struct nlattr *key, size_t key_len, const struct nlattr *attrs[], uint64_t *present_attrsp, - int *out_of_range_attrp) + int *out_of_range_attrp, char **errorp) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(10, 10); const struct nlattr *nla; @@ -6219,10 +6253,11 @@ parse_flow_nlattrs(const struct nlattr *key, size_t key_len, if (len != expected_len && expected_len >= 0) { char namebuf[OVS_KEY_ATTR_BUFSIZE]; - VLOG_ERR_RL(&rl, "attribute %s has length %"PRIuSIZE" but should have " - "length %d", ovs_key_attr_to_string(type, namebuf, - sizeof namebuf), - len, expected_len); + ODP_PARSE_ERROR(&rl, errorp, "attribute %s has length %"PRIuSIZE" " + "but should have length %d", + ovs_key_attr_to_string(type, namebuf, + sizeof namebuf), + len, expected_len); return false; } @@ -6232,9 +6267,10 @@ parse_flow_nlattrs(const struct nlattr *key, size_t key_len, if (present_attrs & (UINT64_C(1) << type)) { char namebuf[OVS_KEY_ATTR_BUFSIZE]; - VLOG_ERR_RL(&rl, "duplicate %s attribute in flow key", - ovs_key_attr_to_string(type, - namebuf, sizeof namebuf)); + ODP_PARSE_ERROR(&rl, errorp, + "duplicate %s attribute in flow key", + ovs_key_attr_to_string(type, namebuf, + sizeof namebuf)); return false; } @@ -6243,7 +6279,7 @@ parse_flow_nlattrs(const struct nlattr *key, size_t key_len, } } if (left) { - VLOG_ERR_RL(&rl, "trailing garbage in flow key"); + ODP_PARSE_ERROR(&rl, errorp, "trailing garbage in flow key"); return false; } @@ -6281,7 +6317,8 @@ check_expectations(uint64_t present_attrs, int out_of_range_attr, static bool parse_ethertype(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], uint64_t present_attrs, uint64_t *expected_attrs, - struct flow *flow, const struct flow *src_flow) + struct flow *flow, const struct flow *src_flow, + char **errorp) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); bool is_mask = flow != src_flow; @@ -6289,12 +6326,16 @@ parse_ethertype(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE)) { flow->dl_type = nl_attr_get_be16(attrs[OVS_KEY_ATTR_ETHERTYPE]); if (!is_mask && ntohs(flow->dl_type) < ETH_TYPE_MIN) { - VLOG_ERR_RL(&rl, "invalid Ethertype %"PRIu16" in flow key", - ntohs(flow->dl_type)); + ODP_PARSE_ERROR(&rl, errorp, + "invalid Ethertype %"PRIu16" in flow key", + ntohs(flow->dl_type)); return false; } if (is_mask && ntohs(src_flow->dl_type) < ETH_TYPE_MIN && flow->dl_type != htons(0xffff)) { + ODP_PARSE_ERROR(&rl, errorp, "can't bitwise match non-Ethernet II " + "\"Ethertype\" %#"PRIx16" (with mask %#"PRIx16")", + ntohs(src_flow->dl_type), ntohs(flow->dl_type)); return false; } *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE; @@ -6315,7 +6356,8 @@ parse_ethertype(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], flow->dl_type = htons(0xffff); } else if (ntohs(src_flow->dl_type) < ETH_TYPE_MIN) { /* See comments in odp_flow_key_from_flow__(). */ - VLOG_ERR_RL(&rl, "mask expected for non-Ethernet II frame"); + ODP_PARSE_ERROR(&rl, errorp, + "mask expected for non-Ethernet II frame"); return false; } } @@ -6327,7 +6369,7 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], uint64_t present_attrs, int out_of_range_attr, uint64_t expected_attrs, struct flow *flow, const struct nlattr *key, size_t key_len, - const struct flow *src_flow) + const struct flow *src_flow, char **errorp) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); bool is_mask = src_flow != flow; @@ -6346,9 +6388,15 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], int i; if (!size || size % sizeof(ovs_be32)) { + ODP_PARSE_ERROR(&rl, errorp, + "MPLS LSEs have invalid length %"PRIuSIZE, + size); return ODP_FIT_ERROR; } if (flow->mpls_lse[0] && flow->dl_type != htons(0xffff)) { + ODP_PARSE_ERROR(&rl, errorp, + "unexpected MPLS Ethertype mask %x"PRIx16, + ntohs(flow->dl_type)); return ODP_FIT_ERROR; } @@ -6363,6 +6411,8 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], /* BOS may be set only in the innermost label. */ for (i = 0; i < n - 1; i++) { if (flow->mpls_lse[i] & htonl(MPLS_BOS_MASK)) { + ODP_PARSE_ERROR(&rl, errorp, + "MPLS BOS set in non-innermost label"); return ODP_FIT_ERROR; } } @@ -6386,8 +6436,11 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], ipv4_key = nl_attr_get(attrs[OVS_KEY_ATTR_IPV4]); put_ipv4_key(ipv4_key, flow, is_mask); if (flow->nw_frag > FLOW_NW_FRAG_MASK) { + ODP_PARSE_ERROR(&rl, errorp, "OVS_KEY_ATTR_IPV4 has invalid " + "nw_frag %#"PRIx8, flow->nw_frag); return ODP_FIT_ERROR; } + if (is_mask) { check_start = ipv4_key; check_len = sizeof *ipv4_key; @@ -6404,6 +6457,8 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], ipv6_key = nl_attr_get(attrs[OVS_KEY_ATTR_IPV6]); put_ipv6_key(ipv6_key, flow, is_mask); if (flow->nw_frag > FLOW_NW_FRAG_MASK) { + ODP_PARSE_ERROR(&rl, errorp, "OVS_KEY_ATTR_IPV6 has invalid " + "nw_frag %#"PRIx8, flow->nw_frag); return ODP_FIT_ERROR; } if (is_mask) { @@ -6422,8 +6477,9 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], arp_key = nl_attr_get(attrs[OVS_KEY_ATTR_ARP]); if (!is_mask && (arp_key->arp_op & htons(0xff00))) { - VLOG_ERR_RL(&rl, "unsupported ARP opcode %"PRIu16" in flow " - "key", ntohs(arp_key->arp_op)); + ODP_PARSE_ERROR(&rl, errorp, + "unsupported ARP opcode %"PRIu16" in flow " + "key", ntohs(arp_key->arp_op)); return ODP_FIT_ERROR; } put_arp_key(arp_key, flow); @@ -6438,7 +6494,10 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_NSH; } if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_NSH)) { - odp_nsh_key_from_attr(attrs[OVS_KEY_ATTR_NSH], &flow->nsh, NULL); + if (odp_nsh_key_from_attr(attrs[OVS_KEY_ATTR_NSH], &flow->nsh, + NULL, errorp) == ODP_FIT_ERROR) { + return ODP_FIT_ERROR; + } if (is_mask) { check_start = nl_attr_get(attrs[OVS_KEY_ATTR_NSH]); check_len = nl_attr_get_size(attrs[OVS_KEY_ATTR_NSH]); @@ -6451,6 +6510,10 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], if (check_len > 0) { /* Happens only when 'is_mask'. */ if (!is_all_zeros(check_start, check_len) && flow->dl_type != htons(0xffff)) { + ODP_PARSE_ERROR(&rl, errorp, "unexpected L3 matching with " + "masked Ethertype %#"PRIx16"/%#"PRIx16, + ntohs(src_flow->dl_type), + ntohs(flow->dl_type)); return ODP_FIT_ERROR; } else { expected_attrs |= UINT64_C(1) << expected_bit; @@ -6551,6 +6614,12 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], if (!is_all_zeros(nd_key, sizeof *nd_key) && (flow->tp_src != htons(0xff) || flow->tp_dst != 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)); return ODP_FIT_ERROR; } else { expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ND; @@ -6567,6 +6636,8 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], } if (is_mask && expected_bit != OVS_KEY_ATTR_UNSPEC) { if ((flow->tp_src || flow->tp_dst) && flow->nw_proto != 0xff) { + ODP_PARSE_ERROR(&rl, errorp, "flow matches on L4 ports but does " + "not define an L4 protocol"); return ODP_FIT_ERROR; } else { expected_attrs |= UINT64_C(1) << expected_bit; @@ -6584,7 +6655,7 @@ parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], uint64_t present_attrs, int out_of_range_attr, uint64_t expected_attrs, struct flow *flow, const struct nlattr *key, size_t key_len, - const struct flow *src_flow) + const struct flow *src_flow, char **errorp) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); bool is_mask = src_flow != flow; @@ -6636,9 +6707,9 @@ parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], } return fitness; } else if (!(flow->vlans[encaps].tci & htons(VLAN_CFI))) { - VLOG_ERR_RL(&rl, "OVS_KEY_ATTR_VLAN 0x%04"PRIx16" is nonzero " - "but CFI bit is not set", - ntohs(flow->vlans[encaps].tci)); + ODP_PARSE_ERROR( + &rl, errorp, "OVS_KEY_ATTR_VLAN 0x%04"PRIx16" is nonzero " + "but CFI bit is not set", ntohs(flow->vlans[encaps].tci)); return ODP_FIT_ERROR; } } else { @@ -6649,13 +6720,14 @@ parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], /* Now parse the encapsulated attributes. */ if (!parse_flow_nlattrs(nl_attr_get(encap), nl_attr_get_size(encap), - attrs, &present_attrs, &out_of_range_attr)) { + attrs, &present_attrs, &out_of_range_attr, + errorp)) { return ODP_FIT_ERROR; } expected_attrs = 0; if (!parse_ethertype(attrs, present_attrs, &expected_attrs, - flow, src_flow)) { + flow, src_flow, errorp)) { return ODP_FIT_ERROR; } @@ -6664,7 +6736,7 @@ parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], encap_fitness = parse_l2_5_onward(attrs, present_attrs, out_of_range_attr, expected_attrs, flow, key, key_len, - src_flow); + src_flow, errorp); /* The overall fitness is the worse of the outer and inner attributes. */ return MAX(fitness, encap_fitness); @@ -6672,8 +6744,14 @@ parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], static enum odp_key_fitness odp_flow_key_to_flow__(const struct nlattr *key, size_t key_len, - struct flow *flow, const struct flow *src_flow) + struct flow *flow, const struct flow *src_flow, + char **errorp) { + enum odp_key_fitness fitness = ODP_FIT_ERROR; + if (errorp) { + *errorp = NULL; + } + const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1]; uint64_t expected_attrs; uint64_t present_attrs; @@ -6684,8 +6762,8 @@ odp_flow_key_to_flow__(const struct nlattr *key, size_t key_len, /* Parse attributes. */ if (!parse_flow_nlattrs(key, key_len, attrs, &present_attrs, - &out_of_range_attr)) { - return ODP_FIT_ERROR; + &out_of_range_attr, errorp)) { + goto exit; } expected_attrs = 0; @@ -6754,9 +6832,9 @@ odp_flow_key_to_flow__(const struct nlattr *key, size_t key_len, enum odp_key_fitness res; res = odp_tun_key_from_attr__(attrs[OVS_KEY_ATTR_TUNNEL], is_mask, - &flow->tunnel); + &flow->tunnel, errorp); if (res == ODP_FIT_ERROR) { - return ODP_FIT_ERROR; + goto exit; } else if (res == ODP_FIT_PERFECT) { expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_TUNNEL; } @@ -6803,27 +6881,55 @@ odp_flow_key_to_flow__(const struct nlattr *key, size_t key_len, /* Get Ethertype or 802.1Q TPID or FLOW_DL_TYPE_NONE. */ if (!parse_ethertype(attrs, present_attrs, &expected_attrs, flow, - src_flow)) { - return ODP_FIT_ERROR; + src_flow, errorp)) { + goto exit; } if (is_mask ? (src_flow->vlans[0].tci & htons(VLAN_CFI)) != 0 : eth_type_vlan(src_flow->dl_type)) { - return parse_8021q_onward(attrs, present_attrs, out_of_range_attr, - expected_attrs, flow, key, key_len, src_flow); + fitness = parse_8021q_onward(attrs, present_attrs, out_of_range_attr, + expected_attrs, flow, key, key_len, + src_flow, errorp); + } else { + if (is_mask) { + /* A missing VLAN mask means exact match on vlan_tci 0 (== no + * VLAN). */ + flow->vlans[0].tpid = htons(0xffff); + flow->vlans[0].tci = htons(0xffff); + if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_VLAN)) { + flow->vlans[0].tci = nl_attr_get_be16(attrs[OVS_KEY_ATTR_VLAN]); + expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_VLAN); + } + } + fitness = parse_l2_5_onward(attrs, present_attrs, out_of_range_attr, + expected_attrs, flow, key, key_len, + src_flow, errorp); } - if (is_mask) { - /* A missing VLAN mask means exact match on vlan_tci 0 (== no VLAN). */ - flow->vlans[0].tpid = htons(0xffff); - flow->vlans[0].tci = htons(0xffff); - if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_VLAN)) { - flow->vlans[0].tci = nl_attr_get_be16(attrs[OVS_KEY_ATTR_VLAN]); - expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_VLAN); + +exit:; + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + if (fitness == ODP_FIT_ERROR && (errorp || !VLOG_DROP_WARN(&rl))) { + struct ds s = DS_EMPTY_INITIALIZER; + if (is_mask) { + ds_put_cstr(&s, "the flow mask in error is: "); + odp_flow_key_format(key, key_len, &s); + ds_put_cstr(&s, ", for the following flow key: "); + flow_format(&s, src_flow, NULL); + } else { + ds_put_cstr(&s, "the flow key in error is: "); + odp_flow_key_format(key, key_len, &s); + } + if (errorp) { + char *old_error = *errorp; + *errorp = xasprintf("%s; %s", old_error, ds_cstr(&s)); + free(old_error); + } else { + VLOG_WARN("%s", ds_cstr(&s)); } + ds_destroy(&s); } - return parse_l2_5_onward(attrs, present_attrs, out_of_range_attr, - expected_attrs, flow, key, key_len, src_flow); + return fitness; } /* Converts the 'key_len' bytes of OVS_KEY_ATTR_* attributes in 'key' to a flow @@ -6840,28 +6946,40 @@ odp_flow_key_to_flow__(const struct nlattr *key, size_t key_len, * by looking at the attributes for lower-level protocols, e.g. if the network * protocol in OVS_KEY_ATTR_IPV4 or OVS_KEY_ATTR_IPV6 is IPPROTO_TCP then we * know that a OVS_KEY_ATTR_TCP attribute must appear and that otherwise it - * must be absent. */ + * must be absent. + * + * If 'errorp' is nonull, this function stores a detailed error report in it, + * which the caller must eventually free(), when it returns ODP_FIT_ERROR, When + * it returns anything else, it sets '*errorp' to NULL. */ enum odp_key_fitness odp_flow_key_to_flow(const struct nlattr *key, size_t key_len, - struct flow *flow) + struct flow *flow, char **errorp) { - return odp_flow_key_to_flow__(key, key_len, flow, flow); + return odp_flow_key_to_flow__(key, key_len, flow, flow, errorp); } /* Converts the 'mask_key_len' bytes of OVS_KEY_ATTR_* attributes in 'mask_key' * to a mask structure in 'mask'. 'flow' must be a previously translated flow * corresponding to 'mask' and similarly flow_key/flow_key_len must be the * attributes from that flow. Returns an ODP_FIT_* value that indicates how - * well 'key' fits our expectations for what a flow key should contain. */ + * well 'key' fits our expectations for what a flow key should contain. + * + * If 'errorp' is nonull, this function stores a detailed error report in it, + * which the caller must eventually free(), when it returns ODP_FIT_ERROR, When + * it returns anything else, it sets '*errorp' to NULL. */ enum odp_key_fitness odp_flow_key_to_mask(const struct nlattr *mask_key, size_t mask_key_len, - struct flow_wildcards *mask, const struct flow *src_flow) + struct flow_wildcards *mask, const struct flow *src_flow, + char **errorp) { if (mask_key_len) { return odp_flow_key_to_flow__(mask_key, mask_key_len, - &mask->masks, src_flow); - + &mask->masks, src_flow, errorp); } else { + if (errorp) { + *errorp = NULL; + } + /* A missing mask means that the flow should be exact matched. * Generate an appropriate exact wildcard for the flow. */ flow_wildcards_init_for_packet(mask, src_flow); @@ -6880,7 +6998,7 @@ parse_key_and_mask_to_match(const struct nlattr *key, size_t key_len, { enum odp_key_fitness fitness; - fitness = odp_flow_key_to_flow(key, key_len, &match->flow); + fitness = odp_flow_key_to_flow(key, key_len, &match->flow, NULL); if (fitness) { /* This should not happen: it indicates that * odp_flow_key_from_flow() and odp_flow_key_to_flow() disagree on @@ -6900,7 +7018,8 @@ parse_key_and_mask_to_match(const struct nlattr *key, size_t key_len, return EINVAL; } - fitness = odp_flow_key_to_mask(mask, mask_len, &match->wc, &match->flow); + fitness = odp_flow_key_to_mask(mask, mask_len, &match->wc, &match->flow, + NULL); if (fitness) { /* This should not happen: it indicates that * odp_flow_key_from_mask() and odp_flow_key_to_mask() diff --git a/lib/odp-util.h b/lib/odp-util.h index 6e684f8e6d8b..722ddfabc40d 100644 --- a/lib/odp-util.h +++ b/lib/odp-util.h @@ -157,10 +157,11 @@ struct odputil_keybuf { }; enum odp_key_fitness odp_tun_key_from_attr(const struct nlattr *, - struct flow_tnl *); + struct flow_tnl *, char **errorp); enum odp_key_fitness odp_nsh_key_from_attr(const struct nlattr *, struct ovs_key_nsh *, - struct ovs_key_nsh *); + struct ovs_key_nsh *, + char **errorp); enum odp_key_fitness odp_nsh_hdr_from_attr(const struct nlattr *, struct nsh_hdr *, size_t); @@ -172,9 +173,8 @@ void odp_flow_format(const struct nlattr *key, size_t key_len, const struct hmap *portno_names, struct ds *, bool verbose); void odp_flow_key_format(const struct nlattr *, size_t, struct ds *); -int odp_flow_from_string(const char *s, - const struct simap *port_names, - struct ofpbuf *, struct ofpbuf *); +int odp_flow_from_string(const char *s, const struct simap *port_names, + struct ofpbuf *, struct ofpbuf *, char **errorp); /* ODP_SUPPORT_FIELD(TYPE, FIELD_NAME, FIELD_DESCRIPTION) * @@ -261,11 +261,12 @@ enum odp_key_fitness { ODP_FIT_ERROR, /* The key was invalid. */ }; enum odp_key_fitness odp_flow_key_to_flow(const struct nlattr *, size_t, - struct flow *); + struct flow *, char **errorp); enum odp_key_fitness odp_flow_key_to_mask(const struct nlattr *mask_key, size_t mask_key_len, struct flow_wildcards *mask, - const struct flow *flow); + const struct flow *flow, + char **errorp); int parse_key_and_mask_to_match(const struct nlattr *key, size_t key_len, const struct nlattr *mask, size_t mask_len, struct match *match); diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c index 7da31753c758..c2d2d10ef65c 100644 --- a/ofproto/ofproto-dpif-sflow.c +++ b/ofproto/ofproto-dpif-sflow.c @@ -988,7 +988,7 @@ sflow_read_set_action(const struct nlattr *attr, /* Do not handle multi-encap for now. */ sflow_actions->tunnel_err = true; } else { - if (odp_tun_key_from_attr(attr, &sflow_actions->tunnel) + if (odp_tun_key_from_attr(attr, &sflow_actions->tunnel, NULL) == ODP_FIT_ERROR) { /* Tunnel parsing error. */ sflow_actions->tunnel_err = true; diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c index eca61cee250d..5bdb91741e37 100644 --- a/ofproto/ofproto-dpif-trace.c +++ b/ofproto/ofproto-dpif-trace.c @@ -347,22 +347,22 @@ parse_flow_and_packet(int argc, const char *argv[], * bridge is specified. If function odp_flow_key_from_string() * returns 0, the flow is a odp_flow. If function * parse_ofp_exact_flow() returns NULL, the flow is a br_flow. */ + char *error_s; if (!odp_flow_from_string(args[n_args - 1], &port_names, - &odp_key, &odp_mask)) { + &odp_key, &odp_mask, &error_s)) { if (!backer) { error = "Cannot find the datapath"; goto exit; } - if (odp_flow_key_to_flow(odp_key.data, odp_key.size, flow) == ODP_FIT_ERROR) { - error = "Failed to parse datapath flow key"; + if (odp_flow_key_to_flow(odp_key.data, odp_key.size, flow, &m_err) + == ODP_FIT_ERROR) { goto exit; } *ofprotop = xlate_lookup_ofproto(backer, flow, - &flow->in_port.ofp_port); + &flow->in_port.ofp_port, &m_err); if (*ofprotop == NULL) { - error = "Invalid datapath flow"; goto exit; } @@ -385,13 +385,15 @@ parse_flow_and_packet(int argc, const char *argv[], goto exit; } } + } else if (n_args != 2) { + m_err = xasprintf("%s (or the bridge name was omitted)", error_s); + free(error_s); + goto exit; } else { - char *err; + free(m_err); + m_err = NULL; - if (n_args != 2) { - error = "Must specify bridge name"; - goto exit; - } + char *err; *ofprotop = ofproto_dpif_lookup_by_name(args[0]); if (!*ofprotop) { diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index dc3082477106..a19aa9576b5c 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -796,7 +796,7 @@ recv_upcalls(struct handler *handler) } upcall->fitness = odp_flow_key_to_flow(dupcall->key, dupcall->key_len, - flow); + flow, NULL); if (upcall->fitness == ODP_FIT_ERROR) { goto free_dupcall; } @@ -1437,7 +1437,8 @@ process_upcall(struct udpif *udpif, struct upcall *upcall, memset(&ipfix_actions, 0, sizeof ipfix_actions); if (upcall->out_tun_key) { - odp_tun_key_from_attr(upcall->out_tun_key, &output_tunnel_key); + odp_tun_key_from_attr(upcall->out_tun_key, &output_tunnel_key, + NULL); } actions_len = dpif_read_actions(udpif, upcall, flow, @@ -2093,7 +2094,7 @@ xlate_key(struct udpif *udpif, const struct nlattr *key, unsigned int len, struct xlate_in xin; int error; - fitness = odp_flow_key_to_flow(key, len, &ctx->flow); + fitness = odp_flow_key_to_flow(key, len, &ctx->flow, NULL); if (fitness == ODP_FIT_ERROR) { return EINVAL; } @@ -2186,7 +2187,8 @@ revalidate_ukey__(struct udpif *udpif, const struct udpif_key *ukey, struct ofproto_dpif *ofproto; ofp_port_t ofp_in_port; - ofproto = xlate_lookup_ofproto(udpif->backer, &ctx.flow, &ofp_in_port); + ofproto = xlate_lookup_ofproto(udpif->backer, &ctx.flow, &ofp_in_port, + NULL); ofpbuf_clear(odp_actions); @@ -2199,7 +2201,8 @@ revalidate_ukey__(struct udpif *udpif, const struct udpif_key *ukey, ofproto->up.slowpath_meter_id, &ofproto->uuid); } - if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, &dp_mask, &ctx.flow) + if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, &dp_mask, &ctx.flow, + NULL) == ODP_FIT_ERROR) { goto exit; } @@ -2523,7 +2526,7 @@ ukey_to_flow_netdev(struct udpif *udpif, struct udpif_key *ukey) netdev_close(ukey->in_netdev); ukey->in_netdev = NULL; } - res = odp_tun_key_from_attr(k, &tnl); + res = odp_tun_key_from_attr(k, &tnl, NULL); if (res != ODP_FIT_ERROR) { ukey->in_netdev = flow_get_tunnel_netdev(&tnl); break; diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 8c13d45d385b..422b38a19bcc 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -1482,8 +1482,10 @@ xlate_ofport_remove(struct ofport_dpif *ofport) } static struct ofproto_dpif * -xlate_lookup_ofproto_(const struct dpif_backer *backer, const struct flow *flow, - ofp_port_t *ofp_in_port, const struct xport **xportp) +xlate_lookup_ofproto_(const struct dpif_backer *backer, + const struct flow *flow, + ofp_port_t *ofp_in_port, const struct xport **xportp, + char **errorp) { struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp); const struct xport *xport; @@ -1495,6 +1497,10 @@ xlate_lookup_ofproto_(const struct dpif_backer *backer, const struct flow *flow, recirc_id_node = recirc_id_node_find(flow->recirc_id); if (OVS_UNLIKELY(!recirc_id_node)) { + if (errorp) { + *errorp = xasprintf("no recirculation data for recirc_id " + "%"PRIu32, flow->recirc_id); + } return NULL; } @@ -1514,10 +1520,19 @@ xlate_lookup_ofproto_(const struct dpif_backer *backer, const struct flow *flow, ? tnl_port_receive(flow) : odp_port_to_ofport(backer, flow->in_port.odp_port)); if (OVS_UNLIKELY(!xport)) { + if (errorp) { + *errorp = (tnl_port_should_receive(flow) + ? xstrdup("no OpenFlow tunnel port for this packet") + : xasprintf("no OpenFlow tunnel port for datapath " + "port %"PRIu32, flow->in_port.odp_port)); + } return NULL; } out: + if (errorp) { + *errorp = NULL; + } *xportp = xport; if (ofp_in_port) { *ofp_in_port = xport->ofp_port; @@ -1529,11 +1544,11 @@ out: * returns the corresponding struct ofproto_dpif and OpenFlow port number. */ struct ofproto_dpif * xlate_lookup_ofproto(const struct dpif_backer *backer, const struct flow *flow, - ofp_port_t *ofp_in_port) + ofp_port_t *ofp_in_port, char **errorp) { const struct xport *xport; - return xlate_lookup_ofproto_(backer, flow, ofp_in_port, &xport); + return xlate_lookup_ofproto_(backer, flow, ofp_in_port, &xport, errorp); } /* Given a datapath and flow metadata ('backer', and 'flow' respectively), @@ -1554,7 +1569,7 @@ xlate_lookup(const struct dpif_backer *backer, const struct flow *flow, struct ofproto_dpif *ofproto; const struct xport *xport; - ofproto = xlate_lookup_ofproto_(backer, flow, ofp_in_port, &xport); + ofproto = xlate_lookup_ofproto_(backer, flow, ofp_in_port, &xport, NULL); if (!ofproto) { return ENODEV; diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h index 0a5a52887b80..5a51d7b303ca 100644 --- a/ofproto/ofproto-dpif-xlate.h +++ b/ofproto/ofproto-dpif-xlate.h @@ -199,7 +199,8 @@ void xlate_ofport_remove(struct ofport_dpif *); struct ofproto_dpif * xlate_lookup_ofproto(const struct dpif_backer *, const struct flow *, - ofp_port_t *ofp_in_port); + ofp_port_t *ofp_in_port, + char **errorp); int xlate_lookup(const struct dpif_backer *, const struct flow *, struct ofproto_dpif **, struct dpif_ipfix **, struct dpif_sflow **, struct netflow **, diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 2dc5778e1cb3..9c2947a3b9ec 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -5694,8 +5694,10 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn, while (dpif_flow_dump_next(flow_dump_thread, &f, 1)) { struct flow flow; - if (odp_flow_key_to_flow(f.key, f.key_len, &flow) == ODP_FIT_ERROR - || xlate_lookup_ofproto(ofproto->backer, &flow, NULL) != ofproto) { + if ((odp_flow_key_to_flow(f.key, f.key_len, &flow, NULL) + == ODP_FIT_ERROR) + || (xlate_lookup_ofproto(ofproto->backer, &flow, NULL, NULL) + != ofproto)) { continue; } diff --git a/tests/odp.at b/tests/odp.at index 96b4b47dc45a..f5bc064b1a5c 100644 --- a/tests/odp.at +++ b/tests/odp.at @@ -396,6 +396,6 @@ AT_DATA([odp-in.txt], [dnl encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap())))))))))))))))))))))))))))))))) ]) AT_CHECK_UNQUOTED([ovstest test-odp parse-keys < odp-in.txt], [0], [dnl -odp_flow_from_string: error +odp_flow_from_string: error (syntax error at encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap()))))))))))))))))))))))))))))))))) ]) AT_CLEANUP diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index ea51467cc154..61c42caee049 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -5735,7 +5735,7 @@ m4_foreach( [AT_CHECK([ovs-appctl ofproto/trace "$br_flow" option], [2], [], [stderr]) AT_CHECK([tail -2 stderr], [0], [dnl -Must specify bridge name +syntax error at in_port=1,dl_src=50:54:00:00:00:01,dl_dst=50:54:00:00:00:02 (or the bridge name was omitted) ovs-appctl: ovs-vswitchd: server returned an error ])]) diff --git a/tests/test-odp.c b/tests/test-odp.c index f61846422051..196d607aef85 100644 --- a/tests/test-odp.c +++ b/tests/test-odp.c @@ -46,10 +46,12 @@ parse_keys(bool wc_keys) /* Convert string to OVS DP key. */ ofpbuf_init(&odp_key, 0); ofpbuf_init(&odp_mask, 0); + char *error_s; error = odp_flow_from_string(ds_cstr(&in), NULL, - &odp_key, &odp_mask); + &odp_key, &odp_mask, &error_s); if (error) { - printf("odp_flow_from_string: error\n"); + printf("odp_flow_from_string: error (%s)\n", error_s); + free(error_s); goto next; } @@ -67,7 +69,8 @@ parse_keys(bool wc_keys) }; /* Convert odp_key to flow. */ - fitness = odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow); + fitness = odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow, + &error_s); switch (fitness) { case ODP_FIT_PERFECT: break; @@ -81,7 +84,8 @@ parse_keys(bool wc_keys) break; case ODP_FIT_ERROR: - printf("odp_flow_key_to_flow: error\n"); + printf("odp_flow_key_to_flow: error (%s)\n", error_s); + free(error_s); goto next; } /* Convert cls_rule back to odp_key. */ @@ -182,8 +186,10 @@ parse_filter(char *filter_parse) /* Convert string to OVS DP key. */ ofpbuf_init(&odp_key, 0); ofpbuf_init(&odp_mask, 0); - if (odp_flow_from_string(ds_cstr(&in), NULL, &odp_key, &odp_mask)) { - printf("odp_flow_from_string: error\n"); + char *error_s; + if (odp_flow_from_string(ds_cstr(&in), NULL, &odp_key, &odp_mask, + &error_s)) { + printf("odp_flow_from_string: error (%s)\n", error_s); goto next; } @@ -193,8 +199,9 @@ parse_filter(char *filter_parse) struct match match, match_filter; struct minimatch minimatch; - odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow); - odp_flow_key_to_mask(odp_mask.data, odp_mask.size, &wc, &flow); + odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow, NULL); + odp_flow_key_to_mask(odp_mask.data, odp_mask.size, &wc, &flow, + NULL); match_init(&match, &flow, &wc); match_init(&match_filter, &flow_filter, &wc); diff --git a/tests/tunnel.at b/tests/tunnel.at index 55fb1d391ea1..035c54f675ad 100644 --- a/tests/tunnel.at +++ b/tests/tunnel.at @@ -55,7 +55,7 @@ AT_CHECK([tail -1 stdout], [0], dnl nonexistent tunnel AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'tunnel(src=5.5.5.5,dst=6.6.6.6,ttl=64,flags()),in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9)'], [2], [ignore], [dnl -Invalid datapath flow +no OpenFlow tunnel port for this packet ovs-appctl: ovs-vswitchd: server returned an error ]) @@ -314,7 +314,7 @@ set(tunnel(tun_id=0x3,dst=1.1.1.1,ttl=64,flags(df|key))),1 ]) AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'tunnel(tun_id=0xf,src=1.1.1.1,dst=2.2.2.2,ttl=64,flags(key)),in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9)'], [2], [ignore], [dnl -Invalid datapath flow +no OpenFlow tunnel port for this packet ovs-appctl: ovs-vswitchd: server returned an error ]) OVS_VSWITCHD_STOP(["/receive tunnel port not found/d"])