| Message ID | 20190423125706.26989-3-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix overflow bug in qcow2 discard | expand |
On Tue, Apr 23, 2019 at 03:57:05PM +0300, Vladimir Sementsov-Ogievskiy wrote: > This fixes at least one overflow in qcow2_process_discards, which > passes 64bit region length to bdrv_pdiscard where bytes (or sectors in > the past) parameter is int since its introduction in 0b919fae. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/block.h | 4 ++-- > block/io.c | 16 ++++++++-------- > 2 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/include/block/block.h b/include/block/block.h > index c7a26199aa..69fa18867e 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -432,8 +432,8 @@ void bdrv_drain_all(void); > AIO_WAIT_WHILE(bdrv_get_aio_context(bs_), \ > cond); }) > > -int bdrv_pdiscard(BdrvChild *child, int64_t offset, int bytes); > -int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes); > +int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes); > +int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes); > int bdrv_has_zero_init_1(BlockDriverState *bs); > int bdrv_has_zero_init(BlockDriverState *bs); > bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs); > diff --git a/block/io.c b/block/io.c > index dfc153b8d8..16b6c5d855 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -2653,7 +2653,7 @@ int bdrv_flush(BlockDriverState *bs) > typedef struct DiscardCo { > BdrvChild *child; > int64_t offset; > - int bytes; > + int64_t bytes; > int ret; > } DiscardCo; > static void coroutine_fn bdrv_pdiscard_co_entry(void *opaque) > @@ -2664,14 +2664,15 @@ static void coroutine_fn bdrv_pdiscard_co_entry(void *opaque) > aio_wait_kick(); > } > > -int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes) > +int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, > + int64_t bytes) > { > BdrvTrackedRequest req; > int max_pdiscard, ret; > int head, tail, align; > BlockDriverState *bs = child->bs; > > - if (!bs || !bs->drv) { > + if (!bs || !bs->drv || !bdrv_is_inserted(bs)) { Should we describe this change in the commit message? IIUC you added this check because you removed bdrv_check_byte_request() below, Maybe we can also remove '!bs->drv', since it is checked in bdrv_is_inserted(). > return -ENOMEDIUM; > } > > @@ -2679,9 +2680,8 @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes) > return -EPERM; > } > > - ret = bdrv_check_byte_request(bs, offset, bytes); > - if (ret < 0) { > - return ret; > + if (offset < 0 || bytes < 0 || bytes > INT64_MAX - offset) { > + return -EIO; > } Should we check if 'bytes' is greater than 'BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS'? Thanks, Stefano
30.04.2019 12:24, Stefano Garzarella wrote: > On Tue, Apr 23, 2019 at 03:57:05PM +0300, Vladimir Sementsov-Ogievskiy wrote: >> This fixes at least one overflow in qcow2_process_discards, which >> passes 64bit region length to bdrv_pdiscard where bytes (or sectors in >> the past) parameter is int since its introduction in 0b919fae. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> include/block/block.h | 4 ++-- >> block/io.c | 16 ++++++++-------- >> 2 files changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/include/block/block.h b/include/block/block.h >> index c7a26199aa..69fa18867e 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -432,8 +432,8 @@ void bdrv_drain_all(void); >> AIO_WAIT_WHILE(bdrv_get_aio_context(bs_), \ >> cond); }) >> >> -int bdrv_pdiscard(BdrvChild *child, int64_t offset, int bytes); >> -int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes); >> +int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes); >> +int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes); >> int bdrv_has_zero_init_1(BlockDriverState *bs); >> int bdrv_has_zero_init(BlockDriverState *bs); >> bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs); >> diff --git a/block/io.c b/block/io.c >> index dfc153b8d8..16b6c5d855 100644 >> --- a/block/io.c >> +++ b/block/io.c >> @@ -2653,7 +2653,7 @@ int bdrv_flush(BlockDriverState *bs) >> typedef struct DiscardCo { >> BdrvChild *child; >> int64_t offset; >> - int bytes; >> + int64_t bytes; >> int ret; >> } DiscardCo; >> static void coroutine_fn bdrv_pdiscard_co_entry(void *opaque) >> @@ -2664,14 +2664,15 @@ static void coroutine_fn bdrv_pdiscard_co_entry(void *opaque) >> aio_wait_kick(); >> } >> >> -int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes) >> +int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, >> + int64_t bytes) >> { >> BdrvTrackedRequest req; >> int max_pdiscard, ret; >> int head, tail, align; >> BlockDriverState *bs = child->bs; >> >> - if (!bs || !bs->drv) { >> + if (!bs || !bs->drv || !bdrv_is_inserted(bs)) { > > Should we describe this change in the commit message? Honestly, don't want to resend the series for this. > IIUC you added this check because you removed bdrv_check_byte_request() > below, > > Maybe we can also remove '!bs->drv', since it is checked in > bdrv_is_inserted(). Hmm, on v4 Kevin commented, that bdrv_is_inserted not needed, and, as I understand, not only in bdrv_co_pdiscard it should be removed, but it may be done later. So, I'd prefer to keep it as is for now. > >> return -ENOMEDIUM; >> } >> >> @@ -2679,9 +2680,8 @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes) >> return -EPERM; >> } >> >> - ret = bdrv_check_byte_request(bs, offset, bytes); >> - if (ret < 0) { >> - return ret; >> + if (offset < 0 || bytes < 0 || bytes > INT64_MAX - offset) { >> + return -EIO; >> } > > Should we check if 'bytes' is greater than > 'BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS'? > No, as we are contrariwise trying to support large bytes parameter in bdrv_co_pdiscard, which will exceed max request. If @bytes is large, it will be divided into several smaller requests in the following loop.
Am 30.04.2019 um 12:03 hat Vladimir Sementsov-Ogievskiy geschrieben: > 30.04.2019 12:24, Stefano Garzarella wrote: > > On Tue, Apr 23, 2019 at 03:57:05PM +0300, Vladimir Sementsov-Ogievskiy wrote: > >> This fixes at least one overflow in qcow2_process_discards, which > >> passes 64bit region length to bdrv_pdiscard where bytes (or sectors in > >> the past) parameter is int since its introduction in 0b919fae. > >> > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > >> --- > >> include/block/block.h | 4 ++-- > >> block/io.c | 16 ++++++++-------- > >> 2 files changed, 10 insertions(+), 10 deletions(-) > >> > >> diff --git a/include/block/block.h b/include/block/block.h > >> index c7a26199aa..69fa18867e 100644 > >> --- a/include/block/block.h > >> +++ b/include/block/block.h > >> @@ -432,8 +432,8 @@ void bdrv_drain_all(void); > >> AIO_WAIT_WHILE(bdrv_get_aio_context(bs_), \ > >> cond); }) > >> > >> -int bdrv_pdiscard(BdrvChild *child, int64_t offset, int bytes); > >> -int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes); > >> +int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes); > >> +int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes); > >> int bdrv_has_zero_init_1(BlockDriverState *bs); > >> int bdrv_has_zero_init(BlockDriverState *bs); > >> bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs); > >> diff --git a/block/io.c b/block/io.c > >> index dfc153b8d8..16b6c5d855 100644 > >> --- a/block/io.c > >> +++ b/block/io.c > >> @@ -2653,7 +2653,7 @@ int bdrv_flush(BlockDriverState *bs) > >> typedef struct DiscardCo { > >> BdrvChild *child; > >> int64_t offset; > >> - int bytes; > >> + int64_t bytes; > >> int ret; > >> } DiscardCo; > >> static void coroutine_fn bdrv_pdiscard_co_entry(void *opaque) > >> @@ -2664,14 +2664,15 @@ static void coroutine_fn bdrv_pdiscard_co_entry(void *opaque) > >> aio_wait_kick(); > >> } > >> > >> -int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes) > >> +int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, > >> + int64_t bytes) > >> { > >> BdrvTrackedRequest req; > >> int max_pdiscard, ret; > >> int head, tail, align; > >> BlockDriverState *bs = child->bs; > >> > >> - if (!bs || !bs->drv) { > >> + if (!bs || !bs->drv || !bdrv_is_inserted(bs)) { > > > > Should we describe this change in the commit message? > > Honestly, don't want to resend the series for this. I haven't reviewed the patches yet, but if this remains the only thing to change, it can be updated while applying the series if we have a specific proposal for a new commit message. Kevin
On Tue, Apr 30, 2019 at 10:03:08AM +0000, Vladimir Sementsov-Ogievskiy wrote: > 30.04.2019 12:24, Stefano Garzarella wrote: > > On Tue, Apr 23, 2019 at 03:57:05PM +0300, Vladimir Sementsov-Ogievskiy wrote: > >> This fixes at least one overflow in qcow2_process_discards, which > >> passes 64bit region length to bdrv_pdiscard where bytes (or sectors in > >> the past) parameter is int since its introduction in 0b919fae. > >> > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > >> --- > >> include/block/block.h | 4 ++-- > >> block/io.c | 16 ++++++++-------- > >> 2 files changed, 10 insertions(+), 10 deletions(-) > >> > >> diff --git a/include/block/block.h b/include/block/block.h > >> index c7a26199aa..69fa18867e 100644 > >> --- a/include/block/block.h > >> +++ b/include/block/block.h > >> @@ -432,8 +432,8 @@ void bdrv_drain_all(void); > >> AIO_WAIT_WHILE(bdrv_get_aio_context(bs_), \ > >> cond); }) > >> > >> -int bdrv_pdiscard(BdrvChild *child, int64_t offset, int bytes); > >> -int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes); > >> +int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes); > >> +int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes); > >> int bdrv_has_zero_init_1(BlockDriverState *bs); > >> int bdrv_has_zero_init(BlockDriverState *bs); > >> bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs); > >> diff --git a/block/io.c b/block/io.c > >> index dfc153b8d8..16b6c5d855 100644 > >> --- a/block/io.c > >> +++ b/block/io.c > >> @@ -2653,7 +2653,7 @@ int bdrv_flush(BlockDriverState *bs) > >> typedef struct DiscardCo { > >> BdrvChild *child; > >> int64_t offset; > >> - int bytes; > >> + int64_t bytes; > >> int ret; > >> } DiscardCo; > >> static void coroutine_fn bdrv_pdiscard_co_entry(void *opaque) > >> @@ -2664,14 +2664,15 @@ static void coroutine_fn bdrv_pdiscard_co_entry(void *opaque) > >> aio_wait_kick(); > >> } > >> > >> -int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes) > >> +int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, > >> + int64_t bytes) > >> { > >> BdrvTrackedRequest req; > >> int max_pdiscard, ret; > >> int head, tail, align; > >> BlockDriverState *bs = child->bs; > >> > >> - if (!bs || !bs->drv) { > >> + if (!bs || !bs->drv || !bdrv_is_inserted(bs)) { > > > > Should we describe this change in the commit message? > > Honestly, don't want to resend the series for this. > > > IIUC you added this check because you removed bdrv_check_byte_request() > > below, > > > > Maybe we can also remove '!bs->drv', since it is checked in > > bdrv_is_inserted(). > > Hmm, on v4 Kevin commented, that bdrv_is_inserted not needed, and, as I understand, not only > in bdrv_co_pdiscard it should be removed, but it may be done later. So, I'd prefer to keep it > as is for now. > Make sense if it will be removed. > > > >> return -ENOMEDIUM; > >> } > >> > >> @@ -2679,9 +2680,8 @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes) > >> return -EPERM; > >> } > >> > >> - ret = bdrv_check_byte_request(bs, offset, bytes); > >> - if (ret < 0) { > >> - return ret; > >> + if (offset < 0 || bytes < 0 || bytes > INT64_MAX - offset) { > >> + return -EIO; > >> } > > > > Should we check if 'bytes' is greater than > > 'BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS'? > > > > No, as we are contrariwise trying to support large bytes parameter in bdrv_co_pdiscard, which will > exceed max request. If @bytes is large, it will be divided into several smaller requests in the > following loop. > I understood. I saw that we limit the request to the driver to 'max_pdiscard' or 'INT_MAX'. As future work, could we use int64_t also for the driver callbacks? Anyway, the patch LGTM. Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Thanks, Stefano
On 4/30/19 6:09 AM, Kevin Wolf wrote: > Am 30.04.2019 um 12:03 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 30.04.2019 12:24, Stefano Garzarella wrote: >>> On Tue, Apr 23, 2019 at 03:57:05PM +0300, Vladimir Sementsov-Ogievskiy wrote: >>>> This fixes at least one overflow in qcow2_process_discards, which >>>> passes 64bit region length to bdrv_pdiscard where bytes (or sectors in >>>> the past) parameter is int since its introduction in 0b919fae. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> - if (!bs || !bs->drv) { >>>> + if (!bs || !bs->drv || !bdrv_is_inserted(bs)) { >>> >>> Should we describe this change in the commit message? >> >> Honestly, don't want to resend the series for this. > > I haven't reviewed the patches yet, but if this remains the only thing > to change, it can be updated while applying the series if we have a > specific proposal for a new commit message. How about: This fixes at least one overflow in qcow2_process_discards, which was inadvertently truncating a 64-bit region length to the bdrv_pdiscard 'int bytes' parameter (previously bdrv_discard's 'int sectors') since its introduction in 0b919fae. By inlining the remaining portions of bdrv_check_byte_request() that are still independent from the code previously validating a 32-bit request, we can drop the call to that function. A request larger than 31 bits (or the driver's max discard limit, if specified) is still split into smaller chunks by the block layer before reaching the driver. At any rate, you can add: Reviewed-by: Eric Blake <eblake@redhat.com>
On Tue, Apr 30, 2019 at 10:41:02AM -0500, Eric Blake wrote: > On 4/30/19 6:09 AM, Kevin Wolf wrote: > > Am 30.04.2019 um 12:03 hat Vladimir Sementsov-Ogievskiy geschrieben: > >> 30.04.2019 12:24, Stefano Garzarella wrote: > >>> On Tue, Apr 23, 2019 at 03:57:05PM +0300, Vladimir Sementsov-Ogievskiy wrote: > >>>> This fixes at least one overflow in qcow2_process_discards, which > >>>> passes 64bit region length to bdrv_pdiscard where bytes (or sectors in > >>>> the past) parameter is int since its introduction in 0b919fae. > >>>> > >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > >>>> - if (!bs || !bs->drv) { > >>>> + if (!bs || !bs->drv || !bdrv_is_inserted(bs)) { > >>> > >>> Should we describe this change in the commit message? > >> > >> Honestly, don't want to resend the series for this. > > > > I haven't reviewed the patches yet, but if this remains the only thing > > to change, it can be updated while applying the series if we have a > > specific proposal for a new commit message. > > How about: > > This fixes at least one overflow in qcow2_process_discards, which was > inadvertently truncating a 64-bit region length to the bdrv_pdiscard > 'int bytes' parameter (previously bdrv_discard's 'int sectors') since > its introduction in 0b919fae. > > By inlining the remaining portions of bdrv_check_byte_request() that are > still independent from the code previously validating a 32-bit request, > we can drop the call to that function. A request larger than 31 bits (or > the driver's max discard limit, if specified) is still split into > smaller chunks by the block layer before reaching the driver. > That sounds good to me! Thanks, Stefano
02.05.2019 12:11, Stefano Garzarella wrote: > On Tue, Apr 30, 2019 at 10:41:02AM -0500, Eric Blake wrote: >> On 4/30/19 6:09 AM, Kevin Wolf wrote: >>> Am 30.04.2019 um 12:03 hat Vladimir Sementsov-Ogievskiy geschrieben: >>>> 30.04.2019 12:24, Stefano Garzarella wrote: >>>>> On Tue, Apr 23, 2019 at 03:57:05PM +0300, Vladimir Sementsov-Ogievskiy wrote: >>>>>> This fixes at least one overflow in qcow2_process_discards, which >>>>>> passes 64bit region length to bdrv_pdiscard where bytes (or sectors in >>>>>> the past) parameter is int since its introduction in 0b919fae. >>>>>> >>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> >>>>>> - if (!bs || !bs->drv) { >>>>>> + if (!bs || !bs->drv || !bdrv_is_inserted(bs)) { >>>>> >>>>> Should we describe this change in the commit message? >>>> >>>> Honestly, don't want to resend the series for this. >>> >>> I haven't reviewed the patches yet, but if this remains the only thing >>> to change, it can be updated while applying the series if we have a >>> specific proposal for a new commit message. >> >> How about: >> >> This fixes at least one overflow in qcow2_process_discards, which was >> inadvertently truncating a 64-bit region length to the bdrv_pdiscard >> 'int bytes' parameter (previously bdrv_discard's 'int sectors') since >> its introduction in 0b919fae. >> >> By inlining the remaining portions of bdrv_check_byte_request() that are >> still independent from the code previously validating a 32-bit request, >> we can drop the call to that function. A request larger than 31 bits (or >> the driver's max discard limit, if specified) is still split into >> smaller chunks by the block layer before reaching the driver. >> > > That sounds good to me! > And for me. Thank you, Eric!
diff --git a/include/block/block.h b/include/block/block.h index c7a26199aa..69fa18867e 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -432,8 +432,8 @@ void bdrv_drain_all(void); AIO_WAIT_WHILE(bdrv_get_aio_context(bs_), \ cond); }) -int bdrv_pdiscard(BdrvChild *child, int64_t offset, int bytes); -int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes); +int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes); +int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes); int bdrv_has_zero_init_1(BlockDriverState *bs); int bdrv_has_zero_init(BlockDriverState *bs); bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs); diff --git a/block/io.c b/block/io.c index dfc153b8d8..16b6c5d855 100644 --- a/block/io.c +++ b/block/io.c @@ -2653,7 +2653,7 @@ int bdrv_flush(BlockDriverState *bs) typedef struct DiscardCo { BdrvChild *child; int64_t offset; - int bytes; + int64_t bytes; int ret; } DiscardCo; static void coroutine_fn bdrv_pdiscard_co_entry(void *opaque) @@ -2664,14 +2664,15 @@ static void coroutine_fn bdrv_pdiscard_co_entry(void *opaque) aio_wait_kick(); } -int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes) +int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, + int64_t bytes) { BdrvTrackedRequest req; int max_pdiscard, ret; int head, tail, align; BlockDriverState *bs = child->bs; - if (!bs || !bs->drv) { + if (!bs || !bs->drv || !bdrv_is_inserted(bs)) { return -ENOMEDIUM; } @@ -2679,9 +2680,8 @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes) return -EPERM; } - ret = bdrv_check_byte_request(bs, offset, bytes); - if (ret < 0) { - return ret; + if (offset < 0 || bytes < 0 || bytes > INT64_MAX - offset) { + return -EIO; } /* Do nothing if disabled. */ @@ -2716,7 +2716,7 @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes) assert(max_pdiscard >= bs->bl.request_alignment); while (bytes > 0) { - int num = bytes; + int64_t num = bytes; if (head) { /* Make small requests to get to alignment boundaries. */ @@ -2778,7 +2778,7 @@ out: return ret; } -int bdrv_pdiscard(BdrvChild *child, int64_t offset, int bytes) +int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes) { Coroutine *co; DiscardCo rwco = {
This fixes at least one overflow in qcow2_process_discards, which passes 64bit region length to bdrv_pdiscard where bytes (or sectors in the past) parameter is int since its introduction in 0b919fae. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/block.h | 4 ++-- block/io.c | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-)