diff mbox series

[ovs-dev,v5,1/3] lib/conntrack: Only use given packet in protocol detection.

Message ID 20231211105103.30812-1-viacheslav.galaktionov@arknetworks.am
State Accepted
Commit 14ef8b451f47ef3fa14456790ac8ce4eb824054f
Delegated to: aaron conole
Headers show
Series [ovs-dev,v5,1/3] lib/conntrack: Only use given packet in protocol detection. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Viacheslav Galaktionov Dec. 11, 2023, 10:51 a.m. UTC 
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 <viacheslav.galaktionov@arknetworks.am>
---
 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(-)

Comments

Aaron Conole Jan. 11, 2024, 1:28 a.m. UTC | #1 
Viacheslav Galaktionov via dev <ovs-dev@openvswitch.org> writes:

> 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 <viacheslav.galaktionov@arknetworks.am>
> ---

Thanks, applied.
diff mbox series

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 71c470661..9bb3c17f8 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -657,8 +657,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
@@ -668,26 +667,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;
@@ -1229,8 +1226,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) {
@@ -1251,8 +1247,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,
@@ -1329,7 +1324,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)
 {
@@ -1345,7 +1340,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,
@@ -1354,8 +1349,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 18c182f85..0a888be45 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 9a59a1b03..db37d56b2 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -9232,9 +9232,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);
     }
 
 }