Fix segfault in blockjob race condition

blockjob.c void block_job_start(BlockJob *job) · line 299

Quoted 11 lines · Jeff Cody

blockjob.c +1 -1

@@ -291,10 +291,10 @@ void block_job_start(BlockJob *job)
 {
     assert(job && !block_job_started(job) && job->paused &&
            job->driver && job->driver->start);
-    job->co = qemu_coroutine_create(block_job_co_entry, job);
     job->pause_count--;
     job->busy = true;
     job->paused = false;
+    job->co = qemu_coroutine_create(block_job_co_entry, job);
     bdrv_coroutine_enter(blk_bs(job->blk), job->co);
 }

Please see discussion on v1 about this hunk.

The rest looks good.

Quoted 15 lines · Stefan Hajnoczi

blockjob.c void block_job_start(BlockJob *job) · line 299

Quoted 11 lines · Jeff Cody

blockjob.c +1 -1

@@ -291,10 +291,10 @@ void block_job_start(BlockJob *job)
 {
     assert(job && !block_job_started(job) && job->paused &&
            job->driver && job->driver->start);
-    job->co = qemu_coroutine_create(block_job_co_entry, job);
     job->pause_count--;
     job->busy = true;
     job->paused = false;
+    job->co = qemu_coroutine_create(block_job_co_entry, job);
     bdrv_coroutine_enter(blk_bs(job->blk), job->co);
 }

Please see discussion on v1 about this hunk.

Quoted 1 line

The rest looks good.

I'm okay with this hunk, but I would appreciate that the commit message said why it's okay to delay block job cancellation after block_job_sleep_ns returns.

util/async.c fail: · line 450

Quoted 13 lines · Jeff Cody

util/async.c +9 -0

@@ -438,6 +439,16 @@ fail:
 void aio_co_schedule(AioContext *ctx, Coroutine *co)
 {
     trace_aio_co_schedule(ctx, co);
+    const char *scheduled = atomic_read(&co->scheduled);
+
+    if (scheduled) {
+        fprintf(stderr,
+                "%s: Co-routine was already scheduled in '%s'\n",
+                __func__, scheduled);
+        abort();
+    }
+    atomic_set(&co->scheduled, __func__);

According to docs/devel/atomics.txt atomic_set()/atomic_read() are weakly ordered. They require memory barriers to provide guarantees about ordering. Your patch doesn't include barriers or comments about where the implicit barriers are.

The docs recommend using the following instead of
atomic_read()/atomic_set() to get ordering:

typeof(*ptr) atomic_mb_read(ptr)
void atomic_mb_set(ptr, val)

The previous patch fixed a race condition, in which there were coroutines being executing doubly, or after coroutine deletion.

We can detect common scenarios when this happens, and print an error message and abort before we corrupt memory / data, or segfault.

This patch will abort if an attempt to enter a coroutine is made while it is currently pending execution, either in a specific AioContext bh, or pending execution via a timer. It will also abort if a coroutine is scheduled, before a prior scheduled run has occured.

We cannot rely on the existing co->caller check for recursive re-entry to catch this, as the coroutine may run and exit with COROUTINE_TERMINATE before the scheduled coroutine executes.

(This is the scenario that was occuring and fixed in the previous

patch).

Signed-off-by Jeff Cody <[email protected]>

---

The previous patch fixed a race condition, in which there were coroutines being executing doubly, or after coroutine deletion.

We can detect common scenarios when this happens, and print an error message and abort before we corrupt memory / data, or segfault.

This patch will abort if an attempt to enter a coroutine is made while it is currently pending execution, either in a specific AioContext bh, or pending execution via a timer. It will also abort if a coroutine is scheduled, before a prior scheduled run has occured.

We cannot rely on the existing co->caller check for recursive re-entry to catch this, as the coroutine may run and exit with COROUTINE_TERMINATE before the scheduled coroutine executes.

(This is the scenario that was occuring and fixed in the previous patch).

Signed-off-by Jeff Cody <[email protected]>

---
 include/qemu/coroutine_int.h |  6 ++++++
 util/async.c                 | 11 +++++++++++
 util/qemu-coroutine-sleep.c  | 11 +++++++++++
 util/qemu-coroutine.c        | 11 +++++++++++
 4 files changed, 39 insertions(+)

include/qemu/coroutine_int.h +0 -0

index cb98892..56e4c48 100644

Am 21.11.2017 um 03:23 hat Jeff Cody geschrieben:

Quoted 31 lines

The previous patch fixed a race condition, in which there were coroutines being executing doubly, or after coroutine deletion.

We can detect common scenarios when this happens, and print an error message and abort before we corrupt memory / data, or segfault.

This patch will abort if an attempt to enter a coroutine is made while it is currently pending execution, either in a specific AioContext bh, or pending execution via a timer. It will also abort if a coroutine is scheduled, before a prior scheduled run has occured.

We cannot rely on the existing co->caller check for recursive re-entry to catch this, as the coroutine may run and exit with COROUTINE_TERMINATE before the scheduled coroutine executes.

(This is the scenario that was occuring and fixed in the previous patch).

Signed-off-by Jeff Cody <[email protected]>

---
 include/qemu/coroutine_int.h |  6 ++++++
 util/async.c                 | 11 +++++++++++
 util/qemu-coroutine-sleep.c  | 11 +++++++++++
 util/qemu-coroutine.c        | 11 +++++++++++
 4 files changed, 39 insertions(+)

include/qemu/coroutine_int.h +0 -0

index cb98892..56e4c48 100644

include/qemu/coroutine_int.h struct Coroutine { · line 60

Quoted 8 lines

include/qemu/coroutine_int.h +5 -0

@@ -53,6 +53,12 @@ struct Coroutine {
 
     /* Only used when the coroutine has yielded.  */
     AioContext *ctx;
+
+    /* Used to catch and abort on illegal co-routine entry.
+     * Will contain the name of the function that had first
+     * scheduled the coroutine. */
+    const char *scheduled;

Not sure if it makes any difference in practice, but I just want to mention that the new field is right after a cacheline boundary and the only field that is used in qemu_aio_coroutine_enter() and accesses this second cacheline.

I'm not paying much attention to this kind of thing in most contexts, but entering a coroutine is a hot path that we want to be fast, so maybe it's worth having a second look.