From patchwork Sun Oct 11 12:05:31 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dumitru Ceara X-Patchwork-Id: 1380313 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=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Q6zHk1kK; dkim-atps=neutral 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 4C8L9t1xZdz9sS8 for ; Sun, 11 Oct 2020 23:05:52 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 75DB9204AB; Sun, 11 Oct 2020 12:05:50 +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 msk-t4iTfhQg; Sun, 11 Oct 2020 12:05:41 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by silver.osuosl.org (Postfix) with ESMTP id B393220482; Sun, 11 Oct 2020 12:05:41 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 93954C07FF; Sun, 11 Oct 2020 12:05:41 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 7EF0AC0051 for ; Sun, 11 Oct 2020 12:05:40 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 6DEEC86B31 for ; Sun, 11 Oct 2020 12:05:40 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 5xdKbYucOFaz for ; Sun, 11 Oct 2020 12:05:39 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by fraxinus.osuosl.org (Postfix) with ESMTPS id 4F71886A47 for ; Sun, 11 Oct 2020 12:05:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1602417938; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=0hEChl73AQ9aZu0GE+wX/jKTvnimAPIYhslzqnBN98U=; b=Q6zHk1kKgS/JNNL8Qfrxe24UlWpRmXvR6ih+mZ+fnAK30T6lDrjLE2Ykfvjbh7LuJJmtBk 0w7xqEI83v0/8TNDvmSD9hvQHTlmMVUGgOOPv5Kv1xfTWgoNq8NZaWVJyFo/s3+o3AOA9L UM6uRArYbfercP4f9ouLTJEAqxTESrg= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-104-kmwYQkXcNH2tKh3lnmGt2g-1; Sun, 11 Oct 2020 08:05:36 -0400 X-MC-Unique: kmwYQkXcNH2tKh3lnmGt2g-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2DE1F56BE2; Sun, 11 Oct 2020 12:05:35 +0000 (UTC) Received: from dceara.remote.csb (ovpn-112-46.ams2.redhat.com [10.36.112.46]) by smtp.corp.redhat.com (Postfix) with ESMTP id 268815D9DC; Sun, 11 Oct 2020 12:05:33 +0000 (UTC) From: Dumitru Ceara To: dev@openvswitch.org Date: Sun, 11 Oct 2020 14:05:31 +0200 Message-Id: <20201011120522.3374.57876.stgit@dceara.remote.csb> In-Reply-To: <20201011120456.3374.30821.stgit@dceara.remote.csb> References: <20201011120456.3374.30821.stgit@dceara.remote.csb> User-Agent: StGit/0.17.1-dirty MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dceara@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Cc: hzhou@ovn.org Subject: [ovs-dev] [PATCH v5 ovn 1/4] ofctrl.c: Only merge actions for conjunctive flows. 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" In ofctrl_add_or_append_flow() when merging flow actions make sure we only do that for conjunctive flows. All other actions can not be merged with action "conjunction". CC: Mark Michelson Fixes: e659bab31a91 ("Combine conjunctions with identical matches into one flow.") Signed-off-by: Dumitru Ceara --- controller/ofctrl.c | 124 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 99 insertions(+), 25 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 4425d98..24b55fc 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -206,6 +206,9 @@ struct installed_flow { struct desired_flow *desired_flow; }; +typedef bool +(*desired_flow_match_cb)(const struct desired_flow *candidate, + const void *arg); static struct desired_flow *desired_flow_alloc( uint8_t table_id, uint16_t priority, @@ -214,8 +217,14 @@ static struct desired_flow *desired_flow_alloc( const struct ofpbuf *actions); static struct desired_flow *desired_flow_lookup( struct ovn_desired_flow_table *, + const struct ovn_flow *target); +static struct desired_flow *desired_flow_lookup_check_uuid( + struct ovn_desired_flow_table *, const struct ovn_flow *target, - const struct uuid *sb_uuid); + const struct uuid *); +static struct desired_flow *desired_flow_lookup_conjunctive( + struct ovn_desired_flow_table *, + const struct ovn_flow *target); static void desired_flow_destroy(struct desired_flow *); static struct installed_flow *installed_flow_lookup( @@ -806,6 +815,19 @@ desired_flow_set_active(struct desired_flow *d) d->installed_flow->desired_flow = d; } +static bool +flow_action_has_conj(const struct ovn_flow *f) +{ + const struct ofpact *a = NULL; + + OFPACT_FOR_EACH (a, f->ofpacts, f->ofpacts_len) { + if (a->type == OFPACT_CONJUNCTION) { + return true; + } + } + return false; +} + /* Adds the desired flow to the list of desired flows that have same match * conditions as the installed flow. * @@ -962,7 +984,7 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table, struct desired_flow *f = desired_flow_alloc(table_id, priority, cookie, match, actions); - if (desired_flow_lookup(flow_table, &f->flow, sb_uuid)) { + if (desired_flow_lookup_check_uuid(flow_table, &f->flow, sb_uuid)) { if (log_duplicate_flow) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); if (!VLOG_DROP_DBG(&rl)) { @@ -1002,14 +1024,15 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, const struct ofpbuf *actions, const struct uuid *sb_uuid) { - struct desired_flow *f = desired_flow_alloc(table_id, priority, cookie, - match, actions); - struct desired_flow *existing; - existing = desired_flow_lookup(desired_flows, &f->flow, NULL); + struct desired_flow *f; + + f = desired_flow_alloc(table_id, priority, cookie, match, actions); + existing = desired_flow_lookup_conjunctive(desired_flows, &f->flow); if (existing) { - /* There's already a flow with this particular match. Append the - * action to that flow rather than adding a new flow + /* There's already a flow with this particular match and action + * 'conjunction'. Append the action to that flow rather than + * adding a new flow. */ uint64_t compound_stub[64 / 8]; struct ofpbuf compound; @@ -1248,15 +1271,11 @@ installed_flow_dup(struct desired_flow *src) return dst; } -/* Finds and returns a desired_flow in 'flow_table' whose key is identical to - * 'target''s key, or NULL if there is none. - * - * If sb_uuid is not NULL, the function will also check if the found flow is - * referenced by the sb_uuid. */ static struct desired_flow * -desired_flow_lookup(struct ovn_desired_flow_table *flow_table, - const struct ovn_flow *target, - const struct uuid *sb_uuid) +desired_flow_lookup__(struct ovn_desired_flow_table *flow_table, + const struct ovn_flow *target, + desired_flow_match_cb match_cb, + const void *arg) { struct desired_flow *d; HMAP_FOR_EACH_WITH_HASH (d, match_hmap_node, target->hash, @@ -1265,20 +1284,76 @@ desired_flow_lookup(struct ovn_desired_flow_table *flow_table, if (f->table_id == target->table_id && f->priority == target->priority && minimatch_equal(&f->match, &target->match)) { - if (!sb_uuid) { + + if (!match_cb || match_cb(d, arg)) { return d; } - struct sb_flow_ref *sfr; - LIST_FOR_EACH (sfr, sb_list, &d->references) { - if (uuid_equals(sb_uuid, &sfr->sb_uuid)) { - return d; - } - } } } return NULL; } +/* Finds and returns a desired_flow in 'flow_table' whose key is identical to + * 'target''s key, or NULL if there is none. + */ +static struct desired_flow * +desired_flow_lookup(struct ovn_desired_flow_table *flow_table, + const struct ovn_flow *target) +{ + return desired_flow_lookup__(flow_table, target, NULL, NULL); +} + +static bool +flow_lookup_match_uuid_cb(const struct desired_flow *candidate, + const void *arg) +{ + const struct uuid *sb_uuid = arg; + struct sb_flow_ref *sfr; + + LIST_FOR_EACH (sfr, sb_list, &candidate->references) { + if (uuid_equals(sb_uuid, &sfr->sb_uuid)) { + return true; + } + } + return false; +} + +/* Finds and returns a desired_flow in 'flow_table' whose key is identical to + * 'target''s key, or NULL if there is none. + * + * The function will also check if the found flow is referenced by the + * 'sb_uuid'. + */ +static struct desired_flow * +desired_flow_lookup_check_uuid(struct ovn_desired_flow_table *flow_table, + const struct ovn_flow *target, + const struct uuid *sb_uuid) +{ + return desired_flow_lookup__(flow_table, target, flow_lookup_match_uuid_cb, + sb_uuid); +} + +static bool +flow_lookup_match_conj_cb(const struct desired_flow *candidate, + const void *arg OVS_UNUSED) +{ + return flow_action_has_conj(&candidate->flow); +} + +/* Finds and returns a desired_flow in 'flow_table' whose key is identical to + * 'target''s key, or NULL if there is none. + * + * The function will only return a matching flow if it contains action + * 'conjunction'. + */ +static struct desired_flow * +desired_flow_lookup_conjunctive(struct ovn_desired_flow_table *flow_table, + const struct ovn_flow *target) +{ + return desired_flow_lookup__(flow_table, target, flow_lookup_match_conj_cb, + NULL); +} + /* Finds and returns an installed_flow in installed_flows whose key is * identical to 'target''s key, or NULL if there is none. */ static struct installed_flow * @@ -1676,8 +1751,7 @@ update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table, struct installed_flow *i, *next; HMAP_FOR_EACH_SAFE (i, next, match_hmap_node, &installed_flows) { unlink_all_refs_for_installed_flow(i); - struct desired_flow *d = - desired_flow_lookup(flow_table, &i->flow, NULL); + struct desired_flow *d = desired_flow_lookup(flow_table, &i->flow); if (!d) { /* Installed flow is no longer desirable. Delete it from the * switch and from installed_flows. */ From patchwork Sun Oct 11 12:05:45 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dumitru Ceara X-Patchwork-Id: 1380314 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.137; helo=fraxinus.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=dMzn4Eek; dkim-atps=neutral Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4C8L9y02h9z9sS8 for ; Sun, 11 Oct 2020 23:05:57 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 2825586B72; Sun, 11 Oct 2020 12:05:56 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id XIzWfRaPtdBO; Sun, 11 Oct 2020 12:05:55 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by fraxinus.osuosl.org (Postfix) with ESMTP id 9E43786B8E; Sun, 11 Oct 2020 12:05:55 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 84354C07FF; Sun, 11 Oct 2020 12:05:55 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id AFCBCC0051 for ; Sun, 11 Oct 2020 12:05:54 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 9894686BBD for ; Sun, 11 Oct 2020 12:05:54 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id RPILllbgxbg5 for ; Sun, 11 Oct 2020 12:05:54 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by fraxinus.osuosl.org (Postfix) with ESMTPS id C5A5086B8E for ; Sun, 11 Oct 2020 12:05:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1602417952; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=WPzDiZY3PnGkUke5x3O2JjrgKq3Ua3oIeYutCW8choY=; b=dMzn4Eek5tb/MEhdgRJLzLZlTng1IVzw+YyJK2LQAeXUen5lmpkmaLy1QxIW0UyMQZrkb2 M/Ye4jwHlxaKepe98i2Z25IDMwPrkwtwdCZ8bCqR2lflBp9qzGZygZEvNJ1R2H0t5oF4t2 3+0U8mL1s4qFzuxc4Yqk+qwlImUrr+c= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-208--xaJcQCsPB2x1oGDcOI-eA-1; Sun, 11 Oct 2020 08:05:50 -0400 X-MC-Unique: -xaJcQCsPB2x1oGDcOI-eA-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AB7DF1868400; Sun, 11 Oct 2020 12:05:49 +0000 (UTC) Received: from dceara.remote.csb (ovpn-112-46.ams2.redhat.com [10.36.112.46]) by smtp.corp.redhat.com (Postfix) with ESMTP id 83CEC5D9FC; Sun, 11 Oct 2020 12:05:48 +0000 (UTC) From: Dumitru Ceara To: dev@openvswitch.org Date: Sun, 11 Oct 2020 14:05:45 +0200 Message-Id: <20201011120540.3374.15735.stgit@dceara.remote.csb> In-Reply-To: <20201011120456.3374.30821.stgit@dceara.remote.csb> References: <20201011120456.3374.30821.stgit@dceara.remote.csb> User-Agent: StGit/0.17.1-dirty MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dceara@redhat.com X-Mimecast-Spam-Score: 0.003 X-Mimecast-Originator: redhat.com Cc: hzhou@ovn.org Subject: [ovs-dev] [PATCH v5 ovn 2/4] ofctrl.c: Do not change flow ordering when 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" Instead of removing the old desired flow from the list and inserting the new one at the end we now directly replace the old flow with the new one while maintaining the same ordering. For now order of the flows is not relevant but further commits require maintaining the order of desired flows. Signed-off-by: Dumitru Ceara --- controller/ofctrl.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 24b55fc..20cf3ac 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -848,6 +848,26 @@ link_installed_to_desired(struct installed_flow *i, struct desired_flow *d) d->installed_flow = i; } +/* Replaces 'old_desired' with 'new_desired' in the list of desired flows + * that have same match conditions as the installed flow. + * + * If 'old_desired' was the active flow, 'new_desired' becomes the active one. + */ +static void +replace_installed_to_desired(struct installed_flow *i, + struct desired_flow *old_desired, + struct desired_flow *new_desired) +{ + ovs_assert(old_desired->installed_flow == i); + ovs_list_replace(&new_desired->installed_ref_list_node, + &old_desired->installed_ref_list_node); + old_desired->installed_flow = NULL; + new_desired->installed_flow = i; + if (i->desired_flow == old_desired) { + i->desired_flow = new_desired; + } +} + static void unlink_installed_to_desired(struct installed_flow *i, struct desired_flow *d) { @@ -1842,21 +1862,15 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table) * 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; - unlink_installed_to_desired(i, del_f); - link_installed_to_desired(i, f); + replace_installed_to_desired(del_f->installed_flow, del_f, f); } else { /* f has been installed before, and now was updated to exact * the same flow as del_f. */ 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); From patchwork Sun Oct 11 12:05:59 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dumitru Ceara X-Patchwork-Id: 1380315 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=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=RI/d70rW; dkim-atps=neutral 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 4C8LBD2l5fz9sS8 for ; Sun, 11 Oct 2020 23:06:12 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 93679873BE; Sun, 11 Oct 2020 12:06:10 +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 2v88IIk48u-U; Sun, 11 Oct 2020 12:06:09 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id A96A38732D; Sun, 11 Oct 2020 12:06:09 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 8FB37C07FF; Sun, 11 Oct 2020 12:06:09 +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 20D05C07FF for ; Sun, 11 Oct 2020 12:06:08 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 1005B87745 for ; Sun, 11 Oct 2020 12:06:08 +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 ift70Lq8S3-K for ; Sun, 11 Oct 2020 12:06:07 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by whitealder.osuosl.org (Postfix) with ESMTPS id D3440876F6 for ; Sun, 11 Oct 2020 12:06:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1602417965; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=AEdGwtKqPEMWAsrpUNsDNjz+giboQ+CZ2DQORr4QD6M=; b=RI/d70rWxyesjRgQNAeQ9rr91E0UlmfjX0/o2bJXdTa3O1GRKuVO6PaymYhrrsg8flZp6b oRR7srRgwPZ7220m6FKMnChXw2r0s3dNnmLsYkPLsy2KqXLKcnff/1CCvNGUFrS2ESsNxS gShPX690SULsfRZbKtjOqE9Z93Q22jI= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-399-k293DYUWOGycznqCH1bFvg-1; Sun, 11 Oct 2020 08:06:03 -0400 X-MC-Unique: k293DYUWOGycznqCH1bFvg-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 5BC20107ACF8; Sun, 11 Oct 2020 12:06:02 +0000 (UTC) Received: from dceara.remote.csb (ovpn-112-46.ams2.redhat.com [10.36.112.46]) by smtp.corp.redhat.com (Postfix) with ESMTP id 652725D9DC; Sun, 11 Oct 2020 12:06:01 +0000 (UTC) From: Dumitru Ceara To: dev@openvswitch.org Date: Sun, 11 Oct 2020 14:05:59 +0200 Message-Id: <20201011120554.3374.75656.stgit@dceara.remote.csb> In-Reply-To: <20201011120456.3374.30821.stgit@dceara.remote.csb> References: <20201011120456.3374.30821.stgit@dceara.remote.csb> User-Agent: StGit/0.17.1-dirty MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dceara@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Cc: hzhou@ovn.org Subject: [ovs-dev] [PATCH v5 ovn 3/4] ofctrl.c: Simplify active desired flow selection. 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" Always consider the first "desired flow" in the list of flows that refer an "installed flow" as the active flow. This simplifies the logic of the flow update code and is used in a further commit to implement a partial ordering of desired flows within installed flows. Signed-off-by: Dumitru Ceara --- controller/ofctrl.c | 92 +++++++++++++++++++++------------------------------ 1 file changed, 37 insertions(+), 55 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 20cf3ac..74f98e3 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -188,6 +188,8 @@ struct sb_flow_ref { * relationship is 1 to N. A link is added when a flow addition is processed. * A link is removed when a flow deletion is processed, the desired flow * table is cleared, or the installed flow table is cleared. + * The first desired_flow in the list is the active one, the one that is + * actually installed. */ struct installed_flow { struct ovn_flow flow; @@ -199,11 +201,6 @@ struct installed_flow { * installed flow, e.g. when there are conflict/duplicated ACLs that * generates same match conditions). */ struct ovs_list desired_refs; - - /* The corresponding flow in desired table. It must be one of the flows in - * desired_refs list. If there are more than one flows in references list, - * this is the one that is actually installed. */ - struct desired_flow *desired_flow; }; typedef bool @@ -231,6 +228,8 @@ static struct installed_flow *installed_flow_lookup( const struct ovn_flow *target); static void installed_flow_destroy(struct installed_flow *); static struct installed_flow *installed_flow_dup(struct desired_flow *); +static struct desired_flow *installed_flow_get_active( + struct installed_flow *f); static uint32_t ovn_flow_match_hash(const struct ovn_flow *); static char *ovn_flow_to_string(const struct ovn_flow *); @@ -796,24 +795,6 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type) log_openflow_rl(&rl, VLL_DBG, oh, "OpenFlow packet ignored"); } } - -/* 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 bool flow_action_has_conj(const struct ovn_flow *f) @@ -831,27 +812,22 @@ flow_action_has_conj(const struct ovn_flow *f) /* Adds the desired flow to the list of desired flows that have same match * conditions as the installed flow. * - * If the newly added desired flow is the first one in the list, it is also set - * as the active one. - * * It is caller's responsibility to make sure the link between the pair didn't - * exist before. */ -static void + * exist before. + * + * Returns true if the newly added desired flow is selected to be the active + * one. + */ +static bool link_installed_to_desired(struct installed_flow *i, struct desired_flow *d) { - ovs_assert(i->desired_flow != d); - if (ovs_list_is_empty(&i->desired_refs)) { - ovs_assert(!i->desired_flow); - i->desired_flow = d; - } - ovs_list_insert(&i->desired_refs, &d->installed_ref_list_node); d->installed_flow = i; + ovs_list_push_back(&i->desired_refs, &d->installed_ref_list_node); + return installed_flow_get_active(i) == d; } /* Replaces 'old_desired' with 'new_desired' in the list of desired flows * that have same match conditions as the installed flow. - * - * If 'old_desired' was the active flow, 'new_desired' becomes the active one. */ static void replace_installed_to_desired(struct installed_flow *i, @@ -863,24 +839,22 @@ replace_installed_to_desired(struct installed_flow *i, &old_desired->installed_ref_list_node); old_desired->installed_flow = NULL; new_desired->installed_flow = i; - if (i->desired_flow == old_desired) { - i->desired_flow = new_desired; - } } -static void +/* Removes the desired flow from the list of desired flows that have the same + * match conditions as the installed flow. + * + * Returns true if the desired flow was the previously active flow. + */ +static bool unlink_installed_to_desired(struct installed_flow *i, struct desired_flow *d) { - ovs_assert(i && i->desired_flow && !ovs_list_is_empty(&i->desired_refs)); + struct desired_flow *old_active = installed_flow_get_active(i); + ovs_assert(d && d->installed_flow == i); ovs_list_remove(&d->installed_ref_list_node); d->installed_flow = NULL; - if (i->desired_flow == d) { - i->desired_flow = ovs_list_is_empty(&i->desired_refs) ? NULL : - CONTAINER_OF(ovs_list_front(&i->desired_refs), - struct desired_flow, - installed_ref_list_node); - } + return old_active == d; } static void @@ -1280,7 +1254,6 @@ installed_flow_dup(struct desired_flow *src) { struct installed_flow *dst = xmalloc(sizeof *dst); ovs_list_init(&dst->desired_refs); - dst->desired_flow = NULL; dst->flow.table_id = src->flow.table_id; dst->flow.priority = src->flow.priority; minimatch_clone(&dst->flow.match, &src->flow.match); @@ -1292,6 +1265,17 @@ installed_flow_dup(struct desired_flow *src) } static struct desired_flow * +installed_flow_get_active(struct installed_flow *f) +{ + if (!ovs_list_is_empty(&f->desired_refs)) { + return CONTAINER_OF(ovs_list_front(&f->desired_refs), + struct desired_flow, + installed_ref_list_node); + } + return NULL; +} + +static struct desired_flow * desired_flow_lookup__(struct ovn_desired_flow_table *flow_table, const struct ovn_flow *target, desired_flow_match_cb match_cb, @@ -1439,8 +1423,7 @@ static void installed_flow_destroy(struct installed_flow *f) { if (f) { - ovs_assert(ovs_list_is_empty(&f->desired_refs)); - ovs_assert(!f->desired_flow); + ovs_assert(!installed_flow_get_active(f)); ovn_flow_uninit(&f->flow); free(f); } @@ -1898,10 +1881,10 @@ 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); + bool was_active = unlink_installed_to_desired(i, f); + struct desired_flow *d = installed_flow_get_active(i); - if (!i->desired_flow) { + if (!d) { installed_flow_del(&i->flow, msgs); ovn_flow_log(&i->flow, "removing installed (tracked)"); @@ -1912,7 +1895,6 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, * installed flow, so update the OVS flow for the new * active flow (at least the cookie will be different, * even if the actions are the same). */ - struct desired_flow *d = i->desired_flow; ovn_flow_log(&i->flow, "updating installed (tracked)"); installed_flow_mod(&i->flow, &d->flow, msgs); } @@ -1931,7 +1913,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 (desired_flow_is_active(f)) { + } else if (installed_flow_get_active(i) == 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)"); From patchwork Sun Oct 11 12:06:13 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dumitru Ceara X-Patchwork-Id: 1380316 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.138; helo=whitealder.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=XgQP6act; dkim-atps=neutral Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4C8LDH0qLQz9sS8 for ; Sun, 11 Oct 2020 23:07:58 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id CB88B87707; Sun, 11 Oct 2020 12:07:56 +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 x1YOj-lOOcXp; Sun, 11 Oct 2020 12:07:52 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by whitealder.osuosl.org (Postfix) with ESMTP id DEBCA876DA; Sun, 11 Oct 2020 12:07:52 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id CF00CC07FF; Sun, 11 Oct 2020 12:07:52 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 7D726C0051 for ; Sun, 11 Oct 2020 12:07:51 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 72AEC2048A for ; Sun, 11 Oct 2020 12:07:51 +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 xIoZoJBrHhLn for ; Sun, 11 Oct 2020 12:07:27 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by silver.osuosl.org (Postfix) with ESMTPS id A06B02E363 for ; Sun, 11 Oct 2020 12:06:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1602417981; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=kNzfXIP1fflNrhDmrPejoew6/VdAR5EbFSTDTElm+pM=; b=XgQP6act9mVyW53gmGTHhIfmtWPTj3D6RwoAFbi/kEBG6XjsjAmteLmOlPajKxiRffEY8Z 2tuUXqrMV74v/PDgROnI/rqP3PvesW/mmKbfkkcTfx9MzUhirc24hFgj6kTJCgIwvF+TAU BBJ4zoxOS6uAdo53jt4qGf0Y8vaO2lQ= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-566-8mpi5mWhNgqagnmd93d08g-1; Sun, 11 Oct 2020 08:06:18 -0400 X-MC-Unique: 8mpi5mWhNgqagnmd93d08g-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CD198107ACF5; Sun, 11 Oct 2020 12:06:17 +0000 (UTC) Received: from dceara.remote.csb (ovpn-112-46.ams2.redhat.com [10.36.112.46]) by smtp.corp.redhat.com (Postfix) with ESMTP id B05B16EF56; Sun, 11 Oct 2020 12:06:16 +0000 (UTC) From: Dumitru Ceara To: dev@openvswitch.org Date: Sun, 11 Oct 2020 14:06:13 +0200 Message-Id: <20201011120607.3374.49659.stgit@dceara.remote.csb> In-Reply-To: <20201011120456.3374.30821.stgit@dceara.remote.csb> References: <20201011120456.3374.30821.stgit@dceara.remote.csb> User-Agent: StGit/0.17.1-dirty MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dceara@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Cc: hzhou@ovn.org Subject: [ovs-dev] [PATCH v5 ovn 4/4] ofctrl.c: Add a predictable resolution for conflicting flow actions. 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" Until now, in case the ACL configuration generates openflows that have the same match but different actions, ovn-controller was using the following approach: 1. If the flow being added contains conjunctive actions, merge its actions with the already existing flow. 2. Otherwise, if the flow is being added incrementally (update_installed_flows_by_track), don't install the new flow but instead keep the old one. 3. Otherwise, (update_installed_flows_by_compare), don't install the new flow but instead keep the old one. Even though one can argue that having an ACL with a match that includes the match of another ACL is a misconfiguration, it can happen that the users provision OVN like this. Depending on the order of reading and installing the logical flows, the above operations can yield unpredictable results, e.g., allow specific traffic but then after ovn-controller is restarted (or a recompute happens) that specific traffic starts being dropped. A simple example of ACL configuration is: ovn-nbctl acl-add ls to-lport 3 '(ip4.src==10.0.0.1 || ip4.src==10.0.0.2) && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow ovn-nbctl acl-add ls to-lport 3 'ip4.src==10.0.0.1' allow ovn-nbctl acl-add ls to-lport 2 'arp' allow ovn-nbctl acl-add ls to-lport 1 'ip4' drop This is a pattern used by most CMSs: - define a default deny policy. - punch holes in the default deny policy based on user specific configurations. Without this commit the behavior for traffic from 10.0.0.1 to 10.0.0.5 is unpredictable. Depending on the order of operations traffic might be dropped or allowed. It's also quite hard to force the CMS to ensure that such match overlaps never occur. To address this issue we now ensure that all desired flows refering the same installed flow are partially sorted in the following way: - first all flows with action "allow". - then all flows with action "drop". - then a single flow with action "conjunction" (resulting from merging all flows with the same match and action conjunction). This ensures that "allow" flows have precedence over "drop" flows which in turn have precedence over "conjunction" flows. Essentially less restrictive flows are always preferred over more restrictive flows whenever a match conflict happens. CC: Daniel Alvarez Reported-at: https://bugzilla.redhat.com/1871931 Signed-off-by: Dumitru Ceara --- controller/ofctrl.c | 95 +++++++++++++++++++--- tests/ovn.at | 216 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 296 insertions(+), 15 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 74f98e3..30b5667 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -172,6 +172,18 @@ struct sb_flow_ref { struct uuid sb_uuid; }; +/* Desired flow reference type based on the action of the desired flow. + * + * This is used to maintain a partial ordering between multiple desired + * flows that are linked to the same installed flow. + */ +enum desired_flow_ref_type { + DFLOW_REF_ALLOW, + DFLOW_REF_DROP, + DFLOW_REF_CONJ, + DFLOW_REF_MAX, +}; + /* A installed flow, in static variable installed_flows. * * Installed flows are updated in ofctrl_put for maintaining the flow @@ -188,6 +200,14 @@ struct sb_flow_ref { * relationship is 1 to N. A link is added when a flow addition is processed. * A link is removed when a flow deletion is processed, the desired flow * table is cleared, or the installed flow table is cleared. + * + * To ensure predictable behavior, the list of desired flows is maintained + * partially sorted in the following way (from least restrictive to most + * restrictive wrt. match): + * - allow flows without action conjunction. + * - drop flows without action conjunction. + * - a single flow with action conjunction. + * * The first desired_flow in the list is the active one, the one that is * actually installed. */ @@ -195,12 +215,12 @@ struct installed_flow { struct ovn_flow flow; struct hmap_node match_hmap_node; /* For match based hashing. */ - /* A list of desired ovn_flow nodes (linked by + /* Lists of desired ovn_flow nodes (linked by * desired_flow.installed_ref_list_node), which reference this installed * flow. (There are cases that multiple desired flows reference the same * installed flow, e.g. when there are conflict/duplicated ACLs that * generates same match conditions). */ - struct ovs_list desired_refs; + struct ovs_list desired_refs[DFLOW_REF_MAX]; }; typedef bool @@ -797,6 +817,12 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type) } static bool +flow_action_has_drop(const struct ovn_flow *f) +{ + return f->ofpacts_len == 0; +} + +static bool flow_action_has_conj(const struct ovn_flow *f) { const struct ofpact *a = NULL; @@ -822,7 +848,18 @@ static bool link_installed_to_desired(struct installed_flow *i, struct desired_flow *d) { d->installed_flow = i; - ovs_list_push_back(&i->desired_refs, &d->installed_ref_list_node); + + if (flow_action_has_conj(&d->flow)) { + ovs_assert(ovs_list_is_empty(&i->desired_refs[DFLOW_REF_CONJ])); + ovs_list_push_back(&i->desired_refs[DFLOW_REF_CONJ], + &d->installed_ref_list_node); + } else if (flow_action_has_drop(&d->flow)) { + ovs_list_push_back(&i->desired_refs[DFLOW_REF_DROP], + &d->installed_ref_list_node); + } else { + ovs_list_push_back(&i->desired_refs[DFLOW_REF_ALLOW], + &d->installed_ref_list_node); + } return installed_flow_get_active(i) == d; } @@ -854,15 +891,27 @@ unlink_installed_to_desired(struct installed_flow *i, struct desired_flow *d) ovs_assert(d && d->installed_flow == i); ovs_list_remove(&d->installed_ref_list_node); d->installed_flow = NULL; + + /* If the flow we're removing was a conjunction flow, it should have been + * the only one referring this installed flow. + */ + if (flow_action_has_conj(&d->flow)) { + ovs_assert(ovs_list_is_empty(&i->desired_refs[DFLOW_REF_CONJ])); + } + return old_active == d; } static void unlink_all_refs_for_installed_flow(struct installed_flow *i) { - struct desired_flow *d, *next; - LIST_FOR_EACH_SAFE (d, next, installed_ref_list_node, &i->desired_refs) { - unlink_installed_to_desired(i, d); + for (enum desired_flow_ref_type rt = 0; rt < DFLOW_REF_MAX; rt++) { + struct desired_flow *d, *next; + + LIST_FOR_EACH_SAFE (d, next, installed_ref_list_node, + &i->desired_refs[rt]) { + unlink_installed_to_desired(i, d); + } } } @@ -1253,7 +1302,10 @@ static struct installed_flow * installed_flow_dup(struct desired_flow *src) { struct installed_flow *dst = xmalloc(sizeof *dst); - ovs_list_init(&dst->desired_refs); + + for (enum desired_flow_ref_type rt = 0; rt < DFLOW_REF_MAX; rt++) { + ovs_list_init(&dst->desired_refs[rt]); + } dst->flow.table_id = src->flow.table_id; dst->flow.priority = src->flow.priority; minimatch_clone(&dst->flow.match, &src->flow.match); @@ -1267,10 +1319,12 @@ installed_flow_dup(struct desired_flow *src) static struct desired_flow * installed_flow_get_active(struct installed_flow *f) { - if (!ovs_list_is_empty(&f->desired_refs)) { - return CONTAINER_OF(ovs_list_front(&f->desired_refs), - struct desired_flow, - installed_ref_list_node); + for (enum desired_flow_ref_type rt = 0; rt < DFLOW_REF_MAX; rt++) { + if (!ovs_list_is_empty(&f->desired_refs[rt])) { + return CONTAINER_OF(ovs_list_front(&f->desired_refs[rt]), + struct desired_flow, + installed_ref_list_node); + } } return NULL; } @@ -1790,8 +1844,13 @@ update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table, link_installed_to_desired(i, d); } else if (!d->installed_flow) { /* This is a desired_flow that conflicts with one installed - * previously but not linked yet. */ - link_installed_to_desired(i, d); + * previously but not linked yet. However, if this flow becomes + * active, e.g., it is less restrictive than the previous active + * flow then modify the installed flow. + */ + if (link_installed_to_desired(i, d)) { + installed_flow_mod(&i->flow, &d->flow, msgs); + } } } } @@ -1920,8 +1979,14 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, installed_flow_mod(&i->flow, &f->flow, msgs); } else { /* Adding a new flow that conflicts with an existing installed - * flow, so just add it to the link. */ - link_installed_to_desired(i, f); + * flow, so add it to the link. If this flow becomes active + * active, e.g., it is less restrictive than the previous + * active flow then modify the installed flow. + */ + if (link_installed_to_desired(i, f)) { + ovn_flow_log(&i->flow, "updating installed (tracked)"); + installed_flow_mod(&i->flow, &f->flow, msgs); + } } /* The track_list_node emptyness is used to check if the node is * already added to track list, so initialize it again here. */ diff --git a/tests/ovn.at b/tests/ovn.at index 6f1ab59..8202f38 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -13727,6 +13727,222 @@ grep conjunction.*conjunction.*conjunction | wc -l`]) OVN_CLEANUP([hv1]) AT_CLEANUP +AT_SETUP([ovn -- Superseeding ACLs with conjunction]) +ovn_start + +ovn-nbctl ls-add ls1 + +ovn-nbctl lsp-add ls1 ls1-lp1 \ +-- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01" + +ovn-nbctl lsp-add ls1 ls1-lp2 \ +-- lsp-set-addresses ls1-lp2 "f0:00:00:00:00:02" + +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=ls1-lp1 \ + 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=ls1-lp2 \ + options:tx_pcap=hv1/vif2-tx.pcap \ + options:rxq_pcap=hv1/vif2-rx.pcap \ + ofport-request=2 + +# 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-vif1 $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 +} + +as hv1 ovn-appctl -t ovn-controller vlog/set ofctrl:dbg + +# Add a default deny ACL and an allow ACL for specific IP traffic. +ovn-nbctl acl-add ls1 to-lport 2 'arp' allow +ovn-nbctl acl-add ls1 to-lport 1 'ip4' drop +ovn-nbctl acl-add ls1 to-lport 3 '(ip4.src==10.0.0.1 || ip4.src==10.0.0.2) && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow +ovn-nbctl acl-add ls1 to-lport 3 '(ip4.src==10.0.0.1 || ip4.src==10.0.0.42) && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow +ovn-nbctl --wait=hv sync + +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed. +for src in `seq 1 2`; do + for dst in `seq 3 4`; do + sip=`ip_to_hex 10 0 0 $src` + dip=`ip_to_hex 10 0 0 $dst` + + test_ip 1 f00000000001 f00000000002 $sip $dip 2 + done +done + +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.5 should be dropped. +dip=`ip_to_hex 10 0 0 5` +for src in `seq 1 2`; do + sip=`ip_to_hex 10 0 0 $src` + + test_ip 1 f00000000001 f00000000002 $sip $dip +done + +cat 2.expected > expout +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets +AT_CHECK([cat 2.packets], [0], [expout]) +reset_pcap_file hv1-vif2 hv1/vif2 +rm -f 2.packets +> 2.expected + +# Add two less restrictive allow ACLs for src IP 10.0.0.1. +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1 || ip4.src==10.0.0.1' allow +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow +ovn-nbctl --wait=hv sync + +# Check OVS flows, the less restrictive flows should have been installed. +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \ + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46) +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46) +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2),conjunction(3,1/2) +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2),conjunction(3,1/2) +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46) +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2) +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2) +]) + +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed. +for src in `seq 1 2`; do + for dst in `seq 3 4`; do + sip=`ip_to_hex 10 0 0 $src` + dip=`ip_to_hex 10 0 0 $dst` + + test_ip 1 f00000000001 f00000000002 $sip $dip 2 + done +done + +# Traffic 10.0.0.2 -> 10.0.0.5 should be dropped. +sip=`ip_to_hex 10 0 0 2` +dip=`ip_to_hex 10 0 0 5` +test_ip 1 f00000000001 f00000000002 $sip $dip + +# Traffic 10.0.0.1 -> 10.0.0.5 should be allowed. +sip=`ip_to_hex 10 0 0 1` +dip=`ip_to_hex 10 0 0 5` +test_ip 1 f00000000001 f00000000002 $sip $dip 2 + +cat 2.expected > expout +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets +AT_CHECK([cat 2.packets], [0], [expout]) +reset_pcap_file hv1-vif2 hv1/vif2 +rm -f 2.packets +> 2.expected + +#sleep infinity + +# Remove the first less restrictive allow ACL. +ovn-nbctl acl-del ls1 to-lport 3 'ip4.src==10.0.0.1 || ip4.src==10.0.0.1' +ovn-nbctl --wait=hv sync + +# Check OVS flows, the second less restrictive allow ACL should have been installed. +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \ + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46) +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46) +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2),conjunction(3,1/2) +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2),conjunction(3,1/2) +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46) +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2) +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2) +]) + +# Remove the less restrictive allow ACL. +ovn-nbctl acl-del ls1 to-lport 3 'ip4.src==10.0.0.1' +ovn-nbctl --wait=hv sync + +# Check OVS flows, the 10.0.0.1 conjunction should have been reinstalled. +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \ + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46) +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46) +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2),conjunction(3,1/2) +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2),conjunction(3,1/2) +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2),conjunction(3,2/2) +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2) +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2) +]) + +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed. +for src in `seq 1 2`; do + for dst in `seq 3 4`; do + sip=`ip_to_hex 10 0 0 $src` + dip=`ip_to_hex 10 0 0 $dst` + + test_ip 1 f00000000001 f00000000002 $sip $dip 2 + done +done + +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.5 should be dropped. +dip=`ip_to_hex 10 0 0 5` +for src in `seq 1 2`; do + sip=`ip_to_hex 10 0 0 $src` + + test_ip 1 f00000000001 f00000000002 $sip $dip +done + +cat 2.expected > expout +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets +AT_CHECK([cat 2.packets], [0], [expout]) + +# Re-add the less restrictive allow ACL for src IP 10.0.0.1 +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow +ovn-nbctl --wait=hv sync + +# Check OVS flows, the less restrictive flows should have been installed. +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \ + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46) +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46) +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2),conjunction(3,1/2) +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2),conjunction(3,1/2) +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46) +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2) +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2) +]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP + # 3 hypervisors, one logical switch, 3 logical ports per hypervisor AT_SETUP([ovn -- L2 Drop and Allow ACL w/ Stateful ACL]) ovn_start