[PATCH v2] block: temporarily hold the new AioContext of bs_top in bdrv_append()

[PATCH v2] block: temporarily hold the new AioContext of bs_top in bdrv_append()

[Stefano Garzarella]

bdrv_append() is called with bs_top AioContext held, but
bdrv_attach_child_noperm() could change the AioContext of bs_top.

bdrv_replace_node_noperm() calls bdrv_drained_begin() starting from
commit 2398747128 ("block: Don't poll in bdrv_replace_child_noperm()").
bdrv_drained_begin() can call BDRV_POLL_WHILE that assumes the new lock
is taken, so let's temporarily hold the new AioContext to prevent QEMU
from failing in BDRV_POLL_WHILE when it tries to release the wrong
AioContext.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2168209
Reported-by: Aihua Liang <aliang@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
v2:
- released the right lock in the error path [Kevin]
- held the new lock until the end of the function [Kevin]

v1: https://lore.kernel.org/qemu-devel/20230214105156.316586-1-sgarzare@redhat.com/
---
 block.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/block.c b/block.c
index aa9062f2c1..036fce19e0 100644
--- a/block.c
+++ b/block.c
@@ -5266,6 +5266,8 @@ int bdrv_drop_filter(BlockDriverState *bs, Error **errp)
  * child.
  *
  * This function does not create any image files.
+ *
+ * The caller must hold the AioContext lock for @bs_top.
  */
 int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
                 Error **errp)
@@ -5273,11 +5275,14 @@ int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
     int ret;
     BdrvChild *child;
     Transaction *tran = tran_new();
+    AioContext *old_context, *new_context = NULL;
 
     GLOBAL_STATE_CODE();
 
     assert(!bs_new->backing);
 
+    old_context = bdrv_get_aio_context(bs_top);
+
     child = bdrv_attach_child_noperm(bs_new, bs_top, "backing",
                                      &child_of_bds, bdrv_backing_role(bs_new),
                                      tran, errp);
@@ -5286,6 +5291,19 @@ int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
         goto out;
     }
 
+    /*
+     * bdrv_attach_child_noperm could change the AioContext of bs_top.
+     * bdrv_replace_node_noperm calls bdrv_drained_begin, so let's temporarily
+     * hold the new AioContext, since bdrv_drained_begin calls BDRV_POLL_WHILE
+     * that assumes the new lock is taken.
+     */
+    new_context = bdrv_get_aio_context(bs_top);
+
+    if (old_context != new_context) {
+        aio_context_release(old_context);
+        aio_context_acquire(new_context);
+    }
+
     ret = bdrv_replace_node_noperm(bs_top, bs_new, true, tran, errp);
     if (ret < 0) {
         goto out;
@@ -5297,6 +5315,11 @@ out:
 
     bdrv_refresh_limits(bs_top, NULL, NULL);
 
+    if (new_context && old_context != new_context) {
+        aio_context_release(new_context);
+        aio_context_acquire(old_context);
+    }
+
     return ret;
 }
 
-- 
2.39.1

Re: [PATCH v2] block: temporarily hold the new AioContext of bs_top in bdrv_append()

[Kevin Wolf]

Am 14.02.2023 um 18:16 hat Stefano Garzarella geschrieben:
> bdrv_append() is called with bs_top AioContext held, but
> bdrv_attach_child_noperm() could change the AioContext of bs_top.
> 
> bdrv_replace_node_noperm() calls bdrv_drained_begin() starting from
> commit 2398747128 ("block: Don't poll in bdrv_replace_child_noperm()").
> bdrv_drained_begin() can call BDRV_POLL_WHILE that assumes the new lock
> is taken, so let's temporarily hold the new AioContext to prevent QEMU
> from failing in BDRV_POLL_WHILE when it tries to release the wrong
> AioContext.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2168209
> Reported-by: Aihua Liang <aliang@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks, applied to the block branch.

Kevin