[Qemu-devel] [PATCH v1 0/3] qcow2: fix bugs when cluster size is larger than the default value

[Qemu-devel] [PATCH v1 0/3] qcow2: fix bugs when cluster size is larger than the default value

[Hu Tao]

See each patches for details.

Hu Tao (3):
  qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()
  qcow2: fix offset overflow
  qcow2: check for NULL l2meta

 block/qcow2-cluster.c  | 14 ++++++--------
 block/qcow2-refcount.c |  7 ++++++-
 block/qcow2.c          | 20 +++++++++++---------
 block/qcow2.h          |  2 +-
 trace-events           |  2 +-
 5 files changed, 25 insertions(+), 20 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH v1 1/3] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()

[Hu Tao]

n_start can be actually calculated from offset. The number of
sectors to be allocated(n_end - n_start) can be passed in in
num. By removing n_start and n_end, we can save two parameters.

The side effect is there is a bug in qcow2.c:preallocate() that
passes incorrect n_start to qcow2_alloc_cluster_offset() is
fixed. The bug can be triggerred by a larger cluster size than
the default value(65536), for example:

./qemu-img create -f qcow2 \
  -o 'cluster_size=131072,preallocation=metadata' file.img 4G

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2-cluster.c | 14 ++++++--------
 block/qcow2.c         |  6 +++---
 block/qcow2.h         |  2 +-
 trace-events          |  2 +-
 4 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 8534084..be33781 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1182,7 +1182,7 @@ fail:
  * Return 0 on success and -errno in error cases
  */
 int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
-    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m)
+    int *num, uint64_t *host_offset, QCowL2Meta **m)
 {
     BDRVQcowState *s = bs->opaque;
     uint64_t start, remaining;
@@ -1190,15 +1190,13 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
     uint64_t cur_bytes;
     int ret;
 
-    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset,
-                                      n_start, n_end);
+    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, *num);
 
-    assert(n_start * BDRV_SECTOR_SIZE == offset_into_cluster(s, offset));
-    offset = start_of_cluster(s, offset);
+    assert((offset & ~BDRV_SECTOR_MASK) == 0);
 
 again:
-    start = offset + (n_start << BDRV_SECTOR_BITS);
-    remaining = (n_end - n_start) << BDRV_SECTOR_BITS;
+    start = offset;
+    remaining = (*num) << BDRV_SECTOR_BITS;
     cluster_offset = 0;
     *host_offset = 0;
     cur_bytes = 0;
@@ -1284,7 +1282,7 @@ again:
         }
     }
 
-    *num = (n_end - n_start) - (remaining >> BDRV_SECTOR_BITS);
+    *num = *num - (remaining >> BDRV_SECTOR_BITS);
     assert(*num > 0);
     assert(*host_offset != 0);
 
diff --git a/block/qcow2.c b/block/qcow2.c
index f29aa88..46860d5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1016,14 +1016,14 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
 
         trace_qcow2_writev_start_part(qemu_coroutine_self());
         index_in_cluster = sector_num & (s->cluster_sectors - 1);
-        n_end = index_in_cluster + remaining_sectors;
+        cur_nr_sectors = remaining_sectors;
         if (s->crypt_method &&
             n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors) {
             n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
         }
 
         ret = qcow2_alloc_cluster_offset(bs, sector_num << 9,
-            index_in_cluster, n_end, &cur_nr_sectors, &cluster_offset, &l2meta);
+            &cur_nr_sectors, &cluster_offset, &l2meta);
         if (ret < 0) {
             goto fail;
         }
@@ -1400,7 +1400,7 @@ static int preallocate(BlockDriverState *bs)
 
     while (nb_sectors) {
         num = MIN(nb_sectors, INT_MAX >> 9);
-        ret = qcow2_alloc_cluster_offset(bs, offset, 0, num, &num,
+        ret = qcow2_alloc_cluster_offset(bs, offset, &num,
                                          &host_offset, &meta);
         if (ret < 0) {
             return ret;
diff --git a/block/qcow2.h b/block/qcow2.h
index 303eb26..84e1344 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -468,7 +468,7 @@ void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
 int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
     int *num, uint64_t *cluster_offset);
 int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
-    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m);
+    int *num, uint64_t *host_offset, QCowL2Meta **m);
 uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
                                          uint64_t offset,
                                          int compressed_size);
diff --git a/trace-events b/trace-events
index 9f4456a..9b4e586 100644
--- a/trace-events
+++ b/trace-events
@@ -494,7 +494,7 @@ qcow2_writev_done_part(void *co, int cur_nr_sectors) "co %p cur_nr_sectors %d"
 qcow2_writev_data(void *co, uint64_t offset) "co %p offset %" PRIx64
 
 # block/qcow2-cluster.c
-qcow2_alloc_clusters_offset(void *co, uint64_t offset, int n_start, int n_end) "co %p offet %" PRIx64 " n_start %d n_end %d"
+qcow2_alloc_clusters_offset(void *co, uint64_t offset, int num) "co %p offet %" PRIx64 " num %d"
 qcow2_handle_copied(void *co, uint64_t guest_offset, uint64_t host_offset, uint64_t bytes) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " bytes %" PRIx64
 qcow2_handle_alloc(void *co, uint64_t guest_offset, uint64_t host_offset, uint64_t bytes) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " bytes %" PRIx64
 qcow2_do_alloc_clusters_offset(void *co, uint64_t guest_offset, uint64_t host_offset, int nb_clusters) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " nb_clusters %d"
-- 
1.7.11.7

[Qemu-devel] [PATCH v1 2/3] qcow2: fix offset overflow

[Hu Tao]

When cluster size is big enough it can lead offset overflow
in qcow2_alloc_clusters_at(). This patch fixes it.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2-refcount.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index c974abe..b3ebb7f 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -676,7 +676,12 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
     BDRVQcowState *s = bs->opaque;
     uint64_t cluster_index;
     uint64_t old_free_cluster_index;
-    int i, refcount, ret;
+    uint64_t i;
+    int refcount, ret;
+
+    if (nb_clusters <= 0) {
+        return 0;
+    }
 
     /* Check how many clusters there are free */
     cluster_index = offset >> s->cluster_bits;
-- 
1.7.11.7

[Qemu-devel] [PATCH v1 3/3] qcow2: check for NULL l2meta

[Hu Tao]

In case of do preallocating metadata with a large cluster size,
qcow2_alloc_cluster_offset() can allocate nothing and returns
a NULL l2meta. This patch checks for it and link2 l2 with only
valid l2meta.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 46860d5..380c240 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs)
     offset = 0;
 
     while (nb_sectors) {
-        num = MIN(nb_sectors, INT_MAX >> 9);
+        num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS);
         ret = qcow2_alloc_cluster_offset(bs, offset, &num,
                                          &host_offset, &meta);
         if (ret < 0) {
             return ret;
         }
 
-        ret = qcow2_alloc_cluster_link_l2(bs, meta);
-        if (ret < 0) {
-            qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters,
-                                    QCOW2_DISCARD_NEVER);
-            return ret;
+        if (meta) {
+            ret = qcow2_alloc_cluster_link_l2(bs, meta);
+            if (ret < 0) {
+                qcow2_free_any_clusters(bs, meta->alloc_offset,
+                                        meta->nb_clusters, QCOW2_DISCARD_NEVER);
+                return ret;
+            }
         }
 
         /* There are no dependent requests, but we need to remove our request
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH v1 2/3] qcow2: fix offset overflow

[Hu Tao]

On Mon, Dec 30, 2013 at 01:29:08PM +0800, Hu Tao wrote:
> When cluster size is big enough it can lead offset overflow
> in qcow2_alloc_clusters_at(). This patch fixes it.

ping. and be more descriptive:

The allocation each time is stopped at L2 table boundary(see handle_alloc()),
so the possible maximum bytes could be

  2^(cluster_bits - 3 + cluster_bits)

so int is safe for cluster_bits<=17, unsafe otherwise.


> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  block/qcow2-refcount.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index c974abe..b3ebb7f 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -676,7 +676,12 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
>      BDRVQcowState *s = bs->opaque;
>      uint64_t cluster_index;
>      uint64_t old_free_cluster_index;
> -    int i, refcount, ret;
> +    uint64_t i;
> +    int refcount, ret;
> +
> +    if (nb_clusters <= 0) {
> +        return 0;
> +    }
>  
>      /* Check how many clusters there are free */
>      cluster_index = offset >> s->cluster_bits;
> -- 
> 1.7.11.7
> 

Re: [Qemu-devel] [PATCH v1 0/3] qcow2: fix bugs when cluster size is larger than the default value

[Hu Tao]

ping again

On Mon, Dec 30, 2013 at 01:29:06PM +0800, Hu Tao wrote:
> See each patches for details.
> 
> Hu Tao (3):
>   qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()
>   qcow2: fix offset overflow
>   qcow2: check for NULL l2meta
> 
>  block/qcow2-cluster.c  | 14 ++++++--------
>  block/qcow2-refcount.c |  7 ++++++-
>  block/qcow2.c          | 20 +++++++++++---------
>  block/qcow2.h          |  2 +-
>  trace-events           |  2 +-
>  5 files changed, 25 insertions(+), 20 deletions(-)
> 
> -- 
> 1.7.11.7
> 

Re: [Qemu-devel] [PATCH v1 1/3] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()

[Max Reitz]

On 30.12.2013 06:29, Hu Tao wrote:
> n_start can be actually calculated from offset. The number of
> sectors to be allocated(n_end - n_start) can be passed in in
> num. By removing n_start and n_end, we can save two parameters.
>
> The side effect is there is a bug in qcow2.c:preallocate() that
> passes incorrect n_start to qcow2_alloc_cluster_offset() is
> fixed. The bug can be triggerred by a larger cluster size than
> the default value(65536), for example:
>
> ./qemu-img create -f qcow2 \
>    -o 'cluster_size=131072,preallocation=metadata' file.img 4G
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>   block/qcow2-cluster.c | 14 ++++++--------
>   block/qcow2.c         |  6 +++---
>   block/qcow2.h         |  2 +-
>   trace-events          |  2 +-
>   4 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 8534084..be33781 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1182,7 +1182,7 @@ fail:
>    * Return 0 on success and -errno in error cases
>    */
>   int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> -    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m)
> +    int *num, uint64_t *host_offset, QCowL2Meta **m)
>   {
>       BDRVQcowState *s = bs->opaque;
>       uint64_t start, remaining;
> @@ -1190,15 +1190,13 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
>       uint64_t cur_bytes;
>       int ret;
>   
> -    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset,
> -                                      n_start, n_end);
> +    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, *num);
>   
> -    assert(n_start * BDRV_SECTOR_SIZE == offset_into_cluster(s, offset));
> -    offset = start_of_cluster(s, offset);
> +    assert((offset & ~BDRV_SECTOR_MASK) == 0);
>   
>   again:
> -    start = offset + (n_start << BDRV_SECTOR_BITS);
> -    remaining = (n_end - n_start) << BDRV_SECTOR_BITS;
> +    start = offset;
> +    remaining = (*num) << BDRV_SECTOR_BITS;

If you were to do a respin, the brackets here are unnecessary.

>       cluster_offset = 0;
>       *host_offset = 0;
>       cur_bytes = 0;
> @@ -1284,7 +1282,7 @@ again:
>           }
>       }
>   
> -    *num = (n_end - n_start) - (remaining >> BDRV_SECTOR_BITS);
> +    *num = *num - (remaining >> BDRV_SECTOR_BITS);

And this could be written as "*num -= remaining >> BDRV_SECTOR_BITS". 
But regardless:

Reviewed-by: Max Reitz <mreitz@redhat.com>

>       assert(*num > 0);
>       assert(*host_offset != 0);
>   
> diff --git a/block/qcow2.c b/block/qcow2.c
> index f29aa88..46860d5 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1016,14 +1016,14 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
>   
>           trace_qcow2_writev_start_part(qemu_coroutine_self());
>           index_in_cluster = sector_num & (s->cluster_sectors - 1);
> -        n_end = index_in_cluster + remaining_sectors;
> +        cur_nr_sectors = remaining_sectors;
>           if (s->crypt_method &&
>               n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors) {
>               n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
>           }
>   
>           ret = qcow2_alloc_cluster_offset(bs, sector_num << 9,
> -            index_in_cluster, n_end, &cur_nr_sectors, &cluster_offset, &l2meta);
> +            &cur_nr_sectors, &cluster_offset, &l2meta);
>           if (ret < 0) {
>               goto fail;
>           }
> @@ -1400,7 +1400,7 @@ static int preallocate(BlockDriverState *bs)
>   
>       while (nb_sectors) {
>           num = MIN(nb_sectors, INT_MAX >> 9);
> -        ret = qcow2_alloc_cluster_offset(bs, offset, 0, num, &num,
> +        ret = qcow2_alloc_cluster_offset(bs, offset, &num,
>                                            &host_offset, &meta);
>           if (ret < 0) {
>               return ret;
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 303eb26..84e1344 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -468,7 +468,7 @@ void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
>   int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>       int *num, uint64_t *cluster_offset);
>   int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> -    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m);
> +    int *num, uint64_t *host_offset, QCowL2Meta **m);
>   uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
>                                            uint64_t offset,
>                                            int compressed_size);
> diff --git a/trace-events b/trace-events
> index 9f4456a..9b4e586 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -494,7 +494,7 @@ qcow2_writev_done_part(void *co, int cur_nr_sectors) "co %p cur_nr_sectors %d"
>   qcow2_writev_data(void *co, uint64_t offset) "co %p offset %" PRIx64
>   
>   # block/qcow2-cluster.c
> -qcow2_alloc_clusters_offset(void *co, uint64_t offset, int n_start, int n_end) "co %p offet %" PRIx64 " n_start %d n_end %d"
> +qcow2_alloc_clusters_offset(void *co, uint64_t offset, int num) "co %p offet %" PRIx64 " num %d"
>   qcow2_handle_copied(void *co, uint64_t guest_offset, uint64_t host_offset, uint64_t bytes) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " bytes %" PRIx64
>   qcow2_handle_alloc(void *co, uint64_t guest_offset, uint64_t host_offset, uint64_t bytes) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " bytes %" PRIx64
>   qcow2_do_alloc_clusters_offset(void *co, uint64_t guest_offset, uint64_t host_offset, int nb_clusters) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " nb_clusters %d"

Re: [Qemu-devel] [PATCH v1 2/3] qcow2: fix offset overflow

[Max Reitz]

On 30.12.2013 06:29, Hu Tao wrote:
> When cluster size is big enough it can lead offset overflow
> in qcow2_alloc_clusters_at(). This patch fixes it.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>   block/qcow2-refcount.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index c974abe..b3ebb7f 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -676,7 +676,12 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
>       BDRVQcowState *s = bs->opaque;
>       uint64_t cluster_index;
>       uint64_t old_free_cluster_index;
> -    int i, refcount, ret;
> +    uint64_t i;
> +    int refcount, ret;
> +
> +    if (nb_clusters <= 0) {
> +        return 0;

I think I'd rather return -EINVAL here.

> +    }
>   
>       /* Check how many clusters there are free */
>       cluster_index = offset >> s->cluster_bits;

Re: [Qemu-devel] [PATCH v1 3/3] qcow2: check for NULL l2meta

[Max Reitz]

On 30.12.2013 06:29, Hu Tao wrote:
> In case of do preallocating metadata with a large cluster size,
> qcow2_alloc_cluster_offset() can allocate nothing and returns
> a NULL l2meta. This patch checks for it and link2 l2 with only
> valid l2meta.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>   block/qcow2.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 46860d5..380c240 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs)
>       offset = 0;
>   
>       while (nb_sectors) {
> -        num = MIN(nb_sectors, INT_MAX >> 9);
> +        num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS);

Well, if you're already adjusting this here, you could also replace the 
other occurrences of 9 and 512 in this function. ;-)

>           ret = qcow2_alloc_cluster_offset(bs, offset, &num,
>                                            &host_offset, &meta);
>           if (ret < 0) {
>               return ret;
>           }
>   
> -        ret = qcow2_alloc_cluster_link_l2(bs, meta);
> -        if (ret < 0) {
> -            qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters,
> -                                    QCOW2_DISCARD_NEVER);
> -            return ret;
> +        if (meta) {
> +            ret = qcow2_alloc_cluster_link_l2(bs, meta);
> +            if (ret < 0) {
> +                qcow2_free_any_clusters(bs, meta->alloc_offset,
> +                                        meta->nb_clusters, QCOW2_DISCARD_NEVER);
> +                return ret;
> +            }
>           }
>   
>           /* There are no dependent requests, but we need to remove our request

But this doesn't make this patch wrong, so:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v1 3/3] qcow2: check for NULL l2meta

[Hu Tao]

On Sun, Jan 19, 2014 at 05:18:05PM +0100, Max Reitz wrote:
> On 30.12.2013 06:29, Hu Tao wrote:
> >In case of do preallocating metadata with a large cluster size,
> >qcow2_alloc_cluster_offset() can allocate nothing and returns
> >a NULL l2meta. This patch checks for it and link2 l2 with only
> >valid l2meta.
> >
> >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >---
> >  block/qcow2.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> >diff --git a/block/qcow2.c b/block/qcow2.c
> >index 46860d5..380c240 100644
> >--- a/block/qcow2.c
> >+++ b/block/qcow2.c
> >@@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs)
> >      offset = 0;
> >      while (nb_sectors) {
> >-        num = MIN(nb_sectors, INT_MAX >> 9);
> >+        num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS);
> 
> Well, if you're already adjusting this here, you could also replace
> the other occurrences of 9 and 512 in this function. ;-)
> 
> >          ret = qcow2_alloc_cluster_offset(bs, offset, &num,
> >                                           &host_offset, &meta);
> >          if (ret < 0) {
> >              return ret;
> >          }
> >-        ret = qcow2_alloc_cluster_link_l2(bs, meta);
> >-        if (ret < 0) {
> >-            qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters,
> >-                                    QCOW2_DISCARD_NEVER);
> >-            return ret;
> >+        if (meta) {
> >+            ret = qcow2_alloc_cluster_link_l2(bs, meta);
> >+            if (ret < 0) {
> >+                qcow2_free_any_clusters(bs, meta->alloc_offset,
> >+                                        meta->nb_clusters, QCOW2_DISCARD_NEVER);
> >+                return ret;
> >+            }
> >          }
> >          /* There are no dependent requests, but we need to remove our request
> 
> But this doesn't make this patch wrong, so:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Max,

Thanks for all of you comments!

Re: [Qemu-devel] [PATCH v1 2/3] qcow2: fix offset overflow

[Kevin Wolf]

Am 19.01.2014 um 17:12 hat Max Reitz geschrieben:
> On 30.12.2013 06:29, Hu Tao wrote:
> >When cluster size is big enough it can lead offset overflow
> >in qcow2_alloc_clusters_at(). This patch fixes it.
> >
> >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >---
> >  block/qcow2-refcount.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> >diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> >index c974abe..b3ebb7f 100644
> >--- a/block/qcow2-refcount.c
> >+++ b/block/qcow2-refcount.c
> >@@ -676,7 +676,12 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
> >      BDRVQcowState *s = bs->opaque;
> >      uint64_t cluster_index;
> >      uint64_t old_free_cluster_index;
> >-    int i, refcount, ret;
> >+    uint64_t i;
> >+    int refcount, ret;
> >+
> >+    if (nb_clusters <= 0) {
> >+        return 0;
> 
> I think I'd rather return -EINVAL here.

In fact, I think return 0 is fine for nb_clusters == 0, and we should
assert(nb_clusters >= 0).

Kevin

Re: [Qemu-devel] [PATCH v1 3/3] qcow2: check for NULL l2meta

[Kevin Wolf]

Am 20.01.2014 um 04:04 hat Hu Tao geschrieben:
> On Sun, Jan 19, 2014 at 05:18:05PM +0100, Max Reitz wrote:
> > On 30.12.2013 06:29, Hu Tao wrote:
> > >In case of do preallocating metadata with a large cluster size,
> > >qcow2_alloc_cluster_offset() can allocate nothing and returns
> > >a NULL l2meta. This patch checks for it and link2 l2 with only
> > >valid l2meta.
> > >
> > >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > >---
> > >  block/qcow2.c | 14 ++++++++------
> > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > >
> > >diff --git a/block/qcow2.c b/block/qcow2.c
> > >index 46860d5..380c240 100644
> > >--- a/block/qcow2.c
> > >+++ b/block/qcow2.c
> > >@@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs)
> > >      offset = 0;
> > >      while (nb_sectors) {
> > >-        num = MIN(nb_sectors, INT_MAX >> 9);
> > >+        num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS);
> > 
> > Well, if you're already adjusting this here, you could also replace
> > the other occurrences of 9 and 512 in this function. ;-)
> > 
> > >          ret = qcow2_alloc_cluster_offset(bs, offset, &num,
> > >                                           &host_offset, &meta);
> > >          if (ret < 0) {
> > >              return ret;
> > >          }
> > >-        ret = qcow2_alloc_cluster_link_l2(bs, meta);
> > >-        if (ret < 0) {
> > >-            qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters,
> > >-                                    QCOW2_DISCARD_NEVER);
> > >-            return ret;
> > >+        if (meta) {
> > >+            ret = qcow2_alloc_cluster_link_l2(bs, meta);
> > >+            if (ret < 0) {
> > >+                qcow2_free_any_clusters(bs, meta->alloc_offset,
> > >+                                        meta->nb_clusters, QCOW2_DISCARD_NEVER);
> > >+                return ret;
> > >+            }
> > >          }
> > >          /* There are no dependent requests, but we need to remove our request
> > 
> > But this doesn't make this patch wrong, so:
> > 
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> Max,
> 
> Thanks for all of you comments!

The series looks good in general, but I think the comments are worth
addressing before we merge it. I would also love to see an qemu-iotests
case that tests the cases that would previously crash.

Once you post a new version that addresses these points, I'll merge it.

Kevin

Re: [Qemu-devel] [PATCH v1 3/3] qcow2: check for NULL l2meta

[Hu Tao]

On Mon, Jan 20, 2014 at 04:17:16PM +0100, Kevin Wolf wrote:
> Am 20.01.2014 um 04:04 hat Hu Tao geschrieben:
> > On Sun, Jan 19, 2014 at 05:18:05PM +0100, Max Reitz wrote:
> > > On 30.12.2013 06:29, Hu Tao wrote:
> > > >In case of do preallocating metadata with a large cluster size,
> > > >qcow2_alloc_cluster_offset() can allocate nothing and returns
> > > >a NULL l2meta. This patch checks for it and link2 l2 with only
> > > >valid l2meta.
> > > >
> > > >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > > >---
> > > >  block/qcow2.c | 14 ++++++++------
> > > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > > >
> > > >diff --git a/block/qcow2.c b/block/qcow2.c
> > > >index 46860d5..380c240 100644
> > > >--- a/block/qcow2.c
> > > >+++ b/block/qcow2.c
> > > >@@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs)
> > > >      offset = 0;
> > > >      while (nb_sectors) {
> > > >-        num = MIN(nb_sectors, INT_MAX >> 9);
> > > >+        num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS);
> > > 
> > > Well, if you're already adjusting this here, you could also replace
> > > the other occurrences of 9 and 512 in this function. ;-)
> > > 
> > > >          ret = qcow2_alloc_cluster_offset(bs, offset, &num,
> > > >                                           &host_offset, &meta);
> > > >          if (ret < 0) {
> > > >              return ret;
> > > >          }
> > > >-        ret = qcow2_alloc_cluster_link_l2(bs, meta);
> > > >-        if (ret < 0) {
> > > >-            qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters,
> > > >-                                    QCOW2_DISCARD_NEVER);
> > > >-            return ret;
> > > >+        if (meta) {
> > > >+            ret = qcow2_alloc_cluster_link_l2(bs, meta);
> > > >+            if (ret < 0) {
> > > >+                qcow2_free_any_clusters(bs, meta->alloc_offset,
> > > >+                                        meta->nb_clusters, QCOW2_DISCARD_NEVER);
> > > >+                return ret;
> > > >+            }
> > > >          }
> > > >          /* There are no dependent requests, but we need to remove our request
> > > 
> > > But this doesn't make this patch wrong, so:
> > > 
> > > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > 
> > Max,
> > 
> > Thanks for all of you comments!
> 
> The series looks good in general, but I think the comments are worth
> addressing before we merge it. I would also love to see an qemu-iotests
> case that tests the cases that would previously crash.

Sure.  Thanks for review!

Re: [Qemu-devel] [PATCH v1 3/3] qcow2: check for NULL l2meta

[Hu Tao]

On Tue, Jan 21, 2014 at 11:33:18AM +0800, Hu Tao wrote:
> On Mon, Jan 20, 2014 at 04:17:16PM +0100, Kevin Wolf wrote:
> > Am 20.01.2014 um 04:04 hat Hu Tao geschrieben:
> > > On Sun, Jan 19, 2014 at 05:18:05PM +0100, Max Reitz wrote:
> > > > On 30.12.2013 06:29, Hu Tao wrote:
> > > > >In case of do preallocating metadata with a large cluster size,
> > > > >qcow2_alloc_cluster_offset() can allocate nothing and returns
> > > > >a NULL l2meta. This patch checks for it and link2 l2 with only
> > > > >valid l2meta.
> > > > >
> > > > >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > > > >---
> > > > >  block/qcow2.c | 14 ++++++++------
> > > > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > > > >
> > > > >diff --git a/block/qcow2.c b/block/qcow2.c
> > > > >index 46860d5..380c240 100644
> > > > >--- a/block/qcow2.c
> > > > >+++ b/block/qcow2.c
> > > > >@@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs)
> > > > >      offset = 0;
> > > > >      while (nb_sectors) {
> > > > >-        num = MIN(nb_sectors, INT_MAX >> 9);
> > > > >+        num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS);
> > > > 
> > > > Well, if you're already adjusting this here, you could also replace
> > > > the other occurrences of 9 and 512 in this function. ;-)
> > > > 
> > > > >          ret = qcow2_alloc_cluster_offset(bs, offset, &num,
> > > > >                                           &host_offset, &meta);
> > > > >          if (ret < 0) {
> > > > >              return ret;
> > > > >          }
> > > > >-        ret = qcow2_alloc_cluster_link_l2(bs, meta);
> > > > >-        if (ret < 0) {
> > > > >-            qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters,
> > > > >-                                    QCOW2_DISCARD_NEVER);
> > > > >-            return ret;
> > > > >+        if (meta) {
> > > > >+            ret = qcow2_alloc_cluster_link_l2(bs, meta);
> > > > >+            if (ret < 0) {
> > > > >+                qcow2_free_any_clusters(bs, meta->alloc_offset,
> > > > >+                                        meta->nb_clusters, QCOW2_DISCARD_NEVER);
> > > > >+                return ret;
> > > > >+            }
> > > > >          }
> > > > >          /* There are no dependent requests, but we need to remove our request
> > > > 
> > > > But this doesn't make this patch wrong, so:
> > > > 
> > > > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > > 
> > > Max,
> > > 
> > > Thanks for all of you comments!
> > 
> > The series looks good in general, but I think the comments are worth
> > addressing before we merge it. I would also love to see an qemu-iotests
> > case that tests the cases that would previously crash.

Should I add the test case into an existing file or create a new file?

Re: [Qemu-devel] [PATCH v1 3/3] qcow2: check for NULL l2meta

[Kevin Wolf]

Am 21.01.2014 um 07:02 hat Hu Tao geschrieben:
> On Tue, Jan 21, 2014 at 11:33:18AM +0800, Hu Tao wrote:
> > On Mon, Jan 20, 2014 at 04:17:16PM +0100, Kevin Wolf wrote:
> > > Am 20.01.2014 um 04:04 hat Hu Tao geschrieben:
> > > > On Sun, Jan 19, 2014 at 05:18:05PM +0100, Max Reitz wrote:
> > > > > On 30.12.2013 06:29, Hu Tao wrote:
> > > > > >In case of do preallocating metadata with a large cluster size,
> > > > > >qcow2_alloc_cluster_offset() can allocate nothing and returns
> > > > > >a NULL l2meta. This patch checks for it and link2 l2 with only
> > > > > >valid l2meta.
> > > > > >
> > > > > >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > > > > >---
> > > > > >  block/qcow2.c | 14 ++++++++------
> > > > > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > > > > >
> > > > > >diff --git a/block/qcow2.c b/block/qcow2.c
> > > > > >index 46860d5..380c240 100644
> > > > > >--- a/block/qcow2.c
> > > > > >+++ b/block/qcow2.c
> > > > > >@@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs)
> > > > > >      offset = 0;
> > > > > >      while (nb_sectors) {
> > > > > >-        num = MIN(nb_sectors, INT_MAX >> 9);
> > > > > >+        num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS);
> > > > > 
> > > > > Well, if you're already adjusting this here, you could also replace
> > > > > the other occurrences of 9 and 512 in this function. ;-)
> > > > > 
> > > > > >          ret = qcow2_alloc_cluster_offset(bs, offset, &num,
> > > > > >                                           &host_offset, &meta);
> > > > > >          if (ret < 0) {
> > > > > >              return ret;
> > > > > >          }
> > > > > >-        ret = qcow2_alloc_cluster_link_l2(bs, meta);
> > > > > >-        if (ret < 0) {
> > > > > >-            qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters,
> > > > > >-                                    QCOW2_DISCARD_NEVER);
> > > > > >-            return ret;
> > > > > >+        if (meta) {
> > > > > >+            ret = qcow2_alloc_cluster_link_l2(bs, meta);
> > > > > >+            if (ret < 0) {
> > > > > >+                qcow2_free_any_clusters(bs, meta->alloc_offset,
> > > > > >+                                        meta->nb_clusters, QCOW2_DISCARD_NEVER);
> > > > > >+                return ret;
> > > > > >+            }
> > > > > >          }
> > > > > >          /* There are no dependent requests, but we need to remove our request
> > > > > 
> > > > > But this doesn't make this patch wrong, so:
> > > > > 
> > > > > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > > > 
> > > > Max,
> > > > 
> > > > Thanks for all of you comments!
> > > 
> > > The series looks good in general, but I think the comments are worth
> > > addressing before we merge it. I would also love to see an qemu-iotests
> > > case that tests the cases that would previously crash.
> 
> Should I add the test case into an existing file or create a new file?

The closest existing case I found is 049, which is however more about
option parsing rather than the actual effect of the options. I think a
new file might be better.

If you add a new case, can you please use 079 as its number? There are
several yet unmerged patch series in flight that take the lower numbers.

Kevin

Re: [Qemu-devel] [PATCH v1 3/3] qcow2: check for NULL l2meta

[Hu Tao]

On Tue, Jan 21, 2014 at 12:04:47PM +0100, Kevin Wolf wrote:
> Am 21.01.2014 um 07:02 hat Hu Tao geschrieben:
> > On Tue, Jan 21, 2014 at 11:33:18AM +0800, Hu Tao wrote:
> > > On Mon, Jan 20, 2014 at 04:17:16PM +0100, Kevin Wolf wrote:
> > > > Am 20.01.2014 um 04:04 hat Hu Tao geschrieben:
> > > > > On Sun, Jan 19, 2014 at 05:18:05PM +0100, Max Reitz wrote:
> > > > > > On 30.12.2013 06:29, Hu Tao wrote:
> > > > > > >In case of do preallocating metadata with a large cluster size,
> > > > > > >qcow2_alloc_cluster_offset() can allocate nothing and returns
> > > > > > >a NULL l2meta. This patch checks for it and link2 l2 with only
> > > > > > >valid l2meta.
> > > > > > >
> > > > > > >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > > > > > >---
> > > > > > >  block/qcow2.c | 14 ++++++++------
> > > > > > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > > > > > >
> > > > > > >diff --git a/block/qcow2.c b/block/qcow2.c
> > > > > > >index 46860d5..380c240 100644
> > > > > > >--- a/block/qcow2.c
> > > > > > >+++ b/block/qcow2.c
> > > > > > >@@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs)
> > > > > > >      offset = 0;
> > > > > > >      while (nb_sectors) {
> > > > > > >-        num = MIN(nb_sectors, INT_MAX >> 9);
> > > > > > >+        num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS);
> > > > > > 
> > > > > > Well, if you're already adjusting this here, you could also replace
> > > > > > the other occurrences of 9 and 512 in this function. ;-)
> > > > > > 
> > > > > > >          ret = qcow2_alloc_cluster_offset(bs, offset, &num,
> > > > > > >                                           &host_offset, &meta);
> > > > > > >          if (ret < 0) {
> > > > > > >              return ret;
> > > > > > >          }
> > > > > > >-        ret = qcow2_alloc_cluster_link_l2(bs, meta);
> > > > > > >-        if (ret < 0) {
> > > > > > >-            qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters,
> > > > > > >-                                    QCOW2_DISCARD_NEVER);
> > > > > > >-            return ret;
> > > > > > >+        if (meta) {
> > > > > > >+            ret = qcow2_alloc_cluster_link_l2(bs, meta);
> > > > > > >+            if (ret < 0) {
> > > > > > >+                qcow2_free_any_clusters(bs, meta->alloc_offset,
> > > > > > >+                                        meta->nb_clusters, QCOW2_DISCARD_NEVER);
> > > > > > >+                return ret;
> > > > > > >+            }
> > > > > > >          }
> > > > > > >          /* There are no dependent requests, but we need to remove our request
> > > > > > 
> > > > > > But this doesn't make this patch wrong, so:
> > > > > > 
> > > > > > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > > > > 
> > > > > Max,
> > > > > 
> > > > > Thanks for all of you comments!
> > > > 
> > > > The series looks good in general, but I think the comments are worth
> > > > addressing before we merge it. I would also love to see an qemu-iotests
> > > > case that tests the cases that would previously crash.
> > 
> > Should I add the test case into an existing file or create a new file?
> 
> The closest existing case I found is 049, which is however more about
> option parsing rather than the actual effect of the options. I think a
> new file might be better.
> 
> If you add a new case, can you please use 079 as its number? There are
> several yet unmerged patch series in flight that take the lower numbers.

OK.