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 <mre...@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
