From patchwork Fri Oct 2 18:25:36 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1376027 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.136; helo=silver.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 silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4C2z374tgcz9sS8 for ; Sat, 3 Oct 2020 04:26:26 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 58E9A274E3; Fri, 2 Oct 2020 18:26:22 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id rSnJVMcQshBa; Fri, 2 Oct 2020 18:26:13 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by silver.osuosl.org (Postfix) with ESMTP id CE03D204CA; Fri, 2 Oct 2020 18:26:13 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id A993CC016F; Fri, 2 Oct 2020 18:26:13 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id B2B45C0051 for ; Fri, 2 Oct 2020 18:26:11 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id A95C1873A8 for ; Fri, 2 Oct 2020 18:26:11 +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 0Yc5BJSd5v0o for ; Fri, 2 Oct 2020 18:26:10 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net [217.70.183.197]) by hemlock.osuosl.org (Postfix) with ESMTPS id F2F8987398 for ; Fri, 2 Oct 2020 18:26:09 +0000 (UTC) X-Originating-IP: 73.241.94.255 Received: from localhost.localdomain.localdomain (unknown [73.241.94.255]) (Authenticated sender: hzhou@ovn.org) by relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 14D2C1C0002; Fri, 2 Oct 2020 18:26:05 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Fri, 2 Oct 2020 11:25:36 -0700 Message-Id: <1601663136-19111-1-git-send-email-hzhou@ovn.org> X-Mailer: git-send-email 2.1.0 Cc: Han Zhou Subject: [ovs-dev] [PATCH ovn] ofctrl.c: Fix duplicated flow handling in I-P while merging opposite changes. 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: , MIME-Version: 1.0 Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" In a scenario in I-P when a desired flow is removed and an exactly same flow is added back in the same iteration, the merge_tracked_flows() function will merge the operation without firing any OpenFlow action. However, if there are multiple desired flows (e.g. F1 and F2) matching the same installed flow, and if the one being re-added (say, F1) is the one currently installed in OVS, the current implementation will update the currently installed flow to F2 in the data structure while the actual OVS flow is not updated (still F1). So far there is still no problem, but the member desired_flow of the installed flow is pointing to the wrong desired flow. Now there is only one place where the desired_flow member is consulted, in update_installed_flows_by_track() for handling a tracked *modified* flow. In reality flow modification happens only when conjunction flows need to be combined, which would never happen together with the above scenario of merge_tracked_flows(). So this wrong desired_flow pointer doesn't cause any real problem yet. However, there is a place that can already utilize this active desired_flow information, which is when handling a tracked flow deletion in update_installed_flows_by_track(): we can check if the flow being deleted is the currently active one installed in OVS. If not, we don't need to send and OpenFlow modify to OVS. This optimization is added in this patch, apart from fixing the problem of merge_tracked_flows(). Without the fix, this optimization would cause the test case added in this patch fail. Fixes: f4e508dd7 ("ofctrl.c: Merge opposite changes of tracked flows before installing.") Signed-off-by: Han Zhou Acked-by: Dumitru Ceara --- controller/ofctrl.c | 28 +++++++++++++- tests/ovn.at | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 2 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 81a00c8..e725c00 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -788,6 +788,24 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type) } } +/* Returns true if a desired flow is active (the one currently installed + * among the list of desired flows that are linked to the same installed + * flow). */ +static inline bool +desired_flow_is_active(struct desired_flow *d) +{ + return (d->installed_flow && d->installed_flow->desired_flow == d); +} + +/* Set a desired flow as the active one among the list of desired flows + * that are linked to the same installed flow. */ +static inline void +desired_flow_set_active(struct desired_flow *d) +{ + ovs_assert(d->installed_flow); + d->installed_flow->desired_flow = d; +} + static void link_installed_to_desired(struct installed_flow *i, struct desired_flow *d) { @@ -1740,6 +1758,8 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table) /* del_f must have been installed, otherwise it should have been * removed during track_flow_add_or_modify. */ ovs_assert(del_f->installed_flow); + + bool del_f_was_active = desired_flow_is_active(del_f); if (!f->installed_flow) { /* f is not installed yet. */ struct installed_flow *i = del_f->installed_flow; @@ -1751,6 +1771,9 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table) ovs_assert(f->installed_flow == del_f->installed_flow); unlink_installed_to_desired(del_f->installed_flow, del_f); } + if (del_f_was_active) { + desired_flow_set_active(f); + } hmap_remove(&deleted_flows, &del_f->match_hmap_node); ovs_list_remove(&del_f->track_list_node); desired_flow_destroy(del_f); @@ -1778,6 +1801,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, /* The desired flow was deleted */ if (f->installed_flow) { struct installed_flow *i = f->installed_flow; + bool was_active = desired_flow_is_active(f); unlink_installed_to_desired(i, f); if (!i->desired_flow) { @@ -1786,7 +1810,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, hmap_remove(&installed_flows, &i->match_hmap_node); installed_flow_destroy(i); - } else { + } else if (was_active) { /* There are other desired flow(s) referencing this * installed flow, so update the OVS flow for the new * active flow (at least the cookie will be different, @@ -1810,7 +1834,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, hmap_insert(&installed_flows, &new_node->match_hmap_node, new_node->flow.hash); link_installed_to_desired(new_node, f); - } else if (i->desired_flow == f) { + } else if (desired_flow_is_active(f)) { /* The installed flow is installed for f, but f has change * tracked, so it must have been modified. */ ovn_flow_log(&i->flow, "updating installed (tracked)"); diff --git a/tests/ovn.at b/tests/ovn.at index 7769b69..10b2b5f 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -22208,6 +22208,114 @@ OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected]) OVN_CLEANUP([hv1]) AT_CLEANUP +# This test cases tests a scenario of ACL confliction with address set update. +# It is to cover a corner case when flows are re-processed in the I-P +# iteration, combined with the scenario of conflicting ACLs. +AT_SETUP([ovn -- conflict ACLs with address set]) +ovn_start + +ovn-nbctl ls-add ls1 + +ovn-nbctl lsp-add ls1 lsp1 \ +-- lsp-set-addresses lsp1 "f0:00:00:00:00:01 10.0.0.1" + +ovn-nbctl lsp-add ls1 lsp2 \ +-- lsp-set-addresses lsp2 "f0:00:00:00:00:02 10.0.0.2" + +net_add n1 +sim_add hv1 + +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 +ovs-vsctl -- add-port br-int hv1-vif1 -- \ + set interface hv1-vif1 external-ids:iface-id=lsp1 \ + options:tx_pcap=hv1/vif1-tx.pcap \ + options:rxq_pcap=hv1/vif1-rx.pcap \ + ofport-request=1 + +ovs-vsctl -- add-port br-int hv1-vif2 -- \ + set interface hv1-vif2 external-ids:iface-id=lsp2 \ + options:tx_pcap=hv1/vif2-tx.pcap \ + options:rxq_pcap=hv1/vif2-rx.pcap \ + ofport-request=2 + +# Default drop +ovn-nbctl acl-add ls1 to-lport 1000 \ +'outport == "lsp1" && ip4' drop + +# test_ip INPORT SRC_MAC DST_MAC SRC_IP DST_IP OUTPORT... +# +# This shell function causes an ip packet to be received on INPORT. +# The packet's content has Ethernet destination DST and source SRC +# (each exactly 12 hex digits) and Ethernet type ETHTYPE (4 hex digits). +# The OUTPORTs (zero or more) list the VIFs on which the packet should +# be received. INPORT and the OUTPORTs are specified as logical switch +# port numbers, e.g. 11 for vif11. +test_ip() { + # This packet has bad checksums but logical L3 routing doesn't check. + local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5 + local packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}\ +${dst_ip}0035111100080000 + shift; shift; shift; shift; shift + as hv1 ovs-appctl netdev-dummy/receive hv1-vif$inport $packet + for outport; do + echo $packet >> $outport.expected + done +} + +ip_to_hex() { + printf "%02x%02x%02x%02x" "$@" +} + +reset_pcap_file() { + local iface=$1 + local pcap_file=$2 + ovs-vsctl -- set Interface $iface options:tx_pcap=dummy-tx.pcap \ +options:rxq_pcap=dummy-rx.pcap + rm -f ${pcap_file}*.pcap + ovs-vsctl -- set Interface $iface options:tx_pcap=${pcap_file}-tx.pcap \ +options:rxq_pcap=${pcap_file}-rx.pcap +} + +# Create an address set +ovn-nbctl create Address_Set name=as1 \ +addresses=\"10.0.0.2\",\"10.0.0.3\" + +# Create overlapping ACLs resulting in conflict desired OVS flows +# Add ACL1 uses the address set +ovn-nbctl --wait=hv acl-add ls1 to-lport 1001 \ +'outport == "lsp1" && ip4 && ip4.src == $as1' allow + +# Add ACL2 which uses a single IP, which shouldn't take effect because +# when it is added incrementally there is already a conflict one installed. +ovn-nbctl --wait=hv acl-add ls1 to-lport 1001 \ +'outport == "lsp1" && ip4 && ip4.src == 10.0.0.2' drop + + +sip=`ip_to_hex 10 0 0 2` +dip=`ip_to_hex 10 0 0 1` +test_ip 2 f00000000002 f00000000001 $sip $dip 1 +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected]) + +# Update the address set which causes flow reprocessing but the OVS flow +# for allowing 10.0.0.2 should keep unchanged +ovn-nbctl --wait=hv set Address_Set as1 addresses=\"10.0.0.2\" + +test_ip 2 f00000000002 f00000000001 $sip $dip 1 +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected]) + +# Delete the ACL1 that has "allow" action +ovn-nbctl acl-del ls1 to-lport 1001 \ +'outport == "lsp1" && ip4 && ip4.src == $as1' + +# ACL2 should take effect and packet should be dropped +test_ip 2 f00000000002 f00000000001 $sip $dip +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP + AT_SETUP([ovn -- port bind/unbind change handling with conj flows - IPv6]) ovn_start