Message ID | 1407444475-19516-4-git-send-email-mreitz@redhat.com |
---|---|
State | New |
Headers | show |
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. Kevin
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
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
diff --git a/block.c b/block.c index 8cf519b..2b470d8 100644 --- a/block.c +++ b/block.c @@ -2203,6 +2203,9 @@ bool bdrv_dev_is_medium_locked(BlockDriverState *bs) */ int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix) { + if (bs->drv == NULL) { + return -ENOMEDIUM; + } if (bs->drv->bdrv_check == NULL) { return -ENOTSUP; }
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> --- block.c | 3 +++ 1 file changed, 3 insertions(+)