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

[Benoît Canet]

The Tuesday 03 Jun 2014 à 15:10:52 (+0200), 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>
> Reviewed-by: Stefan Hajnoczi <address@hidden>
> ---
>  block/qcow2-cache.c    | 12 +++++++++++-
>  block/qcow2-cluster.c  | 35 +++++++++++++++++++++++++++--------
>  block/qcow2-refcount.c | 48 ++++++++++++++++++++++++++++++++++++++----------
>  block/qcow2-snapshot.c | 22 +++++++++++++++++-----
>  block/qcow2.c          | 41 +++++++++++++++++++++++++++++++++--------
>  5 files changed, 126 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);
Do we care to free c->entries on failure ?

>  
>      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;
> +    }
>  
>      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..a234c7a 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;
> +        }
>          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;
> +    }
>  
>      /* Fill the new refcount table */
>      memcpy(new_table, s->refcount_table,
> @@ -423,6 +433,7 @@ static int alloc_refcount_block(BlockDriverState *bs,
>      return -EAGAIN;
>  
>  fail_table:
> +    g_free(new_blocks);
>      g_free(new_table);
>  fail_block:
>      if (*refcount_block != NULL) {
> @@ -846,7 +857,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 +873,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 +890,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;
>      }
>  
>      for(i = 0; i < l1_size; i++) {
> @@ -1196,7 +1212,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 +1520,11 @@ 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) {
> +        res->check_errors++;
> +        return -ENOMEM;
> +    }
>  
>      res->bfi.total_clusters =
>          size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE);
> @@ -1752,9 +1776,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..07e8b73 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_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..0b07319 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_table == NULL) {
> +            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;
>  
> -- 
> 1.8.3.1
> 
>