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

[Morten Brørup]

> From: Robin Jarry [mailto:rjarry at redhat.com]
> Sent: Monday, 31 August 2026 16.18
> 
> 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.

Correct. Back when I worked on my FDB optimization patch, this one also caught my eye.

> You didn't mention why the atomic_* calls were removed. Could you
> explain?

Atomic operations are costly, and should not be used unless required.
The timestamp in the gr_fdb_entry is not _Atomic(), and I cannot come up with any reason for this timestamp to be _Atomic().
So I removed _Atomic() for improved performance.

I'm assuming Grout is not for 32-bit architectures, so we don't have to consider torn reads/writes for 64-bit variables?

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

Correct.
I added some cleaning up as part of this patch.

> 
> > +
> > +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?

It's a balance between code consistency and code churn.

Personally, I prefer code consistency, as it's generally easier to review and work with.
Cleaning up generates noise in the logs, but that's a sacrifice I'm willing to make.

Whatever my personal preferences are, I'd still like to understand the project's conventions.
If moving around a few lines of code for consistency only (like the one I moved here) is unwanted, then I'll try to avoid doing that.

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

See reply above.

> 
> >  	struct nat44 nat;
> >  };
> >
> 
> Cheers,

Thank you once again for reviewing and continued guidance, Robin...

Which changes should I completely drop?

Do I need to send this as a series of patches, or is it OK to just send one patch like this?

If a series is required, in which order should the patches be organized? Clean-up first or last?

-Morten