| Message ID | 1404226342-20174-2-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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?
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>
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
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
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
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(-)