[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v2] qcow2-bitmap: Fix uint64_t left-shift overflow
From: |
Tuguoyi |
Subject: |
RE: [PATCH v2] qcow2-bitmap: Fix uint64_t left-shift overflow |
Date: |
Wed, 30 Oct 2019 02:18:19 +0000 |
On 29.10.2019 19:57 Vladimir Sementsov-Ogievskiy wrote:
> 29.10.2019 14:14, Max Reitz wrote:
> > On 28.10.19 07:33, Tuguoyi wrote:
> >> In check_constraints_on_bitmap(), 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. As a result, for a qcow2
> >> disk with cluster_size set to 2M, we could not even create a dirty
> >> bitmap with default granularity. This patch fix the issue by dividing
> >> @len by granularity instead.
> >>
> >> Signed-off-by: Guoyi Tu <address@hidden>
> >> ---
> >> block/qcow2-bitmap.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index
> >> 98294a7..71ac822 100644
> >> --- a/block/qcow2-bitmap.c
> >> +++ b/block/qcow2-bitmap.c
> >> @@ -172,8 +172,8 @@ static int
> check_constraints_on_bitmap(BlockDriverState *bs,
> >> }
> >>
> >> if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
Here is the same problem, we need also to calculate the number of bits instead
of bytes.
> >> - (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
> >> - granularity_bits))
> >> + (DIV_ROUND_UP(len, granularity) >
> (uint64_t)BME_MAX_TABLE_SIZE *
> >> + s->cluster_size))
> >
> > This didn’t change because of this patch, but doesn’t this comparison
> > need a conversion of bits to bytes somewhere?
> >
> > len / granularity gives us the number of bits needed for the bitmap.
> >
> > BME_MAX_TABLE_SIZE is, as far as I can see, a number of bitmap
> > clusters, so multiplying it by the cluster size gives the number of
> > bytes in the bitmap. But the number of bits is eight times higher.
>
> I think you are right. It would be better to fix it in the same patch..
>
I think it would be better to add a inline function to calculate the max bytes
of bitmap with given virtual size and granularity.
I'll send another patch later
> >
> > Another topic: Isn’t BME_MAX_TABLE_SIZE too big?
>
> Maybe) Still, I don't sure that we need to change it..
>
> > As it is, bitmap
> > tables can have a size of 1 GB, and that’s the table alone. Depending
> > on the cluster size, the bitmap would take up at least 64 GB and cover
> > at least 32 TB (at a granularity of 512 bytes).
> >
> > Max
> >
> >> {
> >> error_setg(errp, "Too much space will be occupied by the
> bitmap. "
> >> "Use larger granularity");
> >>
> >
> >
>
>
> --
> Best regards,
> Vladimir