[PATCH] policy: ignore invalid TCP flags combo in conntrack

[Robin Jarry]

Morten Brørup, Aug 31, 2026 at 13:26:
> 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;

This is a similar pattern to the FDB code which was changed recently.
You didn't mention why the atomic_* calls were removed. Could you
explain?


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

This change seems unrelated to the TCP flags check.

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

Could you avoid unnecessary code churn?

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

See my comment above. Could you motivate the removal of _Atomic() here?

>  	struct nat44 nat;
>  };
>  

Cheers,


-- 
Robin

# May not be combined with other discounts.