From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36285) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WsZ2F-0000Ns-BK for qemu-devel@nongnu.org; Thu, 05 Jun 2014 10:53:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WsZ29-00063h-A8 for qemu-devel@nongnu.org; Thu, 05 Jun 2014 10:52:55 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:46238 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WsZ28-00062b-U7 for qemu-devel@nongnu.org; Thu, 05 Jun 2014 10:52:49 -0400 Date: Thu, 5 Jun 2014 16:52:46 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140605145246.GB6287@irqsave.net> References: <1401975393-7255-1-git-send-email-kwolf@redhat.com> <1401975393-7255-12-git-send-email-kwolf@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1401975393-7255-12-git-send-email-kwolf@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 11/21] qcow2: Handle failure for potentially large allocations List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: benoit.canet@irqsave.net, qemu-devel@nongnu.org, stefanha@redhat.com The Thursday 05 Jun 2014 =E0 15:36:23 (+0200), Kevin Wolf wrote : > Some code in the block layer makes potentially huge allocations. Failur= e > is not completely unexpected there, so avoid aborting qemu and handle > out-of-memory situations gracefully. >=20 > This patch addresses the allocations in the qcow2 block driver. >=20 > Signed-off-by: Kevin Wolf > Reviewed-by: Stefan Hajnoczi > --- > block/qcow2-cache.c | 13 ++++++++++++- > block/qcow2-cluster.c | 35 +++++++++++++++++++++++++++-------- > block/qcow2-refcount.c | 48 ++++++++++++++++++++++++++++++++++++++----= ------ > block/qcow2-snapshot.c | 22 +++++++++++++++++----- > block/qcow2.c | 41 +++++++++++++++++++++++++++++++++-------- > 5 files changed, 127 insertions(+), 32 deletions(-) >=20 > diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c > index 8ecbb5b..b3fe851 100644 > --- a/block/qcow2-cache.c > +++ b/block/qcow2-cache.c > @@ -53,10 +53,21 @@ Qcow2Cache *qcow2_cache_create(BlockDriverState *bs= , int num_tables) > c->entries =3D g_malloc0(sizeof(*c->entries) * num_tables); > =20 > for (i =3D 0; i < c->size; i++) { > - c->entries[i].table =3D qemu_blockalign(bs, s->cluster_size); > + c->entries[i].table =3D qemu_try_blockalign(bs, s->cluster_siz= e); > + if (c->entries[i].table =3D=3D NULL) { > + goto fail; > + } > } > =20 > return c; > + > +fail: > + for (i =3D 0; i < c->size; i++) { > + qemu_vfree(c->entries[i].table); > + } > + g_free(c->entries); > + g_free(c); > + return NULL; > } > =20 > int qcow2_cache_destroy(BlockDriverState* bs, Qcow2Cache *c) > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 4208dc0..d391c5a 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -72,14 +72,19 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint6= 4_t min_size, > #endif > =20 > new_l1_size2 =3D sizeof(uint64_t) * new_l1_size; > - new_l1_table =3D g_malloc0(align_offset(new_l1_size2, 512)); > + new_l1_table =3D qemu_try_blockalign(bs, align_offset(new_l1_size2= , 512)); > + if (new_l1_table =3D=3D NULL) { > + return -ENOMEM; > + } > + memset(new_l1_table, 0, align_offset(new_l1_size2, 512)); > + > memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t)); > =20 > /* write new table (align to cluster) */ > BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ALLOC_TABLE); > new_l1_table_offset =3D qcow2_alloc_clusters(bs, new_l1_size2); > if (new_l1_table_offset < 0) { > - g_free(new_l1_table); > + qemu_vfree(new_l1_table); > return new_l1_table_offset; > } > =20 > @@ -113,7 +118,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint6= 4_t min_size, > if (ret < 0) { > goto fail; > } > - g_free(s->l1_table); > + qemu_vfree(s->l1_table); > old_l1_table_offset =3D s->l1_table_offset; > s->l1_table_offset =3D new_l1_table_offset; > s->l1_table =3D new_l1_table; > @@ -123,7 +128,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint6= 4_t min_size, > QCOW2_DISCARD_OTHER); > return 0; > fail: > - g_free(new_l1_table); > + qemu_vfree(new_l1_table); > qcow2_free_clusters(bs, new_l1_table_offset, new_l1_size2, > QCOW2_DISCARD_OTHER); > return ret; > @@ -372,7 +377,10 @@ static int coroutine_fn copy_sectors(BlockDriverSt= ate *bs, > } > =20 > iov.iov_len =3D n * BDRV_SECTOR_SIZE; > - iov.iov_base =3D qemu_blockalign(bs, iov.iov_len); > + iov.iov_base =3D qemu_try_blockalign(bs, iov.iov_len); > + if (iov.iov_base =3D=3D NULL) { > + return -ENOMEM; > + } > =20 > qemu_iovec_init_external(&qiov, &iov, 1); > =20 > @@ -702,7 +710,11 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *= bs, QCowL2Meta *m) > trace_qcow2_cluster_link_l2(qemu_coroutine_self(), m->nb_clusters)= ; > assert(m->nb_clusters > 0); > =20 > - old_cluster =3D g_malloc(m->nb_clusters * sizeof(uint64_t)); > + old_cluster =3D g_try_malloc(m->nb_clusters * sizeof(uint64_t)); > + if (old_cluster =3D=3D NULL) { > + ret =3D -ENOMEM; > + goto err; > + } > =20 > /* copy content of unmodified sectors */ > ret =3D perform_cow(bs, m, &m->cow_start); > @@ -1562,7 +1574,10 @@ static int expand_zero_clusters_in_l1(BlockDrive= rState *bs, uint64_t *l1_table, > if (!is_active_l1) { > /* inactive L2 tables require a buffer to be stored in when lo= ading > * them from disk */ > - l2_table =3D qemu_blockalign(bs, s->cluster_size); > + l2_table =3D qemu_try_blockalign(bs, s->cluster_size); > + if (l2_table =3D=3D NULL) { > + return -ENOMEM; > + } > } > =20 > for (i =3D 0; i < l1_size; i++) { > @@ -1740,7 +1755,11 @@ int qcow2_expand_zero_clusters(BlockDriverState = *bs) > =20 > nb_clusters =3D size_to_clusters(s, bs->file->total_sectors * > BDRV_SECTOR_SIZE); I think we need asset(nb_clusters >=3D 1); else it will trigger a spuriou= s -ENOMEM on very small files. > - expanded_clusters =3D g_malloc0((nb_clusters + 7) / 8); > + expanded_clusters =3D g_try_malloc0((nb_clusters + 7) / 8); > + if (expanded_clusters =3D=3D NULL) { > + ret =3D -ENOMEM; > + goto fail; > + } > =20 > ret =3D expand_zero_clusters_in_l1(bs, s->l1_table, s->l1_size, > &expanded_clusters, &nb_clusters)= ; > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 9507aef..a234c7a 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -45,8 +45,12 @@ int qcow2_refcount_init(BlockDriverState *bs) > =20 > assert(s->refcount_table_size <=3D INT_MAX / sizeof(uint64_t)); > refcount_table_size2 =3D s->refcount_table_size * sizeof(uint64_t)= ; > - s->refcount_table =3D g_malloc(refcount_table_size2); > + s->refcount_table =3D g_try_malloc(refcount_table_size2); > + > if (s->refcount_table_size > 0) { > + if (s->refcount_table =3D=3D NULL) { > + goto fail; > + } > BLKDBG_EVENT(bs->file, BLKDBG_REFTABLE_LOAD); > ret =3D bdrv_pread(bs->file, s->refcount_table_offset, > s->refcount_table, refcount_table_size2); > @@ -343,8 +347,14 @@ static int alloc_refcount_block(BlockDriverState *= bs, > uint64_t meta_offset =3D (blocks_used * refcount_block_clusters) * > s->cluster_size; > uint64_t table_offset =3D meta_offset + blocks_clusters * s->clust= er_size; > - uint16_t *new_blocks =3D g_malloc0(blocks_clusters * s->cluster_si= ze); > - uint64_t *new_table =3D g_malloc0(table_size * sizeof(uint64_t)); > + uint64_t *new_table =3D g_try_malloc0(table_size * sizeof(uint64_t= )); > + uint16_t *new_blocks =3D g_try_malloc0(blocks_clusters * s->cluste= r_size); > + > + assert(table_size > 0 && blocks_clusters > 0); > + if (new_table =3D=3D NULL || new_blocks =3D=3D NULL) { > + ret =3D -ENOMEM; > + goto fail_table; > + } > =20 > /* Fill the new refcount table */ > memcpy(new_table, s->refcount_table, > @@ -423,6 +433,7 @@ static int alloc_refcount_block(BlockDriverState *b= s, > return -EAGAIN; > =20 > fail_table: > + g_free(new_blocks); > g_free(new_table); > fail_block: > if (*refcount_block !=3D NULL) { > @@ -846,7 +857,8 @@ int qcow2_update_snapshot_refcount(BlockDriverState= *bs, > int64_t l1_table_offset, int l1_size, int addend) > { > BDRVQcowState *s =3D bs->opaque; > - uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2, l1_all= ocated; > + uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2; > + bool l1_allocated =3D false; > int64_t old_offset, old_l2_offset; > int i, j, l1_modified =3D 0, nb_csectors, refcount; > int ret; > @@ -861,8 +873,12 @@ int qcow2_update_snapshot_refcount(BlockDriverStat= e *bs, > * l1_table_offset when it is the current s->l1_table_offset! Be c= areful > * when changing this! */ > if (l1_table_offset !=3D s->l1_table_offset) { > - l1_table =3D g_malloc0(align_offset(l1_size2, 512)); > - l1_allocated =3D 1; > + l1_table =3D g_try_malloc0(align_offset(l1_size2, 512)); > + if (l1_size2 && l1_table =3D=3D NULL) { > + ret =3D -ENOMEM; > + goto fail; > + } > + l1_allocated =3D true; > =20 > ret =3D bdrv_pread(bs->file, l1_table_offset, l1_table, l1_siz= e2); > if (ret < 0) { > @@ -874,7 +890,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState= *bs, > } else { > assert(l1_size =3D=3D s->l1_size); > l1_table =3D s->l1_table; > - l1_allocated =3D 0; > + l1_allocated =3D false; > } > =20 > for(i =3D 0; i < l1_size; i++) { > @@ -1196,7 +1212,11 @@ static int check_refcounts_l1(BlockDriverState *= bs, > if (l1_size2 =3D=3D 0) { > l1_table =3D NULL; > } else { > - l1_table =3D g_malloc(l1_size2); > + l1_table =3D g_try_malloc(l1_size2); > + if (l1_table =3D=3D NULL) { > + ret =3D -ENOMEM; > + goto fail; > + } > if (bdrv_pread(bs->file, l1_table_offset, > l1_table, l1_size2) !=3D l1_size2) > goto fail; > @@ -1500,7 +1520,11 @@ int qcow2_check_refcounts(BlockDriverState *bs, = BdrvCheckResult *res, > return -EFBIG; > } > =20 > - refcount_table =3D g_malloc0(nb_clusters * sizeof(uint16_t)); > + refcount_table =3D g_try_malloc0(nb_clusters * sizeof(uint16_t)); > + if (nb_clusters && refcount_table =3D=3D NULL) { > + res->check_errors++; > + return -ENOMEM; > + } > =20 > res->bfi.total_clusters =3D > size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE); > @@ -1752,9 +1776,13 @@ int qcow2_check_metadata_overlap(BlockDriverStat= e *bs, int ign, int64_t offset, > uint64_t l1_ofs =3D s->snapshots[i].l1_table_offset; > uint32_t l1_sz =3D s->snapshots[i].l1_size; > uint64_t l1_sz2 =3D l1_sz * sizeof(uint64_t); > - uint64_t *l1 =3D g_malloc(l1_sz2); > + uint64_t *l1 =3D g_try_malloc(l1_sz2); > int ret; > =20 > + if (l1_sz2 && l1 =3D=3D NULL) { > + return -ENOMEM; > + } > + > ret =3D bdrv_pread(bs->file, l1_ofs, l1, l1_sz2); > if (ret < 0) { > g_free(l1); > diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c > index 0aa9def..07e8b73 100644 > --- a/block/qcow2-snapshot.c > +++ b/block/qcow2-snapshot.c > @@ -381,7 +381,12 @@ int qcow2_snapshot_create(BlockDriverState *bs, QE= MUSnapshotInfo *sn_info) > sn->l1_table_offset =3D l1_table_offset; > sn->l1_size =3D s->l1_size; > =20 > - l1_table =3D g_malloc(s->l1_size * sizeof(uint64_t)); > + l1_table =3D g_try_malloc(s->l1_size * sizeof(uint64_t)); > + if (s->l1_size && l1_table =3D=3D NULL) { > + ret =3D -ENOMEM; > + goto fail; > + } > + > for(i =3D 0; i < s->l1_size; i++) { > l1_table[i] =3D cpu_to_be64(s->l1_table[i]); > } > @@ -499,7 +504,11 @@ int qcow2_snapshot_goto(BlockDriverState *bs, cons= t char *snapshot_id) > * Decrease the refcount referenced by the old one only when the L= 1 > * table is overwritten. > */ > - sn_l1_table =3D g_malloc0(cur_l1_bytes); > + sn_l1_table =3D g_try_malloc0(cur_l1_bytes); > + if (cur_l1_bytes && sn_l1_table =3D=3D NULL) { > + ret =3D -ENOMEM; > + goto fail; > + } > =20 > ret =3D bdrv_pread(bs->file, sn->l1_table_offset, sn_l1_table, sn_= l1_bytes); > if (ret < 0) { > @@ -698,17 +707,20 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs, > return -EFBIG; > } > new_l1_bytes =3D sn->l1_size * sizeof(uint64_t); > - new_l1_table =3D g_malloc0(align_offset(new_l1_bytes, 512)); > + new_l1_table =3D qemu_try_blockalign(bs, align_offset(new_l1_bytes= , 512)); > + if (new_l1_table =3D=3D NULL) { > + return -ENOMEM; > + } > =20 > ret =3D bdrv_pread(bs->file, sn->l1_table_offset, new_l1_table, ne= w_l1_bytes); > if (ret < 0) { > error_setg(errp, "Failed to read l1 table for snapshot"); > - g_free(new_l1_table); > + qemu_vfree(new_l1_table); > return ret; > } > =20 > /* Switch the L1 table */ > - g_free(s->l1_table); > + qemu_vfree(s->l1_table); > =20 > s->l1_size =3D sn->l1_size; > s->l1_table_offset =3D sn->l1_table_offset; > diff --git a/block/qcow2.c b/block/qcow2.c > index a54d2ba..0b07319 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -676,8 +676,13 @@ static int qcow2_open(BlockDriverState *bs, QDict = *options, int flags, > =20 > =20 > if (s->l1_size > 0) { > - s->l1_table =3D g_malloc0( > + s->l1_table =3D qemu_try_blockalign(bs->file, > align_offset(s->l1_size * sizeof(uint64_t), 512)); > + if (s->l1_table =3D=3D NULL) { > + error_setg(errp, "Could not allocate L1 table"); > + ret =3D -ENOMEM; > + goto fail; > + } > ret =3D bdrv_pread(bs->file, s->l1_table_offset, s->l1_table, > s->l1_size * sizeof(uint64_t)); > if (ret < 0) { > @@ -692,11 +697,22 @@ static int qcow2_open(BlockDriverState *bs, QDict= *options, int flags, > /* alloc L2 table/refcount block cache */ > s->l2_table_cache =3D qcow2_cache_create(bs, L2_CACHE_SIZE); > s->refcount_block_cache =3D qcow2_cache_create(bs, REFCOUNT_CACHE_= SIZE); > + if (s->l2_table_cache =3D=3D NULL || s->refcount_block_cache =3D=3D= NULL) { > + error_setg(errp, "Could not allocate metadata caches"); > + ret =3D -ENOMEM; > + goto fail; > + } > =20 > s->cluster_cache =3D g_malloc(s->cluster_size); > /* one more sector for decompressed data alignment */ > - s->cluster_data =3D qemu_blockalign(bs, QCOW_MAX_CRYPT_CLUSTERS * = s->cluster_size > - + 512); > + s->cluster_data =3D qemu_try_blockalign(bs, QCOW_MAX_CRYPT_CLUSTER= S > + * s->cluster_size + 512)= ; > + if (s->cluster_data =3D=3D NULL) { > + error_setg(errp, "Could not allocate temporary cluster buffer"= ); > + ret =3D -ENOMEM; > + goto fail; > + } > + > s->cluster_cache_offset =3D -1; > s->flags =3D flags; > =20 > @@ -840,7 +856,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *= options, int flags, > cleanup_unknown_header_ext(bs); > qcow2_free_snapshots(bs); > qcow2_refcount_close(bs); > - g_free(s->l1_table); > + qemu_vfree(s->l1_table); > /* else pre-write overlap checks in cache_destroy may crash */ > s->l1_table =3D NULL; > if (s->l2_table_cache) { > @@ -1063,7 +1079,12 @@ static coroutine_fn int qcow2_co_readv(BlockDriv= erState *bs, int64_t sector_num, > */ > if (!cluster_data) { > cluster_data =3D > - qemu_blockalign(bs, QCOW_MAX_CRYPT_CLUSTERS * = s->cluster_size); > + qemu_try_blockalign(bs, QCOW_MAX_CRYPT_CLUSTER= S > + * s->cluster_size); > + if (cluster_data =3D=3D NULL) { > + ret =3D -ENOMEM; > + goto fail; > + } > } > =20 > assert(cur_nr_sectors <=3D > @@ -1163,8 +1184,12 @@ static coroutine_fn int qcow2_co_writev(BlockDri= verState *bs, > =20 > if (s->crypt_method) { > if (!cluster_data) { > - cluster_data =3D qemu_blockalign(bs, QCOW_MAX_CRYPT_CL= USTERS * > - s->cluster_size); > + cluster_data =3D qemu_try_blockalign(bs, QCOW_MAX_CRYP= T_CLUSTERS > + * s->cluster_si= ze); > + if (cluster_data =3D=3D NULL) { > + ret =3D -ENOMEM; > + goto fail; > + } > } > =20 > assert(hd_qiov.size <=3D > @@ -1251,7 +1276,7 @@ fail: > static void qcow2_close(BlockDriverState *bs) > { > BDRVQcowState *s =3D bs->opaque; > - g_free(s->l1_table); > + qemu_vfree(s->l1_table); > /* else pre-write overlap checks in cache_destroy may crash */ > s->l1_table =3D NULL; > =20 > --=20 > 1.8.3.1 >=20 >=20 Aside from this Reviewed-by: Benoit Canet