[PATCH 0/1] accel/tcg: Retry page_check_range() on itree.start inconsistency

[PATCH 0/1] accel/tcg: Retry page_check_range() on itree.start inconsistency

[Ilya Leoshkevich]

Hi,

This is an attempt to improve the vma-pthread test situation sparked by
a bug report by wasmtime project. The patch doesn't fix all issues, but
it definitely makes things better and I think I understand what
particular thread interleaving leads to the issue (details are in the
commit message).

There is at least one other thing that I find suspicious about the
current code: page_check_range() fetches itree.last twice, and indeed,
using qatomic_read() there makes things even better, but still not
perfect, and I still cannot clearly elaborate what is the exact
interleaving with pageflags_set_clear() that causes a problem there, so
I'm not sending that one for now.

Best regards,
Ilya

Ilya Leoshkevich (1):
  accel/tcg: Retry page_check_range() on itree.start inconsistency

 accel/tcg/user-exec.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

-- 
2.54.0

[PATCH 1/1] accel/tcg: Retry page_check_range() on itree.start inconsistency

[Ilya Leoshkevich]

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 <alex@alexcrichton.com>
Reported-by: Ulrich Weigand <ulrich.weigand@de.ibm.com>
Fixes: 67ff2186b0a4 ("accel/tcg: Use interval tree for user-only page tracking")
Cc: qemu-stable@nongnu.org
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 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