[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH 3/3] block: Catch !bs->drv in bdrv_check()
Date: Sat, 09 Aug 2014 00:53:18 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.0

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 <address@hidden>
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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]