[Qemu-devel] [PATCH v2] blockjob: update nodes head while removing all bdrv

[Qemu-devel] [PATCH v2] blockjob: update nodes head while removing all bdrv

[Sergio Lopez]

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

v2:
 - Avoid leaking job->nodes (thanks Max Reitz)
---
 blockjob.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
---
diff --git a/blockjob.c b/blockjob.c
index 6e32d1a0c0..ffda6dd1e4 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -187,13 +187,21 @@ static const BdrvChildRole child_job = {
 
 void block_job_remove_all_bdrv(BlockJob *job)
 {
-    GSList *l;
+    GSList *l, *orig_nodes;
+
+    orig_nodes = job->nodes;
     for (l = job->nodes; l; l = l->next) {
         BdrvChild *c = l->data;
         bdrv_op_unblock_all(c->bs, job->blocker);
         bdrv_root_unref_child(c);
+        /*
+         * The call above may reach child_job_[can_]set_aio_ctx(), which will
+         * also traverse job->nodes, so update the head here to make sure it
+         * doesn't attempt to process an already freed BdrvChild.
+         */
+        job->nodes = l->next;
     }
-    g_slist_free(job->nodes);
+    g_slist_free(orig_nodes);
     job->nodes = NULL;
 }
 
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2] blockjob: update nodes head while removing all bdrv

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2017 bytes --]

On 10.09.19 15:36, Sergio Lopez wrote:
> 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>
> ---
> Changelog
> 
> v2:
>  - Avoid leaking job->nodes (thanks Max Reitz)
> ---
>  blockjob.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> ---
> diff --git a/blockjob.c b/blockjob.c
> index 6e32d1a0c0..ffda6dd1e4 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -187,13 +187,21 @@ static const BdrvChildRole child_job = {
>  
>  void block_job_remove_all_bdrv(BlockJob *job)
>  {
> -    GSList *l;
> +    GSList *l, *orig_nodes;
> +
> +    orig_nodes = job->nodes;
>      for (l = job->nodes; l; l = l->next) {
>          BdrvChild *c = l->data;
>          bdrv_op_unblock_all(c->bs, job->blocker);
>          bdrv_root_unref_child(c);
> +        /*
> +         * The call above may reach child_job_[can_]set_aio_ctx(), which will
> +         * also traverse job->nodes, so update the head here to make sure it
> +         * doesn't attempt to process an already freed BdrvChild.
> +         */
> +        job->nodes = l->next;
>      }
> -    g_slist_free(job->nodes);
> +    g_slist_free(orig_nodes);
>      job->nodes = NULL;

Hm, this assignment is now a no-op.

I think I’d just rewrite the whole function in the following fashion:

orig_nodes = job->nodes;
while (job->nodes) {
    BdrvChild *c = job->nodes->data;
    [...]
    job->nodes = job->nodes->next;
}
g_slist_free(orig_nodes);

What do you think?

>  }
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2] blockjob: update nodes head while removing all bdrv

[Sergio Lopez]

[-- Attachment #1: Type: text/plain, Size: 2431 bytes --]


Max Reitz <mreitz@redhat.com> writes:

> On 10.09.19 15:36, Sergio Lopez wrote:
>> 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>
>> ---
>> Changelog
>> 
>> v2:
>>  - Avoid leaking job->nodes (thanks Max Reitz)
>> ---
>>  blockjob.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>> ---
>> diff --git a/blockjob.c b/blockjob.c
>> index 6e32d1a0c0..ffda6dd1e4 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -187,13 +187,21 @@ static const BdrvChildRole child_job = {
>>  
>>  void block_job_remove_all_bdrv(BlockJob *job)
>>  {
>> -    GSList *l;
>> +    GSList *l, *orig_nodes;
>> +
>> +    orig_nodes = job->nodes;
>>      for (l = job->nodes; l; l = l->next) {
>>          BdrvChild *c = l->data;
>>          bdrv_op_unblock_all(c->bs, job->blocker);
>>          bdrv_root_unref_child(c);
>> +        /*
>> +         * The call above may reach child_job_[can_]set_aio_ctx(), which will
>> +         * also traverse job->nodes, so update the head here to make sure it
>> +         * doesn't attempt to process an already freed BdrvChild.
>> +         */
>> +        job->nodes = l->next;
>>      }
>> -    g_slist_free(job->nodes);
>> +    g_slist_free(orig_nodes);
>>      job->nodes = NULL;
>
> Hm, this assignment is now a no-op.
>
> I think I’d just rewrite the whole function in the following fashion:
>
> orig_nodes = job->nodes;
> while (job->nodes) {
>     BdrvChild *c = job->nodes->data;
>     [...]
>     job->nodes = job->nodes->next;
> }
> g_slist_free(orig_nodes);
>
> What do you think?
>

As this is the first time I was touching this code, I was trying to keep
the changes minimal, but I definitely prefer to rewrite the function as
you suggest.

Should I send a v3, or do you want to send a patch yourself? I don't
really mind either, just want to get this fixed ASAP :-)

Thanks Max,
Sergio.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]