From 9e9a1c2f1bf96207499ba7dc1638ab5375b26b04 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <campbell+npf@mumble.net>
Date: Sun, 22 Jan 2023 18:22:48 +0000
Subject: [PATCH] Surround reference count decrement-branch by release/acquire
 membars.

In order to ensure that that all users of an object have finished all
access to it before the last one actually frees it, we need the
decrement to have release/acquire semantics to establish a
happens-before relation.

	... x->f = 42 ... x->f ...		/* A */
	if (--x->refcnt != 0)	/* atomic */
	        return;
	x->a = x->b = ... = x->f = 0;		/* B */
	free(x);

To guarantee that A in one thread happens-before B in another thread,
we need the reference count decrement (and branch) to have
release/acquire semantics, so put membar_release before and
membar_acquire after.  (We could use memory_order_acq_rel with
atomic_fetch_add_explicit in C11.)

Note: membar_producer and membar_consumer are not enough, because
they only order stores on one side and loads on the other, whereas it
is necessary to order loads and stores on both sides.

v2: Nix __HAVE_MEMBAR_AS_ATOMIC.
---
 src/kern/npf_nat.c         |  2 ++
 src/kern/npf_rproc.c       |  2 ++
 src/kern/npf_tableset.c    | 10 +++++++---
 src/kern/stand/npf_stand.h |  2 ++
 4 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/kern/npf_nat.c b/src/kern/npf_nat.c
index 1143c1f..b1caf43 100644
--- a/src/kern/npf_nat.c
+++ b/src/kern/npf_nat.c
@@ -257,9 +257,11 @@ npf_natpolicy_release(npf_natpolicy_t *np)
 {
 	KASSERT(atomic_load_relaxed(&np->n_refcnt) > 0);
 
+	membar_release();
 	if (atomic_dec_uint_nv(&np->n_refcnt) != 0) {
 		return;
 	}
+	membar_acquire();
 	KASSERT(LIST_EMPTY(&np->n_nat_list));
 	mutex_destroy(&np->n_lock);
 	kmem_free(np, sizeof(npf_natpolicy_t));
diff --git a/src/kern/npf_rproc.c b/src/kern/npf_rproc.c
index 82133cd..50a454d 100644
--- a/src/kern/npf_rproc.c
+++ b/src/kern/npf_rproc.c
@@ -308,9 +308,11 @@ npf_rproc_release(npf_rproc_t *rp)
 {
 	KASSERT(atomic_load_relaxed(&rp->rp_refcnt) > 0);
 
+	membar_release();
 	if (atomic_dec_uint_nv(&rp->rp_refcnt) != 0) {
 		return;
 	}
+	membar_acquire();
 	/* XXXintr */
 	for (unsigned i = 0; i < rp->rp_ext_count; i++) {
 		npf_ext_t *ext = rp->rp_ext[i];
diff --git a/src/kern/npf_tableset.c b/src/kern/npf_tableset.c
index 8492b17..23ddca1 100644
--- a/src/kern/npf_tableset.c
+++ b/src/kern/npf_tableset.c
@@ -136,9 +136,13 @@ npf_tableset_destroy(npf_tableset_t *ts)
 	for (u_int tid = 0; tid < ts->ts_nitems; tid++) {
 		npf_table_t *t = ts->ts_map[tid];
 
-		if (t && atomic_dec_uint_nv(&t->t_refcnt) == 0) {
-			npf_table_destroy(t);
-		}
+		if (t == NULL)
+			continue;
+		membar_release();
+		if (atomic_dec_uint_nv(&t->t_refcnt) > 0)
+			continue;
+		membar_acquire();
+		npf_table_destroy(t);
 	}
 	kmem_free(ts, NPF_TABLESET_SIZE(ts->ts_nitems));
 }
diff --git a/src/kern/stand/npf_stand.h b/src/kern/stand/npf_stand.h
index 9538530..5c068d6 100644
--- a/src/kern/stand/npf_stand.h
+++ b/src/kern/stand/npf_stand.h
@@ -136,6 +136,8 @@ npfkern_atomic_swap_ptr(volatile void *ptr, void *newval)
 #define	membar_sync()		__sync_synchronize()
 #define	membar_consumer()	__sync_synchronize()
 #define	membar_producer()	__sync_synchronize()
+#define	membar_release()	__sync_synchronize()
+#define	membar_acquire()	__sync_synchronize()
 #define	atomic_inc_uint(x)	__sync_fetch_and_add((x), 1)
 #define	atomic_inc_uint_nv(x)	__sync_add_and_fetch((x), 1)
 #define	atomic_inc_ulong_nv(x)	__sync_add_and_fetch((x), 1)