| Message ID | 4ba40cd1e7ee4a708b40899952e49f22@h3c.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] qcow2-bitmap: Fix uint64_t left-shift overflow | expand |
01.11.2019 10:37, Tuguoyi wrote: > There are two issues in In check_constraints_on_bitmap(), > 1) The sanity check on the granularity will cause uint64_t > integer left-shift overflow when cluster_size is 2M and the > granularity is BIGGER than 32K. > 2) The way to calculate image size that the maximum bitmap > supported can map to is a bit incorrect. > This patch fix it by add a helper function to calculate the > number of bytes needed by a normal bitmap in image and compare > it to the maximum bitmap bytes supported by qemu. > > Fixes: 5f72826e7fc62167cf3a > Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com> You forget my Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> (I don't see changes except add "Fixes: " to commit msg and put get_bitmap_bytes_needed definition header into one line.) > --- > block/qcow2-bitmap.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index 98294a7..ef9ef62 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -142,6 +142,13 @@ static int check_table_entry(uint64_t entry, int cluster_size) > return 0; > } > > +static int64_t get_bitmap_bytes_needed(int64_t len, uint32_t granularity) > +{ > + int64_t num_bits = DIV_ROUND_UP(len, granularity); > + > + return DIV_ROUND_UP(num_bits, 8); > +} > + > static int check_constraints_on_bitmap(BlockDriverState *bs, > const char *name, > uint32_t granularity, > @@ -150,6 +157,7 @@ static int check_constraints_on_bitmap(BlockDriverState *bs, > BDRVQcow2State *s = bs->opaque; > int granularity_bits = ctz32(granularity); > int64_t len = bdrv_getlength(bs); > + int64_t bitmap_bytes; > > assert(granularity > 0); > assert((granularity & (granularity - 1)) == 0); > @@ -171,9 +179,9 @@ static int check_constraints_on_bitmap(BlockDriverState *bs, > return -EINVAL; > } > > - if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) || > - (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size << > - granularity_bits)) > + bitmap_bytes = get_bitmap_bytes_needed(len, granularity); > + if ((bitmap_bytes > (uint64_t)BME_MAX_PHYS_SIZE) || > + (bitmap_bytes > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size)) > { > error_setg(errp, "Too much space will be occupied by the bitmap. " > "Use larger granularity"); >
On 01.11.2019 17:25 Vladimir Sementsov-Ogievskiy wrote: > 01.11.2019 10:37, Tuguoyi wrote: > > There are two issues in In check_constraints_on_bitmap(), > > 1) The sanity check on the granularity will cause uint64_t integer > > left-shift overflow when cluster_size is 2M and the granularity is > > BIGGER than 32K. > > 2) The way to calculate image size that the maximum bitmap supported > > can map to is a bit incorrect. > > This patch fix it by add a helper function to calculate the number of > > bytes needed by a normal bitmap in image and compare it to the maximum > > bitmap bytes supported by qemu. > > > > Fixes: 5f72826e7fc62167cf3a > > Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com> > > You forget my > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Sorry for that, it's my first time to submit patch to qemu, and should I send another patch or not ? > (I don't see changes except add "Fixes: " to commit msg and put > get_bitmap_bytes_needed definition header into one line.) Yes, Only minor changes are made, including removing 'inline' keyword. Thanks for your patience in reviewing > > --- > > block/qcow2-bitmap.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index > > 98294a7..ef9ef62 100644 > > --- a/block/qcow2-bitmap.c > > +++ b/block/qcow2-bitmap.c > > @@ -142,6 +142,13 @@ static int check_table_entry(uint64_t entry, int > cluster_size) > > return 0; > > } > > > > +static int64_t get_bitmap_bytes_needed(int64_t len, uint32_t > > +granularity) { > > + int64_t num_bits = DIV_ROUND_UP(len, granularity); > > + > > + return DIV_ROUND_UP(num_bits, 8); } > > + > > static int check_constraints_on_bitmap(BlockDriverState *bs, > > const char *name, > > uint32_t granularity, @@ > > -150,6 +157,7 @@ static int check_constraints_on_bitmap(BlockDriverState > *bs, > > BDRVQcow2State *s = bs->opaque; > > int granularity_bits = ctz32(granularity); > > int64_t len = bdrv_getlength(bs); > > + int64_t bitmap_bytes; > > > > assert(granularity > 0); > > assert((granularity & (granularity - 1)) == 0); @@ -171,9 +179,9 > > @@ static int check_constraints_on_bitmap(BlockDriverState *bs, > > return -EINVAL; > > } > > > > - if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) || > > - (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size << > > - granularity_bits)) > > + bitmap_bytes = get_bitmap_bytes_needed(len, granularity); > > + if ((bitmap_bytes > (uint64_t)BME_MAX_PHYS_SIZE) || > > + (bitmap_bytes > (uint64_t)BME_MAX_TABLE_SIZE * > > + s->cluster_size)) > > { > > error_setg(errp, "Too much space will be occupied by the > bitmap. " > > "Use larger granularity"); > > > > > -- > Best regards, > Vladimir
01.11.2019 12:34, Tuguoyi wrote: > On 01.11.2019 17:25 Vladimir Sementsov-Ogievskiy wrote: >> 01.11.2019 10:37, Tuguoyi wrote: >>> There are two issues in In check_constraints_on_bitmap(), >>> 1) The sanity check on the granularity will cause uint64_t integer >>> left-shift overflow when cluster_size is 2M and the granularity is >>> BIGGER than 32K. >>> 2) The way to calculate image size that the maximum bitmap supported >>> can map to is a bit incorrect. >>> This patch fix it by add a helper function to calculate the number of >>> bytes needed by a normal bitmap in image and compare it to the maximum >>> bitmap bytes supported by qemu. >>> >>> Fixes: 5f72826e7fc62167cf3a >>> Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com> >> >> You forget my >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > Sorry for that, it's my first time to submit patch to qemu, and should I send another patch or not ? Good start! No, you shouldn't. Maintainer will take such marks (and may be other proposed improvements for commit message like this "Fixes: ", when applying, so no reason to resend. Also, when sending a new version of patch, summarize what was changed since previous version (you may do it under "---" which follows commit message, or in cover letter if it's a patch set with several patches. > >> (I don't see changes except add "Fixes: " to commit msg and put >> get_bitmap_bytes_needed definition header into one line.) > > Yes, Only minor changes are made, including removing 'inline' keyword. > > Thanks for your patience in reviewing > >>> --- >>> block/qcow2-bitmap.c | 14 +++++++++++--- >>> 1 file changed, 11 insertions(+), 3 deletions(-) >>> >>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index >>> 98294a7..ef9ef62 100644 >>> --- a/block/qcow2-bitmap.c >>> +++ b/block/qcow2-bitmap.c >>> @@ -142,6 +142,13 @@ static int check_table_entry(uint64_t entry, int >> cluster_size) >>> return 0; >>> } >>> >>> +static int64_t get_bitmap_bytes_needed(int64_t len, uint32_t >>> +granularity) { >>> + int64_t num_bits = DIV_ROUND_UP(len, granularity); >>> + >>> + return DIV_ROUND_UP(num_bits, 8); } >>> + >>> static int check_constraints_on_bitmap(BlockDriverState *bs, >>> const char *name, >>> uint32_t granularity, @@ >>> -150,6 +157,7 @@ static int check_constraints_on_bitmap(BlockDriverState >> *bs, >>> BDRVQcow2State *s = bs->opaque; >>> int granularity_bits = ctz32(granularity); >>> int64_t len = bdrv_getlength(bs); >>> + int64_t bitmap_bytes; >>> >>> assert(granularity > 0); >>> assert((granularity & (granularity - 1)) == 0); @@ -171,9 +179,9 >>> @@ static int check_constraints_on_bitmap(BlockDriverState *bs, >>> return -EINVAL; >>> } >>> >>> - if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) || >>> - (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size << >>> - granularity_bits)) >>> + bitmap_bytes = get_bitmap_bytes_needed(len, granularity); >>> + if ((bitmap_bytes > (uint64_t)BME_MAX_PHYS_SIZE) || >>> + (bitmap_bytes > (uint64_t)BME_MAX_TABLE_SIZE * >>> + s->cluster_size)) >>> { >>> error_setg(errp, "Too much space will be occupied by the >> bitmap. " >>> "Use larger granularity"); >>> >> >> >> -- >> Best regards, >> Vladimir
On 01.11.2019 18:09, Vladimir Sementsov-Ogievskiy wrote: > 01.11.2019 12:34, Tuguoyi wrote: > > On 01.11.2019 17:25 Vladimir Sementsov-Ogievskiy wrote: > >> 01.11.2019 10:37, Tuguoyi wrote: > >>> There are two issues in In check_constraints_on_bitmap(), > >>> 1) The sanity check on the granularity will cause uint64_t integer > >>> left-shift overflow when cluster_size is 2M and the granularity is > >>> BIGGER than 32K. > >>> 2) The way to calculate image size that the maximum bitmap supported > >>> can map to is a bit incorrect. > >>> This patch fix it by add a helper function to calculate the number > >>> of bytes needed by a normal bitmap in image and compare it to the > >>> maximum bitmap bytes supported by qemu. > >>> > >>> Fixes: 5f72826e7fc62167cf3a > >>> Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com> > >> > >> You forget my > >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > > > Sorry for that, it's my first time to submit patch to qemu, and should I send > another patch or not ? > > Good start! > > No, you shouldn't. Maintainer will take such marks (and may be other > proposed improvements for commit message like this "Fixes: ", when applying, > so no reason to resend. > > Also, when sending a new version of patch, summarize what was changed > since previous version (you may do it under "---" which follows commit > message, or in cover letter if it's a patch set with several patches. OK, I got it, thanks a lot. > > > >> (I don't see changes except add "Fixes: " to commit msg and put > >> get_bitmap_bytes_needed definition header into one line.) > > > > Yes, Only minor changes are made, including removing 'inline' keyword. > > > > Thanks for your patience in reviewing > > >>> --- > >>> block/qcow2-bitmap.c | 14 +++++++++++--- > >>> 1 file changed, 11 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index > >>> 98294a7..ef9ef62 100644 > >>> --- a/block/qcow2-bitmap.c > >>> +++ b/block/qcow2-bitmap.c > >>> @@ -142,6 +142,13 @@ static int check_table_entry(uint64_t entry, > >>> int > >> cluster_size) > >>> return 0; > >>> } > >>> > >>> +static int64_t get_bitmap_bytes_needed(int64_t len, uint32_t > >>> +granularity) { > >>> + int64_t num_bits = DIV_ROUND_UP(len, granularity); > >>> + > >>> + return DIV_ROUND_UP(num_bits, 8); } > >>> + > >>> static int check_constraints_on_bitmap(BlockDriverState *bs, > >>> const char *name, > >>> uint32_t granularity, > @@ > >>> -150,6 +157,7 @@ static int > >>> check_constraints_on_bitmap(BlockDriverState > >> *bs, > >>> BDRVQcow2State *s = bs->opaque; > >>> int granularity_bits = ctz32(granularity); > >>> int64_t len = bdrv_getlength(bs); > >>> + int64_t bitmap_bytes; > >>> > >>> assert(granularity > 0); > >>> assert((granularity & (granularity - 1)) == 0); @@ -171,9 > >>> +179,9 @@ static int check_constraints_on_bitmap(BlockDriverState *bs, > >>> return -EINVAL; > >>> } > >>> > >>> - if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) || > >>> - (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size << > >>> - granularity_bits)) > >>> + bitmap_bytes = get_bitmap_bytes_needed(len, granularity); > >>> + if ((bitmap_bytes > (uint64_t)BME_MAX_PHYS_SIZE) || > >>> + (bitmap_bytes > (uint64_t)BME_MAX_TABLE_SIZE * > >>> + s->cluster_size)) > >>> { > >>> error_setg(errp, "Too much space will be occupied by the > >> bitmap. " > >>> "Use larger granularity"); > >>> > >> > >> > >> -- > >> Best regards, > >> Vladimir > > > -- > Best regards, > Vladimir
On 01.11.19 08:37, Tuguoyi wrote: > There are two issues in In check_constraints_on_bitmap(), > 1) The sanity check on the granularity will cause uint64_t > integer left-shift overflow when cluster_size is 2M and the > granularity is BIGGER than 32K. > 2) The way to calculate image size that the maximum bitmap > supported can map to is a bit incorrect. > This patch fix it by add a helper function to calculate the > number of bytes needed by a normal bitmap in image and compare > it to the maximum bitmap bytes supported by qemu. > > Fixes: 5f72826e7fc62167cf3a > Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com> > --- > block/qcow2-bitmap.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) Thanks, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block Max
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 98294a7..ef9ef62 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -142,6 +142,13 @@ static int check_table_entry(uint64_t entry, int cluster_size) return 0; } +static int64_t get_bitmap_bytes_needed(int64_t len, uint32_t granularity) +{ + int64_t num_bits = DIV_ROUND_UP(len, granularity); + + return DIV_ROUND_UP(num_bits, 8); +} + static int check_constraints_on_bitmap(BlockDriverState *bs, const char *name, uint32_t granularity, @@ -150,6 +157,7 @@ static int check_constraints_on_bitmap(BlockDriverState *bs, BDRVQcow2State *s = bs->opaque; int granularity_bits = ctz32(granularity); int64_t len = bdrv_getlength(bs); + int64_t bitmap_bytes; assert(granularity > 0); assert((granularity & (granularity - 1)) == 0); @@ -171,9 +179,9 @@ static int check_constraints_on_bitmap(BlockDriverState *bs, return -EINVAL; } - if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) || - (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size << - granularity_bits)) + bitmap_bytes = get_bitmap_bytes_needed(len, granularity); + if ((bitmap_bytes > (uint64_t)BME_MAX_PHYS_SIZE) || + (bitmap_bytes > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size)) { error_setg(errp, "Too much space will be occupied by the bitmap. " "Use larger granularity");
There are two issues in In check_constraints_on_bitmap(), 1) The sanity check on the granularity will cause uint64_t integer left-shift overflow when cluster_size is 2M and the granularity is BIGGER than 32K. 2) The way to calculate image size that the maximum bitmap supported can map to is a bit incorrect. This patch fix it by add a helper function to calculate the number of bytes needed by a normal bitmap in image and compare it to the maximum bitmap bytes supported by qemu. Fixes: 5f72826e7fc62167cf3a Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com> --- block/qcow2-bitmap.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)