[Qemu-devel] [PATCH v5 0/3] Fix overflow bug in qcow2 discard

[Qemu-devel] [PATCH v5 0/3] Fix overflow bug in qcow2 discard

[Vladimir Sementsov-Ogievskiy]

v5: by Kevin's comments
02: add check for invalid @bytes parameter, drop r-b
03: move from qemu-img info to qemu-img map -f raw, drop r-b and t-b

Vladimir Sementsov-Ogievskiy (3):
  block/qcow2-refcount: add trace-point to qcow2_process_discards
  block/io: bdrv_pdiscard: support int64_t bytes parameter
  iotests: test big qcow2 shrink

 include/block/block.h      |  4 +--
 block/io.c                 | 16 ++++-----
 block/qcow2-refcount.c     |  7 +++-
 block/trace-events         |  3 ++
 tests/qemu-iotests/250     | 73 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/250.out | 23 ++++++++++++
 tests/qemu-iotests/group   |  1 +
 7 files changed, 116 insertions(+), 11 deletions(-)
 create mode 100755 tests/qemu-iotests/250
 create mode 100644 tests/qemu-iotests/250.out

-- 
2.18.0

[Qemu-devel] [PATCH v5 0/3] Fix overflow bug in qcow2 discard

[Vladimir Sementsov-Ogievskiy]

v5: by Kevin's comments
02: add check for invalid @bytes parameter, drop r-b
03: move from qemu-img info to qemu-img map -f raw, drop r-b and t-b

Vladimir Sementsov-Ogievskiy (3):
  block/qcow2-refcount: add trace-point to qcow2_process_discards
  block/io: bdrv_pdiscard: support int64_t bytes parameter
  iotests: test big qcow2 shrink

 include/block/block.h      |  4 +--
 block/io.c                 | 16 ++++-----
 block/qcow2-refcount.c     |  7 +++-
 block/trace-events         |  3 ++
 tests/qemu-iotests/250     | 73 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/250.out | 23 ++++++++++++
 tests/qemu-iotests/group   |  1 +
 7 files changed, 116 insertions(+), 11 deletions(-)
 create mode 100755 tests/qemu-iotests/250
 create mode 100644 tests/qemu-iotests/250.out

-- 
2.18.0

[Qemu-devel] [PATCH v5 1/3] block/qcow2-refcount: add trace-point to qcow2_process_discards

[Vladimir Sementsov-Ogievskiy]

Let's at least trace ignored failure.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/qcow2-refcount.c | 7 ++++++-
 block/trace-events     | 3 +++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index e0fe322500..60284bcaac 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -30,6 +30,7 @@
 #include "qemu/range.h"
 #include "qemu/bswap.h"
 #include "qemu/cutils.h"
+#include "trace.h"
 
 static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size,
                                     uint64_t max);
@@ -738,7 +739,11 @@ void qcow2_process_discards(BlockDriverState *bs, int ret)
 
         /* Discard is optional, ignore the return value */
         if (ret >= 0) {
-            bdrv_pdiscard(bs->file, d->offset, d->bytes);
+            int r2 = bdrv_pdiscard(bs->file, d->offset, d->bytes);
+            if (r2 < 0) {
+                trace_qcow2_process_discards_failed_region(d->offset, d->bytes,
+                                                           r2);
+            }
         }
 
         g_free(d);
diff --git a/block/trace-events b/block/trace-events
index 7335a42540..ea508f637e 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -91,6 +91,9 @@ qcow2_cache_get_done(void *co, int c, int i) "co %p is_l2_cache %d index %d"
 qcow2_cache_flush(void *co, int c) "co %p is_l2_cache %d"
 qcow2_cache_entry_flush(void *co, int c, int i) "co %p is_l2_cache %d index %d"
 
+# qcow2-refcount.c
+qcow2_process_discards_failed_region(uint64_t offset, uint64_t bytes, int ret) "offset 0x%" PRIx64 " bytes 0x%" PRIx64 " ret %d"
+
 # qed-l2-cache.c
 qed_alloc_l2_cache_entry(void *l2_cache, void *entry) "l2_cache %p entry %p"
 qed_unref_l2_cache_entry(void *entry, int ref) "entry %p ref %d"
-- 
2.18.0

[Qemu-devel] [PATCH v5 1/3] block/qcow2-refcount: add trace-point to qcow2_process_discards

[Vladimir Sementsov-Ogievskiy]

Let's at least trace ignored failure.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/qcow2-refcount.c | 7 ++++++-
 block/trace-events     | 3 +++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index e0fe322500..60284bcaac 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -30,6 +30,7 @@
 #include "qemu/range.h"
 #include "qemu/bswap.h"
 #include "qemu/cutils.h"
+#include "trace.h"
 
 static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size,
                                     uint64_t max);
@@ -738,7 +739,11 @@ void qcow2_process_discards(BlockDriverState *bs, int ret)
 
         /* Discard is optional, ignore the return value */
         if (ret >= 0) {
-            bdrv_pdiscard(bs->file, d->offset, d->bytes);
+            int r2 = bdrv_pdiscard(bs->file, d->offset, d->bytes);
+            if (r2 < 0) {
+                trace_qcow2_process_discards_failed_region(d->offset, d->bytes,
+                                                           r2);
+            }
         }
 
         g_free(d);
diff --git a/block/trace-events b/block/trace-events
index 7335a42540..ea508f637e 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -91,6 +91,9 @@ qcow2_cache_get_done(void *co, int c, int i) "co %p is_l2_cache %d index %d"
 qcow2_cache_flush(void *co, int c) "co %p is_l2_cache %d"
 qcow2_cache_entry_flush(void *co, int c, int i) "co %p is_l2_cache %d index %d"
 
+# qcow2-refcount.c
+qcow2_process_discards_failed_region(uint64_t offset, uint64_t bytes, int ret) "offset 0x%" PRIx64 " bytes 0x%" PRIx64 " ret %d"
+
 # qed-l2-cache.c
 qed_alloc_l2_cache_entry(void *l2_cache, void *entry) "l2_cache %p entry %p"
 qed_unref_l2_cache_entry(void *entry, int ref) "entry %p ref %d"
-- 
2.18.0

[Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Vladimir Sementsov-Ogievskiy]

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)) {
         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 = {
-- 
2.18.0

[Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Vladimir Sementsov-Ogievskiy]

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)) {
         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 = {
-- 
2.18.0

Re: [Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Stefano Garzarella]

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

Re: [Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Stefano Garzarella]

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

Re: [Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Vladimir Sementsov-Ogievskiy]

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.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Vladimir Sementsov-Ogievskiy]

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.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Eric Blake]

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

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>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Eric Blake]

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

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>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Stefano Garzarella]

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

Re: [Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Stefano Garzarella]

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

Re: [Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Vladimir Sementsov-Ogievskiy]

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!


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Stefano Garzarella]

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

Re: [Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Stefano Garzarella]

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

[Qemu-devel] [PATCH v5 3/3] iotests: test big qcow2 shrink

[Vladimir Sementsov-Ogievskiy]

This test checks bug in qcow2_process_discards, fixed by previous
commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/250     | 73 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/250.out | 23 ++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 97 insertions(+)
 create mode 100755 tests/qemu-iotests/250
 create mode 100644 tests/qemu-iotests/250.out

diff --git a/tests/qemu-iotests/250 b/tests/qemu-iotests/250
new file mode 100755
index 0000000000..850e3546ca
--- /dev/null
+++ b/tests/qemu-iotests/250
@@ -0,0 +1,73 @@
+#!/usr/bin/env bash
+#
+# Test big discard in qcow2 shrink
+#
+# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=vsementsov@virtuozzo.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+# This test checks that qcow2_process_discards does not truncate a discard
+# request > 2G.
+# To reproduce bug we need to overflow int by one sequential discard, so we
+# need size > 2G, bigger cluster size (as with default 64k we may have maximum
+# of 512M sequential data, corresponding to one L1 entry), and we need some
+# data of the beginning of the disk mapped to the end of file to prevent
+# bdrv_co_truncate(bs->file) call in qcow2_co_truncate(), which might succeed
+# anyway.
+
+size=2100M
+IMGOPTS="cluster_size=1M,preallocation=metadata"
+
+_make_test_img $size
+$QEMU_IO -c 'discard 0 10M' -c 'discard 2090M 10M' \
+         -c 'write 2090M 10M' -c 'write 0 10M' "$TEST_IMG" | _filter_qemu_io
+
+# Check that our trick with swapping first and last 10M chunks succeeded.
+# Otherwise test may pass even if bdrv_pdiscard() fails in
+# qcow2_process_discards()
+$QEMU_IMG map "$TEST_IMG" | _filter_testdir
+
+$QEMU_IMG map -f raw "$TEST_IMG" |  _filter_testdir
+
+$QEMU_IMG resize --shrink "$TEST_IMG" 5M
+
+$QEMU_IMG map -f raw "$TEST_IMG" | _filter_testdir
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/250.out b/tests/qemu-iotests/250.out
new file mode 100644
index 0000000000..49baee5eee
--- /dev/null
+++ b/tests/qemu-iotests/250.out
@@ -0,0 +1,23 @@
+QA output created by 250
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 preallocation=metadata
+discard 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 10485760/10485760 bytes at offset 2191523840
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 10485760/10485760 bytes at offset 2191523840
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset          Length          Mapped to       File
+0               0xa00000        0x82f00000      TEST_DIR/t.qcow2
+0x82a00000      0xa00000        0x500000        TEST_DIR/t.qcow2
+Offset          Length          Mapped to       File
+0               0x301000        0               TEST_DIR/t.qcow2
+0x400000        0xb00000        0x400000        TEST_DIR/t.qcow2
+0x82f00000      0xa00000        0x82f00000      TEST_DIR/t.qcow2
+Image resized.
+Offset          Length          Mapped to       File
+0               0x301000        0               TEST_DIR/t.qcow2
+0x400000        0x100000        0x400000        TEST_DIR/t.qcow2
+0x82f00000      0x500000        0x82f00000      TEST_DIR/t.qcow2
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index bae7718380..588ae8b8b1 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -248,3 +248,4 @@
 246 rw auto quick
 247 rw auto quick
 248 rw auto quick
+250 rw auto quick
-- 
2.18.0

[Qemu-devel] [PATCH v5 3/3] iotests: test big qcow2 shrink

[Vladimir Sementsov-Ogievskiy]

This test checks bug in qcow2_process_discards, fixed by previous
commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/250     | 73 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/250.out | 23 ++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 97 insertions(+)
 create mode 100755 tests/qemu-iotests/250
 create mode 100644 tests/qemu-iotests/250.out

diff --git a/tests/qemu-iotests/250 b/tests/qemu-iotests/250
new file mode 100755
index 0000000000..850e3546ca
--- /dev/null
+++ b/tests/qemu-iotests/250
@@ -0,0 +1,73 @@
+#!/usr/bin/env bash
+#
+# Test big discard in qcow2 shrink
+#
+# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=vsementsov@virtuozzo.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+# This test checks that qcow2_process_discards does not truncate a discard
+# request > 2G.
+# To reproduce bug we need to overflow int by one sequential discard, so we
+# need size > 2G, bigger cluster size (as with default 64k we may have maximum
+# of 512M sequential data, corresponding to one L1 entry), and we need some
+# data of the beginning of the disk mapped to the end of file to prevent
+# bdrv_co_truncate(bs->file) call in qcow2_co_truncate(), which might succeed
+# anyway.
+
+size=2100M
+IMGOPTS="cluster_size=1M,preallocation=metadata"
+
+_make_test_img $size
+$QEMU_IO -c 'discard 0 10M' -c 'discard 2090M 10M' \
+         -c 'write 2090M 10M' -c 'write 0 10M' "$TEST_IMG" | _filter_qemu_io
+
+# Check that our trick with swapping first and last 10M chunks succeeded.
+# Otherwise test may pass even if bdrv_pdiscard() fails in
+# qcow2_process_discards()
+$QEMU_IMG map "$TEST_IMG" | _filter_testdir
+
+$QEMU_IMG map -f raw "$TEST_IMG" |  _filter_testdir
+
+$QEMU_IMG resize --shrink "$TEST_IMG" 5M
+
+$QEMU_IMG map -f raw "$TEST_IMG" | _filter_testdir
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/250.out b/tests/qemu-iotests/250.out
new file mode 100644
index 0000000000..49baee5eee
--- /dev/null
+++ b/tests/qemu-iotests/250.out
@@ -0,0 +1,23 @@
+QA output created by 250
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 preallocation=metadata
+discard 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 10485760/10485760 bytes at offset 2191523840
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 10485760/10485760 bytes at offset 2191523840
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset          Length          Mapped to       File
+0               0xa00000        0x82f00000      TEST_DIR/t.qcow2
+0x82a00000      0xa00000        0x500000        TEST_DIR/t.qcow2
+Offset          Length          Mapped to       File
+0               0x301000        0               TEST_DIR/t.qcow2
+0x400000        0xb00000        0x400000        TEST_DIR/t.qcow2
+0x82f00000      0xa00000        0x82f00000      TEST_DIR/t.qcow2
+Image resized.
+Offset          Length          Mapped to       File
+0               0x301000        0               TEST_DIR/t.qcow2
+0x400000        0x100000        0x400000        TEST_DIR/t.qcow2
+0x82f00000      0x500000        0x82f00000      TEST_DIR/t.qcow2
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index bae7718380..588ae8b8b1 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -248,3 +248,4 @@
 246 rw auto quick
 247 rw auto quick
 248 rw auto quick
+250 rw auto quick
-- 
2.18.0

Re: [Qemu-devel] [PATCH v5 0/3] Fix overflow bug in qcow2 discard

[Vladimir Sementsov-Ogievskiy]

ping

23.04.2019 15:57, Vladimir Sementsov-Ogievskiy wrote:
> v5: by Kevin's comments
> 02: add check for invalid @bytes parameter, drop r-b
> 03: move from qemu-img info to qemu-img map -f raw, drop r-b and t-b
> 
> Vladimir Sementsov-Ogievskiy (3):
>    block/qcow2-refcount: add trace-point to qcow2_process_discards
>    block/io: bdrv_pdiscard: support int64_t bytes parameter
>    iotests: test big qcow2 shrink
> 
>   include/block/block.h      |  4 +--
>   block/io.c                 | 16 ++++-----
>   block/qcow2-refcount.c     |  7 +++-
>   block/trace-events         |  3 ++
>   tests/qemu-iotests/250     | 73 ++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/250.out | 23 ++++++++++++
>   tests/qemu-iotests/group   |  1 +
>   7 files changed, 116 insertions(+), 11 deletions(-)
>   create mode 100755 tests/qemu-iotests/250
>   create mode 100644 tests/qemu-iotests/250.out
> 


-- 
Best regards,
Vladimir

[Qemu-devel] ping Re: [PATCH v5 0/3] Fix overflow bug in qcow2 discard

[Vladimir Sementsov-Ogievskiy]

ping 21.05.2019 12:38, Vladimir Sementsov-Ogievskiy wrote:
> ping
> 
> 23.04.2019 15:57, Vladimir Sementsov-Ogievskiy wrote:
>> v5: by Kevin's comments
>> 02: add check for invalid @bytes parameter, drop r-b
>> 03: move from qemu-img info to qemu-img map -f raw, drop r-b and t-b
>>
>> Vladimir Sementsov-Ogievskiy (3):
>>    block/qcow2-refcount: add trace-point to qcow2_process_discards
>>    block/io: bdrv_pdiscard: support int64_t bytes parameter
>>    iotests: test big qcow2 shrink
>>
>>   include/block/block.h      |  4 +--
>>   block/io.c                 | 16 ++++-----
>>   block/qcow2-refcount.c     |  7 +++-
>>   block/trace-events         |  3 ++
>>   tests/qemu-iotests/250     | 73 ++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/250.out | 23 ++++++++++++
>>   tests/qemu-iotests/group   |  1 +
>>   7 files changed, 116 insertions(+), 11 deletions(-)
>>   create mode 100755 tests/qemu-iotests/250
>>   create mode 100644 tests/qemu-iotests/250.out
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 0/3] Fix overflow bug in qcow2 discard

[Kevin Wolf]

Am 23.04.2019 um 14:57 hat Vladimir Sementsov-Ogievskiy geschrieben:
> v5: by Kevin's comments
> 02: add check for invalid @bytes parameter, drop r-b
> 03: move from qemu-img info to qemu-img map -f raw, drop r-b and t-b

The patches look good to me, but the test case fails (this is on XFS; it
does work on tmpfs):

--- /home/kwolf/source/qemu/tests/qemu-iotests/250.out  2019-06-03 15:22:51.212961123 +0200
+++ /home/kwolf/source/qemu/tests/qemu-iotests/250.out.bad      2019-06-03 15:37:37.202959101 +0200
@@ -12,12 +12,10 @@
 0               0xa00000        0x82f00000      TEST_DIR/t.qcow2
 0x82a00000      0xa00000        0x500000        TEST_DIR/t.qcow2
 Offset          Length          Mapped to       File
-0               0x301000        0               TEST_DIR/t.qcow2
-0x400000        0xb00000        0x400000        TEST_DIR/t.qcow2
+0               0xf00000        0               TEST_DIR/t.qcow2
 0x82f00000      0xa00000        0x82f00000      TEST_DIR/t.qcow2
 Image resized.
 Offset          Length          Mapped to       File
-0               0x301000        0               TEST_DIR/t.qcow2
-0x400000        0x100000        0x400000        TEST_DIR/t.qcow2
+0               0x500000        0               TEST_DIR/t.qcow2
 0x82f00000      0x500000        0x82f00000      TEST_DIR/t.qcow2
 *** done

I'll apply the first two patches without the test for now, but please
try if you can tweak the test case to work on all common filesystems.

Kevin

Re: [Qemu-devel] [PATCH v5 0/3] Fix overflow bug in qcow2 discard

[Vladimir Sementsov-Ogievskiy]

03.06.2019 16:40, Kevin Wolf wrote:
> Am 23.04.2019 um 14:57 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> v5: by Kevin's comments
>> 02: add check for invalid @bytes parameter, drop r-b
>> 03: move from qemu-img info to qemu-img map -f raw, drop r-b and t-b
> 
> The patches look good to me, but the test case fails (this is on XFS; it
> does work on tmpfs):
> 
> --- /home/kwolf/source/qemu/tests/qemu-iotests/250.out  2019-06-03 15:22:51.212961123 +0200
> +++ /home/kwolf/source/qemu/tests/qemu-iotests/250.out.bad      2019-06-03 15:37:37.202959101 +0200
> @@ -12,12 +12,10 @@
>   0               0xa00000        0x82f00000      TEST_DIR/t.qcow2
>   0x82a00000      0xa00000        0x500000        TEST_DIR/t.qcow2
>   Offset          Length          Mapped to       File
> -0               0x301000        0               TEST_DIR/t.qcow2
> -0x400000        0xb00000        0x400000        TEST_DIR/t.qcow2
> +0               0xf00000        0               TEST_DIR/t.qcow2
>   0x82f00000      0xa00000        0x82f00000      TEST_DIR/t.qcow2
>   Image resized.
>   Offset          Length          Mapped to       File
> -0               0x301000        0               TEST_DIR/t.qcow2
> -0x400000        0x100000        0x400000        TEST_DIR/t.qcow2
> +0               0x500000        0               TEST_DIR/t.qcow2
>   0x82f00000      0x500000        0x82f00000      TEST_DIR/t.qcow2
>   *** done
> 
> I'll apply the first two patches without the test for now, but please
> try if you can tweak the test case to work on all common filesystems.
> 
> Kevin
> 

Thank you! OK, I'll try

-- 
Best regards,
Vladimir