From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49454) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adiab-0002dY-4o for qemu-devel@nongnu.org; Wed, 09 Mar 2016 13:12:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adiaZ-0002NJ-IO for qemu-devel@nongnu.org; Wed, 09 Mar 2016 13:12:05 -0500 References: <1457495496-28138-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <1457495496-28138-3-git-send-email-xiecl.fnst@cn.fujitsu.com> From: Max Reitz Message-ID: <56E06767.1050202@redhat.com> Date: Wed, 9 Mar 2016 19:11:51 +0100 MIME-Version: 1.0 In-Reply-To: <1457495496-28138-3-git-send-email-xiecl.fnst@cn.fujitsu.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="PRlEqxqeAMfTa5H4CHWG6xTRQBgIk3Dwc" Subject: Re: [Qemu-devel] [PATCH v11 2/3] quorum: implement bdrv_add_child() and bdrv_del_child() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Changlong Xie , qemu devel , Eric Blake , Alberto Garcia , Kevin Wolf , Stefan Hajnoczi Cc: qemu block , Jiang Yunhong , Dong Eddie , Markus Armbruster , "Dr. David Alan Gilbert" , Gonglei , zhanghailiang This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --PRlEqxqeAMfTa5H4CHWG6xTRQBgIk3Dwc Content-Type: multipart/mixed; boundary="SgqOxOvQ4VXqaSwQSEe25w9bTP8bMOVHi" From: Max Reitz To: Changlong Xie , qemu devel , Eric Blake , Alberto Garcia , Kevin Wolf , Stefan Hajnoczi Cc: Markus Armbruster , "Dr. David Alan Gilbert" , Dong Eddie , Jiang Yunhong , Wen Congyang , qemu block , zhanghailiang , Gonglei Message-ID: <56E06767.1050202@redhat.com> Subject: Re: [PATCH v11 2/3] quorum: implement bdrv_add_child() and bdrv_del_child() References: <1457495496-28138-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <1457495496-28138-3-git-send-email-xiecl.fnst@cn.fujitsu.com> In-Reply-To: <1457495496-28138-3-git-send-email-xiecl.fnst@cn.fujitsu.com> --SgqOxOvQ4VXqaSwQSEe25w9bTP8bMOVHi Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 09.03.2016 04:51, Changlong Xie wrote: > From: Wen Congyang >=20 > Signed-off-by: Wen Congyang > Signed-off-by: zhanghailiang > Signed-off-by: Gonglei > Signed-off-by: Changlong Xie > --- > block.c | 8 ++-- > block/quorum.c | 123 ++++++++++++++++++++++++++++++++++++++++++= +++++++- > include/block/block.h | 4 ++ > 3 files changed, 129 insertions(+), 6 deletions(-) >=20 > 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, c= onst char *filename, > return 0; > } > =20 > -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 =3D g_new(BdrvChild, 1); > *child =3D (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" > =20 > #define HASH_LENGTH 32 > =20 > @@ -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; > =20 > QuorumReadPattern read_pattern; > } BDRVQuorumState; > @@ -877,9 +880,9 @@ static int quorum_open(BlockDriverState *bs, QDict = *options, int flags, > ret =3D -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 =3D -EINVAL; > goto exit; > } > @@ -928,6 +931,7 @@ static int quorum_open(BlockDriverState *bs, QDict = *options, int flags, > /* allocate the children array */ > s->children =3D g_new0(BdrvChild *, s->num_children); > opened =3D g_new0(bool, s->num_children); > + s->index_bitmap =3D bitmap_new(s->num_children); > =20 > for (i =3D 0; i < s->num_children; i++) { > char indexstr[32]; > @@ -943,6 +947,8 @@ static int quorum_open(BlockDriverState *bs, QDict = *options, int flags, > =20 > opened[i] =3D true; > } > + bitmap_set(s->index_bitmap, 0, s->num_children); > + s->bsize =3D s->num_children; > =20 > g_free(opened); > goto exit; > @@ -999,6 +1005,116 @@ static void quorum_attach_aio_context(BlockDrive= rState *bs, > } > } > =20 > +static int get_new_child_index(BDRVQuorumState *s) > +{ > + int index; > + > + index =3D find_next_zero_bit(s->index_bitmap, s->bsize, 0); > + if (index < s->bsize) { > + return index; > + } > + > + s->index_bitmap =3D bitmap_zero_extend(s->index_bitmap, s->bsize, > + s->bsize + 1); If s->bsize =3D=3D INT_MAX, then this will overflow to INT_MIN (probably)= =2E 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 =3D=3D 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 =3D s->bsize; > + last_index =3D find_last_bit(s->index_bitmap, s->bsize); > + assert(last_index < old_bsize); > + s->bsize =3D last_index + 1; > + > + if (BITS_TO_LONGS(old_bsize) =3D=3D BITS_TO_LONGS(s->bsize)) { > + return; > + } > + > + new_len =3D BITS_TO_LONGS(s->bsize) * sizeof(unsigned long); > + s->index_bitmap =3D g_realloc(s->index_bitmap, new_len); > +} > + > +static void quorum_add_child(BlockDriverState *bs, BlockDriverState *c= hild_bs, > + Error **errp) > +{ > + BDRVQuorumState *s =3D bs->opaque; > + BdrvChild *child; > + char indexstr[32]; > + int index, ret; > + > + index =3D get_new_child_index(s); > + ret =3D snprintf(indexstr, 32, "children.%d", index); > + if (ret < 0 || ret >=3D 32) { > + error_setg(errp, "cannot generate child name"); > + return; > + } > + > + bdrv_drain(bs); > + > + assert(s->num_children <=3D INT_MAX / sizeof(BdrvChild *)); > + if (s->num_children =3D=3D INT_MAX / sizeof(BdrvChild *)) { > + error_setg(errp, "Too many children"); > + return; > + } > + s->children =3D g_renew(BdrvChild *, s->children, s->num_children = + 1); > + > + bdrv_ref(child_bs); > + child =3D bdrv_attach_child(bs, child_bs, indexstr, &child_format)= ; > + s->children[s->num_children++] =3D child; > + set_bit(index, s->index_bitmap); > +} > + > +static void quorum_del_child(BlockDriverState *bs, BdrvChild *child, > + Error **errp) > +{ > + BDRVQuorumState *s =3D bs->opaque; > + int i, index, rc; > + const char *endptr; > + unsigned long value; > + > + for (i =3D 0; i < s->num_children; i++) { > + if (s->children[i] =3D=3D child) { > + break; > + } > + } > + > + /* we have checked it in bdrv_del_child() */ > + assert(i < s->num_children); > + > + if (s->num_children <=3D s->threshold) { > + error_setg(errp, > + "The number of children cannot be lower than the vote thre= shold %d", > + s->threshold); > + return; > + } > + > + /* child->name is "children.%d" */ > + assert(!strncmp(child->name, "children.", 9)); > + rc =3D 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 > + assert(!rc && value < INT_MAX / sizeof(BdrvChild *)); > + index =3D 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 =3D g_renew(BdrvChild *, s->children, --s->num_childre= n); > + remove_child_index(s, index); > + bdrv_unref_child(bs, child); > +} > + > static void quorum_refresh_filename(BlockDriverState *bs, QDict *optio= ns) > { > BDRVQuorumState *s =3D bs->opaque; > @@ -1054,6 +1170,9 @@ static BlockDriver bdrv_quorum =3D { > .bdrv_detach_aio_context =3D quorum_detach_aio_context,= > .bdrv_attach_aio_context =3D quorum_attach_aio_context,= > =20 > + .bdrv_add_child =3D quorum_add_child, > + .bdrv_del_child =3D quorum_del_child, > + > .is_filter =3D true, > .bdrv_recurse_is_first_non_filter =3D quorum_recurse_is_first_no= n_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 *b= s); > 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); > =20 > bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **= errp); > void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason= ); >=20 --SgqOxOvQ4VXqaSwQSEe25w9bTP8bMOVHi-- --PRlEqxqeAMfTa5H4CHWG6xTRQBgIk3Dwc Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJW4GdnAAoJEDuxQgLoOKyt/fEIAIl5oVJxhu9u26gverdLITOo qwx8O8flVyHEWlBs8b7aGkCaDbqMtkG2bFtKBIXbB7L2kf8M5EHMiVbWhafJyIDw 66cdPPK6VpgyuUQ+F2dpW6IR1q5Wg1lQvauHkkuKtxaQW95NpeDD1dVnCQjBjgC5 2IjClgByuqnak5iQHPb42f4DIKf3x+N3ZJJ9oaz8kuB5fRFU+9HcMczQP3IZ50nC TjF6GLjK4W+eUvRhvOBkmWqVAyFLENllJ0piPBlMCImcWwdP1QaGWwDeTO6XWmkC wPB3EzqrGDGo6i6cgdFX0rkTAgHXWWvXz3HT2e2x1KYI6IegcyVgHaITR2QzSe8= =Rs8J -----END PGP SIGNATURE----- --PRlEqxqeAMfTa5H4CHWG6xTRQBgIk3Dwc--