Re: [Qemu-devel] [PATCH v2 11/20] qcow2: Handle failure for potentially large allocations

[Max Reitz]

On 28.05.2014 16:37, Kevin Wolf wrote:
> Some code in the block layer makes potentially huge allocations. Failure
> is not completely unexpected there, so avoid aborting qemu and handle
> out-of-memory situations gracefully.
>
> This patch addresses the allocations in the qcow2 block driver.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>   block/qcow2-cache.c    | 12 +++++++++++-
>   block/qcow2-cluster.c  | 35 +++++++++++++++++++++++++++--------
>   block/qcow2-refcount.c | 46 ++++++++++++++++++++++++++++++++++++----------
>   block/qcow2-snapshot.c | 22 +++++++++++++++++-----
>   block/qcow2.c          | 41 +++++++++++++++++++++++++++++++++--------
>   5 files changed, 124 insertions(+), 32 deletions(-)
>
> diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
> index 8ecbb5b..351bc01 100644
> --- a/block/qcow2-cache.c
> +++ b/block/qcow2-cache.c
> @@ -53,10 +53,20 @@ Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int 
> num_tables)
>       c->entries = g_malloc0(sizeof(*c->entries) * num_tables);
>   
>       for (i = 0; i < c->size; i++) {
> -        c->entries[i].table = qemu_blockalign(bs, s->cluster_size);
> +        c->entries[i].table = qemu_try_blockalign(bs, s->cluster_size);
> +        if (c->entries[i].table == NULL) {
> +            goto fail;
> +        }
>       }
>   
>       return c;
> +
> +fail:
> +    for (i = 0; i < c->size; i++) {
> +        qemu_vfree(c->entries[i].table);
> +    }
> +    g_free(c);
> +    return NULL;
>   }
>   
>   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, uint64_t 
> min_size,
>   #endif
>   
>       new_l1_size2 = sizeof(uint64_t) * new_l1_size;
> -    new_l1_table = g_malloc0(align_offset(new_l1_size2, 512));
> +    new_l1_table = qemu_try_blockalign(bs, align_offset(new_l1_size2, 512));
> +    if (new_l1_table == 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));
>   
>       /* write new table (align to cluster) */
>       BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ALLOC_TABLE);
>       new_l1_table_offset = 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;
>       }
>   
> @@ -113,7 +118,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
>       if (ret < 0) {
>           goto fail;
>       }
> -    g_free(s->l1_table);
> +    qemu_vfree(s->l1_table);
>       old_l1_table_offset = s->l1_table_offset;
>       s->l1_table_offset = new_l1_table_offset;
>       s->l1_table = new_l1_table;
> @@ -123,7 +128,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_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(BlockDriverState *bs,
>       }
>   
>       iov.iov_len = n * BDRV_SECTOR_SIZE;
> -    iov.iov_base = qemu_blockalign(bs, iov.iov_len);
> +    iov.iov_base = qemu_try_blockalign(bs, iov.iov_len);
> +    if (iov.iov_base == NULL) {
> +        return -ENOMEM;
> +    }

There appears to be a leak of iov.iov_base right below this hunk 
(bs->drv is NULL; now that I think of it, it was me who introduced this 
leak). We could fix it along with this, but on the other hand it's an 
independent change and it's probably easier for me to send an own patch.

>       qemu_iovec_init_external(&qiov, &iov, 1);
>   
> @@ -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);
>   
> -    old_cluster = g_malloc(m->nb_clusters * sizeof(uint64_t));
> +    old_cluster = g_try_malloc(m->nb_clusters * sizeof(uint64_t));
> +    if (old_cluster == NULL) {
> +        ret = -ENOMEM;
> +        goto err;
> +    }
>   
>       /* copy content of unmodified sectors */
>       ret = perform_cow(bs, m, &m->cow_start);
> @@ -1562,7 +1574,10 @@ static int expand_zero_clusters_in_l1(BlockDriverState 
> *bs, uint64_t *l1_table,
>       if (!is_active_l1) {
>           /* inactive L2 tables require a buffer to be stored in when loading
>            * them from disk */
> -        l2_table = qemu_blockalign(bs, s->cluster_size);
> +        l2_table = qemu_try_blockalign(bs, s->cluster_size);
> +        if (l2_table == NULL) {
> +            return -ENOMEM;
> +        }
>       }
>   
>       for (i = 0; i < l1_size; i++) {
> @@ -1740,7 +1755,11 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs)
>   
>       nb_clusters = size_to_clusters(s, bs->file->total_sectors *
>                                      BDRV_SECTOR_SIZE);
> -    expanded_clusters = g_malloc0((nb_clusters + 7) / 8);
> +    expanded_clusters = g_try_malloc0((nb_clusters + 7) / 8);
> +    if (expanded_clusters == NULL) {
> +        ret = -ENOMEM;
> +        goto fail;
> +    }
>   
>       ret = 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..494a182 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -45,8 +45,12 @@ int qcow2_refcount_init(BlockDriverState *bs)
>   
>       assert(s->refcount_table_size <= INT_MAX / sizeof(uint64_t));
>       refcount_table_size2 = s->refcount_table_size * sizeof(uint64_t);
> -    s->refcount_table = g_malloc(refcount_table_size2);
> +    s->refcount_table = g_try_malloc(refcount_table_size2);
> +
>       if (s->refcount_table_size > 0) {
> +        if (s->refcount_table == NULL) {
> +            goto fail;

Very convenient that qcow2_refcount_init() always returns -ENOMEM after 
"fail:" anyway. This doesn't seem correct for bdrv_pread() failing, 
though, perhaps this should be fixed in some other patch.

> +        }
>           BLKDBG_EVENT(bs->file, BLKDBG_REFTABLE_LOAD);
>           ret = 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 = (blocks_used * refcount_block_clusters) *
>           s->cluster_size;
>       uint64_t table_offset = meta_offset + blocks_clusters * s->cluster_size;
> -    uint16_t *new_blocks = g_malloc0(blocks_clusters * s->cluster_size);
> -    uint64_t *new_table = g_malloc0(table_size * sizeof(uint64_t));
> +    uint64_t *new_table = g_try_malloc0(table_size * sizeof(uint64_t));
> +    uint16_t *new_blocks = g_try_malloc0(blocks_clusters * s->cluster_size);
> +
> +    assert(table_size > 0 && blocks_clusters > 0);
> +    if (new_table == NULL || new_blocks == NULL) {
> +        ret = -ENOMEM;
> +        goto fail_table;

Maybe for some weird reason new_table cannot be allocated but new_blocks 
can. In that case, new_blocks would be leaked (after "fail_table:", only 
new_table is freed). And I guess if there's one time we can't afford 
leaks, it's after ENOMEM. ;-)

> +    }
>   
>       /* Fill the new refcount table */
>       memcpy(new_table, s->refcount_table,
> @@ -846,7 +856,8 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>       int64_t l1_table_offset, int l1_size, int addend)
>   {
>       BDRVQcowState *s = bs->opaque;
> -    uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2, l1_allocated;
> +    uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2;
> +    bool l1_allocated = false;
>       int64_t old_offset, old_l2_offset;
>       int i, j, l1_modified = 0, nb_csectors, refcount;
>       int ret;
> @@ -861,8 +872,12 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>        * l1_table_offset when it is the current s->l1_table_offset! Be careful
>        * when changing this! */
>       if (l1_table_offset != s->l1_table_offset) {
> -        l1_table = g_malloc0(align_offset(l1_size2, 512));
> -        l1_allocated = 1;
> +        l1_table = g_try_malloc0(align_offset(l1_size2, 512));
> +        if (l1_size2 && l1_table == NULL) {
> +            ret = -ENOMEM;
> +            goto fail;
> +        }
> +        l1_allocated = true;
>   
>           ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size2);
>           if (ret < 0) {
> @@ -874,7 +889,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>       } else {
>           assert(l1_size == s->l1_size);
>           l1_table = s->l1_table;
> -        l1_allocated = 0;
> +        l1_allocated = false;

Now that l1_allocated is initialized, we could drop this line. But 
perhaps it's clearer to explicitly have it here.

>       }
>   
>       for(i = 0; i < l1_size; i++) {
> @@ -1196,7 +1211,11 @@ static int check_refcounts_l1(BlockDriverState *bs,
>       if (l1_size2 == 0) {
>           l1_table = NULL;
>       } else {
> -        l1_table = g_malloc(l1_size2);
> +        l1_table = g_try_malloc(l1_size2);
> +        if (l1_table == NULL) {
> +            ret = -ENOMEM;
> +            goto fail;
> +        }
>           if (bdrv_pread(bs->file, l1_table_offset,
>                          l1_table, l1_size2) != l1_size2)
>               goto fail;
> @@ -1500,7 +1519,10 @@ int qcow2_check_refcounts(BlockDriverState *bs, 
> BdrvCheckResult *res,
>           return -EFBIG;
>       }
>   
> -    refcount_table = g_malloc0(nb_clusters * sizeof(uint16_t));
> +    refcount_table = g_try_malloc0(nb_clusters * sizeof(uint16_t));
> +    if (nb_clusters && refcount_table == NULL) {
> +        return -ENOMEM;

I'm not sure, but perhaps we should increment res->check_errors here 
(both error catching blocks above this hunk do that).

> +    }
>   
>       res->bfi.total_clusters =
>           size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE);
> @@ -1752,9 +1774,13 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, 
> int ign, int64_t offset,
>               uint64_t l1_ofs = s->snapshots[i].l1_table_offset;
>               uint32_t l1_sz  = s->snapshots[i].l1_size;
>               uint64_t l1_sz2 = l1_sz * sizeof(uint64_t);
> -            uint64_t *l1 = g_malloc(l1_sz2);
> +            uint64_t *l1 = g_try_malloc(l1_sz2);
>               int ret;
>   
> +            if (l1_sz2 && l1 == NULL) {
> +                return -ENOMEM;
> +            }
> +
>               ret = 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..26889bf 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -381,7 +381,12 @@ int qcow2_snapshot_create(BlockDriverState *bs, 
> QEMUSnapshotInfo *sn_info)
>       sn->l1_table_offset = l1_table_offset;
>       sn->l1_size = s->l1_size;
>   
> -    l1_table = g_malloc(s->l1_size * sizeof(uint64_t));
> +    l1_table = g_try_malloc(s->l1_size * sizeof(uint64_t));
> +    if (s->l1_size && l1_table == NULL) {
> +        ret = -ENOMEM;
> +        goto fail;
> +    }
> +
>       for(i = 0; i < s->l1_size; i++) {
>           l1_table[i] = cpu_to_be64(s->l1_table[i]);
>       }
> @@ -499,7 +504,11 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char 
> *snapshot_id)
>        * Decrease the refcount referenced by the old one only when the L1
>        * table is overwritten.
>        */
> -    sn_l1_table = g_malloc0(cur_l1_bytes);
> +    sn_l1_table = g_try_malloc0(cur_l1_bytes);
> +    if (cur_l1_bytes && sn_l1_table == NULL) {
> +        ret = -ENOMEM;
> +        goto fail;
> +    }
>   
>       ret = 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 = sn->l1_size * sizeof(uint64_t);
> -    new_l1_table = g_malloc0(align_offset(new_l1_bytes, 512));
> +    new_l1_table = qemu_try_blockalign(bs, align_offset(new_l1_bytes, 512));
> +    if (new_l1_bytes && new_l1_table == NULL) {
> +        return -ENOMEM;
> +    }
>   
>       ret = bdrv_pread(bs->file, sn->l1_table_offset, new_l1_table, new_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;
>       }
>   
>       /* Switch the L1 table */
> -    g_free(s->l1_table);
> +    qemu_vfree(s->l1_table);
>   
>       s->l1_size = sn->l1_size;
>       s->l1_table_offset = sn->l1_table_offset;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index a54d2ba..d7d5e26 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -676,8 +676,13 @@ static int qcow2_open(BlockDriverState *bs, QDict 
> *options, int flags,
>   
>   
>       if (s->l1_size > 0) {
> -        s->l1_table = g_malloc0(
> +        s->l1_table = qemu_try_blockalign(bs->file,
>               align_offset(s->l1_size * sizeof(uint64_t), 512));
> +        if (s->l1_size && s->l1_table == NULL) {

"s->l1_size" is unnecessary, as this has been checked by the outer 
condition already.

> +            error_setg(errp, "Could not allocate L1 table");
> +            ret = -ENOMEM;
> +            goto fail;
> +        }
>           ret = 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 = qcow2_cache_create(bs, L2_CACHE_SIZE);
>       s->refcount_block_cache = qcow2_cache_create(bs, REFCOUNT_CACHE_SIZE);
> +    if (s->l2_table_cache == NULL || s->refcount_block_cache == NULL) {
> +        error_setg(errp, "Could not allocate metadata caches");
> +        ret = -ENOMEM;
> +        goto fail;
> +    }
>   
>       s->cluster_cache = g_malloc(s->cluster_size);
>       /* one more sector for decompressed data alignment */
> -    s->cluster_data = qemu_blockalign(bs, QCOW_MAX_CRYPT_CLUSTERS * 
> s->cluster_size
> -                                  + 512);
> +    s->cluster_data = qemu_try_blockalign(bs, QCOW_MAX_CRYPT_CLUSTERS
> +                                              * s->cluster_size + 512);
> +    if (s->cluster_data == NULL) {
> +        error_setg(errp, "Could not allocate temporary cluster buffer");
> +        ret = -ENOMEM;
> +        goto fail;
> +    }
> +
>       s->cluster_cache_offset = -1;
>       s->flags = flags;
>   
> @@ -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 = NULL;
>       if (s->l2_table_cache) {
> @@ -1063,7 +1079,12 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState 
> *bs, int64_t sector_num,
>                    */
>                   if (!cluster_data) {
>                       cluster_data =
> -                        qemu_blockalign(bs, QCOW_MAX_CRYPT_CLUSTERS * 
> s->cluster_size);
> +                        qemu_try_blockalign(bs, QCOW_MAX_CRYPT_CLUSTERS
> +                                                * s->cluster_size);
> +                    if (cluster_data == NULL) {
> +                        ret = -ENOMEM;
> +                        goto fail;
> +                    }
>                   }
>   
>                   assert(cur_nr_sectors <=
> @@ -1163,8 +1184,12 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState 
> *bs,
>   
>           if (s->crypt_method) {
>               if (!cluster_data) {
> -                cluster_data = qemu_blockalign(bs, QCOW_MAX_CRYPT_CLUSTERS *
> -                                                 s->cluster_size);
> +                cluster_data = qemu_try_blockalign(bs, QCOW_MAX_CRYPT_CLUSTERS
> +                                                       * s->cluster_size);
> +                if (cluster_data == NULL) {
> +                    ret = -ENOMEM;
> +                    goto fail;
> +                }
>               }
>   
>               assert(hd_qiov.size <=
> @@ -1251,7 +1276,7 @@ fail:
>   static void qcow2_close(BlockDriverState *bs)
>   {
>       BDRVQcowState *s = 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 = NULL;
>