qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v11 2/3] quorum: implement bdrv_add_child() and


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v11 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()
Date: Wed, 9 Mar 2016 19:11:51 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0

On 09.03.2016 04:51, Changlong Xie wrote:
> From: Wen Congyang <address@hidden>
> 
> Signed-off-by: Wen Congyang <address@hidden>
> Signed-off-by: zhanghailiang <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> Signed-off-by: Changlong Xie <address@hidden>
> ---
>  block.c               |   8 ++--
>  block/quorum.c        | 123 
> +++++++++++++++++++++++++++++++++++++++++++++++++-
>  include/block/block.h |   4 ++
>  3 files changed, 129 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index d48f441..66d21af 100644
> --- a/block.c
> +++ b/block.c
> @@ -1194,10 +1194,10 @@ static int bdrv_fill_options(QDict **options, const 
> char *filename,
>      return 0;
>  }
>  
> -static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
> -                                    BlockDriverState *child_bs,
> -                                    const char *child_name,
> -                                    const BdrvChildRole *child_role)
> +BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
> +                             BlockDriverState *child_bs,
> +                             const char *child_name,
> +                             const BdrvChildRole *child_role)
>  {
>      BdrvChild *child = g_new(BdrvChild, 1);
>      *child = (BdrvChild) {
> diff --git a/block/quorum.c b/block/quorum.c
> index 11cc60b..469e4a3 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -24,6 +24,7 @@
>  #include "qapi/qmp/qstring.h"
>  #include "qapi-event.h"
>  #include "crypto/hash.h"
> +#include "qemu/bitmap.h"
>  
>  #define HASH_LENGTH 32
>  
> @@ -81,6 +82,8 @@ typedef struct BDRVQuorumState {
>      bool rewrite_corrupted;/* true if the driver must rewrite-on-read 
> corrupted
>                              * block if Quorum is reached.
>                              */
> +    unsigned long *index_bitmap;
> +    int bsize;
>  
>      QuorumReadPattern read_pattern;
>  } BDRVQuorumState;
> @@ -877,9 +880,9 @@ static int quorum_open(BlockDriverState *bs, QDict 
> *options, int flags,
>          ret = -EINVAL;
>          goto exit;
>      }
> -    if (s->num_children < 2) {
> +    if (s->num_children < 1) {
>          error_setg(&local_err,
> -                   "Number of provided children must be greater than 1");
> +                   "Number of provided children must be 1 or more");
>          ret = -EINVAL;
>          goto exit;
>      }
> @@ -928,6 +931,7 @@ static int quorum_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      /* allocate the children array */
>      s->children = g_new0(BdrvChild *, s->num_children);
>      opened = g_new0(bool, s->num_children);
> +    s->index_bitmap = bitmap_new(s->num_children);
>  
>      for (i = 0; i < s->num_children; i++) {
>          char indexstr[32];
> @@ -943,6 +947,8 @@ static int quorum_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  
>          opened[i] = true;
>      }
> +    bitmap_set(s->index_bitmap, 0, s->num_children);
> +    s->bsize = s->num_children;
>  
>      g_free(opened);
>      goto exit;
> @@ -999,6 +1005,116 @@ static void quorum_attach_aio_context(BlockDriverState 
> *bs,
>      }
>  }
>  
> +static int get_new_child_index(BDRVQuorumState *s)
> +{
> +    int index;
> +
> +    index = find_next_zero_bit(s->index_bitmap, s->bsize, 0);
> +    if (index < s->bsize) {
> +        return index;
> +    }
> +
> +    s->index_bitmap = bitmap_zero_extend(s->index_bitmap, s->bsize,
> +                                         s->bsize + 1);

If s->bsize == INT_MAX, then this will overflow to INT_MIN (probably).
This negative value will then be converted to a smaller negative value
by BITS_TO_LONGS() * sizeof(long) in bitmap_zero_extend(), and this
negative value will then be implicitly casted to a size_t value for the
g_realloc() call. On both 32 and 64 bit systems, allocating this will
probably fail due to insufficient memory which will then crash qemu.

One way to prevent this: Prevent the overflow in this function by
failing if s->bsize == INT_MAX before bitmap_zero_extend() is called.

Another way: Do not limit the number of children in quorum_add_child()
(and additionally in quorum_open()) to INT_MAX, but to something more
sane like 256 or 1024 or 65536 if you want to go really high (I can't
imagine anyone using more than 32 children). That way, s->bsize can
never grow to be INT_MAX in the first place.

In any case, qemu will probably crash long before this overflows because
trying to create 2G BDSs will definitely break something. This is why
I'd prefer the second approach (limiting the number of children to a
sane amount), and this is also why I don't actually care about this
overflow here:

In my opinion you don't need to change anything here. A follow-up patch
can take care of limiting the number of quorum children to a sane amount.

> +    return s->bsize++;
> +}
> +
> +static void remove_child_index(BDRVQuorumState *s, int index)
> +{
> +    int last_index, old_bsize;
> +    size_t new_len;
> +
> +    assert(index < s->bsize);
> +
> +    clear_bit(index, s->index_bitmap);
> +    if (index < s->bsize - 1) {
> +        /* The last bit is always set */
> +        return;
> +    }
> +
> +    /* Clear last bit */
> +    old_bsize = s->bsize;
> +    last_index = find_last_bit(s->index_bitmap, s->bsize);
> +    assert(last_index < old_bsize);
> +    s->bsize = last_index + 1;
> +
> +    if (BITS_TO_LONGS(old_bsize) == BITS_TO_LONGS(s->bsize)) {
> +        return;
> +    }
> +
> +    new_len = BITS_TO_LONGS(s->bsize) * sizeof(unsigned long);
> +    s->index_bitmap = g_realloc(s->index_bitmap, new_len);
> +}
> +
> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState 
> *child_bs,
> +                             Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    BdrvChild *child;
> +    char indexstr[32];
> +    int index, ret;
> +
> +    index = get_new_child_index(s);
> +    ret = snprintf(indexstr, 32, "children.%d", index);
> +    if (ret < 0 || ret >= 32) {
> +        error_setg(errp, "cannot generate child name");
> +        return;
> +    }
> +
> +    bdrv_drain(bs);
> +
> +    assert(s->num_children <= INT_MAX / sizeof(BdrvChild *));
> +    if (s->num_children == INT_MAX / sizeof(BdrvChild *)) {
> +        error_setg(errp, "Too many children");
> +        return;
> +    }
> +    s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
> +
> +    bdrv_ref(child_bs);
> +    child = bdrv_attach_child(bs, child_bs, indexstr, &child_format);
> +    s->children[s->num_children++] = child;
> +    set_bit(index, s->index_bitmap);
> +}
> +
> +static void quorum_del_child(BlockDriverState *bs, BdrvChild *child,
> +                             Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int i, index, rc;
> +    const char *endptr;
> +    unsigned long value;
> +
> +    for (i = 0; i < s->num_children; i++) {
> +        if (s->children[i] == child) {
> +            break;
> +        }
> +    }
> +
> +    /* we have checked it in bdrv_del_child() */
> +    assert(i < s->num_children);
> +
> +    if (s->num_children <= s->threshold) {
> +        error_setg(errp,
> +            "The number of children cannot be lower than the vote threshold 
> %d",
> +            s->threshold);
> +        return;
> +    }
> +
> +    /* child->name is "children.%d" */
> +    assert(!strncmp(child->name, "children.", 9));
> +    rc = qemu_strtoul(child->name + 9, &endptr, 10, &value);

Should be NULL instead of &endptr. With that fixed (and the endptr
declaration removed):

Reviewed-by: Max Reitz <address@hidden>

> +    assert(!rc && value < INT_MAX / sizeof(BdrvChild *));
> +    index = value;

Optional: Make index an unsigned long, replace all instances of "value"
by "index", and then you can drop this assignment.

Max

> +
> +    bdrv_drain(bs);
> +    /* We can safely remove this child now */
> +    memmove(&s->children[i], &s->children[i + 1],
> +            (s->num_children - i - 1) * sizeof(void *));
> +    s->children = g_renew(BdrvChild *, s->children, --s->num_children);
> +    remove_child_index(s, index);
> +    bdrv_unref_child(bs, child);
> +}
> +
>  static void quorum_refresh_filename(BlockDriverState *bs, QDict *options)
>  {
>      BDRVQuorumState *s = bs->opaque;
> @@ -1054,6 +1170,9 @@ static BlockDriver bdrv_quorum = {
>      .bdrv_detach_aio_context            = quorum_detach_aio_context,
>      .bdrv_attach_aio_context            = quorum_attach_aio_context,
>  
> +    .bdrv_add_child                     = quorum_add_child,
> +    .bdrv_del_child                     = quorum_del_child,
> +
>      .is_filter                          = true,
>      .bdrv_recurse_is_first_non_filter   = quorum_recurse_is_first_non_filter,
>  };
> diff --git a/include/block/block.h b/include/block/block.h
> index 7378e74..8a3966d 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -517,6 +517,10 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
>  void bdrv_ref(BlockDriverState *bs);
>  void bdrv_unref(BlockDriverState *bs);
>  void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child);
> +BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
> +                             BlockDriverState *child_bs,
> +                             const char *child_name,
> +                             const BdrvChildRole *child_role);
>  
>  bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
>  void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
> 


Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]