From e949d36040e5e79fe0dfda6a33451111cc5a0476 Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Thu, 15 Sep 2022 16:24:00 -0400 Subject: [PATCH] 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. Reviewed-by: Mateusz Guzik Signed-off-by: Richard Yao Closes #13880 --- module/icp/include/sys/crypto/impl.h | 42 +++++++++++----------- module/icp/include/sys/crypto/sched_impl.h | 7 ++-- 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/module/icp/include/sys/crypto/impl.h b/module/icp/include/sys/crypto/impl.h index 32ac43475..4d17221ea 100644 --- a/module/icp/include/sys/crypto/impl.h +++ b/module/icp/include/sys/crypto/impl.h @@ -126,28 +126,26 @@ 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); \ } \ } @@ -155,9 +153,10 @@ typedef struct kcf_provider_desc { #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,9 +192,9 @@ 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); \ } /* @@ -203,9 +202,10 @@ typedef struct kcf_mech_entry { * 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); \ } diff --git a/module/icp/include/sys/crypto/sched_impl.h b/module/icp/include/sys/crypto/sched_impl.h index 1989d5244..355c1a87f 100644 --- a/module/icp/include/sys/crypto/sched_impl.h +++ b/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); \ }