From patchwork Sun Aug 1 13:09:11 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peng He X-Patchwork-Id: 1512047 X-Patchwork-Delegate: i.maximets@samsung.com 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=smtp4.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20161025 header.b=Ieuj22l5; dkim-atps=neutral Received: from smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4Gd1gX5hWxz9sWc for ; Sun, 1 Aug 2021 23:09:28 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 80DD94024D; Sun, 1 Aug 2021 13:09:25 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id CrVM9U-nEDxC; Sun, 1 Aug 2021 13:09:21 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp4.osuosl.org (Postfix) with ESMTPS id B7D3F40353; Sun, 1 Aug 2021 13:09:20 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 8BD40C001B; Sun, 1 Aug 2021 13:09:20 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp3.osuosl.org (smtp3.osuosl.org [IPv6:2605:bc80:3010::136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 790C0C000E for ; Sun, 1 Aug 2021 13:09:18 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 68FC460753 for ; Sun, 1 Aug 2021 13:09:18 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Authentication-Results: smtp3.osuosl.org (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id jqWoy-Z1mpdX for ; Sun, 1 Aug 2021 13:09:16 +0000 (UTC) X-Greylist: whitelisted by SQLgrey-1.8.0 Received: from mail-pj1-x1033.google.com (mail-pj1-x1033.google.com [IPv6:2607:f8b0:4864:20::1033]) by smtp3.osuosl.org (Postfix) with ESMTPS id 82EDA60712 for ; Sun, 1 Aug 2021 13:09:16 +0000 (UTC) Received: by mail-pj1-x1033.google.com with SMTP id ca5so21680449pjb.5 for ; Sun, 01 Aug 2021 06:09:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=FsGAPhUc8fL2wVPQPWAnwSluHIdjLMr7k7te1g+p5m0=; b=Ieuj22l51ACMzT4cAcWSm/qlDmRCkcBSd1VZgXIIQwD1jH+zG0voonFw8GyiE/6E3A AYWpV66l1TcWmg66AhWj5J32WBwUkUBEnJnkbQDEEXYuh/pgNb+Rx/1zoeOVieHin/By kx8kRob+AQZ/DqPa0Usa3CplZ1sqUhbmHqrLzNaoyRw/ebn4mzy46QVFq4sVgAmjvIC/ rCdz8YZkKy8RFLf7QaKuB/eQccjwc9H4Ntg8tdCaTvZVKGOQbHHOh4i7igC+y2S42Of8 upOWvrJJaVN6DkJptwKKB4ddjgUpa7L9IxZEWjV0014v0jSTLHsFFvHWAZsC5WS2RhiH pvMg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=FsGAPhUc8fL2wVPQPWAnwSluHIdjLMr7k7te1g+p5m0=; b=AmSjvSmDWYloZIaunXZ7Q/NBL9sRPtHT6zlzJXvitEKHsT/MLJE4M0E06mk0iZ/WwR jyYie7xZz6Yiqu1tEm2VRXFT2JHZTGy1jaVXTU8YNJKAh1bGcam0dWMJInz2qYt6Vhg0 sOEAcCTIA7kzSMr4A6RwzYlGwjquwfZy1g5dWuNiesGPfJHIwapYflTqkW/rjA1E295u 40CHKRUsFLtWuSehyR90Jdii0C/n7YQPLRuy5phcLvvlhnN040Wgg56EUaU9LHGRPw8R 2Z9FGebgQ532ppvpTY+lxlMXXUaH5gFuJX+0drwjCfYu5Q+wIYzwBc4PFoO/fe7z6dcX XMfQ== X-Gm-Message-State: AOAM533P6yswbNIt2zGTm57d8bQNO1cFXaASFV5woMtm4FEQVu/CrRpe YDc4asZ3K00q28bN0uUxKtN+FKjISf+2N76u X-Google-Smtp-Source: ABdhPJwLE8tk8/Jzqvpd6RLI+BgV8ak1l8qIOZ/NGi5+2/j06IuWlxvNYxo77yfxRwkpOXsZ7Zd/qQ== X-Received: by 2002:a17:90a:550d:: with SMTP id b13mr12639627pji.28.1627823355023; Sun, 01 Aug 2021 06:09:15 -0700 (PDT) Received: from localhost ([139.177.225.231]) by smtp.gmail.com with ESMTPSA id j19sm8529382pfr.82.2021.08.01.06.09.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 01 Aug 2021 06:09:14 -0700 (PDT) From: Peng He X-Google-Original-From: Peng He To: dev@openvswitch.org, i.maximets@ovn.org Date: Sun, 1 Aug 2021 21:09:11 +0800 Message-Id: <20210801130911.66655-1-hepeng.0320@bytedance.com> X-Mailer: git-send-email 2.23.0 In-Reply-To: <21720f86-85d9-f6e7-1039-10480c89342a@ovn.org> References: <21720f86-85d9-f6e7-1039-10480c89342a@ovn.org> MIME-Version: 1.0 Subject: [ovs-dev] [PATCH v4] ofproto-dpif-xlate: fix zone set from non-frozen-metadata fields 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" CT zone could be set from a field that is not included in frozen metadata. Consider the example rules which are typically seen in OpenStack security group rules: priority=100,in_port=1,tcp,ct_state=-trk,action=ct(zone=5,table=0) priority=100,in_port=1,tcp,ct_state=+trk,action=ct(commit,zone=NXM_NX_CT_ZONE[]),2 The zone is set from the first rule's ct action. These two rules will generate two megaflows: the first one uses zone=5 to query the CT module, the second one sets the zone-id from the first megaflow and commit to CT. The current implementation will generate a megaflow that does not use ct_zone=5 as a match, but directly commit into the ct using zone=5, as zone is set by an Imm not a field. Consider a situation that one changes the zone id (for example to 15) in the first rule, however, still keep the second rule unchanged. During this change, there is traffic hitting the two generated megaflows, the revaldiator would revalidate all megaflows, however, the revalidator will not change the second megaflow, because zone=5 is recorded in the megaflow, so the xlate will still translate the commit action into zone=5, and the new traffic will still commit to CT as zone=5, not zone=15, resulting in taffic drops and other issues. Just like OVS set-field convention, if a field X is set by Y (Y is a variable not an Imm), we should also mask Y as a match in the generated megaflow. An exception is that if the zone-id is set by the field that is included in the frozen state (i.e. regs) and this upcall is a resume of a thawed xlate, the un-wildcarding can be skipped, as the recirc_id is a hash of the values in these fields, and it will change following the changes of these fields. When the recirc_id changes, all megaflows with the old recirc id will be invalid later. Fixes: 07659514c3 ("Add support for connection tracking.") Reported-by: Sai Su Signed-off-by: Peng He Acked-by: Mark D. Gray --- include/openvswitch/meta-flow.h | 1 + lib/meta-flow.c | 13 +++++ ofproto/ofproto-dpif-xlate.c | 32 +++++++++---- tests/system-traffic.at | 85 +++++++++++++++++++++++++++++++++ 4 files changed, 123 insertions(+), 8 deletions(-) diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h index 95e52e358..045dce8f5 100644 --- a/include/openvswitch/meta-flow.h +++ b/include/openvswitch/meta-flow.h @@ -2305,6 +2305,7 @@ void mf_set_flow_value_masked(const struct mf_field *, const union mf_value *mask, struct flow *); bool mf_is_tun_metadata(const struct mf_field *); +bool mf_is_frozen_metadata(const struct mf_field *); bool mf_is_pipeline_field(const struct mf_field *); bool mf_is_set(const struct mf_field *, const struct flow *); void mf_mask_field(const struct mf_field *, struct flow_wildcards *); diff --git a/lib/meta-flow.c b/lib/meta-flow.c index c808d205d..e03cd8d0c 100644 --- a/lib/meta-flow.c +++ b/lib/meta-flow.c @@ -1788,6 +1788,19 @@ mf_is_tun_metadata(const struct mf_field *mf) mf->id < MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS; } +bool +mf_is_frozen_metadata(const struct mf_field *mf) +{ + if (mf->id >= MFF_TUN_ID && mf->id <= MFF_IN_PORT_OXM) { + return true; + } + + if (mf->id >= MFF_REG0 && mf->id < MFF_ETH_SRC) { + return true; + } + return false; +} + bool mf_is_pipeline_field(const struct mf_field *mf) { diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 7108c8a30..7b9f3369b 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -6177,11 +6177,32 @@ static void compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc, bool is_last_action) { - ovs_u128 old_ct_label_mask = ctx->wc->masks.ct_label; - uint32_t old_ct_mark_mask = ctx->wc->masks.ct_mark; - size_t ct_offset; uint16_t zone; + if (ofc->zone_src.field) { + union mf_subvalue value; + memset(&value, 0xff, sizeof(value)); + + zone = mf_get_subfield(&ofc->zone_src, &ctx->xin->flow); + if (ctx->xin->frozen_state) { + /* If the upcall is a resume of a recirculation, we only need to + * unwildcard the fields that are not in the frozen_metadata, as + * when the rules update, OVS will generate a new recirc_id, + * which will invalidate the megaflow with old the recirc_id. + */ + if (!mf_is_frozen_metadata(ofc->zone_src.field)) { + mf_write_subfield_flow(&ofc->zone_src, &value, + &ctx->wc->masks); + } + } else { + mf_write_subfield_flow(&ofc->zone_src, &value, &ctx->wc->masks); + } + } else { + zone = ofc->zone_imm; + } + size_t ct_offset; + ovs_u128 old_ct_label_mask = ctx->wc->masks.ct_label; + uint32_t old_ct_mark_mask = ctx->wc->masks.ct_mark; /* Ensure that any prior actions are applied before composing the new * conntrack action. */ xlate_commit_actions(ctx); @@ -6193,11 +6214,6 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc, do_xlate_actions(ofc->actions, ofpact_ct_get_action_len(ofc), ctx, is_last_action, false); - if (ofc->zone_src.field) { - zone = mf_get_subfield(&ofc->zone_src, &ctx->xin->flow); - } else { - zone = ofc->zone_imm; - } ct_offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CT); if (ofc->flags & NX_CT_F_COMMIT) { diff --git a/tests/system-traffic.at b/tests/system-traffic.at index fb5b9a36d..c3917aa0d 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -1927,6 +1927,91 @@ tcp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=,dport=),reply=(src= OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([conntrack - zones from other field]) +CHECK_CONNTRACK() +OVS_TRAFFIC_VSWITCHD_START() + +ADD_NAMESPACES(at_ns0, at_ns1) + +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") + +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0. +AT_DATA([flows.txt], [dnl +priority=1,action=drop +priority=10,arp,action=normal +priority=10,icmp,action=normal +priority=100,in_port=1,tcp,ct_state=-trk,action=ct(zone=5,table=0) +priority=100,in_port=1,tcp,ct_state=+trk,action=ct(commit,zone=NXM_NX_CT_ZONE[]),2 +priority=100,in_port=2,ct_state=-trk,tcp,action=ct(table=0,zone=5) +priority=100,in_port=2,ct_state=+trk,ct_zone=5,tcp,action=1 +]) + +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt]) + +OVS_START_L7([at_ns1], [http]) + +dnl HTTP requests from p0->p1 should work fine. +NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o wget0.log]) + +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl +tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=,dport=),reply=(src=10.1.1.2,dst=10.1.1.1,sport=,dport=),zone=5,protoinfo=(state=) +]) + +dnl This is to test when the zoneid is set by a field variable like NXM_NX_CT_ZONE, the OVS xlate should generate a megaflow with a form of +dnl "ct_zone(5), ... actions: ct(commit, zone=5)". The match "ct_zone(5)" is needed as if we changes the zoneid into 15 in the following, +dnl the old "ct_zone(5), ... actions: ct(commit, zone=5)" megaflow will not get hit, and OVS will generate a new megaflow with the match "ct_zone(0xf)". +dnl This will make sure that the new packets are committing to zoneid 15 rather than old 5. +AT_CHECK([ovs-appctl dpctl/dump-flows --names filter=in_port=ovs-p0 | grep "+trk" | grep -q "ct_zone(0x5)" ], [0], []) + +AT_CHECK([ovs-ofctl mod-flows br0 'priority=100,ct_state=-trk,tcp,in_port="ovs-p0" actions=ct(table=0,zone=15)']) +NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o wget0.log]) +AT_CHECK([ovs-appctl dpctl/dump-flows --names filter=in_port=ovs-p0 | grep "+trk" | grep -q "ct_zone(0xf)" ], [0], []) + +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP + + +AT_SETUP([conntrack - zones from other field, more tests]) +CHECK_CONNTRACK() +OVS_TRAFFIC_VSWITCHD_START() + +ADD_NAMESPACES(at_ns0, at_ns1) + +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") + +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0. +AT_DATA([flows.txt], [dnl +priority=1,action=drop +priority=10,arp,action=normal +priority=10,icmp,action=normal +priority=100,in_port=1,tcp,ct_state=-trk,action=ct(zone=5,table=0,commit,exec(load:0xffff0005->NXM_NX_CT_LABEL[[0..31]])) +priority=100,in_port=1,tcp,ct_state=+trk,action=ct(commit,zone=NXM_NX_CT_LABEL[[0..15]]),2 +priority=100,in_port=2,ct_state=-trk,tcp,action=ct(table=0,zone=5) +priority=100,in_port=2,ct_state=+trk,ct_zone=5,tcp,action=1 +]) + +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt]) + +OVS_START_L7([at_ns1], [http]) + +dnl HTTP requests from p0->p1 should work fine. +NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o wget0.log]) + +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl +tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=,dport=),reply=(src=10.1.1.2,dst=10.1.1.1,sport=,dport=),zone=5,labels=0xffff0005,protoinfo=(state=) +]) + +AT_CHECK([ovs-appctl dpctl/dump-flows --names filter=in_port=ovs-p0 | grep "+trk" | grep -q "ct_label(0xffff0005/0xffff)" ], [0], []) + +AT_CHECK([ovs-ofctl mod-flows br0 'priority=100,ct_state=-trk,tcp,in_port="ovs-p0" actions=ct(table=0,zone=15,commit,exec(load:0xffff000f->NXM_NX_CT_LABEL[[0..31]]))']) +NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o wget0.log]) +AT_CHECK([ovs-appctl dpctl/dump-flows --names filter=in_port=ovs-p0 | grep "+trk" | grep -q "ct_label(0xffff00ff/0xffff)" ], [0], []) + +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([conntrack - multiple bridges]) CHECK_CONNTRACK() OVS_TRAFFIC_VSWITCHD_START(