[Qemu-devel] [PATCH] blockjob: prevent double enter (with dangling access on the second)

[Qemu-devel] [PATCH] blockjob: prevent double enter (with dangling access on the second)

[Paolo Bonzini]

Because block_job_sleep_ns marks the job as non-busy, it is
possible to enter it again from block_job_enter.  However, the
same coroutine may then be re-entered from co_sleep_cb, and this
time the CoSleepCB is not on the stack anymore.

Fix this by open coding the sleep in block_job_sleep_ns, and
deleting the timer immediately after the coroutine is re-entered.

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 blockjob.c               | 27 +++++++++++++++++++++++----
 include/block/blockjob.h |  3 +--
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 80adb9d..5e59a8f 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -329,8 +329,21 @@ int block_job_complete_sync(BlockJob *job, Error **errp)
     return block_job_finish_sync(job, &block_job_complete, errp);
 }
 
+typedef struct CoSleepCB {
+    QEMUTimer *ts;
+    Coroutine *co;
+} CoSleepCB;
+
+static void co_sleep_cb(void *opaque)
+{
+    CoSleepCB *sleep_cb = opaque;
+
+    qemu_coroutine_enter(sleep_cb->co, NULL);
+}
+
 void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns)
 {
+    CoSleepCB sleep_cb = { .ts = NULL };
     assert(job->busy);
 
     /* Check cancellation *before* setting busy = false, too!  */
@@ -339,10 +352,16 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns)
     }
 
     job->busy = false;
-    if (block_job_is_paused(job)) {
-        qemu_coroutine_yield();
-    } else {
-        co_aio_sleep_ns(bdrv_get_aio_context(job->bs), type, ns);
+    if (!block_job_is_paused(job)) {
+        sleep_cb.co = qemu_coroutine_self(),
+        sleep_cb.ts = aio_timer_new(bdrv_get_aio_context(job->bs), type,
+                                    SCALE_NS, co_sleep_cb, &sleep_cb);
+        timer_mod(sleep_cb.ts, qemu_clock_get_ns(type) + ns);
+    }
+    qemu_coroutine_yield();
+    if (sleep_cb.ts) {
+        timer_del(sleep_cb.ts);
+        timer_free(sleep_cb.ts);
     }
     job->busy = true;
 }
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index d84ccd8..82063f2 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -120,8 +120,7 @@ struct BlockJob {
 
     /**
      * Set to false by the job while it is in a quiescent state, where
-     * no I/O is pending and the job has yielded on any condition
-     * that is not detected by #aio_poll, such as a timer.
+     * no I/O is pending.  The job can be reentered with qemu_coroutine_enter.
      */
     bool busy;
 
-- 
2.5.0

Re: [Qemu-devel] [PATCH] blockjob: prevent double enter (with dangling access on the second)

[Paolo Bonzini]

On 23/12/2015 11:48, Paolo Bonzini wrote:
> Because block_job_sleep_ns marks the job as non-busy, it is
> possible to enter it again from block_job_enter.  However, the
> same coroutine may then be re-entered from co_sleep_cb, and this
> time the CoSleepCB is not on the stack anymore.
> 
> Fix this by open coding the sleep in block_job_sleep_ns, and
> deleting the timer immediately after the coroutine is re-entered.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Forget about this, the current code does exactly the same as what I
wrote, and is correct.  /me needs vacation. :)

Paolo

> ---
>  blockjob.c               | 27 +++++++++++++++++++++++----
>  include/block/blockjob.h |  3 +--
>  2 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 80adb9d..5e59a8f 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -329,8 +329,21 @@ int block_job_complete_sync(BlockJob *job, Error **errp)
>      return block_job_finish_sync(job, &block_job_complete, errp);
>  }
>  
> +typedef struct CoSleepCB {
> +    QEMUTimer *ts;
> +    Coroutine *co;
> +} CoSleepCB;
> +
> +static void co_sleep_cb(void *opaque)
> +{
> +    CoSleepCB *sleep_cb = opaque;
> +
> +    qemu_coroutine_enter(sleep_cb->co, NULL);
> +}
> +
>  void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns)
>  {
> +    CoSleepCB sleep_cb = { .ts = NULL };
>      assert(job->busy);
>  
>      /* Check cancellation *before* setting busy = false, too!  */
> @@ -339,10 +352,16 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns)
>      }
>  
>      job->busy = false;
> -    if (block_job_is_paused(job)) {
> -        qemu_coroutine_yield();
> -    } else {
> -        co_aio_sleep_ns(bdrv_get_aio_context(job->bs), type, ns);
> +    if (!block_job_is_paused(job)) {
> +        sleep_cb.co = qemu_coroutine_self(),
> +        sleep_cb.ts = aio_timer_new(bdrv_get_aio_context(job->bs), type,
> +                                    SCALE_NS, co_sleep_cb, &sleep_cb);
> +        timer_mod(sleep_cb.ts, qemu_clock_get_ns(type) + ns);
> +    }
> +    qemu_coroutine_yield();
> +    if (sleep_cb.ts) {
> +        timer_del(sleep_cb.ts);
> +        timer_free(sleep_cb.ts);
>      }
>      job->busy = true;
>  }
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index d84ccd8..82063f2 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -120,8 +120,7 @@ struct BlockJob {
>  
>      /**
>       * Set to false by the job while it is in a quiescent state, where
> -     * no I/O is pending and the job has yielded on any condition
> -     * that is not detected by #aio_poll, such as a timer.
> +     * no I/O is pending.  The job can be reentered with qemu_coroutine_enter.
>       */
>      bool busy;
>  
>