[Qemu-devel] [PATCH V2] block: introduce BDRV_REQUEST_MAX_SECTORS

[Qemu-devel] [PATCH V2] block: introduce BDRV_REQUEST_MAX_SECTORS

[Peter Lieven]

we check and adjust request sizes at several places with
sometimes inconsistent checks or default values:
 INT_MAX
 INT_MAX >> BDRV_SECTOR_BITS
 UINT_MAX >> BDRV_SECTOR_BITS
 SIZE_MAX >> BDRV_SECTOR_BITS

This patches introdocues a macro for the maximal allowed sectors
per request and uses it at several places.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
v1->v2: use macro in bdrv_check_byte_request alse [Den]

 block.c               |   21 +++++++++------------
 hw/block/virtio-blk.c |    4 ++--
 include/block/block.h |    3 +++
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/block.c b/block.c
index 8272ef9..49e0073 100644
--- a/block.c
+++ b/block.c
@@ -2647,7 +2647,7 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
 {
     int64_t len;
 
-    if (size > INT_MAX) {
+    if (size > BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) {
         return -EIO;
     }
 
@@ -2671,7 +2671,7 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
 static int bdrv_check_request(BlockDriverState *bs, int64_t sector_num,
                               int nb_sectors)
 {
-    if (nb_sectors < 0 || nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) {
+    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
         return -EIO;
     }
 
@@ -2758,7 +2758,7 @@ static int bdrv_rw_co(BlockDriverState *bs, int64_t sector_num, uint8_t *buf,
         .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
     };
 
-    if (nb_sectors < 0 || nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) {
+    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
         return -EINVAL;
     }
 
@@ -2826,13 +2826,10 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
     }
 
     for (;;) {
-        nb_sectors = target_sectors - sector_num;
+        nb_sectors = MIN(target_sectors - sector_num, BDRV_REQUEST_MAX_SECTORS);
         if (nb_sectors <= 0) {
             return 0;
         }
-        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) {
             error_report("error getting block status at sector %" PRId64 ": %s",
@@ -3167,7 +3164,7 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
     int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
     BdrvRequestFlags flags)
 {
-    if (nb_sectors < 0 || nb_sectors > (UINT_MAX >> BDRV_SECTOR_BITS)) {
+    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
         return -EINVAL;
     }
 
@@ -3202,8 +3199,8 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
     struct iovec iov = {0};
     int ret = 0;
 
-    int max_write_zeroes = bs->bl.max_write_zeroes ?
-                           bs->bl.max_write_zeroes : INT_MAX;
+    int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_write_zeroes,
+                                        BDRV_REQUEST_MAX_SECTORS);
 
     while (nb_sectors > 0 && !ret) {
         int num = nb_sectors;
@@ -3458,7 +3455,7 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
     int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
     BdrvRequestFlags flags)
 {
-    if (nb_sectors < 0 || nb_sectors > (INT_MAX >> BDRV_SECTOR_BITS)) {
+    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
         return -EINVAL;
     }
 
@@ -5120,7 +5117,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
         return 0;
     }
 
-    max_discard = bs->bl.max_discard ?  bs->bl.max_discard : INT_MAX;
+    max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
     while (nb_sectors > 0) {
         int ret;
         int num = nb_sectors;
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 8c51a29..1a8a176 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -381,7 +381,7 @@ void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb)
     }
 
     max_xfer_len = blk_get_max_transfer_length(mrb->reqs[0]->dev->blk);
-    max_xfer_len = MIN_NON_ZERO(max_xfer_len, INT_MAX);
+    max_xfer_len = MIN_NON_ZERO(max_xfer_len, BDRV_REQUEST_MAX_SECTORS);
 
     qsort(mrb->reqs, mrb->num_reqs, sizeof(*mrb->reqs),
           &multireq_compare);
@@ -447,7 +447,7 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
     uint64_t nb_sectors = size >> BDRV_SECTOR_BITS;
     uint64_t total_sectors;
 
-    if (nb_sectors > INT_MAX) {
+    if (nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
         return false;
     }
     if (sector & dev->sector_mask) {
diff --git a/include/block/block.h b/include/block/block.h
index 3082d2b..25a6d62 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -83,6 +83,9 @@ typedef enum {
 #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
 #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
 
+#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
+                                     INT_MAX >> BDRV_SECTOR_BITS)
+
 /*
  * Allocation status flags
  * BDRV_BLOCK_DATA: data is read from bs->file or another file
-- 
1.7.9.5

[Qemu-devel] [PATCH 1/1] nbd: fix max_discard/max_transfer_length

[Denis V. Lunev]

nbd_co_discard calls nbd_client_session_co_discard which uses uint32_t
as the length in bytes of the data to discard due to the following
definition:

struct nbd_request {
    uint32_t magic;
    uint32_t type;
    uint64_t handle;
    uint64_t from;
    uint32_t len; <-- the length of data to be discarded, in bytes
} QEMU_PACKED;

Thus we should limit bl_max_discard to UINT32_MAX >> BDRV_SECTOR_BITS to
avoid overflow.

NBD read/write code uses the same structure for transfers. Fix
max_transfer_length accordingly.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Peter Lieven <pl@kamp.de>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/nbd.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/block/nbd.c b/block/nbd.c
index 04cc845..dda0b0b 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -301,6 +301,12 @@ static int nbd_co_flush(BlockDriverState *bs)
     return nbd_client_session_co_flush(&s->client);
 }
 
+static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
+{
+    bs->bl.max_discard = UINT32_MAX >> BDRV_SECTOR_BITS;
+    bs->bl.max_transfer_length = UINT32_MAX >> BDRV_SECTOR_BITS;
+}
+
 static int nbd_co_discard(BlockDriverState *bs, int64_t sector_num,
                           int nb_sectors)
 {
@@ -396,6 +402,7 @@ static BlockDriver bdrv_nbd = {
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_discard            = nbd_co_discard,
+    .bdrv_refresh_limits        = nbd_refresh_limits,
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
@@ -413,6 +420,7 @@ static BlockDriver bdrv_nbd_tcp = {
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_discard            = nbd_co_discard,
+    .bdrv_refresh_limits        = nbd_refresh_limits,
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
@@ -430,6 +438,7 @@ static BlockDriver bdrv_nbd_unix = {
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_discard            = nbd_co_discard,
+    .bdrv_refresh_limits        = nbd_refresh_limits,
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/1] nbd: fix max_discard/max_transfer_length

[Kevin Wolf]

Am 06.02.2015 um 12:24 hat Denis V. Lunev geschrieben:
> nbd_co_discard calls nbd_client_session_co_discard which uses uint32_t
> as the length in bytes of the data to discard due to the following
> definition:
> 
> struct nbd_request {
>     uint32_t magic;
>     uint32_t type;
>     uint64_t handle;
>     uint64_t from;
>     uint32_t len; <-- the length of data to be discarded, in bytes
> } QEMU_PACKED;
> 
> Thus we should limit bl_max_discard to UINT32_MAX >> BDRV_SECTOR_BITS to
> avoid overflow.
> 
> NBD read/write code uses the same structure for transfers. Fix
> max_transfer_length accordingly.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Peter Lieven <pl@kamp.de>
> CC: Kevin Wolf <kwolf@redhat.com>

Thanks, I have applied both Peter's and your patch. Can you guys please
check whether the current state of my block branch is correct or whether
I forgot to include or remove some patch?

By the way, I don't think this NBD patch is strictly necessary as you'll
have a hard time finding a platform where INT_MAX > UINT32_MAX, but I
think it's good documentation at least and a safeguard if we ever decide
to lift the general block layer restrictions.

Kevin

Re: [Qemu-devel] [PATCH 1/1] nbd: fix max_discard/max_transfer_length

[Denis V. Lunev]

On 06/02/15 14:53, Kevin Wolf wrote:
> Am 06.02.2015 um 12:24 hat Denis V. Lunev geschrieben:
>> nbd_co_discard calls nbd_client_session_co_discard which uses uint32_t
>> as the length in bytes of the data to discard due to the following
>> definition:
>>
>> struct nbd_request {
>>      uint32_t magic;
>>      uint32_t type;
>>      uint64_t handle;
>>      uint64_t from;
>>      uint32_t len; <-- the length of data to be discarded, in bytes
>> } QEMU_PACKED;
>>
>> Thus we should limit bl_max_discard to UINT32_MAX >> BDRV_SECTOR_BITS to
>> avoid overflow.
>>
>> NBD read/write code uses the same structure for transfers. Fix
>> max_transfer_length accordingly.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Peter Lieven <pl@kamp.de>
>> CC: Kevin Wolf <kwolf@redhat.com>
> Thanks, I have applied both Peter's and your patch. Can you guys please
> check whether the current state of my block branch is correct or whether
> I forgot to include or remove some patch?
can you give me tree URL?

> By the way, I don't think this NBD patch is strictly necessary as you'll
> have a hard time finding a platform where INT_MAX > UINT32_MAX, but I
> think it's good documentation at least and a safeguard if we ever decide
> to lift the general block layer restrictions.
>
> Kevin
nope, it is absolutely mandatory

stdint.h:

/* Limit of `size_t' type.  */
# if __WORDSIZE == 64
#  define SIZE_MAX              (18446744073709551615UL)
# else
#  define SIZE_MAX              (4294967295U)
# endif

Den

Re: [Qemu-devel] [PATCH 1/1] nbd: fix max_discard/max_transfer_length

[Peter Lieven]

Am 06.02.2015 um 12:59 schrieb Denis V. Lunev:
> On 06/02/15 14:53, Kevin Wolf wrote:
>> Am 06.02.2015 um 12:24 hat Denis V. Lunev geschrieben:
>>> nbd_co_discard calls nbd_client_session_co_discard which uses uint32_t
>>> as the length in bytes of the data to discard due to the following
>>> definition:
>>>
>>> struct nbd_request {
>>>      uint32_t magic;
>>>      uint32_t type;
>>>      uint64_t handle;
>>>      uint64_t from;
>>>      uint32_t len; <-- the length of data to be discarded, in bytes
>>> } QEMU_PACKED;
>>>
>>> Thus we should limit bl_max_discard to UINT32_MAX >> BDRV_SECTOR_BITS to
>>> avoid overflow.
>>>
>>> NBD read/write code uses the same structure for transfers. Fix
>>> max_transfer_length accordingly.
>>>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Peter Lieven <pl@kamp.de>
>>> CC: Kevin Wolf <kwolf@redhat.com>
>> Thanks, I have applied both Peter's and your patch. Can you guys please
>> check whether the current state of my block branch is correct or whether
>> I forgot to include or remove some patch?
> can you give me tree URL?
>
>> By the way, I don't think this NBD patch is strictly necessary as you'll
>> have a hard time finding a platform where INT_MAX > UINT32_MAX, but I
>> think it's good documentation at least and a safeguard if we ever decide
>> to lift the general block layer restrictions.
>>
>> Kevin
> nope, it is absolutely mandatory
>
> stdint.h:
>
> /* Limit of `size_t' type.  */
> # if __WORDSIZE == 64
> #  define SIZE_MAX              (18446744073709551615UL)
> # else
> #  define SIZE_MAX              (4294967295U)
> # endif
>
> Den
Yes, but we limit to MIN(SIZE_MAX >> BDRV_SECTOR_BITS, INT_MAX >> BDRV_SECTOR_BITS) anyway.

I do not know if there is a platform where INT_MAX is 2^63 - 1 and not 2^31 - 1 ?

We can keep Dens patch in. Just in case. It doesn't hurt.

Peter

Re: [Qemu-devel] [PATCH 1/1] nbd: fix max_discard/max_transfer_length

[Kevin Wolf]

Am 06.02.2015 um 12:59 hat Denis V. Lunev geschrieben:
> On 06/02/15 14:53, Kevin Wolf wrote:
> >Am 06.02.2015 um 12:24 hat Denis V. Lunev geschrieben:
> >>nbd_co_discard calls nbd_client_session_co_discard which uses uint32_t
> >>as the length in bytes of the data to discard due to the following
> >>definition:
> >>
> >>struct nbd_request {
> >>     uint32_t magic;
> >>     uint32_t type;
> >>     uint64_t handle;
> >>     uint64_t from;
> >>     uint32_t len; <-- the length of data to be discarded, in bytes
> >>} QEMU_PACKED;
> >>
> >>Thus we should limit bl_max_discard to UINT32_MAX >> BDRV_SECTOR_BITS to
> >>avoid overflow.
> >>
> >>NBD read/write code uses the same structure for transfers. Fix
> >>max_transfer_length accordingly.
> >>
> >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>CC: Peter Lieven <pl@kamp.de>
> >>CC: Kevin Wolf <kwolf@redhat.com>
> >Thanks, I have applied both Peter's and your patch. Can you guys please
> >check whether the current state of my block branch is correct or whether
> >I forgot to include or remove some patch?
> can you give me tree URL?

Sure:

git: git://repo.or.cz/qemu/kevin.git block
Web: http://repo.or.cz/w/qemu/kevin.git/shortlog/refs/heads/block

> >By the way, I don't think this NBD patch is strictly necessary as you'll
> >have a hard time finding a platform where INT_MAX > UINT32_MAX, but I
> >think it's good documentation at least and a safeguard if we ever decide
> >to lift the general block layer restrictions.
> >
> >Kevin
> nope, it is absolutely mandatory
> 
> stdint.h:
> 
> /* Limit of `size_t' type.  */
> # if __WORDSIZE == 64
> #  define SIZE_MAX              (18446744073709551615UL)
> # else
> #  define SIZE_MAX              (4294967295U)
> # endif

But Peter defined it like this:

#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
                                     INT_MAX >> BDRV_SECTOR_BITS)

And having integers with more the 32 bits is at least unusual. I don't
know of any platform that has them.

Anyway, as I said, your patch is good documentation, so I'm happy to
apply it nevertheless.

Kevin

Re: [Qemu-devel] [PATCH 1/1] nbd: fix max_discard/max_transfer_length

[Denis V. Lunev]

On 06/02/15 15:07, Kevin Wolf wrote:
> Am 06.02.2015 um 12:59 hat Denis V. Lunev geschrieben:
>> On 06/02/15 14:53, Kevin Wolf wrote:
>>> Am 06.02.2015 um 12:24 hat Denis V. Lunev geschrieben:
>>>> nbd_co_discard calls nbd_client_session_co_discard which uses uint32_t
>>>> as the length in bytes of the data to discard due to the following
>>>> definition:
>>>>
>>>> struct nbd_request {
>>>>      uint32_t magic;
>>>>      uint32_t type;
>>>>      uint64_t handle;
>>>>      uint64_t from;
>>>>      uint32_t len; <-- the length of data to be discarded, in bytes
>>>> } QEMU_PACKED;
>>>>
>>>> Thus we should limit bl_max_discard to UINT32_MAX >> BDRV_SECTOR_BITS to
>>>> avoid overflow.
>>>>
>>>> NBD read/write code uses the same structure for transfers. Fix
>>>> max_transfer_length accordingly.
>>>>
>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>> CC: Peter Lieven <pl@kamp.de>
>>>> CC: Kevin Wolf <kwolf@redhat.com>
>>> Thanks, I have applied both Peter's and your patch. Can you guys please
>>> check whether the current state of my block branch is correct or whether
>>> I forgot to include or remove some patch?
>> can you give me tree URL?
> Sure:
>
> git: git://repo.or.cz/qemu/kevin.git block
> Web: http://repo.or.cz/w/qemu/kevin.git/shortlog/refs/heads/block
>
>>> By the way, I don't think this NBD patch is strictly necessary as you'll
>>> have a hard time finding a platform where INT_MAX > UINT32_MAX, but I
>>> think it's good documentation at least and a safeguard if we ever decide
>>> to lift the general block layer restrictions.
>>>
>>> Kevin
>> nope, it is absolutely mandatory
>>
>> stdint.h:
>>
>> /* Limit of `size_t' type.  */
>> # if __WORDSIZE == 64
>> #  define SIZE_MAX              (18446744073709551615UL)
>> # else
>> #  define SIZE_MAX              (4294967295U)
>> # endif
> But Peter defined it like this:
>
> #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
>                                       INT_MAX >> BDRV_SECTOR_BITS)
>
> And having integers with more the 32 bits is at least unusual. I don't
> know of any platform that has them.
>
> Anyway, as I said, your patch is good documentation, so I'm happy to
> apply it nevertheless.
>
> Kevin
I have misinterpreted this.

Actually I think then the limit should be MAX() rather then MIN()
as the stack is ready to size_t transfers. In the other case
there is no need at all to use this construction. INT_MAX will be
always less than SIZE_MAX. I do not know any platform
where this is violated.

Den

Re: [Qemu-devel] [PATCH 1/1] nbd: fix max_discard/max_transfer_length

[Peter Lieven]

Am 06.02.2015 um 13:17 schrieb Denis V. Lunev:
> On 06/02/15 15:07, Kevin Wolf wrote:
>> Am 06.02.2015 um 12:59 hat Denis V. Lunev geschrieben:
>>> On 06/02/15 14:53, Kevin Wolf wrote:
>>>> Am 06.02.2015 um 12:24 hat Denis V. Lunev geschrieben:
>>>>> nbd_co_discard calls nbd_client_session_co_discard which uses uint32_t
>>>>> as the length in bytes of the data to discard due to the following
>>>>> definition:
>>>>>
>>>>> struct nbd_request {
>>>>>      uint32_t magic;
>>>>>      uint32_t type;
>>>>>      uint64_t handle;
>>>>>      uint64_t from;
>>>>>      uint32_t len; <-- the length of data to be discarded, in bytes
>>>>> } QEMU_PACKED;
>>>>>
>>>>> Thus we should limit bl_max_discard to UINT32_MAX >> BDRV_SECTOR_BITS to
>>>>> avoid overflow.
>>>>>
>>>>> NBD read/write code uses the same structure for transfers. Fix
>>>>> max_transfer_length accordingly.
>>>>>
>>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>>> CC: Peter Lieven <pl@kamp.de>
>>>>> CC: Kevin Wolf <kwolf@redhat.com>
>>>> Thanks, I have applied both Peter's and your patch. Can you guys please
>>>> check whether the current state of my block branch is correct or whether
>>>> I forgot to include or remove some patch?
>>> can you give me tree URL?
>> Sure:
>>
>> git: git://repo.or.cz/qemu/kevin.git block
>> Web: http://repo.or.cz/w/qemu/kevin.git/shortlog/refs/heads/block
>>
>>>> By the way, I don't think this NBD patch is strictly necessary as you'll
>>>> have a hard time finding a platform where INT_MAX > UINT32_MAX, but I
>>>> think it's good documentation at least and a safeguard if we ever decide
>>>> to lift the general block layer restrictions.
>>>>
>>>> Kevin
>>> nope, it is absolutely mandatory
>>>
>>> stdint.h:
>>>
>>> /* Limit of `size_t' type.  */
>>> # if __WORDSIZE == 64
>>> #  define SIZE_MAX              (18446744073709551615UL)
>>> # else
>>> #  define SIZE_MAX              (4294967295U)
>>> # endif
>> But Peter defined it like this:
>>
>> #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
>>                                       INT_MAX >> BDRV_SECTOR_BITS)
>>
>> And having integers with more the 32 bits is at least unusual. I don't
>> know of any platform that has them.
>>
>> Anyway, as I said, your patch is good documentation, so I'm happy to
>> apply it nevertheless.
>>
>> Kevin
> I have misinterpreted this.
>
> Actually I think then the limit should be MAX() rather then MIN()
> as the stack is ready to size_t transfers. In the other case
> there is no need at all to use this construction. INT_MAX will be
> always less than SIZE_MAX. I do not know any platform
> where this is violated.

That doesn't work. All internal routines have int (32-bit) as type for nb_sectors
whereas size_t is often 64-bit.

I also think that INT_MAX is always less than SIZE_MAX, but I would leave it
in just to be absolutely sure. Its evaluated at compile time and will not
hurt.

Peter

Re: [Qemu-devel] [PATCH 1/1] nbd: fix max_discard/max_transfer_length

[Denis V. Lunev]

On 06/02/15 15:22, Peter Lieven wrote:
> Am 06.02.2015 um 13:17 schrieb Denis V. Lunev:
>> On 06/02/15 15:07, Kevin Wolf wrote:
>>> Am 06.02.2015 um 12:59 hat Denis V. Lunev geschrieben:
>>>> On 06/02/15 14:53, Kevin Wolf wrote:
>>>>> Am 06.02.2015 um 12:24 hat Denis V. Lunev geschrieben:
>>>>>> nbd_co_discard calls nbd_client_session_co_discard which uses uint32_t
>>>>>> as the length in bytes of the data to discard due to the following
>>>>>> definition:
>>>>>>
>>>>>> struct nbd_request {
>>>>>>       uint32_t magic;
>>>>>>       uint32_t type;
>>>>>>       uint64_t handle;
>>>>>>       uint64_t from;
>>>>>>       uint32_t len; <-- the length of data to be discarded, in bytes
>>>>>> } QEMU_PACKED;
>>>>>>
>>>>>> Thus we should limit bl_max_discard to UINT32_MAX >> BDRV_SECTOR_BITS to
>>>>>> avoid overflow.
>>>>>>
>>>>>> NBD read/write code uses the same structure for transfers. Fix
>>>>>> max_transfer_length accordingly.
>>>>>>
>>>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>>>> CC: Peter Lieven <pl@kamp.de>
>>>>>> CC: Kevin Wolf <kwolf@redhat.com>
>>>>> Thanks, I have applied both Peter's and your patch. Can you guys please
>>>>> check whether the current state of my block branch is correct or whether
>>>>> I forgot to include or remove some patch?
>>>> can you give me tree URL?
>>> Sure:
>>>
>>> git: git://repo.or.cz/qemu/kevin.git block
>>> Web: http://repo.or.cz/w/qemu/kevin.git/shortlog/refs/heads/block
>>>
>>>>> By the way, I don't think this NBD patch is strictly necessary as you'll
>>>>> have a hard time finding a platform where INT_MAX > UINT32_MAX, but I
>>>>> think it's good documentation at least and a safeguard if we ever decide
>>>>> to lift the general block layer restrictions.
>>>>>
>>>>> Kevin
>>>> nope, it is absolutely mandatory
>>>>
>>>> stdint.h:
>>>>
>>>> /* Limit of `size_t' type.  */
>>>> # if __WORDSIZE == 64
>>>> #  define SIZE_MAX              (18446744073709551615UL)
>>>> # else
>>>> #  define SIZE_MAX              (4294967295U)
>>>> # endif
>>> But Peter defined it like this:
>>>
>>> #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
>>>                                        INT_MAX >> BDRV_SECTOR_BITS)
>>>
>>> And having integers with more the 32 bits is at least unusual. I don't
>>> know of any platform that has them.
>>>
>>> Anyway, as I said, your patch is good documentation, so I'm happy to
>>> apply it nevertheless.
>>>
>>> Kevin
>> I have misinterpreted this.
>>
>> Actually I think then the limit should be MAX() rather then MIN()
>> as the stack is ready to size_t transfers. In the other case
>> there is no need at all to use this construction. INT_MAX will be
>> always less than SIZE_MAX. I do not know any platform
>> where this is violated.
> That doesn't work. All internal routines have int (32-bit) as type for nb_sectors
> whereas size_t is often 64-bit.
>
> I also think that INT_MAX is always less than SIZE_MAX, but I would leave it
> in just to be absolutely sure. Its evaluated at compile time and will not
> hurt.
>
> Peter
>
OK :) let it be. Staying on safe side is always good.

Kevin, all my stuff we have agreed on is applied.

Regards,
     Den

Re: [Qemu-devel] [PATCH 1/1] nbd: fix max_discard/max_transfer_length

[Peter Lieven]

Am 06.02.2015 um 12:53 schrieb Kevin Wolf:
> Am 06.02.2015 um 12:24 hat Denis V. Lunev geschrieben:
>> nbd_co_discard calls nbd_client_session_co_discard which uses uint32_t
>> as the length in bytes of the data to discard due to the following
>> definition:
>>
>> struct nbd_request {
>>     uint32_t magic;
>>     uint32_t type;
>>     uint64_t handle;
>>     uint64_t from;
>>     uint32_t len; <-- the length of data to be discarded, in bytes
>> } QEMU_PACKED;
>>
>> Thus we should limit bl_max_discard to UINT32_MAX >> BDRV_SECTOR_BITS to
>> avoid overflow.
>>
>> NBD read/write code uses the same structure for transfers. Fix
>> max_transfer_length accordingly.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Peter Lieven <pl@kamp.de>
>> CC: Kevin Wolf <kwolf@redhat.com>
> Thanks, I have applied both Peter's and your patch. Can you guys please
> check whether the current state of my block branch is correct or whether
> I forgot to include or remove some patch?

Looks good from my point of view.

Just to be sure has it to be

if (size > BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)

or

if (size > (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS))

?

If the latter is right, can you please fix that line in my patch. I am afk now.

Thanks,
Peter

Re: [Qemu-devel] [PATCH 1/1] nbd: fix max_discard/max_transfer_length

[Kevin Wolf]

Am 06.02.2015 um 13:16 hat Peter Lieven geschrieben:
> Am 06.02.2015 um 12:53 schrieb Kevin Wolf:
> > Am 06.02.2015 um 12:24 hat Denis V. Lunev geschrieben:
> >> nbd_co_discard calls nbd_client_session_co_discard which uses uint32_t
> >> as the length in bytes of the data to discard due to the following
> >> definition:
> >>
> >> struct nbd_request {
> >>     uint32_t magic;
> >>     uint32_t type;
> >>     uint64_t handle;
> >>     uint64_t from;
> >>     uint32_t len; <-- the length of data to be discarded, in bytes
> >> } QEMU_PACKED;
> >>
> >> Thus we should limit bl_max_discard to UINT32_MAX >> BDRV_SECTOR_BITS to
> >> avoid overflow.
> >>
> >> NBD read/write code uses the same structure for transfers. Fix
> >> max_transfer_length accordingly.
> >>
> >> Signed-off-by: Denis V. Lunev <den@openvz.org>
> >> CC: Peter Lieven <pl@kamp.de>
> >> CC: Kevin Wolf <kwolf@redhat.com>
> > Thanks, I have applied both Peter's and your patch. Can you guys please
> > check whether the current state of my block branch is correct or whether
> > I forgot to include or remove some patch?
> 
> Looks good from my point of view.
> 
> Just to be sure has it to be
> 
> if (size > BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
> 
> or
> 
> if (size > (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS))
> 
> ?
> 
> If the latter is right, can you please fix that line in my patch. I am afk now.

Both versions are correct.

Kevin

Re: [Qemu-devel] [PATCH V2] block: introduce BDRV_REQUEST_MAX_SECTORS

[Denis V. Lunev]

On 06/02/15 13:54, Peter Lieven wrote:
> we check and adjust request sizes at several places with
> sometimes inconsistent checks or default values:
>   INT_MAX
>   INT_MAX >> BDRV_SECTOR_BITS
>   UINT_MAX >> BDRV_SECTOR_BITS
>   SIZE_MAX >> BDRV_SECTOR_BITS
>
> This patches introdocues a macro for the maximal allowed sectors
> per request and uses it at several places.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
> v1->v2: use macro in bdrv_check_byte_request alse [Den]
>
>   block.c               |   21 +++++++++------------
>   hw/block/virtio-blk.c |    4 ++--
>   include/block/block.h |    3 +++
>   3 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/block.c b/block.c
> index 8272ef9..49e0073 100644
> --- a/block.c
> +++ b/block.c
> @@ -2647,7 +2647,7 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
>   {
>       int64_t len;
>   
> -    if (size > INT_MAX) {
> +    if (size > BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) {
>           return -EIO;
>       }
>   
> @@ -2671,7 +2671,7 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
>   static int bdrv_check_request(BlockDriverState *bs, int64_t sector_num,
>                                 int nb_sectors)
>   {
> -    if (nb_sectors < 0 || nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) {
> +    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
>           return -EIO;
>       }
>   
> @@ -2758,7 +2758,7 @@ static int bdrv_rw_co(BlockDriverState *bs, int64_t sector_num, uint8_t *buf,
>           .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
>       };
>   
> -    if (nb_sectors < 0 || nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) {
> +    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
>           return -EINVAL;
>       }
>   
> @@ -2826,13 +2826,10 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
>       }
>   
>       for (;;) {
> -        nb_sectors = target_sectors - sector_num;
> +        nb_sectors = MIN(target_sectors - sector_num, BDRV_REQUEST_MAX_SECTORS);
>           if (nb_sectors <= 0) {
>               return 0;
>           }
> -        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) {
>               error_report("error getting block status at sector %" PRId64 ": %s",
> @@ -3167,7 +3164,7 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
>       int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
>       BdrvRequestFlags flags)
>   {
> -    if (nb_sectors < 0 || nb_sectors > (UINT_MAX >> BDRV_SECTOR_BITS)) {
> +    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
>           return -EINVAL;
>       }
>   
> @@ -3202,8 +3199,8 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>       struct iovec iov = {0};
>       int ret = 0;
>   
> -    int max_write_zeroes = bs->bl.max_write_zeroes ?
> -                           bs->bl.max_write_zeroes : INT_MAX;
> +    int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_write_zeroes,
> +                                        BDRV_REQUEST_MAX_SECTORS);
>   
>       while (nb_sectors > 0 && !ret) {
>           int num = nb_sectors;
> @@ -3458,7 +3455,7 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
>       int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
>       BdrvRequestFlags flags)
>   {
> -    if (nb_sectors < 0 || nb_sectors > (INT_MAX >> BDRV_SECTOR_BITS)) {
> +    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
>           return -EINVAL;
>       }
>   
> @@ -5120,7 +5117,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>           return 0;
>       }
>   
> -    max_discard = bs->bl.max_discard ?  bs->bl.max_discard : INT_MAX;
> +    max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
>       while (nb_sectors > 0) {
>           int ret;
>           int num = nb_sectors;
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 8c51a29..1a8a176 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -381,7 +381,7 @@ void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb)
>       }
>   
>       max_xfer_len = blk_get_max_transfer_length(mrb->reqs[0]->dev->blk);
> -    max_xfer_len = MIN_NON_ZERO(max_xfer_len, INT_MAX);
> +    max_xfer_len = MIN_NON_ZERO(max_xfer_len, BDRV_REQUEST_MAX_SECTORS);
>   
>       qsort(mrb->reqs, mrb->num_reqs, sizeof(*mrb->reqs),
>             &multireq_compare);
> @@ -447,7 +447,7 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
>       uint64_t nb_sectors = size >> BDRV_SECTOR_BITS;
>       uint64_t total_sectors;
>   
> -    if (nb_sectors > INT_MAX) {
> +    if (nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
>           return false;
>       }
>       if (sector & dev->sector_mask) {
> diff --git a/include/block/block.h b/include/block/block.h
> index 3082d2b..25a6d62 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -83,6 +83,9 @@ typedef enum {
>   #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
>   #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
>   
> +#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
> +                                     INT_MAX >> BDRV_SECTOR_BITS)
> +
>   /*
>    * Allocation status flags
>    * BDRV_BLOCK_DATA: data is read from bs->file or another file
Reviewed-by: Denis V. Lunev <den@openvz.org>