races in ARM lockless data structure reads

DynamoRIO / dynamorio

Dynamic Instrumentation Tool Platform

Other

2.66k stars 562 forks source link

races in ARM lockless data structure reads #2502

Open derekbruening opened 7 years ago

derekbruening commented 7 years ago

In DR we have some data structures we read without holding a lock, relying on the hardware write visibility. The design and code was created with x86 in mind and we did not do a thorough enough re-evaluation for ARM and AArch64. For ARM's memory model we need to add barriers in multiple places to ensure that writes are visible in other threads in the order we require.

Suspect data structures include:

The indirect branch lookup (IBL) table: we rely on writing the tag being seen by other threads before the write of the start pc.
flushtime_global's store in increment_global_flushtime()
likely more...the code needs an audit

derekbruening commented 4 years ago

I'm adding a related action item here:

Review the "DMB ISH" barriers in arch_exports.h and downgrade them to stores-only or loads-only as necessary. I based these on g++ 9.2.1's std::atomic generated code and it uses "DMB ISH" for everything which apparently is overkill.

algrant-arm commented 4 years ago

Probably best to leave anything that's generating explicit barriers as DMB ISH. On Armv8 (including all AArch64) DMB ISHLD should be sufficient as an acquire barrier, but this might not be true on pre-v8 systems. And on v8 it's best to use LDAR anyway.

derekbruening commented 4 years ago

There are more variables that may need stronger stores and loads:

exiting_thread_count

derekbruening commented 2 years ago

I caught and diagnosed a plain-DR hang here: https://github.com/DynamoRIO/dynamorio/issues/4928#issuecomment-1043154222 Pasting from there:

Looking through our lock routines I see a number of other reads that should be load-acquires. Some are tricky to fix b/c they're in utils.h which can't include arch_exports.h: we'll have to split the headers or sthg.