Skip to content

Commit

Permalink
Make thread sanitizer cope with rcu locks
Browse files Browse the repository at this point in the history
This is unfortunate, but seems necessecary

tsan in gcc/clang tracks data races by recording memory references made
while various locks are held.  If it finds that a given address is
read/written while under lock (or under no locks without the use of
atomics), it issues a warning

this creates a specific problem for rcu, because on the write side of a
critical section, we write data under the protection of a lock, but by
definition the read side has no lock, and so rcu warns us about it,
which is really a false positive, because we know that, even if a
pointer changes its value, the data it points to will be valid.

The best way to fix it, short of implementing tsan hooks for rcu locks
in any thread sanitizer in the field, is to 'fake it'.  If thread
sanitization is activated, then in ossl_rcu_write_[lock|unlock] we add
annotations to make the sanitizer think that, after the write lock is
taken, that we immediately unlock it, and lock it right before we unlock
it again.  In this way tsan thinks there are no locks held while
referencing protected data on the read or write side.

we still need to use atomics to ensure that tsan recognizes that we are
doing atomic accesses safely, but thats ok, and we still get warnings if
we don't do that properly

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #23671)
  • Loading branch information
nhorman authored and paulidale committed Apr 24, 2024
1 parent a928f26 commit 3bcac46
Showing 1 changed file with 24 additions and 2 deletions.
26 changes: 24 additions & 2 deletions crypto/threads_pthread.c
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,24 @@
#include "internal/rcu.h"
#include "rcu_internal.h"

#if defined(__clang__) && defined(__has_feature)
# if __has_feature(thread_sanitizer)
# define __SANITIZE_THREAD__
# endif
#endif

#if defined(__SANITIZE_THREAD__)
# include <sanitizer/tsan_interface.h>
# define TSAN_FAKE_UNLOCK(x) __tsan_mutex_pre_unlock((x), 0); \
__tsan_mutex_post_unlock((x), 0)

# define TSAN_FAKE_LOCK(x) __tsan_mutex_pre_lock((x), 0); \
__tsan_mutex_post_lock((x), 0, 0)
#else
# define TSAN_FAKE_UNLOCK(x)
# define TSAN_FAKE_LOCK(x)
#endif

#if defined(__sun)
# include <atomic.h>
#endif
Expand Down Expand Up @@ -548,10 +566,12 @@ static struct rcu_qp *allocate_new_qp_group(CRYPTO_RCU_LOCK *lock,
void ossl_rcu_write_lock(CRYPTO_RCU_LOCK *lock)
{
pthread_mutex_lock(&lock->write_lock);
TSAN_FAKE_UNLOCK(&lock->write_lock);
}

void ossl_rcu_write_unlock(CRYPTO_RCU_LOCK *lock)
{
TSAN_FAKE_LOCK(&lock->write_lock);
pthread_mutex_unlock(&lock->write_lock);
}

Expand All @@ -565,8 +585,10 @@ void ossl_synchronize_rcu(CRYPTO_RCU_LOCK *lock)
* __ATOMIC_ACQ_REL is used here to ensure that we get any prior published
* writes before we read, and publish our write immediately
*/
cb_items = ATOMIC_EXCHANGE_N(prcu_cb_item, &lock->cb_items, NULL,
__ATOMIC_ACQ_REL);
pthread_mutex_lock(&lock->write_lock);
cb_items = lock->cb_items;
lock->cb_items = NULL;
pthread_mutex_unlock(&lock->write_lock);

qp = update_qp(lock);

Expand Down

0 comments on commit 3bcac46

Please sign in to comment.