[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2] blockjob: update nodes head while removing a
From: |
Sergio Lopez |
Subject: |
Re: [Qemu-block] [PATCH v2] blockjob: update nodes head while removing all bdrv |
Date: |
Tue, 10 Sep 2019 16:07:47 +0200 |
User-agent: |
mu4e 1.2.0; emacs 26.2 |
Max Reitz <address@hidden> 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 <address@hidden>
>> ---
>> 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.
signature.asc
Description: PGP signature