rework pf_state_expires to avoid confusion around state->timeout.

im going to make it so pf_purge_expired_states() can gather states
largely without sharing a lock with pfsync or actual packet processing
in pf. if pf or pfsync unlink a state while pf_purge_expired_states
is looking at it, we can race with some checks and fall over a
KASSERT.

i'm fixing this by having the caller of pf_state_expires read
state->timeout first, do it's checks, and then pass the value as
an argument into pf_state_expires. this means there's a consistent
view of the state->timeout variable across all the checks that
pf_purge_expired_states in particular does. if pf/pfsync does change
the timeout while pf_purge_expired_states is looking at it, the
worst thing that happens is that it doesn't get picked as a candidate
for purging in this pass and will have to wait for the next sweep.

ok sashan@ as part of a bigger diff

diff --git a/sys/net/pf.c b/sys/net/pf.c
index b684c4b..090ee2c 100644 (file)
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: pf.c,v 1.1118 2021/06/01 09:57:11 dlg Exp $ */
+/*     $OpenBSD: pf.c,v 1.1119 2021/06/23 04:16:32 dlg Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -259,6 +259,7 @@ void                         pf_state_key_link_inpcb(struct pf_state_key *,
 void                    pf_state_key_unlink_inpcb(struct pf_state_key *);
 void                    pf_inpcb_unlink_state_key(struct inpcb *);
 void                    pf_pktenqueue_delayed(void *);
+int32_t                         pf_state_expires(const struct pf_state *, uint8_t);
 
 #if NPFLOG > 0
 void                    pf_log_matches(struct pf_pdesc *, struct pf_rule *,
@@ -1183,7 +1184,7 @@ pf_state_export(struct pfsync_state *sp, struct pf_state *st)
        sp->rt = st->rt;
        sp->rt_addr = st->rt_addr;
        sp->creation = htonl(getuptime() - st->creation);
-       expire = pf_state_expires(st);
+       expire = pf_state_expires(st, st->timeout);
        if (expire <= getuptime())
                sp->expire = htonl(0);
        else
@@ -1290,23 +1291,38 @@ pf_purge(void *xnloops)
 }
 
 int32_t
-pf_state_expires(const struct pf_state *state)
+pf_state_expires(const struct pf_state *state, uint8_t stimeout)
 {
        u_int32_t       timeout;
        u_int32_t       start;
        u_int32_t       end;
        u_int32_t       states;
 
+       /*
+        * pf_state_expires is used by the state purge task to
+        * decide if a state is a candidate for cleanup, and by the
+        * pfsync state export code to populate an expiry time.
+        *
+        * this function may be called by the state purge task while
+        * the state is being modified. avoid inconsistent reads of
+        * state->timeout by having the caller do the read (and any
+        * chacks it needs to do on the same variable) and then pass
+        * their view of the timeout in here for this function to use.
+        * the only consequnce of using a stale timeout value is
+        * that the state won't be a candidate for purging until the
+        * next pass of the purge task.
+        */
+
        /* handle all PFTM_* > PFTM_MAX here */
-       if (state->timeout == PFTM_PURGE)
+       if (stimeout == PFTM_PURGE)
                return (0);
 
-       KASSERT(state->timeout != PFTM_UNLINKED);
-       KASSERT(state->timeout < PFTM_MAX);
+       KASSERT(stimeout != PFTM_UNLINKED);
+       KASSERT(stimeout < PFTM_MAX);
 
-       timeout = state->rule.ptr->timeout[state->timeout];
+       timeout = state->rule.ptr->timeout[stimeout];
        if (!timeout)
-               timeout = pf_default_rule.timeout[state->timeout];
+               timeout = pf_default_rule.timeout[stimeout];
 
        start = state->rule.ptr->timeout[PFTM_ADAPTIVE_START];
        if (start) {
@@ -1467,6 +1483,8 @@ pf_purge_expired_states(u_int32_t maxcheck)
 
        PF_STATE_ENTER_READ();
        while (maxcheck--) {
+               uint8_t stimeout;
+
                /* wrap to start of list when we hit the end */
                if (cur == NULL) {
                        cur = pf_state_ref(TAILQ_FIRST(&state_list));
@@ -1477,8 +1495,9 @@ pf_purge_expired_states(u_int32_t maxcheck)
                /* get next state, as cur may get deleted */
                next = TAILQ_NEXT(cur, entry_list);
 
-               if ((cur->timeout == PFTM_UNLINKED) ||
-                   (pf_state_expires(cur) <= getuptime()))
+               stimeout = cur->timeout;
+               if ((stimeout == PFTM_UNLINKED) ||
+                   (pf_state_expires(cur, stimeout) <= getuptime()))
                        SLIST_INSERT_HEAD(&gcl, cur, gc_list);
                else
                        pf_state_unref(cur);

diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index b16b915..44a7f60 100644 (file)
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: pfvar.h,v 1.500 2021/03/10 10:21:48 jsg Exp $ */
+/*     $OpenBSD: pfvar.h,v 1.501 2021/06/23 04:16:32 dlg Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -1781,7 +1781,6 @@ int       pf_normalize_tcp_stateful(struct pf_pdesc *, u_short *,
            int *);
 int    pf_normalize_mss(struct pf_pdesc *, u_int16_t);
 void   pf_scrub(struct mbuf *, u_int16_t, sa_family_t, u_int8_t, u_int8_t);
-int32_t        pf_state_expires(const struct pf_state *);
 void   pf_purge_expired_fragments(void);
 int    pf_routable(struct pf_addr *addr, sa_family_t af, struct pfi_kif *,
            int);