Merge ~rafaeldtinoco/ubuntu/+source/qemu:lp1805256-groovy into ubuntu/+source/qemu:ubuntu/devel

[Rafael David Tinoco]

BUG: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1805256
PPA: https://launchpad.net/~rafaeldtinoco/+archive/ubuntu/lp1805256

[Rafael David Tinoco (rafaeldtinoco)]

[Impact]

* QEMU locking primitives might face a race condition in QEMU Async I/O bottom halves scheduling. This leads to a dead lock making either QEMU or one of its tools to hang indefinitely.

-> Despite a single patch was mentioned by HWE team, checking upstream repository I saw that there were 3 aarch related fixes in the same merged.

One of those is unrelated to LP: #1805256:

3. aio-posix: signal-proof fdmon-io_uring

So I'm not suggesting. The other 2 patches are:

1. aio-wait: delegate polling of main AioContext if BQL not held

 - a NULL from qemu_get_current_aio_context() makes AIO_WAIT_WHILE()
   to invoke aio_poll() directly - when savevm/restorevm - and that
   is incorrect if qemu big lock is not held: aio_pol() should never
   run concurrently with other concurrent I/O threads.

This is a serious issue and was merged as a Aarch64 fix together with the
one bellow, the most important for our case. There is *not* a 1:1 relation
ship with the test case, but I thought.. since we will stress the AIO logic
checking for regressions, it could be a good time for a fix like this also.

@paelser: Feel free to drop it per SRU guidelines. It would only affect VMs
using I/O threads, allowing a race window to exist in between the threads
and AIO polling logic.

2. async: use explicit memory barriers

 - In the following situation:

          write ctx->notify_me write bh->scheduled
          read bh->scheduled read ctx->notify_me
          if !bh->scheduled, sleep if ctx->notify_me, notify

   happening within the AIO bottom half scheduler needs SEQCST operations
   for reads and writes. This could be expensive due to many places outside
   the bottom half polling status.

   To make this smoother and perform better, a SEQCST mem barrier was added
   in between the writes and the reads for ctx->notify_me (as bh->scheduled
   is never written concurrently).

 This is the EXACT condition that brought us to all this discussion upstream.

[Rafael David Tinoco (rafaeldtinoco)]

BTW I mentioned SRU guidelines because I'm assuming the patch "aio-wait: delegate polling of main AioContext if BQL not held" will also be portable to some (or all) the other versions and I'll suggest together, knowing that you can just ignore that patch if you think its not worth, or it could jeopardize the AIO logic. IMO, I think the risk will be mitigated by the other fix testing.

[dann frazier (dannf)]

+1, since it is the same source changes as the focal package I verified in
https://code.launchpad.net/~rafaeldtinoco/ubuntu/+source/qemu/+git/qemu/+merge/383545/comments/1006846

review: Approve

[Christian Ehrhardt  (paelzer)]

This groovy MP needs a rebase due to CVE-2020-11869 fix in 1:4.2-3ubuntu7

It otherwise looks ok to me

review: Needs Fixing

[Christian Ehrhardt  (paelzer)]

Can you update the PPA (https://launchpad.net/~rafaeldtinoco/+archive/ubuntu/lp1805256/) after the rebase. I'll then run a regression test set and if ok sponsor it into groovy.

0afd3d5... by Rafael David Tinoco on 2020-05-27

changelog

[Rafael David Tinoco (rafaeldtinoco)]

Done!

review: Approve

[Christian Ehrhardt  (paelzer)]

Thanks for the rebase, non case specific regression testing is a few thousand tests in and still working.
Approving the MP now.
If no issue shows up late I'll sponsor this to -unapproved later on.

review: Approve

[Christian Ehrhardt  (paelzer)]

Regression tests completed - sponsored to groovy

Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading qemu_4.2-3ubuntu8.dsc: done.
  Uploading qemu_4.2-3ubuntu8.debian.tar.xz: done.
  Uploading qemu_4.2-3ubuntu8_source.buildinfo: done.
  Uploading qemu_4.2-3ubuntu8_source.changes: done.
Successfully uploaded packages.

Note: tagging for git ubuntu ran into problems, but it isn't the most complex tag structure so that is ok

[Christian Ehrhardt  (paelzer)]

Blocked on a flaky systemd test atm, otherwise LGTM.

[Christian Ehrhardt  (paelzer)]

Migrating right now ...

diff --git a/debian/changelog b/debian/changelog
index 88974cf..e074202 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+qemu (1:4.2-3ubuntu8) groovy; urgency=medium
+
+  * d/p/ubuntu/lp-1805256*: Fixes for QEMU on aarch64 ARM hosts
+    - async: use explicit memory barriers (LP: #1805256)
+    - aio-wait: delegate polling of main AioContext if BQL not held
+
+ -- Rafael David Tinoco <[e-mail]>  Wed, 27 May 2020 21:47:21 +0000
+
 qemu (1:4.2-3ubuntu7) groovy; urgency=medium
 
   * SECURITY UPDATE: DoS via integer overflow in ati_2d_blt()
diff --git a/debian/patches/series b/debian/patches/series
index 9e5edf2..a61ded8 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -16,6 +16,8 @@ lp-1859527-virtio-blk-fix-out-of-bounds-access-to-bitmap-in-not.patch
 ubuntu/vhost-user-gpu-Drop-trailing-json-comma.patch
 ubuntu/lp-1847361-modules-load-upgrade.patch
 ubuntu/lp-1847361-vhost-correctly-turn-on-VIRTIO_F_IOMMU_PLATFORM.patch
+ubuntu/lp-1805256-async_use_explicit_mem_barriers.patch
+ubuntu/lp-1805256-aio-wait_delegate_poll_aioctx_bql.patch
 
 # stabilize 4.2 with patches sent to qemu-stable since 4.2 released
 stable/lp-1867519-arm-arm-powerctl-set-NSACR.-CP11-CP10-bits-in-arm_se.patch
diff --git a/debian/patches/ubuntu/lp-1805256-aio-wait_delegate_poll_aioctx_bql.patch b/debian/patches/ubuntu/lp-1805256-aio-wait_delegate_poll_aioctx_bql.patch
new file mode 100644
index 0000000..ec5d9e1
--- /dev/null
+++ b/debian/patches/ubuntu/lp-1805256-aio-wait_delegate_poll_aioctx_bql.patch
@@ -0,0 +1,110 @@
+Description: aio-wait: delegate polling of main AioContext if BQL not held
+
+Any thread that is not a iothread returns NULL for qemu_get_current_aio_context().
+As a result, it would also return true for
+in_aio_context_home_thread(qemu_get_aio_context()), causing
+AIO_WAIT_WHILE to invoke aio_poll() directly.  This is incorrect
+if the BQL is not held, because aio_poll() does not expect to
+run concurrently from multiple threads, and it can actually
+happen when savevm writes to the vmstate file from the
+migration thread.
+
+Therefore, restrict in_aio_context_home_thread to return true
+for the main AioContext only if the BQL is held.
+
+The function is moved to aio-wait.h because it is mostly used
+there and to avoid a circular reference between main-loop.h
+and block/aio.h.
+
+Signed-off-by: Paolo Bonzini <[e-mail]>
+Message-Id: <[e-mail]>
+Signed-off-by: Stefan Hajnoczi <[e-mail]>
+
+Author: Paolo Bonzini <[e-mail]>
+Origin: upstream, https://github.com/qemu/qemu/commit/3c18a92dc
+Bug: https://launchpad.net/bugs/1805256
+Bug-Ubuntu: https://launchpad.net/bugs/1805256
+Reviewed-By: Rafael David Tinoco <[e-mail]>
+Last-Update: 2020-05-06
+
+--- qemu-4.2.orig/include/block/aio-wait.h
++++ qemu-4.2/include/block/aio-wait.h
+@@ -26,6 +26,7 @@
+ #define QEMU_AIO_WAIT_H
+ 
+ #include "block/aio.h"
++#include "qemu/main-loop.h"
+ 
+ /**
+  * AioWait:
+@@ -124,4 +125,25 @@ void aio_wait_kick(void);
+  */
+ void aio_wait_bh_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque);
+ 
++/**
++ * in_aio_context_home_thread:
++ * @ctx: the aio context
++ *
++ * Return whether we are running in the thread that normally runs @ctx.  Note
++ * that acquiring/releasing ctx does not affect the outcome, each AioContext
++ * still only has one home thread that is responsible for running it.
++ */
++static inline bool in_aio_context_home_thread(AioContext *ctx)
++{
++    if (ctx == qemu_get_current_aio_context()) {
++        return true;
++    }
++
++    if (ctx == qemu_get_aio_context()) {
++        return qemu_mutex_iothread_locked();
++    } else {
++        return false;
++    }
++}
++
+ #endif /* QEMU_AIO_WAIT_H */
+--- qemu-4.2.orig/include/block/aio.h
++++ qemu-4.2/include/block/aio.h
+@@ -60,12 +60,16 @@ struct AioContext {
+     QLIST_HEAD(, AioHandler) aio_handlers;
+ 
+     /* Used to avoid unnecessary event_notifier_set calls in aio_notify;
+-     * accessed with atomic primitives.  If this field is 0, everything
+-     * (file descriptors, bottom halves, timers) will be re-evaluated
+-     * before the next blocking poll(), thus the event_notifier_set call
+-     * can be skipped.  If it is non-zero, you may need to wake up a
+-     * concurrent aio_poll or the glib main event loop, making
+-     * event_notifier_set necessary.
++     * only written from the AioContext home thread, or under the BQL in
++     * the case of the main AioContext.  However, it is read from any
++     * thread so it is still accessed with atomic primitives.
++     *
++     * If this field is 0, everything (file descriptors, bottom halves,
++     * timers) will be re-evaluated before the next blocking poll() or
++     * io_uring wait; therefore, the event_notifier_set call can be
++     * skipped.  If it is non-zero, you may need to wake up a concurrent
++     * aio_poll or the glib main event loop, making event_notifier_set
++     * necessary.
+      *
+      * Bit 0 is reserved for GSource usage of the AioContext, and is 1
+      * between a call to aio_ctx_prepare and the next call to aio_ctx_check.
+@@ -581,19 +585,6 @@ void aio_co_enter(AioContext *ctx, struc
+ AioContext *qemu_get_current_aio_context(void);
+ 
+ /**
+- * in_aio_context_home_thread:
+- * @ctx: the aio context
+- *
+- * Return whether we are running in the thread that normally runs @ctx.  Note
+- * that acquiring/releasing ctx does not affect the outcome, each AioContext
+- * still only has one home thread that is responsible for running it.
+- */
+-static inline bool in_aio_context_home_thread(AioContext *ctx)
+-{
+-    return ctx == qemu_get_current_aio_context();
+-}
+-
+-/**
+  * aio_context_setup:
+  * @ctx: the aio context
+  *
diff --git a/debian/patches/ubuntu/lp-1805256-async_use_explicit_mem_barriers.patch b/debian/patches/ubuntu/lp-1805256-async_use_explicit_mem_barriers.patch
new file mode 100644
index 0000000..3ed09e1
--- /dev/null
+++ b/debian/patches/ubuntu/lp-1805256-async_use_explicit_mem_barriers.patch
@@ -0,0 +1,159 @@
+Description: async: use explicit memory barriers
+
+When using C11 atomics, non-seqcst reads and writes do not participate
+in the total order of seqcst operations.  In util/async.c and util/aio-posix.c,
+in particular, the pattern that we use
+
+          write ctx->notify_me                 write bh->scheduled
+          read bh->scheduled                   read ctx->notify_me
+          if !bh->scheduled, sleep             if ctx->notify_me, notify
+
+needs to use seqcst operations for both the write and the read.  In
+general this is something that we do not want, because there can be
+many sources that are polled in addition to bottom halves.  The
+alternative is to place a seqcst memory barrier between the write
+and the read.  This also comes with a disadvantage, in that the
+memory barrier is implicit on strongly-ordered architectures and
+it wastes a few dozen clock cycles.
+
+Fortunately, ctx->notify_me is never written concurrently by two
+threads, so we can assert that and relax the writes to ctx->notify_me.
+The resulting solution works and performs well on both aarch64 and x86.
+
+Note that the atomic_set/atomic_read combination is not an atomic
+read-modify-write, and therefore it is even weaker than C11 ATOMIC_RELAXED;
+on x86, ATOMIC_RELAXED compiles to a locked operation.
+
+Analyzed-by: Ying Fang <[e-mail]>
+Signed-off-by: Paolo Bonzini <[e-mail]>
+Tested-by: Ying Fang <[e-mail]>
+Message-Id: <[e-mail]>
+Signed-off-by: Stefan Hajnoczi <[e-mail]>
+
+Author: Paolo Bonzini <[e-mail]>
+Origin: upstream, https://github.com/qemu/qemu/commit/5710a3e09f
+Bug: https://bugs.launchpad.net/bugs/1805256
+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1805256
+Reviewed-By: Rafael David Tinoco <[e-mail]>
+Last-Update: 2020-05-06
+
+--- qemu-4.2.orig/util/aio-posix.c
++++ qemu-4.2/util/aio-posix.c
+@@ -616,6 +616,11 @@ bool aio_poll(AioContext *ctx, bool bloc
+     int64_t timeout;
+     int64_t start = 0;
+ 
++    /*
++     * There cannot be two concurrent aio_poll calls for the same AioContext (or
++     * an aio_poll concurrent with a GSource prepare/check/dispatch callback).
++     * We rely on this below to avoid slow locked accesses to ctx->notify_me.
++     */
+     assert(in_aio_context_home_thread(ctx));
+ 
+     /* aio_notify can avoid the expensive event_notifier_set if
+@@ -626,7 +631,13 @@ bool aio_poll(AioContext *ctx, bool bloc
+      * so disable the optimization now.
+      */
+     if (blocking) {
+-        atomic_add(&ctx->notify_me, 2);
++        atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
++        /*
++         * Write ctx->notify_me before computing the timeout
++         * (reading bottom half flags, etc.).  Pairs with
++         * smp_mb in aio_notify().
++         */
++        smp_mb();
+     }
+ 
+     qemu_lockcnt_inc(&ctx->list_lock);
+@@ -671,7 +682,8 @@ bool aio_poll(AioContext *ctx, bool bloc
+     }
+ 
+     if (blocking) {
+-        atomic_sub(&ctx->notify_me, 2);
++        /* Finish the poll before clearing the flag.  */
++        atomic_store_release(&ctx->notify_me, atomic_read(&ctx->notify_me) - 2);
+         aio_notify_accept(ctx);
+     }
+ 
+--- qemu-4.2.orig/util/aio-win32.c
++++ qemu-4.2/util/aio-win32.c
+@@ -321,6 +321,12 @@ bool aio_poll(AioContext *ctx, bool bloc
+     int count;
+     int timeout;
+ 
++    /*
++     * There cannot be two concurrent aio_poll calls for the same AioContext (or
++     * an aio_poll concurrent with a GSource prepare/check/dispatch callback).
++     * We rely on this below to avoid slow locked accesses to ctx->notify_me.
++     */
++    assert(in_aio_context_home_thread(ctx));
+     progress = false;
+ 
+     /* aio_notify can avoid the expensive event_notifier_set if
+@@ -331,7 +337,13 @@ bool aio_poll(AioContext *ctx, bool bloc
+      * so disable the optimization now.
+      */
+     if (blocking) {
+-        atomic_add(&ctx->notify_me, 2);
++        atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
++        /*
++         * Write ctx->notify_me before computing the timeout
++         * (reading bottom half flags, etc.).  Pairs with
++         * smp_mb in aio_notify().
++         */
++        smp_mb();
+     }
+ 
+     qemu_lockcnt_inc(&ctx->list_lock);
+@@ -364,8 +376,7 @@ bool aio_poll(AioContext *ctx, bool bloc
+         ret = WaitForMultipleObjects(count, events, FALSE, timeout);
+         if (blocking) {
+             assert(first);
+-            assert(in_aio_context_home_thread(ctx));
+-            atomic_sub(&ctx->notify_me, 2);
++            atomic_store_release(&ctx->notify_me, atomic_read(&ctx->notify_me) - 2);
+             aio_notify_accept(ctx);
+         }
+ 
+--- qemu-4.2.orig/util/async.c
++++ qemu-4.2/util/async.c
+@@ -220,7 +220,14 @@ aio_ctx_prepare(GSource *source, gint
+ {
+     AioContext *ctx = (AioContext *) source;
+ 
+-    atomic_or(&ctx->notify_me, 1);
++    atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) | 1);
++
++    /*
++     * Write ctx->notify_me before computing the timeout
++     * (reading bottom half flags, etc.).  Pairs with
++     * smp_mb in aio_notify().
++     */
++    smp_mb();
+ 
+     /* We assume there is no timeout already supplied */
+     *timeout = qemu_timeout_ns_to_ms(aio_compute_timeout(ctx));
+@@ -238,7 +245,8 @@ aio_ctx_check(GSource *source)
+     AioContext *ctx = (AioContext *) source;
+     QEMUBH *bh;
+ 
+-    atomic_and(&ctx->notify_me, ~1);
++    /* Finish computing the timeout before clearing the flag.  */
++    atomic_store_release(&ctx->notify_me, atomic_read(&ctx->notify_me) & ~1);
+     aio_notify_accept(ctx);
+ 
+     for (bh = ctx->first_bh; bh; bh = bh->next) {
+@@ -343,10 +351,10 @@ LinuxAioState *aio_get_linux_aio(AioCont
+ void aio_notify(AioContext *ctx)
+ {
+     /* Write e.g. bh->scheduled before reading ctx->notify_me.  Pairs
+-     * with atomic_or in aio_ctx_prepare or atomic_add in aio_poll.
++     * with smp_mb in aio_ctx_prepare or aio_poll.
+      */
+     smp_mb();
+-    if (ctx->notify_me) {
++    if (atomic_read(&ctx->notify_me)) {
+         event_notifier_set(&ctx->notifier);
+         atomic_mb_set(&ctx->notified, true);
+     }