The Monday 08 Sep 2014 à 19:47:31 (+0200), Max Reitz wrote :
> On 08.09.2014 16:40, Benoît Canet wrote:
> >The Friday 05 Sep 2014 à 16:07:18 (+0200), Max Reitz wrote :
> >>Offsets taken from the L1, L2 and refcount tables are generally assumed
> >>to be correctly aligned. However, this cannot be guaranteed if the image
> >>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 <mre...@redhat.com>
> >>---
> >> 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 *bs,
> >>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 second.
> >Can we do ret = -EIO and goto out for consistency ?
> >bs->drv == NULL after qcow2_signal_corruption so we are not afraid of out
> >sides effects.
>
> 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 the
> code size with no real benefit apparent to me (no code deduplication; and as
> far as I remember, we have many functions with fail labels which however use
> a plain "return" before cleaning up is needed).
>
> (before this patch, there were two places using "goto out" in this function,
> both of which were "successes" (cluster found to be unallocated)); and two
> places using "return -errno", both of which were failures (the first one 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 improve
the quality of my reviews.
Best regards
Benoît
>
> Max
>
> >>+ }
> >>+
> >> /* load the l2 table in memory */
> >> ret = l2_load(bs, l2_offset, &l2_table);
> >>@@ -508,8 +515,11 @@ int qcow2_get_cluster_offset(BlockDriverState *bs,
> >>uint64_t offset,
> >> break;
> >> case QCOW2_CLUSTER_ZERO:
> >> if (s->qcow_version < 3) {
> >>- qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table);
> >>- 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,
> >>l2_index);
> >>+ ret = -EIO;
> >>+ goto fail;
> >> }
> >> c = count_contiguous_clusters(nb_clusters, s->cluster_size,
> >> &l2_table[l2_index], QCOW_OFLAG_ZERO);
> >>@@ -525,6 +535,14 @@ int qcow2_get_cluster_offset(BlockDriverState *bs,
> >>uint64_t offset,
> >> c = count_contiguous_clusters(nb_clusters, s->cluster_size,
> >> &l2_table[l2_index], QCOW_OFLAG_ZERO);
> >> *cluster_offset &= 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_offset,
> >>+ l2_offset, l2_index);
> >>+ ret = -EIO;
> >>+ goto fail;
> >>+ }
> >> break;
> >> default:
> >> abort();
> >>@@ -541,6 +559,10 @@ out:
> >> *num = 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 *bs,
> >>uint64_t offset,
> >> assert(l1_index < s->l1_size);
> >> l2_offset = 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,
> >>uint64_t guest_offset,
> >> bool offset_matches =
> >> (cluster_offset & L2E_OFFSET_MASK) == *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 = -EIO;
> >>+ goto out;
> >>+ }
> >>+
> >> if (*host_offset != 0 && !offset_matches) {
> >> *bytes = 0;
> >> ret = 0;
> >>@@ -979,7 +1016,7 @@ out:
> >> /* Only return a host offset if we actually made progress. Otherwise
> >> we
> >> * would make requirements for handle_alloc() that it can't fulfill */
> >>- if (ret) {
> >>+ if (ret > 0) {
> >> *host_offset = (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, int64_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: %#" PRIx64
> >>")",
> >>+ refcount_block_offset,
> >>refcount_table_index);
> >>+ return -EIO;
> >>+ }
> >>+
> >> ret = qcow2_cache_get(bs, s->refcount_block_cache,
> >> refcount_block_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_offset,
> >>+ 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 *bs,
> >>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 cluster
> >>%#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(BlockDriverState
> >>*bs,
> >> old_l2_offset = l2_offset;
> >> l2_offset &= 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 = -EIO;
> >>+ goto fail;
> >>+ }
> >>+
> >> ret = qcow2_cache_get(bs, s->l2_table_cache, l2_offset,
> >> (void**) &l2_table);
> >> if (ret < 0) {
> >>@@ -933,6 +962,17 @@ int qcow2_update_snapshot_refcount(BlockDriverState
> >>*bs,
> >> case QCOW2_CLUSTER_NORMAL:
> >> case QCOW2_CLUSTER_ZERO:
> >>+ if (offset_into_cluster(s, offset &
> >>L2E_OFFSET_MASK)) {
> >>+ qcow2_signal_corruption(bs, true, -1, -1,
> >>"Data "
> >>+ "cluster offset %#llx "
> >>+ "unaligned (L2 offset:
> >>%#"
> >>+ PRIx64 ", L2 index:
> >>%#x)",
> >>+ offset &
> >>L2E_OFFSET_MASK,
> >>+ l2_offset, j);
> >>+ ret = -EIO;
> >>+ goto fail;
> >>+ }
> >>+
> >> cluster_index = (offset & L2E_OFFSET_MASK) >>
> >> s->cluster_bits;
> >> if (!cluster_index) {
> >> /* unallocated */
> >>--
> >>2.1.0
> >>
> >>
>
>
