From 99e6e1ef5c9fa2ffc9cbb56b4b4f522d9b8918e3 Mon Sep 17 00:00:00 2001 From: dlg Date: Wed, 23 Jun 2021 04:16:32 +0000 Subject: [PATCH] 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 --- sys/net/pf.c | 39 +++++++++++++++++++++++++++++---------- sys/net/pfvar.h | 3 +-- 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/sys/net/pf.c b/sys/net/pf.c index b684c4b037a..090ee2c3bc9 100644 --- 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 b16b915230d..44a7f60328b 100644 --- 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); -- 2.20.1