libthr: Avoid TSan false-positives due to internal malloc
AbandonedPublic
Actions

Authored by arichardson on Feb 8 2021, 10:24 AM.

Details

Reviewers

kib
emaste
jhb
jrtc27

Summary

TSan reports lots of false-positive races when multiple threads use
thr_calloc (due to memset accessing the same data without a lock visible
to TSan). To avoid these false-positives this patch calls the tsan_mutex_*
API to notify TSan about the internal locking around malloc(). These are
implemented as weak no-op functions so that the TSan runtime can override
them with the real implementation.

Another option to silence these issues would be to avoid calls to
intercepted functions from within thr_calloc(), etc. Possibly providing
a libthr-internal memset() would be sufficient since all false-positive
backtraces I've seen so far come from the memset() call inside thr_calloc.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 36799
Build 33688: arc lint + arc unit

Event Timeline

arichardson created this revision.Feb 8 2021, 10:24 AM

Herald added a subscriber: imp. · View Herald TranscriptFeb 8 2021, 10:24 AM

arichardson requested review of this revision.Feb 8 2021, 10:24 AM

Harbormaster completed remote builds in B36799: Diff 83529.Feb 8 2021, 10:24 AM

I feel like these shouldn't be compiled in by default, as 99.9999% of the time you're not running with TSan and it will just hurt performance.

In D28531#638711, @jrtc27 wrote:

I feel like these shouldn't be compiled in by default, as 99.9999% of the time you're not running with TSan and it will just hurt performance.

Yeah the other option would be to use memset/memcpy that is not interposed by TSan/MSan/ASan/etc, but right now I don't see libc exporting an alias that is not interposed. See also D28536

readelf --dyn-symbols /lib/libc.so.7 | grep memcpy
  1750: 00000000001ba060   604 FUNC    GLOBAL DEFAULT    12 memcpy@@FBSD_1.0
  2559: 00000000000dec00    14 FUNC    GLOBAL DEFAULT    12 wmemcpy@@FBSD_1.0

In D28531#638716, @arichardson wrote:
In D28531#638711, @jrtc27 wrote:

I feel like these shouldn't be compiled in by default, as 99.9999% of the time you're not running with TSan and it will just hurt performance.

Yeah the other option would be to use memset/memcpy that is not interposed by TSan/MSan/ASan/etc, but right now I don't see libc exporting an alias that is not interposed. See also D28536
readelf --dyn-symbols /lib/libc.so.7 | grep memcpy
  1750: 00000000001ba060   604 FUNC    GLOBAL DEFAULT    12 memcpy@@FBSD_1.0
  2559: 00000000000dec00    14 FUNC    GLOBAL DEFAULT    12 wmemcpy@@FBSD_1.0

You wouldn't want that either as then it wouldn't be able to be inlined by the compiler if beneficial.

In D28531#638718, @jrtc27 wrote:
In D28531#638716, @arichardson wrote:
In D28531#638711, @jrtc27 wrote:

I feel like these shouldn't be compiled in by default, as 99.9999% of the time you're not running with TSan and it will just hurt performance.

Yeah the other option would be to use memset/memcpy that is not interposed by TSan/MSan/ASan/etc, but right now I don't see libc exporting an alias that is not interposed. See also D28536
readelf --dyn-symbols /lib/libc.so.7 | grep memcpy
  1750: 00000000001ba060   604 FUNC    GLOBAL DEFAULT    12 memcpy@@FBSD_1.0
  2559: 00000000000dec00    14 FUNC    GLOBAL DEFAULT    12 wmemcpy@@FBSD_1.0
You wouldn't want that either as then it wouldn't be able to be inlined by the compiler if beneficial.

The two calls where this matters (thr_calloc/thr_realloc) use a non-constant size, so it won't be inlined anyway.

Are you sure that it is enough to put plain calls to the weak symbols? Usually you have to check them for NULL and avoid calls if they are.

That said, I dislike this utterly. IMO you should take some measures to prevent this instrumentation to hook into either libthr as a whole, or at least into thr_malloc.c

lib/libthr/thread/thr_malloc.c
98	You probably want this before thr_ast(), which might not even return.

In D28531#638750, @kib wrote:

Are you sure that it is enough to put plain calls to the weak symbols? Usually you have to check them for NULL and avoid calls if they are.

That bit is fine, there are weak stubs added to thr_umtx.c to ensure they're defined.

This needs changes since it can result in deadlock. Probably best to export a memcpy/memset version from libc that TSan doesn't intercept.

Deadlock in tests/sys/file/newfileops_on_fork_test.c

Thread 2 (LWP 100085 of process 1114):
#0  _accept () at _accept.S:4
#1  0x00000000402c9f28 in __thr_accept (s=3, addr=0x0, addrlen=0x0) at /local/scratch/alr48/cheri/freebsd/lib/libthr/thread/thr_syscalls.c:108
#2  0x0000000000210d10 in do_accept (arg=<optimized out>) at /local/scratch/alr48/cheri/freebsd/tests/sys/file/newfileops_on_fork_test.c:64
#3  0x00000000402bd3a8 in thread_start (curthread=0x40a12500) at /local/scratch/alr48/cheri/freebsd/lib/libthr/thread/thr_create.c:292
#4  0x00000000402bcef4 in _pthread_create (thread=0xffffffffea88, attr=<optimized out>, start_routine=<optimized out>, arg=<optimized out>) at /local/scratch/alr48/cheri/freebsd/lib/libthr/thread/thr_create.c:187
#5  0x0000000000000001 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
 
Thread 1 (LWP 100063 of process 1114):
#0  _umtx_op () at _umtx_op.S:4
#1  0x00000000402cc3c8 in _umtx_op_err (obj=0x4, op=12, val=0, uaddr=0x0, uaddr2=0x0) at /local/scratch/alr48/cheri/freebsd/lib/libthr/thread/thr_umtx.c:39
#2  __thr_rwlock_rdlock (rwlock=0x4, flags=<optimized out>, tsp=<optimized out>) at /local/scratch/alr48/cheri/freebsd/lib/libthr/thread/thr_umtx.c:307
#3  0x00000000402c5cf4 in _thr_rwlock_rdlock (rwlock=0x402eea00, flags=0, tsp=0x0) at /local/scratch/alr48/cheri/freebsd/lib/libthr/thread/thr_umtx.h:232
#4  _thr_rtld_rlock_acquire (lock=0x402eea00) at /local/scratch/alr48/cheri/freebsd/lib/libthr/thread/thr_rtld.c:125
#5  0x00000000402506a8 in rlock_acquire (lock=0x4027c010 <rtld_locks>, lockstate=0xffffffffe630) at /local/scratch/alr48/cheri/freebsd/libexec/rtld-elf/rtld_lock.c:232
#6  0x0000000040249fb0 in _rtld_bind (obj=0x40280408, reloff=1416) at /local/scratch/alr48/cheri/freebsd/libexec/rtld-elf/rtld.c:878
#7  0x000000004024667c in _rtld_bind_start () at /local/scratch/alr48/cheri/freebsd/libexec/rtld-elf/aarch64/rtld_start.S:92
#8  0x00000000402c23f0 in __thr_malloc_postfork (curthread=0x40a12000) at /local/scratch/alr48/cheri/freebsd/lib/libthr/thread/thr_malloc.c:162
#9  0x00000000402be1cc in thr_fork_impl (a=<optimized out>) at /local/scratch/alr48/cheri/freebsd/lib/libthr/thread/thr_fork.c:280
#10 0x00000000402be03c in __thr_fork () at /local/scratch/alr48/cheri/freebsd/lib/libthr/thread/thr_fork.c:314
#11 0x0000000000210d40 in do_fork () at /local/scratch/alr48/cheri/freebsd/tests/sys/file/newfileops_on_fork_test.c:77
#12 0x0000000000210ce4 in main () at /local/scratch/alr48/cheri/freebsd/tests/sys/file/newfileops_on_fork_test.c:122
(gdb)

arichardson mentioned this in D28536: libthr: Avoid interposable calls to memcpy/memset/memmove.Apr 1 2021, 2:48 PM

Will try to silence the false-positives inside TSan instead.