Skip to content

Commit

Permalink
lib/conntrack: Only use given packet in protocol detection.
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Signed-off-by: 0-day Robot <[email protected]>
  • Loading branch information
@okt-galaktionov @ovsrobot
okt-galaktionov authored and ovsrobot committed Dec 11, 2023
1 parent cc670e7 commit f1c61ff
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 30 deletions.
40 changes: 17 additions & 23 deletions lib/conntrack.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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,
Expand Down Expand Up @@ -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)
{
Expand All @@ -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,
Expand All @@ -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);
}
}

Expand Down
2 changes: 1 addition & 1 deletion lib/conntrack.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
5 changes: 2 additions & 3 deletions lib/dpif-netdev.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
6 changes: 3 additions & 3 deletions tests/test-conntrack.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -178,15 +178,15 @@ 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);
}

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);
}

}
Expand Down

0 comments on commit f1c61ff

Please sign in to comment.