Hello,

this is almost identical diff I've sent to bugs@ [1].
The change I'd like to commit adds PF_ASSERT_LOCKED() to
pfr_insert_kentry() function.

the diff fixes the issue which got introduced in 2018 by my
commit here:

    revision 1.1074
    date: 2018/09/11 07:53:38;  author: sashan;  state: Exp;  lines: +117 -33;\
	commitid: L4l6Ihj5YoSZO1NA;
    - moving state look up outside of PF_LOCK()

    this change adds a pf_state_lock rw-lock, which protects consistency
    of state table in PF. The code delivered in this change is guarded
    by 'WITH_PF_LOCK', which is still undefined. People, who are willing
    to experiment and want to run it must do two things:

	    - compile kernel with -DWITH_PF_LOCK
	    - bump NET_TASKQ from 1 to ... sky is the limit,
	      (just select some sensible value for number of tasks your
	      system is able to handle)

    OK bluhm@

the thing is the pfr_insert_kentry() assumes it's being called under PF_LOCK(),
but this is not true when we deal with overload action. pf(4) holds no
lock when adding offending address to overload table. on this code path
the pf(4) grabs reader lock on state table, grabs reference to state and
drops the state lock. all further processing runs with no global pf(40 locks.

it's interesting the bug was sitting there waiting to bite for 8 years.

I think the diff below is safe enough to commit even it is late in release
cycle. I plan to bring improved diff after release. The improved diff
does:
    - shrinks the PF_LOCK() scope just to cover pfr_insert_kentry()

    - adds mtx_lock()/mtx_unlock() on state mutex to pf_get_src_node()
    so source node list traversal is safe. The diff below should take
    care of it because purging state grabs the PF_LOCK() these days.
    the snippet here comes from pf_purged_expired_states()

	2354         rw_enter_write(&pf_state_list.pfs_rwl);
	2355         PF_LOCK();
	2356         PF_STATE_ENTER_WRITE();
	2357         SLIST_FOREACH(st, &gcl, gc_list) {
	2358                 if (st->timeout != PFTM_UNLINKED)
	2359                         pf_remove_state(st);
	2360
	2361                 pf_free_state(st);
	2362         }
	2363         PF_STATE_EXIT_WRITE();
	2364         PF_UNLOCK();
	2365         rw_exit_write(&pf_state_list.pfs_rwl);

    pf_remove_state() calls to pf_src_tree_remove_state() where the
    list of source nodes (the same list pf(4) traverses in pf_get_src_node()
    gets purged.

The follow up change I plan to commit is larger, thus more risky IMO to go for
it now.

OK to commit diff below? or leave the bug to be fixed with next release.

thanks and
regards
sashan


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

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/pf.c b/sys/net/pf.c
index b465a77b487..e6c67842a07 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -757,9 +757,12 @@ pf_src_connlimit(struct pf_state **stp)
 	int			 bad = 0;
 	struct pf_src_node	*sn;
 	u_int32_t		 sn_conn;
+	int			 rv = 0;
+
+	PF_LOCK();

 	if ((sn = pf_get_src_node((*stp), PF_SN_NONE)) == NULL)
-		return (0);
+		goto done;

 	/*
 	 * Note: conn limit is bumped on SYN_SENT->ESTBLISHED
@@ -784,7 +787,7 @@ pf_src_connlimit(struct pf_state **stp)
 	}

 	if (!bad)
-		return (0);
+		goto done;

 	if ((*stp)->rule.ptr->overload_tbl) {
 		struct pfr_addr p;
@@ -822,6 +825,7 @@ pf_src_connlimit(struct pf_state **stp)
 			struct pf_state *st;

 			pf_status.lcounters[LCNT_OVERLOAD_FLUSH]++;
+			PF_STATE_ENTER_READ();
 			RBT_FOREACH(st, pf_state_tree_id, &tree_id) {
 				sk = st->key[PF_SK_WIRE];
 				/*
@@ -844,6 +848,7 @@ pf_src_connlimit(struct pf_state **stp)
 					killed++;
 				}
 			}
+			PF_STATE_EXIT_READ();
 			if (pf_status.debug >= LOG_NOTICE)
 				addlog(", %u states killed", killed);
 		}
@@ -854,7 +859,12 @@ pf_src_connlimit(struct pf_state **stp)
 	/* kill this state */
 	pf_update_state_timeout(*stp, PFTM_PURGE);
 	pf_set_protostate(*stp, PF_PEER_BOTH, TCPS_CLOSED);
-	return (1);
+
+	rv = 1;
+done:
+	PF_UNLOCK();
+
+	return (rv);
 }

 int
diff --git a/sys/net/pf_table.c b/sys/net/pf_table.c
index a33fa296497..f83cada05cc 100644
--- a/sys/net/pf_table.c
+++ b/sys/net/pf_table.c
@@ -1145,6 +1145,8 @@ pfr_insert_kentry(struct pfr_ktable *kt, struct pfr_addr *ad, time_t tzero)
 	struct pfr_kentry	*p;
 	int			 rv;

+	PF_ASSERT_LOCKED();
+
 	p = pfr_lookup_addr(kt, ad, 1);
 	if (p != NULL)
 		return (0);