From: Sergio Lopez <slp@redhat.com>

block_job_remove_all_bdrv() iterates through job->nodes, calling
bdrv_root_unref_child() for each entry. The call to the latter may
reach child_job_[can_]set_aio_ctx(), which will also attempt to
traverse job->nodes, potentially finding entries that where freed
on previous iterations.

To avoid this situation, update job->nodes head on each iteration to
ensure that already freed entries are no longer linked to the list.

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1746631
Signed-off-by: Sergio Lopez <slp@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
v3:
- Rewrote the loop to make the whole function a bit simpler
  (Also, remove the node from the job->nodes list before unref'ing it,
  just to be extra-safe)
---
 blockjob.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 2abed0f551..c6e20e2fcd 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -175,14 +175,23 @@ static const BdrvChildRole child_job = {
 
 void block_job_remove_all_bdrv(BlockJob *job)
 {
-    GSList *l;
-    for (l = job->nodes; l; l = l->next) {
+    /*
+     * bdrv_root_unref_child() may reach child_job_[can_]set_aio_ctx(),
+     * which will also traverse job->nodes, so consume the list one by
+     * one to make sure that such a concurrent access does not attempt
+     * to process an already freed BdrvChild.
+     */
+    while (job->nodes) {
+        GSList *l = job->nodes;
         BdrvChild *c = l->data;
+
+        job->nodes = l->next;
+
         bdrv_op_unblock_all(c->bs, job->blocker);
         bdrv_root_unref_child(c);
+
+        g_slist_free_1(l);
     }
-    g_slist_free(job->nodes);
-    job->nodes = NULL;
 }
 
 bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs)
-- 
2.21.0

Max Reitz <mreitz@redhat.com> writes:

> From: Sergio Lopez <slp@redhat.com>
>
> block_job_remove_all_bdrv() iterates through job->nodes, calling
> bdrv_root_unref_child() for each entry. The call to the latter may
> reach child_job_[can_]set_aio_ctx(), which will also attempt to
> traverse job->nodes, potentially finding entries that where freed
> on previous iterations.
>
> To avoid this situation, update job->nodes head on each iteration to
> ensure that already freed entries are no longer linked to the list.
>
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1746631
> Signed-off-by: Sergio Lopez <slp@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> v3:
> - Rewrote the loop to make the whole function a bit simpler
>   (Also, remove the node from the job->nodes list before unref'ing it,
>   just to be extra-safe)
> ---
>  blockjob.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/blockjob.c b/blockjob.c
> index 2abed0f551..c6e20e2fcd 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -175,14 +175,23 @@ static const BdrvChildRole child_job = {
>  
>  void block_job_remove_all_bdrv(BlockJob *job)
>  {
> -    GSList *l;
> -    for (l = job->nodes; l; l = l->next) {
> +    /*
> +     * bdrv_root_unref_child() may reach child_job_[can_]set_aio_ctx(),
> +     * which will also traverse job->nodes, so consume the list one by
> +     * one to make sure that such a concurrent access does not attempt
> +     * to process an already freed BdrvChild.
> +     */
> +    while (job->nodes) {
> +        GSList *l = job->nodes;
>          BdrvChild *c = l->data;
> +
> +        job->nodes = l->next;
> +
>          bdrv_op_unblock_all(c->bs, job->blocker);
>          bdrv_root_unref_child(c);
> +
> +        g_slist_free_1(l);
>      }
> -    g_slist_free(job->nodes);
> -    job->nodes = NULL;
>  }
>  
>  bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs)

Reviewed-by: Sergio Lopez <slp@redhat.com>

On 11.09.19 12:03, Max Reitz wrote:
> From: Sergio Lopez <slp@redhat.com>
> 
> block_job_remove_all_bdrv() iterates through job->nodes, calling
> bdrv_root_unref_child() for each entry. The call to the latter may
> reach child_job_[can_]set_aio_ctx(), which will also attempt to
> traverse job->nodes, potentially finding entries that where freed
> on previous iterations.
> 
> To avoid this situation, update job->nodes head on each iteration to
> ensure that already freed entries are no longer linked to the list.
> 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1746631
> Signed-off-by: Sergio Lopez <slp@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> v3:
> - Rewrote the loop to make the whole function a bit simpler
>   (Also, remove the node from the job->nodes list before unref'ing it,
>   just to be extra-safe)
> ---
>  blockjob.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)

Thanks Sergio for tracking down the bug’s cause, the original patch, and
the review; I’ve applied the patch to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max