throttle: fix a qemu crash problem when calling blk_delete

Commit Message

sochin jiang Nov. 24, 2013, 3:55 a.m. UTC

From: "sochin.jiang" <[e-mail]>

commit 7ca7f0 moves the throttling related part of the BDS life cycle management
to BlockBackend, adds call to throttle_timers_detach_aio_context in blk_remove_bs.
commit 1606e remove a block device from its throttle group in blk_delete by calling
blk_io_limits_disable, this fix an easily reproducible qemu crash. But delete a
BB without a BDS inserted could easily cause a qemu crash too by calling
bdrv_drained_begin in blk_io_limits_disable. Say, a simply drive_add and then a
drive_del command.

Signed-off-by: sochin.jiang <[e-mail]>
---
 block/io.c      | 8 ++++++++
 util/throttle.c | 6 +++---
 2 files changed, 11 insertions(+), 3 deletions(-)

Comments

Alberto Garcia Oct. 20, 2017, 11:43 a.m. UTC | #1

On Sun 24 Nov 2013 04:55:52 AM CET, sochin.jiang wrote:
       ^^^^^^^^^^^
I guess the date in your computer is wrong :-)

> commit 7ca7f0 moves the throttling related part of the BDS life cycle
> management to BlockBackend, adds call to
> throttle_timers_detach_aio_context in blk_remove_bs.  commit 1606e
> remove a block device from its throttle group in blk_delete by calling
> blk_io_limits_disable, this fix an easily reproducible qemu crash. But
> delete a BB without a BDS inserted could easily cause a qemu crash too
> by calling bdrv_drained_begin in blk_io_limits_disable. Say, a simply
> drive_add and then a drive_del command.

Thanks, I can reproduce this easily by running QEMU and doing

   drive_add 0 if=none,throttling.iops-total=5000

followed by

   drive_del none0

>  void bdrv_drained_begin(BlockDriverState *bs)
>  {
> +    if (!bs) {
> +        return;
> +    }
> +
>      if (qemu_in_coroutine()) {
>          bdrv_co_yield_to_drain(bs, true);
>          return;
> @@ -284,6 +288,10 @@ void bdrv_drained_begin(BlockDriverState *bs)
>  
>  void bdrv_drained_end(BlockDriverState *bs)
>  {
> +    if (!bs) {
> +        return;
> +    }
> +

I'd say that if someone calls bdrv_drained_begin() with a NULL pointer
then the problem is in the caller...

>  static void throttle_timer_destroy(QEMUTimer **timer)
>  {
> -    assert(*timer != NULL);
> -
>      timer_del(*timer);
>      timer_free(*timer);
>      *timer = NULL;
> @@ -258,7 +256,9 @@ void throttle_timers_detach_aio_context(ThrottleTimers *tt)
>      int i;
>  
>      for (i = 0; i < 2; i++) {
> -        throttle_timer_destroy(&tt->timers[i]);
> +        if (tt->timers[i]) {
> +            throttle_timer_destroy(&tt->timers[i]);
> +        }
>      }
>  }

Why is this part necessary? In what situation you end up calling
throttle_timers_detach_aio_context() twice?

Berto

sochin jiang Oct. 21, 2017, 8:02 a.m. UTC | #2

Thanks for replying.

Indeed, the problem comes from the caller. I guest

some code should be reconsidered in blk_remove_bs

and blk_delete, especially with throttling.

Secondly, when handling drive_del in hmp_drive_del,

throttle_timers_detach_aio_context() is called

first time in blk_remove_bs and again throughing blk_unref,

see below:

hmp_drive_del->
    blk_remove_bs->
        *throttle_timers_detach_aio_context*->
    ...
    blk_unref->
        blk_delete->
            blk_io_limits_disable->
                throttle_group_unregister_tgm->
                    throttle_timers_destroy->
                        *throttle_timers_detach_aio_context*->**...**


sochin

.      


On 2017/10/20 19:43, Alberto Garcia wrote:
> On Sun 24 Nov 2013 04:55:52 AM CET, sochin.jiang wrote:
>        ^^^^^^^^^^^
> I guess the date in your computer is wrong :-)
>
>> commit 7ca7f0 moves the throttling related part of the BDS life cycle
>> management to BlockBackend, adds call to
>> throttle_timers_detach_aio_context in blk_remove_bs.  commit 1606e
>> remove a block device from its throttle group in blk_delete by calling
>> blk_io_limits_disable, this fix an easily reproducible qemu crash. But
>> delete a BB without a BDS inserted could easily cause a qemu crash too
>> by calling bdrv_drained_begin in blk_io_limits_disable. Say, a simply
>> drive_add and then a drive_del command.
> Thanks, I can reproduce this easily by running QEMU and doing
>
>    drive_add 0 if=none,throttling.iops-total=5000
>
> followed by
>
>    drive_del none0
>
>>  void bdrv_drained_begin(BlockDriverState *bs)
>>  {
>> +    if (!bs) {
>> +        return;
>> +    }
>> +
>>      if (qemu_in_coroutine()) {
>>          bdrv_co_yield_to_drain(bs, true);
>>          return;
>> @@ -284,6 +288,10 @@ void bdrv_drained_begin(BlockDriverState *bs)
>>  
>>  void bdrv_drained_end(BlockDriverState *bs)
>>  {
>> +    if (!bs) {
>> +        return;
>> +    }
>> +
> I'd say that if someone calls bdrv_drained_begin() with a NULL pointer
> then the problem is in the caller...
>
>>  static void throttle_timer_destroy(QEMUTimer **timer)
>>  {
>> -    assert(*timer != NULL);
>> -
>>      timer_del(*timer);
>>      timer_free(*timer);
>>      *timer = NULL;
>> @@ -258,7 +256,9 @@ void throttle_timers_detach_aio_context(ThrottleTimers *tt)
>>      int i;
>>  
>>      for (i = 0; i < 2; i++) {
>> -        throttle_timer_destroy(&tt->timers[i]);
>> +        if (tt->timers[i]) {
>> +            throttle_timer_destroy(&tt->timers[i]);
>> +        }
>>      }
>>  }
> Why is this part necessary? In what situation you end up calling
> throttle_timers_detach_aio_context() twice?
>
> Berto
>
> .
>

Patch

diff --git a/block/io.c b/block/io.c
index 0854e0f..c411a87 100644
--- a/block/io.c
+++ b/block/io.c
@@ -269,6 +269,10 @@  static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
 
 void bdrv_drained_begin(BlockDriverState *bs)
 {
+    if (!bs) {
+        return;
+    }
+
     if (qemu_in_coroutine()) {
         bdrv_co_yield_to_drain(bs, true);
         return;
@@ -284,6 +288,10 @@  void bdrv_drained_begin(BlockDriverState *bs)
 
 void bdrv_drained_end(BlockDriverState *bs)
 {
+    if (!bs) {
+        return;
+    }
+
     if (qemu_in_coroutine()) {
         bdrv_co_yield_to_drain(bs, false);
         return;
diff --git a/util/throttle.c b/util/throttle.c
index b38e742..35a21fc 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -245,8 +245,6 @@  void throttle_timers_init(ThrottleTimers *tt,
 /* destroy a timer */
 static void throttle_timer_destroy(QEMUTimer **timer)
 {
-    assert(*timer != NULL);
-
     timer_del(*timer);
     timer_free(*timer);
     *timer = NULL;
@@ -258,7 +256,9 @@  void throttle_timers_detach_aio_context(ThrottleTimers *tt)
     int i;
 
     for (i = 0; i < 2; i++) {
-        throttle_timer_destroy(&tt->timers[i]);
+        if (tt->timers[i]) {
+            throttle_timer_destroy(&tt->timers[i]);
+        }
     }
 }