[PATCH] policy: ignore invalid TCP flags combo in conntrack
Morten Brørup
mb at smartsharesystems.com
Mon Aug 31 13:26:40 CEST 2026
Do not update the connection state when passed a packet with an invalid
TCP flags combination.
Clean up and optimize conntrack:
The flow direction (forward/reverse) is encoded in the least significant
bit of the connection data pointer.
Change the value (and name) of this bit, so it has the same value as the
conn_flow_t enum; i.e. 0 means forward, and 1 means reverse direction.
Add more comments to clarify how tcp_flagstate() is implemented.
Remove _Atomic from the connection timestamp; there is no need for it.
Only update the connection timestamp if more than 1/4 second has passed
since the last update.
Remove the local variable "data" in gr_conn_insert(); it was only used
for type casting.
Use signed types for time in do_ageing(), and simplify the code.
Make the code for hash iterations similar throughout the file, so the
check for forward flow direction always comes first.
Signed-off-by: Morten Brørup <mb at smartsharesystems.com>
---
modules/policy/control/conntrack.c | 133 ++++++++++++++++++-----------
modules/policy/control/conntrack.h | 2 +-
2 files changed, 84 insertions(+), 51 deletions(-)
diff --git a/modules/policy/control/conntrack.c b/modules/policy/control/conntrack.c
index 9b5391ff..2954f10d 100644
--- a/modules/policy/control/conntrack.c
+++ b/modules/policy/control/conntrack.c
@@ -48,32 +48,45 @@ static const gr_conn_state_t generic_state_machine[CONN_S_COUNT][CONN_FLOW_COUNT
typedef enum {
TCP_FS_INVALID = 0,
- TCP_FS_SYN,
- TCP_FS_SYNACK,
- TCP_FS_ACK,
- TCP_FS_FIN,
+ TCP_FS_SYN, // 1
+ TCP_FS_SYNACK, // 2
+ TCP_FS_ACK, // 3
+ TCP_FS_FIN, // 4
TCP_FS_COUNT,
} tcp_flagstate_t;
-static inline tcp_flagstate_t tcp_flagstate(const uint8_t tcp_flags) {
+static inline tcp_flagstate_t tcp_flagstate(uint8_t tcp_flags) {
tcp_flagstate_t s;
// Flags are shifted to use three least significant bits, thus each
- // flag combination has a unique number ranging from 0 to 7, e.g.
- // TH_SYN | TH_ACK has number 6, since (0x02 | (0x10 >> 2)) == 6.
+ // flag combination has a unique number ranging from 0 to 0x7, e.g.
+ // SYN | ACK has number 0x6, since (0x02 | (0x10 >> 2)) == 0x6.
// However, the requirement is to have number 0 for invalid cases,
- // such as TH_SYN | TH_FIN, and to have the same number for TH_FIN
- // and TH_FIN|TH_ACK cases. Thus, we generate a mask assigning 3
- // bits for each number, which contains the actual case numbers:
+ // such as SYN | FIN, and to have the same number for FIN and
+ // and FIN | ACK cases. Thus, we use a lookup table indexed by
+ // the flags combination to get the enum value.
+ // The lookup table is implemented as a word of nibbles, ordered
+ // from the least to the most significant nibble, where lookup is
+ // performed by shifting down the word by index number of nibbles,
+ // and masking the least significant nibble.
//
- // TCP_FS_SYNACK << (6 << 2) == 0x2000000 (6 - SYN,ACK)
- // TCP_FS_FIN << (5 << 2) == 0x0400000 (5 - FIN,ACK)
- // ...
+ // +---+------+------+------+---------+
+ // |idx|ACK(4)|SYN(2)|FIN(1)|flagstate|
+ // +---+------+------+------+---------+
+ // | 0| | | |INVALID 0|
+ // | 1| | | X |FIN 4|
+ // | 2| | X | |SYN 1|
+ // | 3| | X | X |INVALID 0|
+ // | 4| X | | |ACK 3|
+ // | 5| X | | X |FIN 4|
+ // | 6| X | X | |SYNACK 2|
+ // | 7| X | X | X |INVALID 0|
+ // +---+------+------+------+---------+
//
- // Hence, OR'ed mask value is 0x2430140.
- s = tcp_flags & (RTE_TCP_SYN_FLAG | RTE_TCP_FIN_FLAG);
- s |= (tcp_flags & RTE_TCP_ACK_FLAG) >> 2;
- s = (0x2430140 >> (s << 2)) & 7;
+ // Hence, the lookup table word is 0x02430140.
+ tcp_flags = (tcp_flags & (RTE_TCP_SYN_FLAG /*0x2*/ | RTE_TCP_FIN_FLAG /*0x1*/))
+ | ((tcp_flags & RTE_TCP_ACK_FLAG /*0x10*/) >> 2);
+ s = (0x02430140 >> (tcp_flags << 2)) & 0xF;
assert(s < TCP_FS_COUNT);
@@ -229,19 +242,31 @@ void gr_conn_update(struct conn *c, conn_flow_t flow, const struct rte_tcp_hdr *
again:
cur_state = atomic_load(&c->state);
- if (c->fwd_key.proto == IPPROTO_TCP)
+ if (c->fwd_key.proto == IPPROTO_TCP) {
new_state = tcp_state_machine[cur_state][flow][tcp_flagstate(tcp->tcp_flags)];
- else
- new_state = generic_state_machine[cur_state][flow];
- // TODO: inspect TCP window to determine if packet is part of the connection.
+ // When the state machine is passed an invalid flags combination,
+ // it returns CONN_S_CLOSED.
+ // In that case, ignore the request to update the connection state.
+ if (unlikely(new_state == CONN_S_CLOSED))
+ return;
+
+ // TODO: inspect TCP window to determine if packet is part of the connection.
+ } else
+ new_state = generic_state_machine[cur_state][flow];
- if (new_state != cur_state) {
+ if (unlikely(new_state != cur_state)) {
if (!atomic_compare_exchange_weak(&c->state, &cur_state, new_state))
goto again;
+
+ c->last_update = gr_clock_ns();
+ return;
}
- atomic_store(&c->last_update, gr_clock_ns());
+ // Update the timestamp if at least 1/4 second since the last update.
+ gr_clock_ns_t now = gr_clock_ns();
+ if (unlikely(now - c->last_update > GR_NS_PER_S / 4))
+ c->last_update = now;
}
bool gr_conn_parse_key(
@@ -328,21 +353,24 @@ static _Atomic(struct rte_hash *) conn_hash;
static _Atomic(struct rte_mempool *) conn_pool;
static struct event *ageing_timer;
-#define CONN_FLOW_FWD_BIT ((uintptr_t)0x1)
+#define CONN_FLOW_REV_BIT ((uintptr_t)0x1)
+
+static_assert(CONN_FLOW_REV_BIT != 0);
+static_assert(CONN_FLOW_REV_BIT == (uintptr_t)CONN_FLOW_REV);
static inline conn_flow_t conn_flow(void *data) {
- if ((uintptr_t)data & CONN_FLOW_FWD_BIT)
- return CONN_FLOW_FWD;
- return CONN_FLOW_REV;
+ return (conn_flow_t)((uintptr_t)data & CONN_FLOW_REV_BIT);
}
static inline struct conn *conn_ptr(void *data) {
- return (struct conn *)((uintptr_t)data & ~CONN_FLOW_FWD_BIT);
+ return (struct conn *)((uintptr_t)data & ~CONN_FLOW_REV_BIT);
}
static inline void *conn_data(struct conn *conn, conn_flow_t flow) {
- if (flow == CONN_FLOW_FWD)
- return (void *)((uintptr_t)conn | CONN_FLOW_FWD_BIT);
+ if (!__rte_constant(flow))
+ return (void *)((uintptr_t)conn | (uintptr_t)flow);
+ if (flow == CONN_FLOW_REV)
+ return (void *)((uintptr_t)conn | CONN_FLOW_REV_BIT);
return conn;
}
@@ -359,28 +387,32 @@ struct conn *gr_conn_lookup(const struct conn_key *key, conn_flow_t *flow) {
struct conn *gr_conn_insert(const struct conn_key *fwd_key, const struct conn_key *rev_key) {
struct conn *conn;
- void *data;
// create a new connection object
- if (rte_mempool_get(conn_pool, &data) < 0)
+ if (rte_mempool_get(conn_pool, (void **)&conn) < 0)
return NULL;
- conn = data;
- memset(conn, 0, sizeof(*conn));
- conn->rev_key = *rev_key;
+ static_assert(
+ offsetof(struct conn, fwd_key) == 0
+ && offsetof(struct conn, rev_key) == RTE_SIZEOF_FIELD(struct conn, fwd_key)
+ );
conn->fwd_key = *fwd_key;
+ conn->rev_key = *rev_key;
+ memset((char *)conn + 2 * sizeof(struct conn_key),
+ 0,
+ sizeof(*conn) - 2 * sizeof(struct conn_key));
- if (rte_hash_add_key_data(conn_hash, fwd_key, conn_data(data, CONN_FLOW_FWD)) < 0) {
+ if (rte_hash_add_key_data(conn_hash, fwd_key, conn_data(conn, CONN_FLOW_FWD)) < 0) {
// hash full
- rte_mempool_put(conn_pool, data);
+ rte_mempool_put(conn_pool, conn);
return NULL;
}
// Also reference the conntrack by its *reverse* key for replies.
- if (rte_hash_add_key_data(conn_hash, rev_key, conn_data(data, CONN_FLOW_REV)) < 0) {
+ if (rte_hash_add_key_data(conn_hash, rev_key, conn_data(conn, CONN_FLOW_REV)) < 0) {
// hash full, remove forward key,
rte_hash_del_key(conn_hash, fwd_key);
- rte_mempool_put(conn_pool, data);
+ rte_mempool_put(conn_pool, conn);
return NULL;
}
@@ -388,8 +420,8 @@ struct conn *gr_conn_insert(const struct conn_key *fwd_key, const struct conn_ke
}
static void do_ageing(evutil_socket_t, short /*what*/, void * /*priv*/) {
- gr_clock_ns_t now = gr_clock_ns(), last;
- uint64_t age, timeout;
+ gr_clock_ns_t now = gr_clock_ns();
+ time_t timeout;
struct conn *conn;
const void *key;
uint32_t iter;
@@ -397,10 +429,10 @@ static void do_ageing(evutil_socket_t, short /*what*/, void * /*priv*/) {
iter = 0;
while (rte_hash_iterate(conn_hash, &key, &data, &iter) >= 0) {
- conn = conn_ptr(data);
if (conn_flow(data) != CONN_FLOW_FWD)
continue;
+ conn = conn_ptr(data);
switch (atomic_load(&conn->state)) {
case CONN_S_NEW:
case CONN_S_SIMSYN_SENT:
@@ -413,7 +445,10 @@ static void do_ageing(evutil_socket_t, short /*what*/, void * /*priv*/) {
timeout = conf.timeout_tcp_established_sec;
break;
case IPPROTO_UDP:
- if (conn->fwd_key.dst_id == RTE_BE16(53))
+ // RFC 4787 section 4.3 REQ-5a:
+ // UDP ports in the well-known range may have shorter timers,
+ // specific to the application running over that specific port.
+ if (conn->fwd_key.dst_id == RTE_BE16(53)) // DNS
timeout = 2;
else
timeout = conf.timeout_udp_established_sec;
@@ -440,11 +475,7 @@ static void do_ageing(evutil_socket_t, short /*what*/, void * /*priv*/) {
break;
}
- last = atomic_load(&conn->last_update);
- if (last > now)
- continue;
- age = (now - last) / GR_NS_PER_S;
- if (age > timeout)
+ if (now - conn->last_update > timeout * GR_NS_PER_S)
gr_conn_destroy(conn);
}
}
@@ -459,6 +490,7 @@ void gr_conn_snat44_purge(struct snat44_policy *policy) {
while (rte_hash_iterate(h, &key, &data, &next) >= 0) {
if (conn_flow(data) != CONN_FLOW_FWD)
continue;
+
conn = conn_ptr(data);
if (conn->nat.policy == policy)
gr_conn_destroy(conn);
@@ -588,7 +620,7 @@ static struct api_out conntrack_list(const void * /*request*/, struct api_ctx *c
.dst_id = conn->rev_key.dst_id,
},
.state = atomic_load(&conn->state),
- .last_update = atomic_load(&conn->last_update),
+ .last_update = conn->last_update,
};
api_send(ctx, sizeof(ct), &ct);
}
@@ -604,9 +636,10 @@ static struct api_out conntrack_flush(const void * /*request*/, struct api_ctx *
iter = 0;
while (rte_hash_iterate(conn_hash, &key, &data, &iter) >= 0) {
- conn = conn_ptr(data);
if (conn_flow(data) != CONN_FLOW_FWD)
continue;
+
+ conn = conn_ptr(data);
gr_conn_destroy(conn);
}
diff --git a/modules/policy/control/conntrack.h b/modules/policy/control/conntrack.h
index 51f8dd7c..bdeb5ea9 100644
--- a/modules/policy/control/conntrack.h
+++ b/modules/policy/control/conntrack.h
@@ -43,7 +43,7 @@ struct conn {
struct conn_key fwd_key;
struct conn_key rev_key;
_Atomic(gr_conn_state_t) state;
- _Atomic(gr_clock_ns_t) last_update;
+ gr_clock_ns_t last_update;
struct nat44 nat;
};
--
2.43.0
More information about the grout
mailing list