From patchwork Thu Feb 18 08:50:43 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Frode Nordahl X-Patchwork-Id: 1441577 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=) 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 4Dh7jN0k1Sz9sRf for ; Thu, 18 Feb 2021 19:51:20 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 9248A873A1; Thu, 18 Feb 2021 08:51:18 +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 0jLVJG7NJuYP; Thu, 18 Feb 2021 08:51:16 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id 53FB587301; Thu, 18 Feb 2021 08:51:08 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 297FFC000D; Thu, 18 Feb 2021 08:51:08 +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 CE087C000E for ; Thu, 18 Feb 2021 08:51:04 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id CB77986DF5 for ; Thu, 18 Feb 2021 08:51:04 +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 ESIW9UVghbw1 for ; Thu, 18 Feb 2021 08:51:01 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from frode-threadripper.home (ti0189a330-0925.bb.online.no [88.88.218.161]) by whitealder.osuosl.org (Postfix) with ESMTP id 3E15486D6F for ; Thu, 18 Feb 2021 08:51:01 +0000 (UTC) From: Frode Nordahl To: dev@openvswitch.org Date: Thu, 18 Feb 2021 09:50:43 +0100 Message-Id: <622934ad57bcf547d42a479d23099e051d4cbf64.1613636533.git.frode.nordahl@canonical.com> X-Mailer: git-send-email 2.30.0 In-Reply-To: References: MIME-Version: 1.0 Subject: [ovs-dev] [PATCH ovn branch-20.03 07/16] 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: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" From: Han Zhou 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.") Acked-by: Dumitru Ceara Signed-off-by: Han Zhou (cherry picked from commit 9d2e8d32fb9865513b70408a665184a67564390d) Signed-off-by: Frode Nordahl --- 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 b526c68f4..41495c41f 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) { @@ -1679,6 +1697,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; @@ -1690,6 +1710,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); @@ -1717,6 +1740,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) { @@ -1725,7 +1749,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, @@ -1749,7 +1773,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 a0946d195..92c37a74d 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -18906,3 +18906,111 @@ OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [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