[Qemu-devel] [PATCH] block: Fix max nb_sectors in bdrv_make_zero

[Qemu-devel] [PATCH] block: Fix max nb_sectors in bdrv_make_zero

[Fam Zheng]

In bdrv_rw_co we report -EINVAL for nb_sectors > INT_MAX /
BDRV_SECTOR_SIZE, so a caller shouldn't exceed it.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index dacd881..5513379 100644
--- a/block.c
+++ b/block.c
@@ -2790,8 +2790,8 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
         if (nb_sectors <= 0) {
             return 0;
         }
-        if (nb_sectors > INT_MAX) {
-            nb_sectors = INT_MAX;
+        if (nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) {
+            nb_sectors = INT_MAX / BDRV_SECTOR_SIZE;
         }
         ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
         if (ret < 0) {
-- 
1.9.3

Re: [Qemu-devel] [PATCH] block: Fix max nb_sectors in bdrv_make_zero

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> In bdrv_rw_co we report -EINVAL for nb_sectors > INT_MAX /
> BDRV_SECTOR_SIZE, so a caller shouldn't exceed it.

It's not obvious to me why we do that there.  iovec member iov_len is
size_t, not int.

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block.c b/block.c
> index dacd881..5513379 100644
> --- a/block.c
> +++ b/block.c
> @@ -2790,8 +2790,8 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
>          if (nb_sectors <= 0) {
>              return 0;
>          }
> -        if (nb_sectors > INT_MAX) {
> -            nb_sectors = INT_MAX;
> +        if (nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) {
> +            nb_sectors = INT_MAX / BDRV_SECTOR_SIZE;
>          }
>          ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
>          if (ret < 0) {

Noticed while checkout out bdrv_get_block_status(): function comment of
bdrv_co_get_block_status() claims it returns true/false.  It doesn't.

Re: [Qemu-devel] [PATCH] block: Fix max nb_sectors in bdrv_make_zero

[Fam Zheng]

On Mon, 11/10 09:33, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> 
> > In bdrv_rw_co we report -EINVAL for nb_sectors > INT_MAX /
> > BDRV_SECTOR_SIZE, so a caller shouldn't exceed it.

I noticed this while testing unmap / zero write with scsi_debug:

# dd if=/dev/zero of=/tmp/a bs=1M count=32
# modprobe scsi_debug dev_size_mb=1024 lbpu=1
# qemu-img convert -t none /tmp/a /dev/sde
qemu-img: error writing zeroes at sector 0: Invalid argument

> 
> It's not obvious to me why we do that there.  iovec member iov_len is
> size_t, not int.

Deeper in the call stack we use int bytes everywhere:

static int coroutine_fn bdrv_co_do_preadv(BlockDriverState *bs,
    int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
    BdrvRequestFlags flags);
static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
    int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
    BdrvRequestFlags flags);

static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
    BdrvTrackedRequest *req, int64_t offset, unsigned int bytes,
    int64_t align, QEMUIOVector *qiov, int flags)

So this is a easier way to fix the specific bug :)

> 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index dacd881..5513379 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2790,8 +2790,8 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
> >          if (nb_sectors <= 0) {
> >              return 0;
> >          }
> > -        if (nb_sectors > INT_MAX) {
> > -            nb_sectors = INT_MAX;
> > +        if (nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) {
> > +            nb_sectors = INT_MAX / BDRV_SECTOR_SIZE;
> >          }
> >          ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
> >          if (ret < 0) {
> 
> Noticed while checkout out bdrv_get_block_status(): function comment of
> bdrv_co_get_block_status() claims it returns true/false.  It doesn't.
> 

Yeah, we should fix that.

Fam

Re: [Qemu-devel] [PATCH] block: Fix max nb_sectors in bdrv_make_zero

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Mon, 11/10 09:33, Markus Armbruster wrote:
>> Fam Zheng <famz@redhat.com> writes:
>> 
>> > In bdrv_rw_co we report -EINVAL for nb_sectors > INT_MAX /
>> > BDRV_SECTOR_SIZE, so a caller shouldn't exceed it.
>
> I noticed this while testing unmap / zero write with scsi_debug:
>
> # dd if=/dev/zero of=/tmp/a bs=1M count=32
> # modprobe scsi_debug dev_size_mb=1024 lbpu=1
> # qemu-img convert -t none /tmp/a /dev/sde
> qemu-img: error writing zeroes at sector 0: Invalid argument
>
>> 
>> It's not obvious to me why we do that there.  iovec member iov_len is
>> size_t, not int.
>
> Deeper in the call stack we use int bytes everywhere:
>
> static int coroutine_fn bdrv_co_do_preadv(BlockDriverState *bs,
>     int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
>     BdrvRequestFlags flags);
> static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
>     int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
>     BdrvRequestFlags flags);
>
> static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
>     BdrvTrackedRequest *req, int64_t offset, unsigned int bytes,
>     int64_t align, QEMUIOVector *qiov, int flags)
>
> So this is a easier way to fix the specific bug :)

Okay, that's enough for my R-by.

Aside: using bytes makes sense, using int not so much.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[...]

Re: [Qemu-devel] [PATCH] block: Fix max nb_sectors in bdrv_make_zero

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 401 bytes --]

On Mon, Nov 10, 2014 at 03:07:44PM +0800, Fam Zheng wrote:
> In bdrv_rw_co we report -EINVAL for nb_sectors > INT_MAX /
> BDRV_SECTOR_SIZE, so a caller shouldn't exceed it.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]