On Fri, Apr 23, 2021 at 01:01:26AM +0200, Alexandr Nedvedicky wrote: > sure, updated diff is below.
Thanks for analysis. OK bluhm@ > > looks like there must be one more difference between OpenBSD and genua. > It looks like genua-pf is able to run state expiration concurrently with > pf_test() function somehow. In OpenBSD pf_purge_expired_states() removes > states under write lock from look up table. > > pf_state_key_link_reverse() on OpenBSD always runs as reader on > state lock. > > the pf_purge_expired_states() on OpenBSD has two parts. The first part > runs > as a reader on state lock. It walks through entire state table finding > expired states. It puts those states to private garbage collector list. > Once done the state lock is dropped and re-acquired as a writer to > enter a second part of pf_purge_expired_states(). > > At this point no packet is able to do a state look up, create state. Also > no pf_state_key_link_reverse() functions are running at this point > (because > they acquire state lock as reader). having exclusive access to state > table > pf_purge_expired_states() walks its private list of garbage collected > states removing them from look up table, removing state keys as well. > > so it seems to me the reverse key in OpenBSD is either set > to correct peer or NULL. > > thanks and > regards > sashan > > --------8<---------------8<---------------8<------------------8<-------- > diff --git a/sys/net/pf.c b/sys/net/pf.c > index 28aa6157552..74293d6b70b 100644 > --- a/sys/net/pf.c > +++ b/sys/net/pf.c > @@ -7366,11 +7366,19 @@ pf_inp_unlink(struct inpcb *inp) > void > pf_state_key_link_reverse(struct pf_state_key *sk, struct pf_state_key > *skrev) > { > - /* Note that sk and skrev may be equal, then we refcount twice. */ > - KASSERT(sk->reverse == NULL); > - KASSERT(skrev->reverse == NULL); > - sk->reverse = pf_state_key_ref(skrev); > - skrev->reverse = pf_state_key_ref(sk); > + struct pf_state_key *old_reverse; > + > + old_reverse = atomic_cas_ptr(&sk->reverse, NULL, skrev); > + if (old_reverse != NULL) > + KASSERT(old_reverse == skrev); > + else > + pf_state_key_ref(skrev); > + > + old_reverse = atomic_cas_ptr(&skrev->reverse, NULL, sk); > + if (old_reverse != NULL) > + KASSERT(old_reverse == sk); > + else > + pf_state_key_ref(sk); > } > > #if NPFLOG > 0