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: address@hidden
> Signed-off-by: Paolo Bonzini <address@hidden>

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;
>  
>