From patchwork Thu Mar 10 19:42:43 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1604101 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=2605:bc80:3010::137; helo=smtp4.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp4.osuosl.org (smtp4.osuosl.org [IPv6:2605:bc80:3010::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 bilbo.ozlabs.org (Postfix) with ESMTPS id 4KF0Bl47T4z9sBy for ; Fri, 11 Mar 2022 06:54:21 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id DB35941915; Thu, 10 Mar 2022 19:54:18 +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 s5Hvsd5wCHhu; Thu, 10 Mar 2022 19:54:17 +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 B5D15418F8; Thu, 10 Mar 2022 19:54:16 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 6D4D0C001D; Thu, 10 Mar 2022 19:54:16 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp2.osuosl.org (smtp2.osuosl.org [IPv6:2605:bc80:3010::133]) by lists.linuxfoundation.org (Postfix) with ESMTP id 50683C000B for ; Thu, 10 Mar 2022 19:54:15 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id 4B98E4053E for ; Thu, 10 Mar 2022 19:54:15 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id mjhrnP1C5F5T for ; Thu, 10 Mar 2022 19:54:13 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from mslow1.mail.gandi.net (mslow1.mail.gandi.net [217.70.178.240]) by smtp2.osuosl.org (Postfix) with ESMTPS id 835674018E for ; Thu, 10 Mar 2022 19:54:12 +0000 (UTC) Received: from relay6-d.mail.gandi.net (unknown [IPv6:2001:4b98:dc4:8::226]) by mslow1.mail.gandi.net (Postfix) with ESMTP id DAF4AC51A3 for ; Thu, 10 Mar 2022 19:42:58 +0000 (UTC) Received: (Authenticated sender: numans@ovn.org) by mail.gandi.net (Postfix) with ESMTPSA id DC209C0005; Thu, 10 Mar 2022 19:42:51 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Thu, 10 Mar 2022 14:42:43 -0500 Message-Id: <20220310194243.2624700-1-numans@ovn.org> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Cc: Michael Washer Subject: [ovs-dev] [PATCH ovn v2] ovn-northd: Don't commit to conntrack if the conntrack was skipped. 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: Numan Siddique Presently, if the inport or outport is a peer port (of router port), then we skip sending the packet to conntrack by not setting the reg0[0]/reg0[1] bits. But the packet still goes through the stages - ls_in_acl_hint, ls_in_acl and ls_in_stateful in the ingress pipeline and similarly ls_out_acl_hint, ls_out_acl and ls_out_stateful in the egress pipeline of the logical switch. In these mentioned stages, the logical flows match on the ct states (ct.new, ct.est etc) and this can be problematic if the packet was sent to conntrack and committed in the ingress pipeline. When that packet enters the egress pipeline with outport set to the peer port, we skip sending the packet to conntrack (which is expected) but ct state values carry over from the ingress state. If the ct.new flag was set in the ingress pipeline, then the below flows are hit and the packet gets committed in the conntrack zone of the inport logical port. table=4 (ls_out_acl ), priority=1 , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[1] = 1; next;) table=7 (ls_out_stateful ), priority=100 , match=(reg0[1] == 1 && reg0[13] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) With this extra commit to the same conntrack zone, sometimes the packet gets dropped or doesn't reach the destination. It is not very clear how the packet gets misplaced, but avoiding this resolves the issue. And OVN ideally should not do this extra commit. This patch addresses this issue by adding the below logical flow both in ls_in_acl and ls_out_acl stage priority=2 , match=(reg0[0] == 0 && reg0[1] == 0), action=(next;) Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2060688 Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2062431 Reported-by: Michael Washer Signed-off-by: Numan Siddique --- v1 -> v2 ------- * Changed the approach to solve the issue. northd/northd.c | 17 +++++++++++++++-- northd/ovn-northd.8.xml | 6 ++++++ tests/ovn-northd.at | 5 +++++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index b264fb850b..23ab5ac260 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -6529,7 +6529,7 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL_AFTER_LB, 0, "1", "next;"); if (has_stateful) { - /* Ingress and Egress ACL Table (Priority 1). + /* Ingress and Egress ACL Table (Priority 1 and 2). * * By default, traffic is allowed. This is partially handled by * the Priority 0 ACL flows added earlier, but we also need to @@ -6549,7 +6549,20 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, * We need to set ct_label.blocked=0 to let the connection continue, * which will be done by ct_commit() in the "stateful" stage. * Subsequent packets will hit the flow at priority 0 that just - * uses "next;". */ + * uses "next;". + * + * However, we don't want to commit if the packet was not sent to + * conntrack in the first place. + * Eg. if inport or outport is a router peer port. */ + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 2, + REGBIT_CONNTRACK_DEFRAG" == 0 && " + REGBIT_CONNTRACK_NAT" == 0", + "next;"); + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 2, + REGBIT_CONNTRACK_DEFRAG" == 0 && " + REGBIT_CONNTRACK_NAT" == 0", + "next;"); + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 1, "ip && (!ct.est || (ct.est && ct_label.blocked == 1))", REGBIT_CONNTRACK_COMMIT" = 1; next;"); diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index 4784bff041..d788efe70b 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -784,6 +784,12 @@

    +
  • + A priority-2 flow that advances the packet to the next stage if the + packet was not sent to conntrack earlier, by matching on + reg0[0] == 0 && reg0[1] == 1. +
    • +
  • A priority-1 flow that sets the hint to commit IP traffic to the connection tracker (with action reg0[1] = 1; next;). This diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 60d91a7717..bfd0f2382b 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -2265,6 +2265,7 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e table=4 (ls_out_acl ), priority=1 , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;) table=4 (ls_out_acl ), priority=1001 , match=(reg0[[7]] == 1 && (ip)), action=(reg0[[1]] = 1; next;) table=4 (ls_out_acl ), priority=1001 , match=(reg0[[8]] == 1 && (ip)), action=(next;) + table=4 (ls_out_acl ), priority=2 , match=(reg0[[0]] == 0 && reg0[[2]] == 0), action=(next;) table=4 (ls_out_acl ), priority=34000, match=(eth.src == $svc_monitor_mac), action=(next;) table=4 (ls_out_acl ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) table=4 (ls_out_acl ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) @@ -2282,6 +2283,7 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e table=9 (ls_in_acl ), priority=1 , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;) table=9 (ls_in_acl ), priority=1001 , match=(reg0[[7]] == 1 && (ip)), action=(reg0[[1]] = 1; next;) table=9 (ls_in_acl ), priority=1001 , match=(reg0[[8]] == 1 && (ip)), action=(next;) + table=9 (ls_in_acl ), priority=2 , match=(reg0[[0]] == 0 && reg0[[2]] == 0), action=(next;) table=9 (ls_in_acl ), priority=34000, match=(eth.dst == $svc_monitor_mac), action=(next;) table=9 (ls_in_acl ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) table=9 (ls_in_acl ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(reg0[[9]] = 0; reg0[[10]] = 0; next;) @@ -6328,6 +6330,7 @@ AT_CAPTURE_FILE([lsflows]) AT_CHECK([grep -e "ls_in_acl" lsflows | sed 's/table=../table=??/' | sort], [0], [dnl table=??(ls_in_acl ), priority=0 , match=(1), action=(next;) table=??(ls_in_acl ), priority=1 , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;) + table=??(ls_in_acl ), priority=2 , match=(reg0[[0]] == 0 && reg0[[2]] == 0), action=(next;) table=??(ls_in_acl ), priority=2001 , match=(reg0[[10]] == 1 && (ip4)), action=(ct_commit { ct_label.blocked = 1; }; /* drop */) table=??(ls_in_acl ), priority=2001 , match=(reg0[[9]] == 1 && (ip4)), action=(/* drop */) table=??(ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (ip4 && tcp)), action=(reg0[[1]] = 1; next;) @@ -6380,6 +6383,7 @@ AT_CAPTURE_FILE([lsflows]) AT_CHECK([grep -e "ls_in_acl" lsflows | sed 's/table=../table=??/' | sort], [0], [dnl table=??(ls_in_acl ), priority=0 , match=(1), action=(next;) table=??(ls_in_acl ), priority=1 , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;) + table=??(ls_in_acl ), priority=2 , match=(reg0[[0]] == 0 && reg0[[2]] == 0), action=(next;) table=??(ls_in_acl ), priority=34000, match=(eth.dst == $svc_monitor_mac), action=(next;) table=??(ls_in_acl ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) table=??(ls_in_acl ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(reg0[[9]] = 0; reg0[[10]] = 0; next;) @@ -6432,6 +6436,7 @@ AT_CAPTURE_FILE([lsflows]) AT_CHECK([grep -e "ls_in_acl" lsflows | sed 's/table=../table=??/' | sort], [0], [dnl table=??(ls_in_acl ), priority=0 , match=(1), action=(next;) table=??(ls_in_acl ), priority=1 , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;) + table=??(ls_in_acl ), priority=2 , match=(reg0[[0]] == 0 && reg0[[2]] == 0), action=(next;) table=??(ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (ip4 && tcp)), action=(reg0[[1]] = 1; next;) table=??(ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (ip4 && tcp)), action=(next;) table=??(ls_in_acl ), priority=2003 , match=(reg0[[7]] == 1 && (ip4 && icmp)), action=(reg0[[1]] = 1; next;)