From patchwork Thu Oct 19 11:22:41 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viacheslav Galaktionov X-Patchwork-Id: 1851576 X-Patchwork-Delegate: aconole@redhat.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=fail reason="key not found in DNS" header.d=arknetworks.am header.i=@arknetworks.am header.a=rsa-sha256 header.s=default header.b=OdmDc5GV; dkim-atps=neutral Authentication-Results: legolas.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=patchwork.ozlabs.org) 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 ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4SB53H4SHbz20cx for ; Thu, 19 Oct 2023 22:24:43 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 42D8441969; Thu, 19 Oct 2023 11:24:41 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 42D8441969 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 dTWD6SrNeo9J; Thu, 19 Oct 2023 11:24:39 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp4.osuosl.org (Postfix) with ESMTPS id E21BA42D42; Thu, 19 Oct 2023 11:24:37 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org E21BA42D42 Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id E72E2C0DD7; Thu, 19 Oct 2023 11:24:36 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id E4588C0039 for ; Thu, 19 Oct 2023 11:24:34 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id F323E6FB3E for ; Thu, 19 Oct 2023 11:24:32 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org F323E6FB3E X-Virus-Scanned: amavisd-new at osuosl.org 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 dJmRpWMFm1Lu for ; Thu, 19 Oct 2023 11:24:32 +0000 (UTC) Received: from agw.arknetworks.am (agw.arknetworks.am [79.141.165.80]) by smtp3.osuosl.org (Postfix) with ESMTPS id B011360A70 for ; Thu, 19 Oct 2023 11:24:31 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org B011360A70 Received: from localhost.localdomain (unknown [141.136.88.27]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by agw.arknetworks.am (Postfix) with ESMTPSA id BA31BE0D40 for ; Thu, 19 Oct 2023 15:24:28 +0400 (+04) DKIM-Filter: OpenDKIM Filter v2.11.0 agw.arknetworks.am BA31BE0D40 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arknetworks.am; s=default; t=1697714668; bh=J14VJfmE8qccCK/wuyPv5fAG6t6Nsl2ljGHc6LX1nqc=; h=From:To:Subject:Date:From; b=OdmDc5GVXtlEwpbjh+10fzTBhvVZUL+/eFlbaVIfw3S3w78FzMLdLDgG8uJlHizrr +PTKzkOe/bCc+05c4DWatXBwCo1jPrVResEITC/20xXgeb6lwFF+VQ26DQy+uZE6QD lvFP1LO8ktLQxLeFbFndd9U3h/b4vESARDNyWBZdwOtDQK+sspednbmnur5Y/s2Eir tpV+TUdZ1Ydf9g3WLu1NkUBA+AMftRE4h3tdk+ZbneCEigPGujQsc+ot4e0jcdpVP6 0HyaF59k0aBlXwMp6Y9lVVyygZpUqVdLremIbBoF/qcQ7+Xx89qohzhvqspLnyXqWQ xkjWnNLVKnyTA== To: dev@openvswitch.org Date: Thu, 19 Oct 2023 15:22:41 +0400 Message-ID: <20231019112243.2421-1-viacheslav.galaktionov@arknetworks.am> X-Mailer: git-send-email 2.42.0 MIME-Version: 1.0 Subject: [ovs-dev] [PATCH v3 1/3] lib/conntrack: Only use given packet in protocol detection. 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: , X-Patchwork-Original-From: Viacheslav Galaktionov via dev From: Viacheslav Galaktionov Reply-To: Viacheslav Galaktionov Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" The current protocol detection logic relies on two pieces of metadata passed as arguments: tp_src and tp_dst, which represent the L4 source and destination port numbers from the flow that triggered the current flow rule first, and was responsible for creating the current DP flow. Since multiple network flows of many different kinds, potentially using different protocols on all layers, can be processed by one flow rule, using the metadata of some unrelated flow might lead to unexpected results. For example, ICMP type and code can be interpreted as TCP source and destination ports. This can confuse the code responsible for the helper selection, leading to errors in traffic handling and incorrect detection of related flows. One of the easiest ways to fix this problem is to simply remove the tp_src and tp_dst parameters from the picture. The current code base has no good use for them. The helper selection logic was based on these values and therefore needs to be changed. Ensure that the helper specified in a flow rule is used, given it is compatible with the L4 protocol of the packet. When a flow rule does not specify a helper, one can still be picked using the given packet's metadata like TCP/UDP ports. Signed-off-by: Viacheslav Galaktionov --- lib/conntrack.c | 40 +++++++++++++++++----------------------- lib/conntrack.h | 2 +- lib/dpif-netdev.c | 5 ++--- tests/test-conntrack.c | 6 +++--- 4 files changed, 23 insertions(+), 30 deletions(-) diff --git a/lib/conntrack.c b/lib/conntrack.c index 47a443fba..c27ac5a6f 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -655,8 +655,7 @@ is_ftp_ctl(const enum ct_alg_ctl_type ct_alg_ctl) } static enum ct_alg_ctl_type -get_alg_ctl_type(const struct dp_packet *pkt, ovs_be16 tp_src, ovs_be16 tp_dst, - const char *helper) +get_alg_ctl_type(const struct dp_packet *pkt, const char *helper) { /* CT_IPPORT_FTP/TFTP is used because IPPORT_FTP/TFTP in not defined * in OSX, at least in in.h. Since these values will never change, remove @@ -666,26 +665,24 @@ get_alg_ctl_type(const struct dp_packet *pkt, ovs_be16 tp_src, ovs_be16 tp_dst, uint8_t ip_proto = get_ip_proto(pkt); struct udp_header *uh = dp_packet_l4(pkt); struct tcp_header *th = dp_packet_l4(pkt); - ovs_be16 ftp_src_port = htons(CT_IPPORT_FTP); - ovs_be16 ftp_dst_port = htons(CT_IPPORT_FTP); - ovs_be16 tftp_dst_port = htons(CT_IPPORT_TFTP); + ovs_be16 ftp_port = htons(CT_IPPORT_FTP); + ovs_be16 tftp_port = htons(CT_IPPORT_TFTP); - if (OVS_UNLIKELY(tp_dst)) { - if (helper && !strncmp(helper, "ftp", strlen("ftp"))) { - ftp_dst_port = tp_dst; - } else if (helper && !strncmp(helper, "tftp", strlen("tftp"))) { - tftp_dst_port = tp_dst; + if (helper) { + if ((ip_proto == IPPROTO_TCP) && + !strncmp(helper, "ftp", strlen("ftp"))) { + return CT_ALG_CTL_FTP; } - } else if (OVS_UNLIKELY(tp_src)) { - if (helper && !strncmp(helper, "ftp", strlen("ftp"))) { - ftp_src_port = tp_src; + if ((ip_proto == IPPROTO_UDP) && + !strncmp(helper, "tftp", strlen("tftp"))) { + return CT_ALG_CTL_TFTP; } } - if (ip_proto == IPPROTO_UDP && uh->udp_dst == tftp_dst_port) { + if (ip_proto == IPPROTO_UDP && uh->udp_dst == tftp_port) { return CT_ALG_CTL_TFTP; } else if (ip_proto == IPPROTO_TCP && - (th->tcp_src == ftp_src_port || th->tcp_dst == ftp_dst_port)) { + (th->tcp_src == ftp_port || th->tcp_dst == ftp_port)) { return CT_ALG_CTL_FTP; } return CT_ALG_CTL_NONE; @@ -1227,8 +1224,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, bool force, bool commit, long long now, const uint32_t *setmark, const struct ovs_key_ct_labels *setlabel, const struct nat_action_info_t *nat_action_info, - ovs_be16 tp_src, ovs_be16 tp_dst, const char *helper, - uint32_t tp_id) + const char *helper, uint32_t tp_id) { /* Reset ct_state whenever entering a new zone. */ if (pkt->md.ct_state && pkt->md.ct_zone != zone) { @@ -1249,8 +1245,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, conn = NULL; } - enum ct_alg_ctl_type ct_alg_ctl = get_alg_ctl_type(pkt, tp_src, tp_dst, - helper); + enum ct_alg_ctl_type ct_alg_ctl = get_alg_ctl_type(pkt, helper); if (OVS_LIKELY(conn)) { if (OVS_LIKELY(!conn_update_state_alg(ct, pkt, ctx, conn, @@ -1327,7 +1322,7 @@ conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch, ovs_be16 dl_type, bool force, bool commit, uint16_t zone, const uint32_t *setmark, const struct ovs_key_ct_labels *setlabel, - ovs_be16 tp_src, ovs_be16 tp_dst, const char *helper, + const char *helper, const struct nat_action_info_t *nat_action_info, long long now, uint32_t tp_id) { @@ -1343,7 +1338,7 @@ conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch, write_ct_md(packet, zone, NULL, NULL, NULL); } else if (conn && conn->key_node[CT_DIR_FWD].key.zone == zone && !force && - !get_alg_ctl_type(packet, tp_src, tp_dst, helper)) { + !get_alg_ctl_type(packet, helper)) { process_one_fast(zone, setmark, setlabel, nat_action_info, conn, packet); } else if (OVS_UNLIKELY(!conn_key_extract(ct, packet, dl_type, &ctx, @@ -1352,8 +1347,7 @@ conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch, write_ct_md(packet, zone, NULL, NULL, NULL); } else { process_one(ct, packet, &ctx, zone, force, commit, now, setmark, - setlabel, nat_action_info, tp_src, tp_dst, helper, - tp_id); + setlabel, nat_action_info, helper, tp_id); } } diff --git a/lib/conntrack.h b/lib/conntrack.h index 57d5159b6..0ef415738 100644 --- a/lib/conntrack.h +++ b/lib/conntrack.h @@ -92,7 +92,7 @@ int conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch, ovs_be16 dl_type, bool force, bool commit, uint16_t zone, const uint32_t *setmark, const struct ovs_key_ct_labels *setlabel, - ovs_be16 tp_src, ovs_be16 tp_dst, const char *helper, + const char *helper, const struct nat_action_info_t *nat_action_info, long long now, uint32_t tp_id); void conntrack_clear(struct dp_packet *packet); diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 157694bcf..48cacdb8d 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -9228,9 +9228,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_, } conntrack_execute(dp->conntrack, packets_, aux->flow->dl_type, force, - commit, zone, setmark, setlabel, aux->flow->tp_src, - aux->flow->tp_dst, helper, nat_action_info_ref, - pmd->ctx.now / 1000, tp_id); + commit, zone, setmark, setlabel, helper, + nat_action_info_ref, pmd->ctx.now / 1000, tp_id); break; } diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c index 24c93e4a4..292b6c048 100644 --- a/tests/test-conntrack.c +++ b/tests/test-conntrack.c @@ -91,7 +91,7 @@ ct_thread_main(void *aux_) ovs_barrier_block(&barrier); for (i = 0; i < n_pkts; i += batch_size) { conntrack_execute(ct, pkt_batch, dl_type, false, true, 0, NULL, NULL, - 0, 0, NULL, NULL, now, 0); + NULL, NULL, now, 0); DP_PACKET_BATCH_FOR_EACH (j, pkt, pkt_batch) { pkt_metadata_init_conn(&pkt->md); } @@ -178,7 +178,7 @@ pcap_batch_execute_conntrack(struct conntrack *ct_, if (flow.dl_type != dl_type) { conntrack_execute(ct_, &new_batch, dl_type, false, true, 0, - NULL, NULL, 0, 0, NULL, NULL, now, 0); + NULL, NULL, NULL, NULL, now, 0); dp_packet_batch_init(&new_batch); } dp_packet_batch_add(&new_batch, packet); @@ -186,7 +186,7 @@ pcap_batch_execute_conntrack(struct conntrack *ct_, if (!dp_packet_batch_is_empty(&new_batch)) { conntrack_execute(ct_, &new_batch, dl_type, false, true, 0, NULL, NULL, - 0, 0, NULL, NULL, now, 0); + NULL, NULL, now, 0); } }