From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41053) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XFt2j-0006CV-6i for qemu-devel@nongnu.org; Fri, 08 Aug 2014 18:53:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XFt2e-00073O-9C for qemu-devel@nongnu.org; Fri, 08 Aug 2014 18:53:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43452) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XFt2e-00072r-1J for qemu-devel@nongnu.org; Fri, 08 Aug 2014 18:53:44 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s78MrhsV029897 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Fri, 8 Aug 2014 18:53:43 -0400 Message-ID: <53E554DE.5050409@redhat.com> Date: Sat, 09 Aug 2014 00:53:18 +0200 From: Max Reitz MIME-Version: 1.0 References: <1407444475-19516-1-git-send-email-mreitz@redhat.com> <1407444475-19516-4-git-send-email-mreitz@redhat.com> <20140808091527.GE4118@noname.redhat.com> <53E53D0F.3030808@redhat.com> In-Reply-To: <53E53D0F.3030808@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3] block: Catch !bs->drv in bdrv_check() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi 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 >> 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