From patchwork Mon Sep 28 00:09:04 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1372184 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.133; helo=hemlock.osuosl.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 hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4C02vR751Mz9sSC for ; Mon, 28 Sep 2020 10:09:35 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 77AF187082; Mon, 28 Sep 2020 00:09:32 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id izjD30gUDIfE; Mon, 28 Sep 2020 00:09:31 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id 3B74187072; Mon, 28 Sep 2020 00:09:31 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 0DDF3C0889; Mon, 28 Sep 2020 00:09:31 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id DBDC1C0051 for ; Mon, 28 Sep 2020 00:09:28 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id C470A84DFD for ; Mon, 28 Sep 2020 00:09:28 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id fFIVR2U-yB8A for ; Mon, 28 Sep 2020 00:09:27 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay10.mail.gandi.net (relay10.mail.gandi.net [217.70.178.230]) by whitealder.osuosl.org (Postfix) with ESMTPS id B31A884DE1 for ; Mon, 28 Sep 2020 00:09:26 +0000 (UTC) Received: from localhost.localdomain (unknown [73.241.94.255]) (Authenticated sender: hzhou@ovn.org) by relay10.mail.gandi.net (Postfix) with ESMTPSA id 0F51F240009; Mon, 28 Sep 2020 00:09:21 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Sun, 27 Sep 2020 17:09:04 -0700 Message-Id: <20200928000904.858685-1-hzhou@ovn.org> X-Mailer: git-send-email 2.26.2 MIME-Version: 1.0 Cc: Han Zhou Subject: [ovs-dev] [PATCH ovn] ovn-northd.c: Fix ACL priority related to tcp_reset action. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" When there "reject" is used as ACL action, there are logical flows generated to handle TCP and non-TCP packets separately, so that tcp_reset is used to reject TCP packets while ICMP is used to reject non-TCP packets. The current implementation uses priority OVN_ACL_PRI_OFFSET + acl_priroity + 10 for handling TCP packets, while OVN_ACL_PRI_OFFSET + acl_priroity for non-TCP packets. This can cause the ACL priorities defined by users incorrectly handled. For example, a user creates two ACLs: 1. priority: 1001, match: tcp.dst == 443, action: allow 2. priority: 1000, match: any, action: reject The generates lflows would be: 1. priority: 2010, match: any, action: tcp_reset 2. priority: 2001, match: tcp.dst == 443, action: allow 3. priority: 2000, match: any, action: icmp Now if a TCP packet with dst port 443 comes, it will be rejected. This patch fixes the problem by using OVN_ACL_PRI_OFFSET + acl_priority * 2 + 1 as flow priority for the tcp_reset flow, and OVN_ACL_PRI_OFFSET + acl_priority * 2 for other ACL related flows including the ICMP flows. Fixes: 366ac0d89 ("OVN: add tcp_reset action to ovn acl reject support") Signed-off-by: Han Zhou --- NEWS | 1 + northd/ovn-northd.c | 28 ++++++++++++++++++---------- ovn-nb.ovsschema | 6 +++--- ovn-nb.xml | 8 ++++---- tests/ovn.at | 37 +++++++++++++++++++++++++++++-------- 5 files changed, 55 insertions(+), 25 deletions(-) diff --git a/NEWS b/NEWS index ee5c2c393..90982b743 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,6 @@ Post-v20.09.0 --------------------- + - ACL priority range changed from 0 - 32767 to 0 - 30000. OVN v20.09.0 - xx xxx xxxx diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 3324c9e81..3649c0614 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -5362,6 +5362,14 @@ build_acl_log(struct ds *actions, const struct nbrec_acl *acl) ds_put_cstr(actions, "); "); } +static uint16_t +acl_priority_base(uint16_t acl_priority) +{ + /* acl_priority range is 0 - 30000, so the resulted priority + * is between 1000 and 61000. */ + return OVN_ACL_PRI_OFFSET + acl_priority * 2; +} + static void build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, enum ovn_stage stage, struct nbrec_acl *acl, @@ -5384,7 +5392,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, ingress ? "next(pipeline=egress,table=5);" : "next(pipeline=ingress,table=20);"); ovn_lflow_add_with_hint(lflows, od, stage, - acl->priority + OVN_ACL_PRI_OFFSET + 10, + acl_priority_base(acl->priority) + 1, ds_cstr(&match), ds_cstr(&actions), stage_hint); ds_clear(&match); ds_clear(&actions); @@ -5399,7 +5407,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, ingress ? "next(pipeline=egress,table=5);" : "next(pipeline=ingress,table=20);"); ovn_lflow_add_with_hint(lflows, od, stage, - acl->priority + OVN_ACL_PRI_OFFSET + 10, + acl_priority_base(acl->priority) + 1, ds_cstr(&match), ds_cstr(&actions), stage_hint); /* IP traffic */ @@ -5419,7 +5427,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, ingress ? "next(pipeline=egress,table=5);" : "next(pipeline=ingress,table=20);"); ovn_lflow_add_with_hint(lflows, od, stage, - acl->priority + OVN_ACL_PRI_OFFSET, + acl_priority_base(acl->priority), ds_cstr(&match), ds_cstr(&actions), stage_hint); ds_clear(&match); ds_clear(&actions); @@ -5437,7 +5445,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, ingress ? "next(pipeline=egress,table=5);" : "next(pipeline=ingress,table=20);"); ovn_lflow_add_with_hint(lflows, od, stage, - acl->priority + OVN_ACL_PRI_OFFSET, + acl_priority_base(acl->priority), ds_cstr(&match), ds_cstr(&actions), stage_hint); ds_destroy(&match); @@ -5463,7 +5471,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, build_acl_log(&actions, acl); ds_put_cstr(&actions, "next;"); ovn_lflow_add_with_hint(lflows, od, stage, - acl->priority + OVN_ACL_PRI_OFFSET, + acl_priority_base(acl->priority), acl->match, ds_cstr(&actions), &acl->header_); ds_destroy(&actions); @@ -5489,7 +5497,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, build_acl_log(&actions, acl); ds_put_cstr(&actions, "next;"); ovn_lflow_add_with_hint(lflows, od, stage, - acl->priority + OVN_ACL_PRI_OFFSET, + acl_priority_base(acl->priority), ds_cstr(&match), ds_cstr(&actions), &acl->header_); @@ -5508,7 +5516,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, build_acl_log(&actions, acl); ds_put_cstr(&actions, "next;"); ovn_lflow_add_with_hint(lflows, od, stage, - acl->priority + OVN_ACL_PRI_OFFSET, + acl_priority_base(acl->priority), ds_cstr(&match), ds_cstr(&actions), &acl->header_); @@ -5536,7 +5544,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, build_acl_log(&actions, acl); ds_put_cstr(&actions, "/* drop */"); ovn_lflow_add_with_hint(lflows, od, stage, - acl->priority + OVN_ACL_PRI_OFFSET, + acl_priority_base(acl->priority), ds_cstr(&match), ds_cstr(&actions), &acl->header_); } @@ -5563,7 +5571,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, build_acl_log(&actions, acl); ds_put_cstr(&actions, "/* drop */"); ovn_lflow_add_with_hint(lflows, od, stage, - acl->priority + OVN_ACL_PRI_OFFSET, + acl_priority_base(acl->priority), ds_cstr(&match), ds_cstr(&actions), &acl->header_); } @@ -5578,7 +5586,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, build_acl_log(&actions, acl); ds_put_cstr(&actions, "/* drop */"); ovn_lflow_add_with_hint(lflows, od, stage, - acl->priority + OVN_ACL_PRI_OFFSET, + acl_priority_base(acl->priority), acl->match, ds_cstr(&actions), &acl->header_); } diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema index 092322ab2..82d0933e6 100644 --- a/ovn-nb.ovsschema +++ b/ovn-nb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Northbound", - "version": "5.27.0", - "cksum": "3507518247 26773", + "version": "5.28.0", + "cksum": "3562758453 26773", "tables": { "NB_Global": { "columns": { @@ -211,7 +211,7 @@ "min": 0, "max": 1}}, "priority": {"type": {"key": {"type": "integer", "minInteger": 0, - "maxInteger": 32767}}}, + "maxInteger": 30000}}}, "direction": {"type": {"key": {"type": "string", "enum": ["set", ["from-lport", "to-lport"]]}}}, "match": {"type": "string"}, diff --git a/ovn-nb.xml b/ovn-nb.xml index 86195af34..07854de94 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -1646,10 +1646,10 @@

- The ACL rule's priority. Rules with numerically higher priority - take precedence over those with lower. If two ACL rules with - the same priority both match, then the one actually applied to a - packet is undefined. + The ACL rule's priority, an integer between 0 and 30000. Rules with + numerically higher priority take precedence over those with lower. If + two ACL rules with the same priority both match, then the one actually + applied to a packet is undefined.

diff --git a/tests/ovn.at b/tests/ovn.at index b6c8622ba..3ae1bbc85 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -12817,13 +12817,19 @@ test_ipv6_packet() { as hv$hv ovs-appctl netdev-dummy/receive vif$inport $packet } -# test_tcp_syn_packet INPORT HV ETH_SRC ETH_DST IPV4_SRC IPV4_DST IP_CHKSUM TCP_SPORT TCP_DPORT TCP_CHKSUM EXP_IP_CHKSUM EXP_TCP_RST_CHKSUM +# test_tcp_syn_packet INPORT HV ETH_SRC ETH_DST IPV4_SRC IPV4_DST IP_CHKSUM +# TCP_SPORT TCP_DPORT TCP_CHKSUM EXP_IP_CHKSUM EXP_TCP_RST_CHKSUM [OUTPORT] # -# Causes a packet to be received on INPORT of the hypervisor HV. The packet is an TCP syn segment with -# ETH_SRC, ETH_DST, IPV4_SRC, IPV4_DST, IP_CHKSUM, TCP_SPORT, TCP_DPORT, TCP_CHKSUM as specified. -# EXP_IP_CHKSUM and EXP_TCP_RST_CHKSUM are the ip and tcp checksums of the tcp reset segment generated from ACL rule hit +# Causes a TCP packet to be sent from the INPORT. If the OUTPORT is specified, +# the original packet is expected to be received at OUTPORT. Otherwise, a TCP +# reset packet is expected to be received at INPORT. # -# INPORT is an lport number, e.g. 11 for vif11. +# The packet is an TCP syn segment with ETH_SRC, ETH_DST, IPV4_SRC, IPV4_DST, +# IP_CHKSUM, TCP_SPORT, TCP_DPORT, TCP_CHKSUM as specified. EXP_IP_CHKSUM and +# EXP_TCP_RST_CHKSUM are the ip and tcp checksums of the tcp reset segment +# generated from ACL rule hit. +# +# INPORT and OUTPORT are lport numbers, e.g. 11 for vif11. # HV is an hypervisor number # ETH_SRC and ETH_DST are each 12 hex digits. # IPV4_SRC and IPV4_DST are each 8 hex digits. @@ -12834,13 +12840,20 @@ test_tcp_syn_packet() { local tcp_sport=$8 tcp_dport=$9 tcp_chksum=${10} local exp_ip_chksum=${11} exp_tcp_rst_chksum=${12} shift 12 + local outport=$1 local ip_ttl=ff local packet=${eth_dst}${eth_src}08004500002800004000${ip_ttl}06${ip_chksum}${ipv4_src}${ipv4_dst}${tcp_sport}${tcp_dport}000000010000000050027210${tcp_chksum}0000 - local tcp_rst_ttl=3f - local reply=${eth_src}${eth_dst}08004500002800004000${tcp_rst_ttl}06${exp_ip_chksum}${ipv4_dst}${ipv4_src}${tcp_dport}${tcp_sport}000000000000000250140000${exp_tcp_rst_chksum}0000 - echo $reply >> vif$inport.expected + if test x$outport == x; then + echo TCPRESET + local tcp_rst_ttl=3f + local reply=${eth_src}${eth_dst}08004500002800004000${tcp_rst_ttl}06${exp_ip_chksum}${ipv4_dst}${ipv4_src}${tcp_dport}${tcp_sport}000000000000000250140000${exp_tcp_rst_chksum}0000 + echo $reply >> vif$inport.expected + else + echo ORIGINAL + echo $packet >> vif$outport.expected + fi as hv$hv ovs-appctl netdev-dummy/receive vif$inport $packet } @@ -12886,6 +12899,10 @@ ovn-nbctl --log acl-add sw0 to-lport 1000 "outport == \"sw0-p12\"" reject ovn-nbctl --log acl-add sw0 from-lport 1000 "inport == \"sw0-p11\"" reject ovn-nbctl --log acl-add sw0 from-lport 1000 "inport == \"sw0-p21\"" reject +# A higher priority rule that allows all traffic sent from p21 to dst IP of p31 +ovn-nbctl --log acl-add sw0 from-lport 1001 "inport == \"sw0-p21\" + && ip4.dst == 192.168.1.31" allow + # Allow some time for ovn-northd and ovn-controller to catch up. ovn-nbctl --timeout=3 --wait=hv sync @@ -12899,6 +12916,10 @@ test_tcp_syn_packet 11 1 000000000011 000000000021 $(ip_to_hex 192 168 1 11) $(i test_tcp_syn_packet 21 2 000000000021 000000000011 $(ip_to_hex 192 168 1 21) $(ip_to_hex 192 168 1 11) 0000 8b40 3039 0000 b85f 70e4 test_tcp_syn_packet 31 3 000000000031 000000000012 $(ip_to_hex 192 168 1 31) $(ip_to_hex 192 168 1 12) 0000 8b40 3039 0000 b854 70d9 +# From p21 to p31, the packet should be received in p31, because of the last +# rule. +test_tcp_syn_packet 21 2 000000000021 000000000031 $(ip_to_hex 192 168 1 21) $(ip_to_hex 192 168 1 31) 0000 8b40 3039 0000 b854 70d9 31 + for i in 1 2 3; do OVN_CHECK_PACKETS([hv$i/vif${i}1-tx.pcap], [vif${i}1.expected]) done