page_check_range() may race with pageflags_set_clear() as follows:
T1 T2
------------------------------------- --------------------------------
p = pageflags_find(start, last);
interval_tree_remove(&p->itree, ...);
p->itree.start = last + 1;
if (start < p->itree.start) {
ret = false;
interval_tree_insert(&p->itree, ...);
leading to errors like
fail indirect write 0x72f0a659aff0 (Bad address)
in vma-pthread test. I am able to reliably reproduce this on a machine
with 32 SMT threads as follows in about 25 seconds:
jobs=32; \
seq "$jobs" | \
time -p parallel \
--jobs="$jobs" \
--halt=now,done=1 \
--ungroup \
'
_={};
while ./qemu-s390x tests/tcg/s390x-linux-user/vma-pthread; do
printf .;
done
'
Also wasmtime project reported a similar failure pattern in their CI [1]
with a similar reproducer [2].
Fix by retrying under a lock. This makes the failure rate go down to
roughly once in 50 seconds.
[1] https://github.com/bytecodealliance/wasmtime/issues/10000
[2] https://gist.github.com/alexcrichton/f14f23a892ffb9df2522754572d51b1c
Reported-by: Alex Crichton <[email protected]>
Reported-by: Ulrich Weigand <[email protected]>
Fixes: 67ff2186b0a4 ("accel/tcg: Use interval tree for user-only page tracking")
Cc: [email protected]
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
accel/tcg/user-exec.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 7704e4017dd..b70955697fe 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -510,24 +510,23 @@ bool page_check_range(vaddr start, vaddr len, int flags)
PageFlagsNode *p = pageflags_find(start, last);
int missing;
- if (!p) {
- if (!locked) {
- /*
- * Lockless lookups have false negatives.
- * Retry with the lock held.
- */
- mmap_lock();
- locked = -1;
- p = pageflags_find(start, last);
- }
- if (!p) {
- ret = false; /* entire region invalid */
+ /*
+ * Lockless lookups race with pageflags_set_clear() and may have false
+ * negatives:
+ * - The node pageflags_find() was looking for could have been
+ * temporarily removed and not added back yet.
+ * - pageflags_find() could have found a node which was subsequently
+ * removed and whose itree.start has been adjusted.
+ */
+ if (!p || start < p->itree.start) {
+ if (locked) {
+ ret = false;
break;
}
- }
- if (start < p->itree.start) {
- ret = false; /* initial bytes invalid */
- break;
+ /* Retry with the lock held. */
+ mmap_lock();
+ locked = -1;
+ continue;
}
missing = flags & ~p->flags;
--
2.54.0
