From: Alexandr Nedvedicky <sashan@fastmail.net>
Subject: pf_update_state_timeout() without mutex
To: tech@openbsd.org
Date: Mon, 1 Jan 2024 23:27:17 +0100

Hello,

diff below improves pf_update_state_timeout() and pf_remove_state().
the change has been discussed here [1] already. I think it's worth
to commit it. OK?

thanks and
regards
sashan

[1] https://marc.info/?l=openbsd-tech&m=170150545629295&w=2

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/pf.c b/sys/net/pf.c
index c859a1467ec..14f85b9f7d9 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -470,12 +470,24 @@ pf_state_list_remove(struct pf_state_list *pfs, struct pf_state *st)
 }
 
 void
-pf_update_state_timeout(struct pf_state *st, int to)
+pf_update_state_timeout(struct pf_state *st, unsigned int new_to)
 {
-	mtx_enter(&st->mtx);
-	if (st->timeout != PFTM_UNLINKED)
-		st->timeout = to;
-	mtx_leave(&st->mtx);
+	unsigned int	current_to, to;
+
+	current_to = st->timeout;
+
+	if (current_to == PFTM_UNLINKED)
+		return;
+
+	do {
+		to = atomic_cas_uint(&st->timeout, current_to, new_to);
+		if (to == PFTM_UNLINKED)
+			return;
+		if (to != current_to) {
+			current_to = to;
+			continue;
+		}
+	} while (0);
 }
 
 int
@@ -1762,8 +1774,24 @@ pf_src_tree_remove_state(struct pf_state *st)
 void
 pf_remove_state(struct pf_state *st)
 {
+	unsigned int	current_to, to;
+
 	PF_ASSERT_LOCKED();
 
+	current_to = st->timeout;
+	if (current_to == PFTM_UNLINKED)
+		return;
+
+	do {
+		to = atomic_cas_uint(&st->timeout, current_to, PFTM_UNLINKED);
+		if (to == PFTM_UNLINKED)
+			return;
+		if (to != current_to) {
+			current_to = to;
+			continue;
+		}
+	} while (0);
+
 	mtx_enter(&st->mtx);
 	if (st->timeout == PFTM_UNLINKED) {
 		mtx_leave(&st->mtx);
diff --git a/sys/net/pfvar_priv.h b/sys/net/pfvar_priv.h
index 53d983432a9..f436379520a 100644
--- a/sys/net/pfvar_priv.h
+++ b/sys/net/pfvar_priv.h
@@ -123,12 +123,12 @@ struct pf_state {
 	int32_t			 expire;
 	int32_t			 pfsync_time;	/* [S] */
 	int			 rtableid[2];	/* [I] stack and wire */
+	unsigned int		 timeout;
 	u_int16_t		 qid;		/* [I] */
 	u_int16_t		 pqid;		/* [I] */
 	u_int16_t		 tag;		/* [I] */
 	u_int16_t		 state_flags;	/* [M] */
 	u_int8_t		 log;		/* [I] */
-	u_int8_t		 timeout;
 	u_int8_t		 sync_state;	/* [S] PFSYNC_S_x */
 	u_int8_t		 sync_updates;	/* [S] */
 	u_int8_t		 min_ttl;	/* [I] */