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
