Re: [Qemu-devel] [PATCH 3/3] block: Catch !bs->drv in bdrv_check()

[Max Reitz <mreitz@redhat.com>]

On 08.08.2014 11:15, Kevin Wolf wrote:
> Am 07.08.2014 um 22:47 hat Max Reitz geschrieben:
>> qemu-img check calls bdrv_check() twice if the first run repaired some
>> inconsistencies. If the first run however again triggered corruption
>> prevention (on qcow2) due to very bad inconsistencies, bs->drv may be
>> NULL afterwards. Thus, bdrv_check() should check whether bs->drv is set.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> I suppose there was a real case of this happening? I think bdrv_check()
> triggering corruption prevention is a rather bad sign. The most
> important point for image repair should be that it doesn't make the
> situation any worse. Smells like a follow-up patch to the qcow2 code.

Yes, as I wrote in the cover letter, using the image provided in 
https://bugs.launchpad.net/qemu/+bug/1353456 and setting the refblock 
offset to 0 (the reftable entry) results in a segmentation fault.

A simple way to trigger corruption during bdrv_check() is creating an 
image, setting the first (and only) reftable entry to 0 and running 
qemu-img check -r all. bdrv_check() will try to allocate a refblock, but 
since the first clusters are unallocated, it will allocate them there 
which would obviously overwrite the image header and/or L1 table and/or 
reftable.

The only way I can imagine to fix this is to completely disregard the 
on-disk refcount information during bdrv_check() and instead only use 
the calculated refcounts. This would require own allocation functions 
which may probably be rather simple, but in any case we'd need to write 
them.

I think I should have some time, so I'll have a look into it.

Max