Hello,
people who will be running pf(4) with bluhm's diff [1], may trip
one of the asserts triggered by pf_state_key_link_reverse() here:
7366 void
7367 pf_state_key_link_reverse(struct pf_state_key *sk, struct pf_state_key
*skrev)
7368 {
7369 /* Note that sk and skrev may be equal, then we refcount twice. */
7370 KASSERT(sk->reverse == NULL);
7371 KASSERT(skrev->reverse == NULL);
7372 sk->reverse = pf_state_key_ref(skrev);
7373 skrev->reverse = pf_state_key_ref(sk);
7374 }
pf(4) currently does not provide a mutual access to state instances.
so it may happen pf(4) will be handling more packets, which will be
updating the same state. This is the situation of one winner and
more losers. At this point I'm suggesting to change those asserts
to take the race into account. diff below makes pf_state_key_link_reverse()
happy when pf(4) runs in parallel.
would it be OK to commit it once bluhm's diff [1] will be in?
thanks and
regards
sashan
[1] https://marc.info/?l=openbsd-tech&m=161903387904923&w=2
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/pf.c b/sys/net/pf.c
index 28aa6157552..d4212c21d7b 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -7366,11 +7366,20 @@ 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
