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

[Max Reitz <mreitz@redhat.com>]

On 08.08.2014 23:11, Max Reitz wrote:
> 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.

Okay, after thinking about the situation (which involved looking through 
the other bug reports by Maria), I think there is only one way to truly 
do the repair operation correctly. The general problem is that a damaged 
refcount structure may lead to a new reftable or new refblocks being 
allocated during the repair process. However, since the refcounts are 
not accurate, these new clusters may collide with existing allocations. 
We could fix this by replicating all the refcount operations for 
in-memory refcounts (which qcow2_check_refcounts() creates), but I think 
this to be a rather bad idea.

Instead, I'd rather create completely new refcount structures in 
qcow2_check_refcounts() when so much as a single referenced cluster with 
refcount=0 is encountered. If there is any cluster which is indeed 
referenced but for which the refcount structures say it's free, any new 
allocation may break things. Since changing refcounts may result in new 
cluster allocations, we should not update the existing refcount 
structures at all.

Alternatively, we can rewrite the refcount update functions to take an 
in-memory refcount table to know which clusters to avoid, but 
considering that those functions are complicated enough already, I'd 
rather refrain from that.

Max