Message ID | 20171114184127.24238-1-mreitz@redhat.com |
---|---|
State | New |
Headers | show |
Series | [for-2.11] qcow2: Fix overly broad madvise() | expand |
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>
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
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-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
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
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
@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(-)