From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36104) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XR3If-000811-44 for qemu-devel@nongnu.org; Mon, 08 Sep 2014 14:04:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XR3IY-0000wD-Q1 for qemu-devel@nongnu.org; Mon, 08 Sep 2014 14:04:25 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:36772 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XR3IY-0000vD-GV for qemu-devel@nongnu.org; Mon, 08 Sep 2014 14:04:18 -0400 Date: Mon, 8 Sep 2014 20:03:25 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140908180325.GA12965@irqsave.net> References: <1409926039-29044-1-git-send-email-mreitz@redhat.com> <1409926039-29044-5-git-send-email-mreitz@redhat.com> <20140908144041.GF22582@irqsave.net> <540DEBB3.2060702@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <540DEBB3.2060702@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 4/5] qcow2: Check L1/L2/reftable entries for alignment List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi The Monday 08 Sep 2014 =E0 19:47:31 (+0200), Max Reitz wrote : > On 08.09.2014 16:40, Beno=EEt Canet wrote: > >The Friday 05 Sep 2014 =E0 16:07:18 (+0200), Max Reitz wrote : > >>Offsets taken from the L1, L2 and refcount tables are generally assum= ed > >>to be correctly aligned. However, this cannot be guaranteed if the im= age > >>has been written to by something different than qemu, thus check all > >>offsets taken from these tables for correct cluster alignment. > >> > >>Signed-off-by: Max Reitz > >>--- > >> block/qcow2-cluster.c | 43 +++++++++++++++++++++++++++++++++++++++= +--- > >> block/qcow2-refcount.c | 44 +++++++++++++++++++++++++++++++++++++++= +++-- > >> 2 files changed, 82 insertions(+), 5 deletions(-) > >> > >>diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > >>index 735f687..f7dd8c0 100644 > >>--- a/block/qcow2-cluster.c > >>+++ b/block/qcow2-cluster.c > >>@@ -486,6 +486,13 @@ int qcow2_get_cluster_offset(BlockDriverState *b= s, uint64_t offset, > >> goto out; > >> } > >>+ if (offset_into_cluster(s, l2_offset)) { > >>+ qcow2_signal_corruption(bs, true, -1, -1, "L2 table offset %= #" PRIx64 > >>+ " unaligned (L1 index: %#" PRIx64 ")= ", > >>+ l2_offset, l1_index); > >>+ return -EIO; > >This function mix return ret and goto out and there is more of the sec= ond. > >Can we do ret =3D -EIO and goto out for consistency ? > >bs->drv =3D=3D NULL after qcow2_signal_corruption so we are not afraid= of out > >sides effects. >=20 > The "out" label here is for success; that's why I introduced the "fail" > label in this series. I could make qcow2_cache_put() in the fail path > optional and then use goto fail, though. But this would only increase t= he > code size with no real benefit apparent to me (no code deduplication; a= nd as > far as I remember, we have many functions with fail labels which howeve= r use > a plain "return" before cleaning up is needed). >=20 > (before this patch, there were two places using "goto out" in this func= tion, > both of which were "successes" (cluster found to be unallocated)); and = two > places using "return -errno", both of which were failures (the first on= e due > to l2_load() failing and the second one due to a zero cluster found in = a > pre-v3 image)) Thanks for the explanation this make me think I should question and impro= ve the quality of my reviews. Best regards Beno=EEt >=20 > Max >=20 > >>+ } > >>+ > >> /* load the l2 table in memory */ > >> ret =3D l2_load(bs, l2_offset, &l2_table); > >>@@ -508,8 +515,11 @@ int qcow2_get_cluster_offset(BlockDriverState *b= s, uint64_t offset, > >> break; > >> case QCOW2_CLUSTER_ZERO: > >> if (s->qcow_version < 3) { > >>- qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_tabl= e); > >>- return -EIO; > >>+ qcow2_signal_corruption(bs, true, -1, -1, "Zero cluster = entry found" > >>+ " in pre-v3 image (L2 offset: %#= " PRIx64 > >>+ ", L2 index: %#x)", l2_offset, l= 2_index); > >>+ ret =3D -EIO; > >>+ goto fail; > >> } > >> c =3D count_contiguous_clusters(nb_clusters, s->cluster_siz= e, > >> &l2_table[l2_index], QCOW_OFLAG_ZERO); > >>@@ -525,6 +535,14 @@ int qcow2_get_cluster_offset(BlockDriverState *b= s, uint64_t offset, > >> c =3D count_contiguous_clusters(nb_clusters, s->cluster_siz= e, > >> &l2_table[l2_index], QCOW_OFLAG_ZERO); > >> *cluster_offset &=3D L2E_OFFSET_MASK; > >>+ if (offset_into_cluster(s, *cluster_offset)) { > >>+ qcow2_signal_corruption(bs, true, -1, -1, "Data cluster = offset %#" > >>+ PRIx64 " unaligned (L2 offset: %= #" PRIx64 > >>+ ", L2 index: %#x)", *cluster_off= set, > >>+ l2_offset, l2_index); > >>+ ret =3D -EIO; > >>+ goto fail; > >>+ } > >> break; > >> default: > >> abort(); > >>@@ -541,6 +559,10 @@ out: > >> *num =3D nb_available - index_in_cluster; > >> return ret; > >>+ > >>+fail: > >>+ qcow2_cache_put(bs, s->l2_table_cache, (void **)&l2_table); > >>+ return ret; > >> } > >> /* > >>@@ -576,6 +598,12 @@ static int get_cluster_table(BlockDriverState *b= s, uint64_t offset, > >> assert(l1_index < s->l1_size); > >> l2_offset =3D s->l1_table[l1_index] & L1E_OFFSET_MASK; > >>+ if (offset_into_cluster(s, l2_offset)) { > >>+ qcow2_signal_corruption(bs, true, -1, -1, "L2 table offset %= #" PRIx64 > >>+ " unaligned (L1 index: %#" PRIx64 ")= ", > >>+ l2_offset, l1_index); > >>+ return -EIO; > >>+ } > >> /* seek the l2 table of the given l2 offset */ > >>@@ -948,6 +976,15 @@ static int handle_copied(BlockDriverState *bs, u= int64_t guest_offset, > >> bool offset_matches =3D > >> (cluster_offset & L2E_OFFSET_MASK) =3D=3D *host_offset; > >>+ if (offset_into_cluster(s, cluster_offset & L2E_OFFSET_MASK)= ) { > >>+ qcow2_signal_corruption(bs, true, -1, -1, "Data cluster = offset " > >>+ "%#llx unaligned (guest offset: = %#" PRIx64 > >>+ ")", cluster_offset & L2E_OFFSET= _MASK, > >>+ guest_offset); > >>+ ret =3D -EIO; > >>+ goto out; > >>+ } > >>+ > >> if (*host_offset !=3D 0 && !offset_matches) { > >> *bytes =3D 0; > >> ret =3D 0; > >>@@ -979,7 +1016,7 @@ out: > >> /* Only return a host offset if we actually made progress. Othe= rwise we > >> * would make requirements for handle_alloc() that it can't ful= fill */ > >>- if (ret) { > >>+ if (ret > 0) { > >> *host_offset =3D (cluster_offset & L2E_OFFSET_MASK) > >> + offset_into_cluster(s, guest_offset); > >> } > >>diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > >>index b9d421e..2bcaaf9 100644 > >>--- a/block/qcow2-refcount.c > >>+++ b/block/qcow2-refcount.c > >>@@ -108,6 +108,13 @@ static int get_refcount(BlockDriverState *bs, in= t64_t cluster_index) > >> if (!refcount_block_offset) > >> return 0; > >>+ if (offset_into_cluster(s, refcount_block_offset)) { > >>+ qcow2_signal_corruption(bs, true, -1, -1, "Refblock offset %= #" PRIx64 > >>+ " unaligned (reftable index: %#" PRI= x64 ")", > >>+ refcount_block_offset, refcount_tabl= e_index); > >>+ return -EIO; > >>+ } > >>+ > >> ret =3D qcow2_cache_get(bs, s->refcount_block_cache, refcount_b= lock_offset, > >> (void**) &refcount_block); > >> if (ret < 0) { > >>@@ -181,6 +188,14 @@ static int alloc_refcount_block(BlockDriverState= *bs, > >> /* If it's already there, we're done */ > >> if (refcount_block_offset) { > >>+ if (offset_into_cluster(s, refcount_block_offset)) { > >>+ qcow2_signal_corruption(bs, true, -1, -1, "Refblock = offset %#" > >>+ PRIx64 " unaligned (reftable= index: " > >>+ "%#x)", refcount_block_offse= t, > >>+ refcount_table_index); > >>+ return -EIO; > >>+ } > >>+ > >> return load_refcount_block(bs, refcount_block_offset, > >> (void**) refcount_block); > >> } > >>@@ -836,8 +851,14 @@ void qcow2_free_any_clusters(BlockDriverState *b= s, uint64_t l2_entry, > >> case QCOW2_CLUSTER_NORMAL: > >> case QCOW2_CLUSTER_ZERO: > >> if (l2_entry & L2E_OFFSET_MASK) { > >>- qcow2_free_clusters(bs, l2_entry & L2E_OFFSET_MASK, > >>- nb_clusters << s->cluster_bits, type= ); > >>+ if (offset_into_cluster(s, l2_entry & L2E_OFFSET_MASK)) = { > >>+ qcow2_signal_corruption(bs, false, -1, -1, > >>+ "Cannot free unaligned clust= er %#llx", > >>+ l2_entry & L2E_OFFSET_MASK); > >>+ } else { > >>+ qcow2_free_clusters(bs, l2_entry & L2E_OFFSET_MASK, > >>+ nb_clusters << s->cluster_bits, = type); > >>+ } > >> } > >> break; > >> case QCOW2_CLUSTER_UNALLOCATED: > >>@@ -901,6 +922,14 @@ int qcow2_update_snapshot_refcount(BlockDriverSt= ate *bs, > >> old_l2_offset =3D l2_offset; > >> l2_offset &=3D L1E_OFFSET_MASK; > >>+ if (offset_into_cluster(s, l2_offset)) { > >>+ qcow2_signal_corruption(bs, true, -1, -1, "L2 table = offset %#" > >>+ PRIx64 " unaligned (L1 index= : %#x)", > >>+ l2_offset, i); > >>+ ret =3D -EIO; > >>+ goto fail; > >>+ } > >>+ > >> ret =3D qcow2_cache_get(bs, s->l2_table_cache, l2_offse= t, > >> (void**) &l2_table); > >> if (ret < 0) { > >>@@ -933,6 +962,17 @@ int qcow2_update_snapshot_refcount(BlockDriverSt= ate *bs, > >> case QCOW2_CLUSTER_NORMAL: > >> case QCOW2_CLUSTER_ZERO: > >>+ if (offset_into_cluster(s, offset & L2E_OFFS= ET_MASK)) { > >>+ qcow2_signal_corruption(bs, true, -1, -1= , "Data " > >>+ "cluster offset = %#llx " > >>+ "unaligned (L2 o= ffset: %#" > >>+ PRIx64 ", L2 ind= ex: %#x)", > >>+ offset & L2E_OFF= SET_MASK, > >>+ l2_offset, j); > >>+ ret =3D -EIO; > >>+ goto fail; > >>+ } > >>+ > >> cluster_index =3D (offset & L2E_OFFSET_MASK= ) >> s->cluster_bits; > >> if (!cluster_index) { > >> /* unallocated */ > >>--=20 > >>2.1.0 > >> > >> >=20 >=20