* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.