Page MenuHomeFreeBSD
Differential D28531

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

Authored by arichardson on Feb 8 2021, 10:24 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 18 2024, 5:01 AM
Unknown Object (File)
Jan 14 2024, 11:10 AM
Unknown Object (File)
Dec 20 2023, 6:55 AM
Unknown Object (File)
Nov 24 2023, 9:54 AM
Unknown Object (File)
Nov 19 2023, 12:36 AM
Unknown Object (File)
Nov 5 2023, 12:03 AM
Unknown Object (File)
Oct 19 2023, 4:30 AM
Unknown Object (File)
Oct 14 2023, 9:02 AM
View All 12 Files
Subscribers
imp

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
jrtc27 added a comment.Feb 8 2021, 12:56 PM

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.

arichardson added a comment.Feb 8 2021, 1:01 PM
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
jrtc27 added a comment.Feb 8 2021, 1:02 PM
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.

arichardson added a comment.Feb 8 2021, 1:07 PM
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.

kib added a comment.Feb 8 2021, 1:47 PM

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.

jrtc27 added a comment.Feb 8 2021, 1:49 PM
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.

arichardson planned changes to this revision.Feb 23 2021, 4:58 PM

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
arichardson abandoned this revision.Apr 1 2021, 7:52 PM

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

Revision Contents
Changeset List

PathSize
lib/
libthr/
8 lines
thread/
17 lines
9 lines
33 lines

Diff 83529

View Options

lib/libthr/pthread.map

Loading...
View Options

lib/libthr/thread/thr_malloc.c

Loading...
View Options

lib/libthr/thread/thr_private.h

Loading...
View Options

lib/libthr/thread/thr_umtx.c

Loading...