| Message ID | 20220815090216.1818622-2-alexander.ivanov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | parallels: Refactor the code of images checks and fix a bug | expand |
On 8/15/22 12:02, Alexander Ivanov wrote: > data_end field in BDRVParallelsState is set to the biggest offset present > in BAT. If this offset is outside of the image, any further write will create > the cluster at this offset and/or the image will be truncated to this > offset on close. This is definitely not correct and should be fixed. > > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> > --- > v2: No change. > v3: Fix commit message. > > block/parallels.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/block/parallels.c b/block/parallels.c > index a229c06f25..a76cf9d993 100644 > --- a/block/parallels.c > +++ b/block/parallels.c > @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, > BDRVParallelsState *s = bs->opaque; > ParallelsHeader ph; > int ret, size, i; > + int64_t file_size; > QemuOpts *opts = NULL; > Error *local_err = NULL; > char *buf; > @@ -811,6 +812,22 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, > } > } > > + file_size = bdrv_getlength(bs->file->bs); > + if (file_size < 0) { > + goto fail; > + } > + > + file_size >>= BDRV_SECTOR_BITS; > + if (s->data_end > file_size) { > + if (flags & BDRV_O_CHECK) { > + s->data_end = file_size; Hm. but with this, any further allocation may lead to twice-allocted clusters, as you just modify s->data_end, but havn't yet fixed the BAT entry.. It seems unsafe. Or what I miss? > + } else { > + error_setg(errp, "parallels: Offset in BAT is out of image"); > + ret = -EINVAL; > + goto fail; > + } > + } > + > if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { > /* Image was not closed correctly. The check is mandatory */ > s->header_unclean = true;
On 17.08.2022 21:13, Vladimir Sementsov-Ogievskiy wrote: > On 8/15/22 12:02, Alexander Ivanov wrote: >> data_end field in BDRVParallelsState is set to the biggest offset >> present >> in BAT. If this offset is outside of the image, any further write >> will create >> the cluster at this offset and/or the image will be truncated to this >> offset on close. This is definitely not correct and should be fixed. >> >> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> >> --- >> v2: No change. >> v3: Fix commit message. >> >> block/parallels.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/block/parallels.c b/block/parallels.c >> index a229c06f25..a76cf9d993 100644 >> --- a/block/parallels.c >> +++ b/block/parallels.c >> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, >> QDict *options, int flags, >> BDRVParallelsState *s = bs->opaque; >> ParallelsHeader ph; >> int ret, size, i; >> + int64_t file_size; >> QemuOpts *opts = NULL; >> Error *local_err = NULL; >> char *buf; >> @@ -811,6 +812,22 @@ static int parallels_open(BlockDriverState *bs, >> QDict *options, int flags, >> } >> } >> + file_size = bdrv_getlength(bs->file->bs); >> + if (file_size < 0) { >> + goto fail; >> + } >> + >> + file_size >>= BDRV_SECTOR_BITS; >> + if (s->data_end > file_size) { >> + if (flags & BDRV_O_CHECK) { >> + s->data_end = file_size; > > Hm. but with this, any further allocation may lead to twice-allocted > clusters, as you just modify s->data_end, but havn't yet fixed the BAT > entry.. It seems unsafe. Or what I miss? > if O_CHECK is specified, we are going to execute parallels_co_check which will correctly handle this. In the other case (normal open) we will face the error, which is pretty much correct under this logic. >> + } else { >> + error_setg(errp, "parallels: Offset in BAT is out of >> image"); >> + ret = -EINVAL; >> + goto fail; >> + } >> + } >> + >> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { >> /* Image was not closed correctly. The check is mandatory */ >> s->header_unclean = true; > >
On 8/17/22 22:27, Denis V. Lunev wrote: > On 17.08.2022 21:13, Vladimir Sementsov-Ogievskiy wrote: >> On 8/15/22 12:02, Alexander Ivanov wrote: >>> data_end field in BDRVParallelsState is set to the biggest offset present >>> in BAT. If this offset is outside of the image, any further write will create >>> the cluster at this offset and/or the image will be truncated to this >>> offset on close. This is definitely not correct and should be fixed. >>> >>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> >>> --- >>> v2: No change. >>> v3: Fix commit message. >>> >>> block/parallels.c | 17 +++++++++++++++++ >>> 1 file changed, 17 insertions(+) >>> >>> diff --git a/block/parallels.c b/block/parallels.c >>> index a229c06f25..a76cf9d993 100644 >>> --- a/block/parallels.c >>> +++ b/block/parallels.c >>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, >>> BDRVParallelsState *s = bs->opaque; >>> ParallelsHeader ph; >>> int ret, size, i; >>> + int64_t file_size; >>> QemuOpts *opts = NULL; >>> Error *local_err = NULL; >>> char *buf; >>> @@ -811,6 +812,22 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, >>> } >>> } >>> + file_size = bdrv_getlength(bs->file->bs); >>> + if (file_size < 0) { >>> + goto fail; >>> + } >>> + >>> + file_size >>= BDRV_SECTOR_BITS; >>> + if (s->data_end > file_size) { >>> + if (flags & BDRV_O_CHECK) { >>> + s->data_end = file_size; >> >> Hm. but with this, any further allocation may lead to twice-allocted clusters, as you just modify s->data_end, but havn't yet fixed the BAT entry.. It seems unsafe. Or what I miss? >> > if O_CHECK is specified, we are going to execute parallels_co_check which > will correctly handle this. In the other case (normal open) we will > face the error, which is pretty much correct under this logic. Sounds like "s->data_end = file_size" is part of this handling and should be in parallels_co_check().. Looking at it, seems more correct to recalculate s->data_end exactly after for-loop, which fixes out-of-image clusters. Also it would work better in case when we have leaked clusters at the end of file. Otherwise, ideally, you should have comment at top of parallels_co_check(), that we must first fix out-of-image clusters, before doing any kind of allocation, because data_end is already tweaked. I agree that patch should work as is. > >>> + } else { >>> + error_setg(errp, "parallels: Offset in BAT is out of image"); >>> + ret = -EINVAL; >>> + goto fail; >>> + } >>> + } >>> + >>> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { >>> /* Image was not closed correctly. The check is mandatory */ >>> s->header_unclean = true; >> >> >
On 17.08.2022 21:43, Vladimir Sementsov-Ogievskiy wrote: > On 8/17/22 22:27, Denis V. Lunev wrote: >> On 17.08.2022 21:13, Vladimir Sementsov-Ogievskiy wrote: >>> On 8/15/22 12:02, Alexander Ivanov wrote: >>>> data_end field in BDRVParallelsState is set to the biggest offset >>>> present >>>> in BAT. If this offset is outside of the image, any further write >>>> will create >>>> the cluster at this offset and/or the image will be truncated to this >>>> offset on close. This is definitely not correct and should be fixed. >>>> >>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> >>>> --- >>>> v2: No change. >>>> v3: Fix commit message. >>>> >>>> block/parallels.c | 17 +++++++++++++++++ >>>> 1 file changed, 17 insertions(+) >>>> >>>> diff --git a/block/parallels.c b/block/parallels.c >>>> index a229c06f25..a76cf9d993 100644 >>>> --- a/block/parallels.c >>>> +++ b/block/parallels.c >>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, >>>> QDict *options, int flags, >>>> BDRVParallelsState *s = bs->opaque; >>>> ParallelsHeader ph; >>>> int ret, size, i; >>>> + int64_t file_size; >>>> QemuOpts *opts = NULL; >>>> Error *local_err = NULL; >>>> char *buf; >>>> @@ -811,6 +812,22 @@ static int parallels_open(BlockDriverState >>>> *bs, QDict *options, int flags, >>>> } >>>> } >>>> + file_size = bdrv_getlength(bs->file->bs); >>>> + if (file_size < 0) { >>>> + goto fail; >>>> + } >>>> + >>>> + file_size >>= BDRV_SECTOR_BITS; >>>> + if (s->data_end > file_size) { >>>> + if (flags & BDRV_O_CHECK) { >>>> + s->data_end = file_size; >>> >>> Hm. but with this, any further allocation may lead to twice-allocted >>> clusters, as you just modify s->data_end, but havn't yet fixed the >>> BAT entry.. It seems unsafe. Or what I miss? >>> >> if O_CHECK is specified, we are going to execute parallels_co_check >> which >> will correctly handle this. In the other case (normal open) we will >> face the error, which is pretty much correct under this logic. > > Sounds like "s->data_end = file_size" is part of this handling and > should be in parallels_co_check().. > > Looking at it, seems more correct to recalculate s->data_end exactly > after for-loop, which fixes out-of-image clusters. Also it would work > better in case when we have leaked clusters at the end of file. > > Otherwise, ideally, you should have comment at top of > parallels_co_check(), that we must first fix out-of-image clusters, > before doing any kind of allocation, because data_end is already tweaked. > > I agree that patch should work as is. > but that will change the game rules. Ideally we should call parallels_co_check if we face a error on open and assume that the error is recoverable like is done in QCOW2, but IMHO this is not the time within this patchset. This patch is needed here as we would have tests broken immediately once we start checking duplicated cluster management. That is why I am happy with this code but in general this would need further rework. This is a question of the chicken and egg and we can not do all at once :( >> >>>> + } else { >>>> + error_setg(errp, "parallels: Offset in BAT is out of >>>> image"); >>>> + ret = -EINVAL; >>>> + goto fail; >>>> + } >>>> + } >>>> + >>>> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { >>>> /* Image was not closed correctly. The check is mandatory */ >>>> s->header_unclean = true; >>> >>> >> > >
On 17.08.2022 21:43, Vladimir Sementsov-Ogievskiy wrote: > On 8/17/22 22:27, Denis V. Lunev wrote: >> On 17.08.2022 21:13, Vladimir Sementsov-Ogievskiy wrote: >>> On 8/15/22 12:02, Alexander Ivanov wrote: >>>> data_end field in BDRVParallelsState is set to the biggest offset >>>> present >>>> in BAT. If this offset is outside of the image, any further write >>>> will create >>>> the cluster at this offset and/or the image will be truncated to this >>>> offset on close. This is definitely not correct and should be fixed. >>>> >>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> >>>> --- >>>> v2: No change. >>>> v3: Fix commit message. >>>> >>>> block/parallels.c | 17 +++++++++++++++++ >>>> 1 file changed, 17 insertions(+) >>>> >>>> diff --git a/block/parallels.c b/block/parallels.c >>>> index a229c06f25..a76cf9d993 100644 >>>> --- a/block/parallels.c >>>> +++ b/block/parallels.c >>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, >>>> QDict *options, int flags, >>>> BDRVParallelsState *s = bs->opaque; >>>> ParallelsHeader ph; >>>> int ret, size, i; >>>> + int64_t file_size; >>>> QemuOpts *opts = NULL; >>>> Error *local_err = NULL; >>>> char *buf; >>>> @@ -811,6 +812,22 @@ static int parallels_open(BlockDriverState >>>> *bs, QDict *options, int flags, >>>> } >>>> } >>>> + file_size = bdrv_getlength(bs->file->bs); >>>> + if (file_size < 0) { >>>> + goto fail; >>>> + } >>>> + >>>> + file_size >>= BDRV_SECTOR_BITS; >>>> + if (s->data_end > file_size) { >>>> + if (flags & BDRV_O_CHECK) { >>>> + s->data_end = file_size; >>> >>> Hm. but with this, any further allocation may lead to twice-allocted >>> clusters, as you just modify s->data_end, but havn't yet fixed the >>> BAT entry.. It seems unsafe. Or what I miss? >>> >> if O_CHECK is specified, we are going to execute parallels_co_check >> which >> will correctly handle this. In the other case (normal open) we will >> face the error, which is pretty much correct under this logic. > > Sounds like "s->data_end = file_size" is part of this handling and > should be in parallels_co_check().. > > Looking at it, seems more correct to recalculate s->data_end exactly > after for-loop, which fixes out-of-image clusters. Also it would work > better in case when we have leaked clusters at the end of file. > > Otherwise, ideally, you should have comment at top of > parallels_co_check(), that we must first fix out-of-image clusters, > before doing any kind of allocation, because data_end is already tweaked. > > I agree that patch should work as is. I would like to leave this check in parallels_open(). I think it's a good idea to have an error on a corrupted image. Later we can replace it by checking&fixing images in parallels_open(). But I agree, it would be better to move the fix of s->data_end to parallels_co_check() and then move to parallels_check_outside_image(). > >> >>>> + } else { >>>> + error_setg(errp, "parallels: Offset in BAT is out of >>>> image"); >>>> + ret = -EINVAL; >>>> + goto fail; >>>> + } >>>> + } >>>> + >>>> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { >>>> /* Image was not closed correctly. The check is mandatory */ >>>> s->header_unclean = true; >>> >>> >> > >
On 8/18/22 11:49, Alexander Ivanov wrote: > > On 17.08.2022 21:43, Vladimir Sementsov-Ogievskiy wrote: >> On 8/17/22 22:27, Denis V. Lunev wrote: >>> On 17.08.2022 21:13, Vladimir Sementsov-Ogievskiy wrote: >>>> On 8/15/22 12:02, Alexander Ivanov wrote: >>>>> data_end field in BDRVParallelsState is set to the biggest offset present >>>>> in BAT. If this offset is outside of the image, any further write will create >>>>> the cluster at this offset and/or the image will be truncated to this >>>>> offset on close. This is definitely not correct and should be fixed. >>>>> >>>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> >>>>> --- >>>>> v2: No change. >>>>> v3: Fix commit message. >>>>> >>>>> block/parallels.c | 17 +++++++++++++++++ >>>>> 1 file changed, 17 insertions(+) >>>>> >>>>> diff --git a/block/parallels.c b/block/parallels.c >>>>> index a229c06f25..a76cf9d993 100644 >>>>> --- a/block/parallels.c >>>>> +++ b/block/parallels.c >>>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, >>>>> BDRVParallelsState *s = bs->opaque; >>>>> ParallelsHeader ph; >>>>> int ret, size, i; >>>>> + int64_t file_size; >>>>> QemuOpts *opts = NULL; >>>>> Error *local_err = NULL; >>>>> char *buf; >>>>> @@ -811,6 +812,22 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, >>>>> } >>>>> } >>>>> + file_size = bdrv_getlength(bs->file->bs); >>>>> + if (file_size < 0) { >>>>> + goto fail; >>>>> + } >>>>> + >>>>> + file_size >>= BDRV_SECTOR_BITS; >>>>> + if (s->data_end > file_size) { >>>>> + if (flags & BDRV_O_CHECK) { >>>>> + s->data_end = file_size; >>>> >>>> Hm. but with this, any further allocation may lead to twice-allocted clusters, as you just modify s->data_end, but havn't yet fixed the BAT entry.. It seems unsafe. Or what I miss? >>>> >>> if O_CHECK is specified, we are going to execute parallels_co_check which >>> will correctly handle this. In the other case (normal open) we will >>> face the error, which is pretty much correct under this logic. >> >> Sounds like "s->data_end = file_size" is part of this handling and should be in parallels_co_check().. >> >> Looking at it, seems more correct to recalculate s->data_end exactly after for-loop, which fixes out-of-image clusters. Also it would work better in case when we have leaked clusters at the end of file. >> >> Otherwise, ideally, you should have comment at top of parallels_co_check(), that we must first fix out-of-image clusters, before doing any kind of allocation, because data_end is already tweaked. >> >> I agree that patch should work as is. > > I would like to leave this check in parallels_open(). I think it's a good idea to have an error on a corrupted image. Later we can replace it by checking&fixing images in parallels_open(). > > But I agree, it would be better to move the fix of s->data_end to parallels_co_check() and then move to parallels_check_outside_image(). Yes, right, agree. > >> >>> >>>>> + } else { >>>>> + error_setg(errp, "parallels: Offset in BAT is out of image"); >>>>> + ret = -EINVAL; >>>>> + goto fail; >>>>> + } >>>>> + } >>>>> + >>>>> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { >>>>> /* Image was not closed correctly. The check is mandatory */ >>>>> s->header_unclean = true; >>>> >>>> >>> >> >>
diff --git a/block/parallels.c b/block/parallels.c index a229c06f25..a76cf9d993 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, BDRVParallelsState *s = bs->opaque; ParallelsHeader ph; int ret, size, i; + int64_t file_size; QemuOpts *opts = NULL; Error *local_err = NULL; char *buf; @@ -811,6 +812,22 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, } } + file_size = bdrv_getlength(bs->file->bs); + if (file_size < 0) { + goto fail; + } + + file_size >>= BDRV_SECTOR_BITS; + if (s->data_end > file_size) { + if (flags & BDRV_O_CHECK) { + s->data_end = file_size; + } else { + error_setg(errp, "parallels: Offset in BAT is out of image"); + ret = -EINVAL; + goto fail; + } + } + if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { /* Image was not closed correctly. The check is mandatory */ s->header_unclean = true;
data_end field in BDRVParallelsState is set to the biggest offset present in BAT. If this offset is outside of the image, any further write will create the cluster at this offset and/or the image will be truncated to this offset on close. This is definitely not correct and should be fixed. Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> --- v2: No change. v3: Fix commit message. block/parallels.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)