@mem_size and @offset are both size_t, thus subtracting them from one
another will just return a big size_t if mem_size < offset -- even more
obvious here because the result is stored in another size_t.
Checking that result to be positive is therefore not sufficient to
excluse the case that offset > mem_size. Thus, we currently sometimes
issue an madvise() over a very large address range.
This is triggered by iotest 163, but with -m64, this does not result in
tangible problems. But with -m32, this test produces three segfaults,
all of which are fixed by this patch.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2-cache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 75746a7f43..5222a7b94d 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -73,7 +73,7 @@ static void qcow2_cache_table_release(BlockDriverState *bs, Qcow2Cache *c,
size_t mem_size = (size_t) s->cluster_size * num_tables;
size_t offset = QEMU_ALIGN_UP((uintptr_t) t, align) - (uintptr_t) t;
size_t length = QEMU_ALIGN_DOWN(mem_size - offset, align);
- if (length > 0) {
+ if (mem_size > offset && length > 0) {
madvise((uint8_t *) t + offset, length, MADV_DONTNEED);
}
#endif
--
2.13.6
On 11/14/2017 12:41 PM, Max Reitz wrote: > @mem_size and @offset are both size_t, thus subtracting them from one > another will just return a big size_t if mem_size < offset -- even more > obvious here because the result is stored in another size_t. > > Checking that result to be positive is therefore not sufficient to > excluse the case that offset > mem_size. Thus, we currently sometimes s/excluse/exclude/ > issue an madvise() over a very large address range. > > This is triggered by iotest 163, but with -m64, this does not result in > tangible problems. But with -m32, this test produces three segfaults, > all of which are fixed by this patch. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2-cache.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue 14 Nov 2017 07:41:27 PM CET, Max Reitz wrote: > @mem_size and @offset are both size_t, thus subtracting them from one > another will just return a big size_t if mem_size < offset -- even more > obvious here because the result is stored in another size_t. > > Checking that result to be positive is therefore not sufficient to > excluse the case that offset > mem_size. Thus, we currently sometimes > issue an madvise() over a very large address range. > > This is triggered by iotest 163, but with -m64, this does not result in > tangible problems. But with -m32, this test produces three segfaults, > all of which are fixed by this patch. > > Signed-off-by: Max Reitz <mreitz@redhat.com> Oh, I guess this happens when the page size is larger than the cluster size? Otherwise I don't see how... Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
On 2017-11-15 10:09, Alberto Garcia wrote: > On Tue 14 Nov 2017 07:41:27 PM CET, Max Reitz wrote: >> @mem_size and @offset are both size_t, thus subtracting them from one >> another will just return a big size_t if mem_size < offset -- even more >> obvious here because the result is stored in another size_t. >> >> Checking that result to be positive is therefore not sufficient to >> excluse the case that offset > mem_size. Thus, we currently sometimes >> issue an madvise() over a very large address range. >> >> This is triggered by iotest 163, but with -m64, this does not result in >> tangible problems. But with -m32, this test produces three segfaults, >> all of which are fixed by this patch. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> > > Oh, I guess this happens when the page size is larger than the cluster > size? Otherwise I don't see how... > > Reviewed-by: Alberto Garcia <berto@igalia.com> Yes, the test uses 512 byte clusters. Max
FWIW,
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Thanks,
Darren.
On Tue, Nov 14, 2017 at 07:41:27PM +0100, Max Reitz wrote:
>@mem_size and @offset are both size_t, thus subtracting them from one
>another will just return a big size_t if mem_size < offset -- even more
>obvious here because the result is stored in another size_t.
>
>Checking that result to be positive is therefore not sufficient to
>excluse the case that offset > mem_size. Thus, we currently sometimes
>issue an madvise() over a very large address range.
>
>This is triggered by iotest 163, but with -m64, this does not result in
>tangible problems. But with -m32, this test produces three segfaults,
>all of which are fixed by this patch.
>
>Signed-off-by: Max Reitz <mreitz@redhat.com>
>---
> block/qcow2-cache.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
>index 75746a7f43..5222a7b94d 100644
>--- a/block/qcow2-cache.c
>+++ b/block/qcow2-cache.c
>@@ -73,7 +73,7 @@ static void qcow2_cache_table_release(BlockDriverState *bs, Qcow2Cache *c,
> size_t mem_size = (size_t) s->cluster_size * num_tables;
> size_t offset = QEMU_ALIGN_UP((uintptr_t) t, align) - (uintptr_t) t;
> size_t length = QEMU_ALIGN_DOWN(mem_size - offset, align);
>- if (length > 0) {
>+ if (mem_size > offset && length > 0) {
> madvise((uint8_t *) t + offset, length, MADV_DONTNEED);
> }
> #endif
>--
>2.13.6
>
>
Should have said that this is subject to the typo that Eric pointed
out, of course.
Thanks,
Darren.
On Wed, Nov 15, 2017 at 11:04:19AM +0000, Darren Kenny wrote:
>FWIW,
>
>Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
>
>Thanks,
>
>Darren.
>
>On Tue, Nov 14, 2017 at 07:41:27PM +0100, Max Reitz wrote:
>>@mem_size and @offset are both size_t, thus subtracting them from one
>>another will just return a big size_t if mem_size < offset -- even more
>>obvious here because the result is stored in another size_t.
>>
>>Checking that result to be positive is therefore not sufficient to
>>excluse the case that offset > mem_size. Thus, we currently sometimes
>>issue an madvise() over a very large address range.
>>
>>This is triggered by iotest 163, but with -m64, this does not result in
>>tangible problems. But with -m32, this test produces three segfaults,
>>all of which are fixed by this patch.
>>
>>Signed-off-by: Max Reitz <mreitz@redhat.com>
>>---
>>block/qcow2-cache.c | 2 +-
>>1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
>>index 75746a7f43..5222a7b94d 100644
>>--- a/block/qcow2-cache.c
>>+++ b/block/qcow2-cache.c
>>@@ -73,7 +73,7 @@ static void qcow2_cache_table_release(BlockDriverState *bs, Qcow2Cache *c,
>> size_t mem_size = (size_t) s->cluster_size * num_tables;
>> size_t offset = QEMU_ALIGN_UP((uintptr_t) t, align) - (uintptr_t) t;
>> size_t length = QEMU_ALIGN_DOWN(mem_size - offset, align);
>>- if (length > 0) {
>>+ if (mem_size > offset && length > 0) {
>> madvise((uint8_t *) t + offset, length, MADV_DONTNEED);
>> }
>>#endif
>>--
>>2.13.6
>>
>>
On 2017-11-14 19:41, Max Reitz wrote: > @mem_size and @offset are both size_t, thus subtracting them from one > another will just return a big size_t if mem_size < offset -- even more > obvious here because the result is stored in another size_t. > > Checking that result to be positive is therefore not sufficient to > excluse the case that offset > mem_size. Thus, we currently sometimes > issue an madvise() over a very large address range. > > This is triggered by iotest 163, but with -m64, this does not result in > tangible problems. But with -m32, this test produces three segfaults, > all of which are fixed by this patch. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2-cache.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Fixed the typo and applied to my block branch: https://github.com/XanClic/qemu/commits/block Max
© 2016 - 2024 Red Hat, Inc.