[Qemu-devel] [PATCH for-2.1 0/2] Fix mirror segfault with unaligned size

[Qemu-devel] [PATCH for-2.1 0/2] Fix mirror segfault with unaligned size

[Kevin Wolf]

Kevin Wolf (2):
  mirror: Fix qiov size for short requests
  block: Assert qiov length matches request length

 block.c                    |  2 ++
 block/mirror.c             |  4 +++-
 block/raw-posix.c          | 12 ++++++++----
 tests/qemu-iotests/041     |  5 +++++
 tests/qemu-iotests/041.out |  4 ++--
 5 files changed, 20 insertions(+), 7 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.1 1/2] mirror: Fix qiov size for short requests

[Kevin Wolf]

When mirroring an image of a size that is not a multiple of the
mirror job granularity, the last request would have the right nb_sectors
argument, but a qiov that is rounded up to the next multiple of the
granularity. Don't do this.

This fixes a segfault that is caused by raw-posix being confused by this
and allocating a buffer with request length, but operating on it with
qiov length.

Reported-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c             | 4 +++-
 tests/qemu-iotests/041     | 5 +++++
 tests/qemu-iotests/041.out | 4 ++--
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 6c3ee70..c7a655f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -265,9 +265,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
     next_sector = sector_num;
     while (nb_chunks-- > 0) {
         MirrorBuffer *buf = QSIMPLEQ_FIRST(&s->buf_free);
+        size_t remaining = (nb_sectors * BDRV_SECTOR_SIZE) - op->qiov.size;
+
         QSIMPLEQ_REMOVE_HEAD(&s->buf_free, next);
         s->buf_free_count--;
-        qemu_iovec_add(&op->qiov, buf, s->granularity);
+        qemu_iovec_add(&op->qiov, buf, MIN(s->granularity, remaining));
 
         /* Advance the HBitmapIter in parallel, so that we do not examine
          * the same sector twice.
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 2b1e8a0..3013a26 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -219,6 +219,11 @@ class TestSingleDriveZeroLength(TestSingleDrive):
     test_small_buffer2 = None
     test_large_cluster = None
 
+class TestSingleDriverUnalignedLength(TestSingleDrive):
+    image_len = 1025 * 1024
+    test_small_buffer2 = None
+    test_large_cluster = None
+
 class TestMirrorNoBacking(ImageMirroringTestCase):
     image_len = 2 * 1024 * 1024 # MB
 
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index 42147c0..24093bc 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-..............................................
+......................................................
 ----------------------------------------------------------------------
-Ran 46 tests
+Ran 54 tests
 
 OK
-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.1 2/2] block: Assert qiov length matches request length

[Kevin Wolf]

At least raw-posix relies on this because it can allocate bounce buffers
based on the request length, but access it using all of the qiov entries
later.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c           |  2 ++
 block/raw-posix.c | 12 ++++++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 362c85c..4358ed6 100644
--- a/block.c
+++ b/block.c
@@ -3021,6 +3021,7 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
 
     assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
     assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+    assert(bytes == qiov->size);
 
     /* Handle Copy on Read and associated serialisation */
     if (flags & BDRV_REQ_COPY_ON_READ) {
@@ -3278,6 +3279,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
 
     assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
     assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+    assert(bytes == qiov->size);
 
     waited = wait_serialising_requests(req);
     assert(!waited || !req->serialising);
diff --git a/block/raw-posix.c b/block/raw-posix.c
index dacf4fb..924fe69 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -990,12 +990,14 @@ static int paio_submit_co(BlockDriverState *bs, int fd,
     acb->aio_type = type;
     acb->aio_fildes = fd;
 
+    acb->aio_nbytes = nb_sectors * BDRV_SECTOR_SIZE;
+    acb->aio_offset = sector_num * BDRV_SECTOR_SIZE;
+
     if (qiov) {
         acb->aio_iov = qiov->iov;
         acb->aio_niov = qiov->niov;
+        assert(qiov->size == acb->aio_nbytes);
     }
-    acb->aio_nbytes = nb_sectors * 512;
-    acb->aio_offset = sector_num * 512;
 
     trace_paio_submit_co(sector_num, nb_sectors, type);
     pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
@@ -1013,12 +1015,14 @@ static BlockDriverAIOCB *paio_submit(BlockDriverState *bs, int fd,
     acb->aio_type = type;
     acb->aio_fildes = fd;
 
+    acb->aio_nbytes = nb_sectors * BDRV_SECTOR_SIZE;
+    acb->aio_offset = sector_num * BDRV_SECTOR_SIZE;
+
     if (qiov) {
         acb->aio_iov = qiov->iov;
         acb->aio_niov = qiov->niov;
+        assert(qiov->size == acb->aio_nbytes);
     }
-    acb->aio_nbytes = nb_sectors * 512;
-    acb->aio_offset = sector_num * 512;
 
     trace_paio_submit(acb, opaque, sector_num, nb_sectors, type);
     pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.1 2/2] block: Assert qiov length matches request length

[Kevin Wolf]

Am 01.07.2014 um 16:52 hat Kevin Wolf geschrieben:
> At least raw-posix relies on this because it can allocate bounce buffers
> based on the request length, but access it using all of the qiov entries
> later.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Self-NACK, this breaks some test cases. Patch 1 is still valid.

Kevin

Re: [Qemu-devel] [PATCH for-2.1 1/2] mirror: Fix qiov size for short requests

[Eric Blake]

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

On 07/01/2014 08:52 AM, Kevin Wolf wrote:
> When mirroring an image of a size that is not a multiple of the
> mirror job granularity, the last request would have the right nb_sectors
> argument, but a qiov that is rounded up to the next multiple of the
> granularity. Don't do this.
> 
> This fixes a segfault that is caused by raw-posix being confused by this
> and allocating a buffer with request length, but operating on it with
> qiov length.
> 
> Reported-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/mirror.c             | 4 +++-
>  tests/qemu-iotests/041     | 5 +++++
>  tests/qemu-iotests/041.out | 4 ++--
>  3 files changed, 10 insertions(+), 3 deletions(-)
> 

> +++ b/tests/qemu-iotests/041
> @@ -219,6 +219,11 @@ class TestSingleDriveZeroLength(TestSingleDrive):
>      test_small_buffer2 = None
>      test_large_cluster = None
>  
> +class TestSingleDriverUnalignedLength(TestSingleDrive):

s/Driver/Drive/ for consistency in the class name?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH for-2.1 1/2] mirror: Fix qiov size for short requests

[Eric Blake]

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

On 07/01/2014 10:52 AM, Eric Blake wrote:
> On 07/01/2014 08:52 AM, Kevin Wolf wrote:
>> When mirroring an image of a size that is not a multiple of the
>> mirror job granularity, the last request would have the right nb_sectors
>> argument, but a qiov that is rounded up to the next multiple of the
>> granularity. Don't do this.
>>
>> This fixes a segfault that is caused by raw-posix being confused by this
>> and allocating a buffer with request length, but operating on it with
>> qiov length.
>>
>> Reported-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  block/mirror.c             | 4 +++-
>>  tests/qemu-iotests/041     | 5 +++++
>>  tests/qemu-iotests/041.out | 4 ++--
>>  3 files changed, 10 insertions(+), 3 deletions(-)
>>
> 
>> +++ b/tests/qemu-iotests/041
>> @@ -219,6 +219,11 @@ class TestSingleDriveZeroLength(TestSingleDrive):
>>      test_small_buffer2 = None
>>      test_large_cluster = None
>>  
>> +class TestSingleDriverUnalignedLength(TestSingleDrive):
> 
> s/Driver/Drive/ for consistency in the class name?
> 

Other than that:

Tested-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH for-2.1 1/2] mirror: Fix qiov size for short requests

[Stefan Hajnoczi]

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

On Tue, Jul 01, 2014 at 04:52:21PM +0200, Kevin Wolf wrote:
> When mirroring an image of a size that is not a multiple of the
> mirror job granularity, the last request would have the right nb_sectors
> argument, but a qiov that is rounded up to the next multiple of the
> granularity. Don't do this.
> 
> This fixes a segfault that is caused by raw-posix being confused by this
> and allocating a buffer with request length, but operating on it with
> qiov length.
> 
> Reported-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/mirror.c             | 4 +++-
>  tests/qemu-iotests/041     | 5 +++++
>  tests/qemu-iotests/041.out | 4 ++--
>  3 files changed, 10 insertions(+), 3 deletions(-)

Applied Eric's suggestion.

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

Stefan

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

Re: [Qemu-devel] [PATCH for-2.1 1/2] mirror: Fix qiov size for short requests

[Kevin Wolf]

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

Am 01.07.2014 um 18:52 hat Eric Blake geschrieben:
> On 07/01/2014 08:52 AM, Kevin Wolf wrote:
> > When mirroring an image of a size that is not a multiple of the
> > mirror job granularity, the last request would have the right nb_sectors
> > argument, but a qiov that is rounded up to the next multiple of the
> > granularity. Don't do this.
> > 
> > This fixes a segfault that is caused by raw-posix being confused by this
> > and allocating a buffer with request length, but operating on it with
> > qiov length.
> > 
> > Reported-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/mirror.c             | 4 +++-
> >  tests/qemu-iotests/041     | 5 +++++
> >  tests/qemu-iotests/041.out | 4 ++--
> >  3 files changed, 10 insertions(+), 3 deletions(-)
> > 
> 
> > +++ b/tests/qemu-iotests/041
> > @@ -219,6 +219,11 @@ class TestSingleDriveZeroLength(TestSingleDrive):
> >      test_small_buffer2 = None
> >      test_large_cluster = None
> >  
> > +class TestSingleDriverUnalignedLength(TestSingleDrive):
> 
> s/Driver/Drive/ for consistency in the class name?

Yes, that was a typo. Thanks for catching it.

Kevin

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