Re: [Qemu-devel] [PATCH 1/2] qcow2: Undo leaked allocations in co_writev

[Max Reitz <mreitz@redhat.com>]

On 2013-10-10 14:26, Kevin Wolf wrote:
> Am 10.10.2013 um 10:52 hat Max Reitz geschrieben:
>> If the write request spans more than one L2 table,
>> qcow2_alloc_cluster_offset cannot handle the required allocations
>> atomically. This results in leaks if it allocated new clusters in any
>> but the last L2 table touched and an error occurs in qcow2_co_writev
>> before having established the L2 link. These non-atomic allocations
>> were, however, indeed successful and are therefore given to the caller
>> in the L2Meta list.
>>
>> If an error occurs in qcow2_co_writev and the L2Meta list is unwound,
>> all its remaining entries are clusters whose L2 links were not yet
>> established. Thus, all allocations in that list should be undone.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/qcow2.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index b2489fb..6bedd5d 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1017,6 +1017,13 @@ fail:
>>       while (l2meta != NULL) {
>>           QCowL2Meta *next;
>>   
>> +        /* Undo all leaked allocations */
>> +        if (l2meta->nb_clusters != 0) {
>> +            qcow2_free_clusters(bs, l2meta->alloc_offset,
>> +                                l2meta->nb_clusters << s->cluster_bits,
>> +                                QCOW2_DISCARD_ALWAYS);
>> +        }
>> +
>>           if (l2meta->nb_clusters != 0) {
>>               QLIST_REMOVE(l2meta, next_in_flight);
>>           }
> This feels a bit risky.
>
> I think currently it does work, because qcow2_alloc_cluster_link_l2()
> can only return an error when it didn't update the L2 entry in the cache
> yet, but adding any error condition between that point and the L2Meta
> unwinding would result in corruption. I'm unsure, but perhaps a cluster
> leak is the lesser evil. Did you consider this? Do other people have an
> opinion on it?

What error conditions are there which can occur between 
qcow2_alloc_cluster_link_l2 and the L2Meta unwinding? If all 
qcow2_alloc_cluster_link_l2 calls are successful, the list is empty and 
the while loop either goes into another iteration or the function 
returns successfully (without any further need to unwind the list). If 
some call fails, all previous (successful) calls have already been 
removed from the list, therefore the unwinding only affects L2Meta 
request with failed calls to qcow2_alloc_cluster_link_l2 (or ones where 
that function wasn't called at all).

If the "currently" implied that this will turn out bad if there is a new 
error condition between a successful call to qcow2_alloc_cluster_link_l2 
and the removal of the L2Meta request from the list: Yes, that's true, 
of course. However, as you've said, currently, there is no such 
condition; and I don't see why it should be introduced. The sole purpose 
of the list seems to be (to me) to execute qcow2_alloc_cluster_link_l2 
on every of its elements. Thus, as soon as qcow2_alloc_cluster_link_l2 
is successful, the corresponding request should be removed from the list.

So, in case you do agree that it currently works fine, I would not 
consider it risky; if this patch is applied and some time in the future 
anything introduces a "goto fail" between qcow2_alloc_cluster_link_l2 
and l2_meta = next, this patch would simply have to make sure that 
qcow2_free_clusters isn't called in this case. In the probably very 
unlikely case all my previous assumptions and conclusions were true, I'd 
just add a comment in the qcow2_alloc_cluster_link_l2 loop informing 
about this case (“If you add a goto fail here, make sure to pay 
attention” or something along these lines).

> Also, shouldn't it be QCOW2_DISCARD_OTHER?

I'm always unsure about the discard flags. ;-)

I try to follow the rule of “use the specific type (or ‘other’) for 
freeing ‘out of the blue’, but use ‘always’ if it's just a very recent 
allocation that is being undone again”. I'd gladly accept better 
recommendations. ;-)

Max