* [Qemu-devel] [PATCH] memory: move MemoryRegion::size cleanup to memory_region_finalize() @ 2018-10-09 13:50 Igor Mammedov 2018-10-09 15:23 ` Laszlo Ersek 2018-10-09 15:47 ` Paolo Bonzini 0 siblings, 2 replies; 7+ messages in thread From: Igor Mammedov @ 2018-10-09 13:50 UTC (permalink / raw) To: qemu-devel; +Cc: lersek, pbonzini It's not necessary to clean up MemoryRegion::size manually in multiple places like it's been implemented in (1cd3d49262 "memory: cleanup side effects of memory_region_init_foo() on failure") since each memory_region_init_foo() now calls object_unparent(mr) on failure, memory region destructor memory_region_finalize() will be called upon its completion for each memory_region_init_foo(). It's sufficient to clean MemoryRegion::size only from memory_region_finalize(), so do it. Suggested-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- smoke tested with reproducer from 1cd3d49262, thing still works as expected --- memory.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/memory.c b/memory.c index d852f11..ad24b57 100644 --- a/memory.c +++ b/memory.c @@ -1491,7 +1491,6 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr, mr->ram_block = qemu_ram_alloc(size, share, mr, &err); mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; if (err) { - mr->size = int128_zero(); object_unparent(OBJECT(mr)); error_propagate(errp, err); } @@ -1516,7 +1515,6 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr, mr, &err); mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; if (err) { - mr->size = int128_zero(); object_unparent(OBJECT(mr)); error_propagate(errp, err); } @@ -1541,7 +1539,6 @@ void memory_region_init_ram_from_file(MemoryRegion *mr, mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, &err); mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; if (err) { - mr->size = int128_zero(); object_unparent(OBJECT(mr)); error_propagate(errp, err); } @@ -1565,7 +1562,6 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr, fd, &err); mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; if (err) { - mr->size = int128_zero(); object_unparent(OBJECT(mr)); error_propagate(errp, err); } @@ -1628,7 +1624,6 @@ void memory_region_init_rom_nomigrate(MemoryRegion *mr, mr->ram_block = qemu_ram_alloc(size, false, mr, &err); mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; if (err) { - mr->size = int128_zero(); object_unparent(OBJECT(mr)); error_propagate(errp, err); } @@ -1652,7 +1647,6 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr, mr->destructor = memory_region_destructor_ram; mr->ram_block = qemu_ram_alloc(size, false, mr, &err); if (err) { - mr->size = int128_zero(); object_unparent(OBJECT(mr)); error_propagate(errp, err); } @@ -1701,6 +1695,7 @@ static void memory_region_finalize(Object *obj) memory_region_clear_coalescing(mr); g_free((char *)mr->name); g_free(mr->ioeventfds); + mr->size = int128_zero(); } Object *memory_region_owner(MemoryRegion *mr) -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] memory: move MemoryRegion::size cleanup to memory_region_finalize() 2018-10-09 13:50 [Qemu-devel] [PATCH] memory: move MemoryRegion::size cleanup to memory_region_finalize() Igor Mammedov @ 2018-10-09 15:23 ` Laszlo Ersek 2018-10-09 15:47 ` Paolo Bonzini 1 sibling, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2018-10-09 15:23 UTC (permalink / raw) To: Igor Mammedov, qemu-devel; +Cc: pbonzini On 10/09/18 15:50, Igor Mammedov wrote: > It's not necessary to clean up MemoryRegion::size manually in multiple places > like it's been implemented in > (1cd3d49262 "memory: cleanup side effects of memory_region_init_foo() on failure") > since each memory_region_init_foo() now calls object_unparent(mr) on failure, > memory region destructor memory_region_finalize() will be called upon its > completion for each memory_region_init_foo(). > It's sufficient to clean MemoryRegion::size only from memory_region_finalize(), > so do it. > > Suggested-by: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > smoke tested with reproducer from 1cd3d49262, thing still works as expected > --- > memory.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/memory.c b/memory.c > index d852f11..ad24b57 100644 > --- a/memory.c > +++ b/memory.c > @@ -1491,7 +1491,6 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr, > mr->ram_block = qemu_ram_alloc(size, share, mr, &err); > mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; > if (err) { > - mr->size = int128_zero(); > object_unparent(OBJECT(mr)); > error_propagate(errp, err); > } > @@ -1516,7 +1515,6 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr, > mr, &err); > mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; > if (err) { > - mr->size = int128_zero(); > object_unparent(OBJECT(mr)); > error_propagate(errp, err); > } > @@ -1541,7 +1539,6 @@ void memory_region_init_ram_from_file(MemoryRegion *mr, > mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, &err); > mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; > if (err) { > - mr->size = int128_zero(); > object_unparent(OBJECT(mr)); > error_propagate(errp, err); > } > @@ -1565,7 +1562,6 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr, > fd, &err); > mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; > if (err) { > - mr->size = int128_zero(); > object_unparent(OBJECT(mr)); > error_propagate(errp, err); > } > @@ -1628,7 +1624,6 @@ void memory_region_init_rom_nomigrate(MemoryRegion *mr, > mr->ram_block = qemu_ram_alloc(size, false, mr, &err); > mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; > if (err) { > - mr->size = int128_zero(); > object_unparent(OBJECT(mr)); > error_propagate(errp, err); > } > @@ -1652,7 +1647,6 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr, > mr->destructor = memory_region_destructor_ram; > mr->ram_block = qemu_ram_alloc(size, false, mr, &err); > if (err) { > - mr->size = int128_zero(); > object_unparent(OBJECT(mr)); > error_propagate(errp, err); > } > @@ -1701,6 +1695,7 @@ static void memory_region_finalize(Object *obj) > memory_region_clear_coalescing(mr); > g_free((char *)mr->name); > g_free(mr->ioeventfds); > + mr->size = int128_zero(); > } > > Object *memory_region_owner(MemoryRegion *mr) > Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] memory: move MemoryRegion::size cleanup to memory_region_finalize() 2018-10-09 13:50 [Qemu-devel] [PATCH] memory: move MemoryRegion::size cleanup to memory_region_finalize() Igor Mammedov 2018-10-09 15:23 ` Laszlo Ersek @ 2018-10-09 15:47 ` Paolo Bonzini 2018-10-09 17:09 ` Laszlo Ersek 1 sibling, 1 reply; 7+ messages in thread From: Paolo Bonzini @ 2018-10-09 15:47 UTC (permalink / raw) To: Igor Mammedov, qemu-devel; +Cc: lersek On 09/10/2018 15:50, Igor Mammedov wrote: > It's not necessary to clean up MemoryRegion::size manually in multiple places > like it's been implemented in > (1cd3d49262 "memory: cleanup side effects of memory_region_init_foo() on failure") > since each memory_region_init_foo() now calls object_unparent(mr) on failure, > memory region destructor memory_region_finalize() will be called upon its > completion for each memory_region_init_foo(). > It's sufficient to clean MemoryRegion::size only from memory_region_finalize(), > so do it. Stupid question, why is it necessary to clear mr->size at all?... Paolo > --- > memory.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/memory.c b/memory.c > index d852f11..ad24b57 100644 > --- a/memory.c > +++ b/memory.c > @@ -1491,7 +1491,6 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr, > mr->ram_block = qemu_ram_alloc(size, share, mr, &err); > mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; > if (err) { > - mr->size = int128_zero(); > object_unparent(OBJECT(mr)); > error_propagate(errp, err); > } > @@ -1516,7 +1515,6 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr, > mr, &err); > mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; > if (err) { > - mr->size = int128_zero(); > object_unparent(OBJECT(mr)); > error_propagate(errp, err); > } > @@ -1541,7 +1539,6 @@ void memory_region_init_ram_from_file(MemoryRegion *mr, > mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, &err); > mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; > if (err) { > - mr->size = int128_zero(); > object_unparent(OBJECT(mr)); > error_propagate(errp, err); > } > @@ -1565,7 +1562,6 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr, > fd, &err); > mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; > if (err) { > - mr->size = int128_zero(); > object_unparent(OBJECT(mr)); > error_propagate(errp, err); > } > @@ -1628,7 +1624,6 @@ void memory_region_init_rom_nomigrate(MemoryRegion *mr, > mr->ram_block = qemu_ram_alloc(size, false, mr, &err); > mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; > if (err) { > - mr->size = int128_zero(); > object_unparent(OBJECT(mr)); > error_propagate(errp, err); > } > @@ -1652,7 +1647,6 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr, > mr->destructor = memory_region_destructor_ram; > mr->ram_block = qemu_ram_alloc(size, false, mr, &err); > if (err) { > - mr->size = int128_zero(); > object_unparent(OBJECT(mr)); > error_propagate(errp, err); > } > @@ -1701,6 +1695,7 @@ static void memory_region_finalize(Object *obj) > memory_region_clear_coalescing(mr); > g_free((char *)mr->name); > g_free(mr->ioeventfds); > + mr->size = int128_zero(); > } > > Object *memory_region_owner(MemoryRegion *mr) > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] memory: move MemoryRegion::size cleanup to memory_region_finalize() 2018-10-09 15:47 ` Paolo Bonzini @ 2018-10-09 17:09 ` Laszlo Ersek 2018-10-10 12:36 ` Igor Mammedov 2018-10-10 18:56 ` Paolo Bonzini 0 siblings, 2 replies; 7+ messages in thread From: Laszlo Ersek @ 2018-10-09 17:09 UTC (permalink / raw) To: Paolo Bonzini, Igor Mammedov, qemu-devel On 10/09/18 17:47, Paolo Bonzini wrote: > On 09/10/2018 15:50, Igor Mammedov wrote: >> It's not necessary to clean up MemoryRegion::size manually in multiple places >> like it's been implemented in >> (1cd3d49262 "memory: cleanup side effects of memory_region_init_foo() on failure") >> since each memory_region_init_foo() now calls object_unparent(mr) on failure, >> memory region destructor memory_region_finalize() will be called upon its >> completion for each memory_region_init_foo(). >> It's sufficient to clean MemoryRegion::size only from memory_region_finalize(), >> so do it. > > Stupid question, why is it necessary to clear mr->size at all?... IIRC it is explained in the above-mentioned, earlier commit 1cd3d49262: > if MemoryRegion intialization fails it's left in semi-initialized state, > where it's size is not 0 and attached as child to owner object. > And this leds to crash in following use-case: > (monitor) object_add memory-backend-file,id=mem1,size=99999G,mem-path=/tmp/foo,discard-data=yes > memory.c:2083: memory_region_get_ram_ptr: Assertion `mr->ram_block' failed > Aborted (core dumped) > it happens due to assumption that memory region is intialized when > memory_region_size() != 0 > and therefore it's ok to access it in > file_backend_unparent() > if (memory_region_size() != 0) > memory_region_get_ram_ptr() > > which happens when object_add fails and unparents failed backend making > file_backend_unparent() access invalid memory region. I think it makes sense to zero out the size even if unparenting would, in itself, prevent the above crash. Because, in host_memory_backend_mr_inited(), we have: /* * NOTE: We forbid zero-length memory backend, so here zero means * "we haven't inited the backend memory region yet". */ I'm unsure how general that invariant is, but it can't hurt to honor it everywhere. (Especially if we can do the zeroing in one common place.) Anyway, I should defer to Igor on this! :) Thanks, Laszlo >> memory.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/memory.c b/memory.c >> index d852f11..ad24b57 100644 >> --- a/memory.c >> +++ b/memory.c >> @@ -1491,7 +1491,6 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr, >> mr->ram_block = qemu_ram_alloc(size, share, mr, &err); >> mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; >> if (err) { >> - mr->size = int128_zero(); >> object_unparent(OBJECT(mr)); >> error_propagate(errp, err); >> } >> @@ -1516,7 +1515,6 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr, >> mr, &err); >> mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; >> if (err) { >> - mr->size = int128_zero(); >> object_unparent(OBJECT(mr)); >> error_propagate(errp, err); >> } >> @@ -1541,7 +1539,6 @@ void memory_region_init_ram_from_file(MemoryRegion *mr, >> mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, &err); >> mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; >> if (err) { >> - mr->size = int128_zero(); >> object_unparent(OBJECT(mr)); >> error_propagate(errp, err); >> } >> @@ -1565,7 +1562,6 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr, >> fd, &err); >> mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; >> if (err) { >> - mr->size = int128_zero(); >> object_unparent(OBJECT(mr)); >> error_propagate(errp, err); >> } >> @@ -1628,7 +1624,6 @@ void memory_region_init_rom_nomigrate(MemoryRegion *mr, >> mr->ram_block = qemu_ram_alloc(size, false, mr, &err); >> mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; >> if (err) { >> - mr->size = int128_zero(); >> object_unparent(OBJECT(mr)); >> error_propagate(errp, err); >> } >> @@ -1652,7 +1647,6 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr, >> mr->destructor = memory_region_destructor_ram; >> mr->ram_block = qemu_ram_alloc(size, false, mr, &err); >> if (err) { >> - mr->size = int128_zero(); >> object_unparent(OBJECT(mr)); >> error_propagate(errp, err); >> } >> @@ -1701,6 +1695,7 @@ static void memory_region_finalize(Object *obj) >> memory_region_clear_coalescing(mr); >> g_free((char *)mr->name); >> g_free(mr->ioeventfds); >> + mr->size = int128_zero(); >> } >> >> Object *memory_region_owner(MemoryRegion *mr) >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] memory: move MemoryRegion::size cleanup to memory_region_finalize() 2018-10-09 17:09 ` Laszlo Ersek @ 2018-10-10 12:36 ` Igor Mammedov 2018-10-10 18:56 ` Paolo Bonzini 1 sibling, 0 replies; 7+ messages in thread From: Igor Mammedov @ 2018-10-10 12:36 UTC (permalink / raw) To: Laszlo Ersek; +Cc: Paolo Bonzini, qemu-devel On Tue, 9 Oct 2018 19:09:06 +0200 Laszlo Ersek <lersek@redhat.com> wrote: > On 10/09/18 17:47, Paolo Bonzini wrote: > > On 09/10/2018 15:50, Igor Mammedov wrote: > >> It's not necessary to clean up MemoryRegion::size manually in multiple places > >> like it's been implemented in > >> (1cd3d49262 "memory: cleanup side effects of memory_region_init_foo() on failure") > >> since each memory_region_init_foo() now calls object_unparent(mr) on failure, > >> memory region destructor memory_region_finalize() will be called upon its > >> completion for each memory_region_init_foo(). > >> It's sufficient to clean MemoryRegion::size only from memory_region_finalize(), > >> so do it. > > > > Stupid question, why is it necessary to clear mr->size at all?... > > IIRC it is explained in the above-mentioned, earlier commit 1cd3d49262: > > > if MemoryRegion intialization fails it's left in semi-initialized state, > > where it's size is not 0 and attached as child to owner object. > > And this leds to crash in following use-case: > > (monitor) object_add memory-backend-file,id=mem1,size=99999G,mem-path=/tmp/foo,discard-data=yes > > memory.c:2083: memory_region_get_ram_ptr: Assertion `mr->ram_block' failed > > Aborted (core dumped) > > it happens due to assumption that memory region is intialized when > > memory_region_size() != 0 > > and therefore it's ok to access it in > > file_backend_unparent() > > if (memory_region_size() != 0) > > memory_region_get_ram_ptr() > > > > which happens when object_add fails and unparents failed backend making > > file_backend_unparent() access invalid memory region. > > I think it makes sense to zero out the size even if unparenting would, in itself, prevent the above crash. Because, in host_memory_backend_mr_inited(), we have: > > /* > * NOTE: We forbid zero-length memory backend, so here zero means > * "we haven't inited the backend memory region yet". > */ > > I'm unsure how general that invariant is, but it can't hurt to honor it everywhere. (Especially if we can do the zeroing in one common place.) yep, we are (ab)using memory_region_size(MemoryRegion *mr) == 0 as a check if memory pointed by 'mr' is valid memory region. In cases where MemoryRegion is not dynamically allocated, it allows us to avoid using an external validity flag. Even though unparenting solves 1cd3d49262 issue I'd prefer to leave clean state on failure if it doesn't incur a heavy penalty (performance|code complexity wise) > Anyway, I should defer to Igor on this! :) > > Thanks, > Laszlo > > >> memory.c | 7 +------ > >> 1 file changed, 1 insertion(+), 6 deletions(-) > >> > >> diff --git a/memory.c b/memory.c > >> index d852f11..ad24b57 100644 > >> --- a/memory.c > >> +++ b/memory.c > >> @@ -1491,7 +1491,6 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr, > >> mr->ram_block = qemu_ram_alloc(size, share, mr, &err); > >> mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; > >> if (err) { > >> - mr->size = int128_zero(); > >> object_unparent(OBJECT(mr)); > >> error_propagate(errp, err); > >> } > >> @@ -1516,7 +1515,6 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr, > >> mr, &err); > >> mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; > >> if (err) { > >> - mr->size = int128_zero(); > >> object_unparent(OBJECT(mr)); > >> error_propagate(errp, err); > >> } > >> @@ -1541,7 +1539,6 @@ void memory_region_init_ram_from_file(MemoryRegion *mr, > >> mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, &err); > >> mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; > >> if (err) { > >> - mr->size = int128_zero(); > >> object_unparent(OBJECT(mr)); > >> error_propagate(errp, err); > >> } > >> @@ -1565,7 +1562,6 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr, > >> fd, &err); > >> mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; > >> if (err) { > >> - mr->size = int128_zero(); > >> object_unparent(OBJECT(mr)); > >> error_propagate(errp, err); > >> } > >> @@ -1628,7 +1624,6 @@ void memory_region_init_rom_nomigrate(MemoryRegion *mr, > >> mr->ram_block = qemu_ram_alloc(size, false, mr, &err); > >> mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; > >> if (err) { > >> - mr->size = int128_zero(); > >> object_unparent(OBJECT(mr)); > >> error_propagate(errp, err); > >> } > >> @@ -1652,7 +1647,6 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr, > >> mr->destructor = memory_region_destructor_ram; > >> mr->ram_block = qemu_ram_alloc(size, false, mr, &err); > >> if (err) { > >> - mr->size = int128_zero(); > >> object_unparent(OBJECT(mr)); > >> error_propagate(errp, err); > >> } > >> @@ -1701,6 +1695,7 @@ static void memory_region_finalize(Object *obj) > >> memory_region_clear_coalescing(mr); > >> g_free((char *)mr->name); > >> g_free(mr->ioeventfds); > >> + mr->size = int128_zero(); > >> } > >> > >> Object *memory_region_owner(MemoryRegion *mr) > >> > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] memory: move MemoryRegion::size cleanup to memory_region_finalize() 2018-10-09 17:09 ` Laszlo Ersek 2018-10-10 12:36 ` Igor Mammedov @ 2018-10-10 18:56 ` Paolo Bonzini 2018-10-10 19:03 ` Laszlo Ersek 1 sibling, 1 reply; 7+ messages in thread From: Paolo Bonzini @ 2018-10-10 18:56 UTC (permalink / raw) To: Laszlo Ersek, Igor Mammedov, qemu-devel On 09/10/2018 19:09, Laszlo Ersek wrote: >> memory_region_size() != 0 >> and therefore it's ok to access it in >> file_backend_unparent() >> if (memory_region_size() != 0) >> memory_region_get_ram_ptr() >> >> which happens when object_add fails and unparents failed backend making >> file_backend_unparent() access invalid memory region. > > I think it makes sense to zero out the size even if unparenting > would, in itself, prevent the above crash. Because, in > host_memory_backend_mr_inited(), we have: > > /* > * NOTE: We forbid zero-length memory backend, so here zero means > * "we haven't inited the backend memory region yet". > */ > > I'm unsure how general that invariant is, but it can't hurt to honor > it everywhere. (Especially if we can do the zeroing in one common > place.) Yeah, that's the part that I'm not sure about. If we do it in finalize, no one should be able to observe that we are zeroing it; finalize runs just before the object is g_free-d. I agree with Igor that it's nicer to leave the object in good state, but the right place to zero is exactly where the first patch placed it, i.e. where the error is detected and the initialization of memory_region_init is unwound. Paolo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] memory: move MemoryRegion::size cleanup to memory_region_finalize() 2018-10-10 18:56 ` Paolo Bonzini @ 2018-10-10 19:03 ` Laszlo Ersek 0 siblings, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2018-10-10 19:03 UTC (permalink / raw) To: Paolo Bonzini, Igor Mammedov, qemu-devel On 10/10/18 20:56, Paolo Bonzini wrote: > On 09/10/2018 19:09, Laszlo Ersek wrote: >>> memory_region_size() != 0 >>> and therefore it's ok to access it in >>> file_backend_unparent() >>> if (memory_region_size() != 0) >>> memory_region_get_ram_ptr() >>> >>> which happens when object_add fails and unparents failed backend making >>> file_backend_unparent() access invalid memory region. >> >> I think it makes sense to zero out the size even if unparenting >> would, in itself, prevent the above crash. Because, in >> host_memory_backend_mr_inited(), we have: >> >> /* >> * NOTE: We forbid zero-length memory backend, so here zero means >> * "we haven't inited the backend memory region yet". >> */ >> >> I'm unsure how general that invariant is, but it can't hurt to honor >> it everywhere. (Especially if we can do the zeroing in one common >> place.) > > Yeah, that's the part that I'm not sure about. If we do it in finalize, > no one should be able to observe that we are zeroing it; finalize runs > just before the object is g_free-d. I agree with Igor that it's nicer > to leave the object in good state, but the right place to zero is > exactly where the first patch placed it, i.e. where the error is > detected and the initialization of memory_region_init is unwound. OK. Thanks Laszlo ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-10-10 19:03 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-10-09 13:50 [Qemu-devel] [PATCH] memory: move MemoryRegion::size cleanup to memory_region_finalize() Igor Mammedov 2018-10-09 15:23 ` Laszlo Ersek 2018-10-09 15:47 ` Paolo Bonzini 2018-10-09 17:09 ` Laszlo Ersek 2018-10-10 12:36 ` Igor Mammedov 2018-10-10 18:56 ` Paolo Bonzini 2018-10-10 19:03 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).