Fix assertions in crypto reference helpers

The assertions are racy and the use of `membar_exit()` did nothing to
fix that.

The helpers use atomic functions, so we cleverly get values from the
atomics that we can use to ensure that the assertions operate on the
correct values.

We also use `membar_producer()` prior to decrementing reference counts
so that operations that happened prior to a decrement to 0 will be
guaranteed to happen before the decrement on architectures that reorder
atomics.

This also slightly improves performance by eliminating unnecessary
reads, although I doubt it would be measurable in any benchmark.

Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>

module/icp/include/sys/crypto/impl.h

@@ -126,38 +126,37 @@ typedef struct kcf_provider_desc {
 	crypto_provider_id_t		pd_prov_id;
 } kcf_provider_desc_t;
 
-/* atomic operations in linux implicitly form a memory barrier */
-#define	membar_exit()
-
 /*
  * If a component has a reference to a kcf_provider_desc_t,
  * it REFHOLD()s. A new provider descriptor which is referenced only
  * by the providers table has a reference counter of one.
  */
-#define	KCF_PROV_REFHOLD(desc) {		\
-	atomic_add_32(&(desc)->pd_refcnt, 1);	\
-	ASSERT((desc)->pd_refcnt != 0);		\
+#define	KCF_PROV_REFHOLD(desc) {				\
+	int newval = atomic_add_32_nv(&(desc)->pd_refcnt, 1);	\
+	ASSERT(newval != 0);					\
 }
 
-#define	KCF_PROV_IREFHOLD(desc) {		\
-	atomic_add_32(&(desc)->pd_irefcnt, 1);	\
-	ASSERT((desc)->pd_irefcnt != 0);	\
+#define	KCF_PROV_IREFHOLD(desc) {				\
+	int newval = atomic_add_32_nv(&(desc)->pd_irefcnt, 1);	\
+	ASSERT(newval != 0);					\
 }
 
 #define	KCF_PROV_IREFRELE(desc) {				\
-	ASSERT((desc)->pd_irefcnt != 0);			\
-	membar_exit();						\
-	if (atomic_add_32_nv(&(desc)->pd_irefcnt, -1) == 0) {	\
+	membar_producer();					\
+	int newval = atomic_add_32_nv(&(desc)->pd_irefcnt, -1);	\
+	ASSERT(newval != -1);					\
+	if (newval == 0) {					\
 		cv_broadcast(&(desc)->pd_remove_cv);		\
 	}							\
 }
 
 #define	KCF_PROV_REFHELD(desc)	((desc)->pd_refcnt >= 1)
 
 #define	KCF_PROV_REFRELE(desc) {				\
-	ASSERT((desc)->pd_refcnt != 0);				\
-	membar_exit();						\
-	if (atomic_add_32_nv(&(desc)->pd_refcnt, -1) == 0) {	\
+	membar_producer();					\
+	int newval = atomic_add_32_nv(&(desc)->pd_refcnt, -1);	\
+	ASSERT(newval != -1);					\
+	if (newval == 0) {					\
 		kcf_provider_zero_refcnt((desc));		\
 	}							\
 }
@@ -193,19 +192,20 @@ typedef	struct kcf_mech_entry {
  * it REFHOLD()s. A new policy descriptor which is referenced only
  * by the policy table has a reference count of one.
  */
-#define	KCF_POLICY_REFHOLD(desc) {		\
-	atomic_add_32(&(desc)->pd_refcnt, 1);	\
-	ASSERT((desc)->pd_refcnt != 0);		\
+#define	KCF_POLICY_REFHOLD(desc) {				\
+	int newval = atomic_add_32_nv(&(desc)->pd_refcnt, 1);	\
+	ASSERT(newval != 0);					\
 }
 
 /*
  * Releases a reference to a policy descriptor. When the last
  * reference is released, the descriptor is freed.
  */
 #define	KCF_POLICY_REFRELE(desc) {				\
-	ASSERT((desc)->pd_refcnt != 0);				\
-	membar_exit();						\
-	if (atomic_add_32_nv(&(desc)->pd_refcnt, -1) == 0)	\
+	membar_producer();					\
+	int newval = atomic_add_32_nv(&(desc)->pd_refcnt, -1);	\
+	ASSERT(newval != -1);					\
+	if (newval == 0)					\
 		kcf_policy_free_desc(desc);			\
 }
 

module/icp/include/sys/crypto/sched_impl.h

@@ -73,9 +73,10 @@ typedef struct kcf_context {
  * context structure is freed along with the global context.
  */
 #define	KCF_CONTEXT_REFRELE(ictx) {				\
-	ASSERT((ictx)->kc_refcnt != 0);				\
-	membar_exit();						\
-	if (atomic_add_32_nv(&(ictx)->kc_refcnt, -1) == 0)	\
+	membar_producer();					\
+	int newval = atomic_add_32_nv(&(ictx)->kc_refcnt, -1);	\
+	ASSERT(newval != -1);					\
+	if (newval == 0)					\
 		kcf_free_context(ictx);				\
 }