| Message ID | 20230915111130.24064-8-ray.huang@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support blob memory and venus on qemu | expand |
On 2023/09/15 20:11, Huang Rui wrote: > From: Xenia Ragiadakou <xenia.ragiadakou@amd.com> > > When the memory region has a different life-cycle from that of her parent, > could be automatically released, once has been unparent and once all of her > references have gone away, via the object's free callback. > > However, currently, references to the memory region are held by its owner > without first incrementing the memory region object's reference count. > As a result, the automatic deallocation of the object, not taking into > account those references, results in use-after-free memory corruption. > > This patch increases the reference count of an owned memory region object > on each memory_region_ref() and decreases it on each memory_region_unref(). > > Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> > Signed-off-by: Huang Rui <ray.huang@amd.com> > --- > > V4 -> V5: > - ref/unref only owned memory regions (Akihiko) > > softmmu/memory.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/softmmu/memory.c b/softmmu/memory.c > index 7d9494ce70..15e1699750 100644 > --- a/softmmu/memory.c > +++ b/softmmu/memory.c > @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr) > /* MMIO callbacks most likely will access data that belongs > * to the owner, hence the need to ref/unref the owner whenever > * the memory region is in use. > + * Likewise, the owner keeps references to the memory region, > + * hence the need to ref/unref the memory region object to prevent > + * its automatic deallocation while still referenced by its owner. This comment does not make sense. Traditionally no such automatic deallocation happens so the owner has been always required to free the memory region when it gets finalized. "[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands" introduces a different kind of memory region, which can be freed anytime before the device gets finalized. Even in this case, the owner removes the reference to the memory owner by doing res->region = NULL; Regards, Akihiko Odaki
On 15/9/23 18:11, Akihiko Odaki wrote: > On 2023/09/15 20:11, Huang Rui wrote: >> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com> >> >> When the memory region has a different life-cycle from that of her >> parent, >> could be automatically released, once has been unparent and once all >> of her >> references have gone away, via the object's free callback. >> >> However, currently, references to the memory region are held by its >> owner >> without first incrementing the memory region object's reference count. >> As a result, the automatic deallocation of the object, not taking into >> account those references, results in use-after-free memory corruption. >> >> This patch increases the reference count of an owned memory region >> object >> on each memory_region_ref() and decreases it on each >> memory_region_unref(). >> >> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> >> Signed-off-by: Huang Rui <ray.huang@amd.com> >> --- >> >> V4 -> V5: >> - ref/unref only owned memory regions (Akihiko) >> >> softmmu/memory.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/softmmu/memory.c b/softmmu/memory.c >> index 7d9494ce70..15e1699750 100644 >> --- a/softmmu/memory.c >> +++ b/softmmu/memory.c >> @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr) >> /* MMIO callbacks most likely will access data that belongs >> * to the owner, hence the need to ref/unref the owner whenever >> * the memory region is in use. >> + * Likewise, the owner keeps references to the memory region, >> + * hence the need to ref/unref the memory region object to prevent >> + * its automatic deallocation while still referenced by its owner. > > This comment does not make sense. Traditionally no such automatic > deallocation happens so the owner has been always required to free the > memory region when it gets finalized. > > "[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands" > introduces a different kind of memory region, which can be freed > anytime before the device gets finalized. Even in this case, the owner > removes the reference to the memory owner by doing res->region = NULL; Hi Akihiko, You are right, the word "owner" is not correct. The issue observed was due to the references kept in flatview ranges and the fact that flatview_destroy() is asynchronous and was called after memory region's destruction. If I replace the word "owner" with "memory subsystem" in the commit message and drop the comment, would that be ok with you? or do want to suggest something else? Xenia > > > Regards, > Akihiko Odaki
On 2023/09/19 19:28, Xenia Ragiadakou wrote: > > On 15/9/23 18:11, Akihiko Odaki wrote: >> On 2023/09/15 20:11, Huang Rui wrote: >>> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com> >>> >>> When the memory region has a different life-cycle from that of her >>> parent, >>> could be automatically released, once has been unparent and once all >>> of her >>> references have gone away, via the object's free callback. >>> >>> However, currently, references to the memory region are held by its >>> owner >>> without first incrementing the memory region object's reference count. >>> As a result, the automatic deallocation of the object, not taking into >>> account those references, results in use-after-free memory corruption. >>> >>> This patch increases the reference count of an owned memory region >>> object >>> on each memory_region_ref() and decreases it on each >>> memory_region_unref(). >>> >>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> >>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>> --- >>> >>> V4 -> V5: >>> - ref/unref only owned memory regions (Akihiko) >>> >>> softmmu/memory.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/softmmu/memory.c b/softmmu/memory.c >>> index 7d9494ce70..15e1699750 100644 >>> --- a/softmmu/memory.c >>> +++ b/softmmu/memory.c >>> @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr) >>> /* MMIO callbacks most likely will access data that belongs >>> * to the owner, hence the need to ref/unref the owner whenever >>> * the memory region is in use. >>> + * Likewise, the owner keeps references to the memory region, >>> + * hence the need to ref/unref the memory region object to prevent >>> + * its automatic deallocation while still referenced by its owner. >> >> This comment does not make sense. Traditionally no such automatic >> deallocation happens so the owner has been always required to free the >> memory region when it gets finalized. >> >> "[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands" >> introduces a different kind of memory region, which can be freed >> anytime before the device gets finalized. Even in this case, the owner >> removes the reference to the memory owner by doing res->region = NULL; > > Hi Akihiko, > > You are right, the word "owner" is not correct. The issue observed was > due to the references kept in flatview ranges and the fact that > flatview_destroy() is asynchronous and was called after memory region's > destruction. > > If I replace the word "owner" with "memory subsystem" in the commit > message and drop the comment, would that be ok with you? or do want to > suggest something else? This will extend the lifetime of the memory region, but the underlying memory is still synchronously freed. Can you show that the flatview range will not be used to read the freed memory? Regards, Akihiko Odaki
On 19/9/23 13:44, Akihiko Odaki wrote: > On 2023/09/19 19:28, Xenia Ragiadakou wrote: >> >> On 15/9/23 18:11, Akihiko Odaki wrote: >>> On 2023/09/15 20:11, Huang Rui wrote: >>>> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com> >>>> >>>> When the memory region has a different life-cycle from that of her >>>> parent, >>>> could be automatically released, once has been unparent and once >>>> all of her >>>> references have gone away, via the object's free callback. >>>> >>>> However, currently, references to the memory region are held by its >>>> owner >>>> without first incrementing the memory region object's reference count. >>>> As a result, the automatic deallocation of the object, not taking into >>>> account those references, results in use-after-free memory corruption. >>>> >>>> This patch increases the reference count of an owned memory region >>>> object >>>> on each memory_region_ref() and decreases it on each >>>> memory_region_unref(). >>>> >>>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> >>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>> --- >>>> >>>> V4 -> V5: >>>> - ref/unref only owned memory regions (Akihiko) >>>> >>>> softmmu/memory.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/softmmu/memory.c b/softmmu/memory.c >>>> index 7d9494ce70..15e1699750 100644 >>>> --- a/softmmu/memory.c >>>> +++ b/softmmu/memory.c >>>> @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr) >>>> /* MMIO callbacks most likely will access data that belongs >>>> * to the owner, hence the need to ref/unref the owner whenever >>>> * the memory region is in use. >>>> + * Likewise, the owner keeps references to the memory region, >>>> + * hence the need to ref/unref the memory region object to >>>> prevent >>>> + * its automatic deallocation while still referenced by its >>>> owner. >>> >>> This comment does not make sense. Traditionally no such automatic >>> deallocation happens so the owner has been always required to free >>> the memory region when it gets finalized. >>> >>> "[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands" >>> introduces a different kind of memory region, which can be freed >>> anytime before the device gets finalized. Even in this case, the >>> owner removes the reference to the memory owner by doing res->region >>> = NULL; >> >> Hi Akihiko, >> >> You are right, the word "owner" is not correct. The issue observed >> was due to the references kept in flatview ranges and the fact that >> flatview_destroy() is asynchronous and was called after memory >> region's destruction. >> >> If I replace the word "owner" with "memory subsystem" in the commit >> message and drop the comment, would that be ok with you? or do want >> to suggest something else? > > This will extend the lifetime of the memory region, but the underlying > memory is still synchronously freed. Can you show that the flatview > range will not be used to read the freed memory? Yes, the intention of this patch is to delay the mr object finalization until all memory_region_unref() on this mr have been taken place. What do you mean by "the underlying memory is still synchronously freed"?
On 2023/09/19 23:21, Xenia Ragiadakou wrote: > > On 19/9/23 13:44, Akihiko Odaki wrote: >> On 2023/09/19 19:28, Xenia Ragiadakou wrote: >>> >>> On 15/9/23 18:11, Akihiko Odaki wrote: >>>> On 2023/09/15 20:11, Huang Rui wrote: >>>>> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com> >>>>> >>>>> When the memory region has a different life-cycle from that of her >>>>> parent, >>>>> could be automatically released, once has been unparent and once >>>>> all of her >>>>> references have gone away, via the object's free callback. >>>>> >>>>> However, currently, references to the memory region are held by its >>>>> owner >>>>> without first incrementing the memory region object's reference count. >>>>> As a result, the automatic deallocation of the object, not taking into >>>>> account those references, results in use-after-free memory corruption. >>>>> >>>>> This patch increases the reference count of an owned memory region >>>>> object >>>>> on each memory_region_ref() and decreases it on each >>>>> memory_region_unref(). >>>>> >>>>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> >>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>>> --- >>>>> >>>>> V4 -> V5: >>>>> - ref/unref only owned memory regions (Akihiko) >>>>> >>>>> softmmu/memory.c | 5 +++++ >>>>> 1 file changed, 5 insertions(+) >>>>> >>>>> diff --git a/softmmu/memory.c b/softmmu/memory.c >>>>> index 7d9494ce70..15e1699750 100644 >>>>> --- a/softmmu/memory.c >>>>> +++ b/softmmu/memory.c >>>>> @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr) >>>>> /* MMIO callbacks most likely will access data that belongs >>>>> * to the owner, hence the need to ref/unref the owner whenever >>>>> * the memory region is in use. >>>>> + * Likewise, the owner keeps references to the memory region, >>>>> + * hence the need to ref/unref the memory region object to >>>>> prevent >>>>> + * its automatic deallocation while still referenced by its >>>>> owner. >>>> >>>> This comment does not make sense. Traditionally no such automatic >>>> deallocation happens so the owner has been always required to free >>>> the memory region when it gets finalized. >>>> >>>> "[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands" >>>> introduces a different kind of memory region, which can be freed >>>> anytime before the device gets finalized. Even in this case, the >>>> owner removes the reference to the memory owner by doing res->region >>>> = NULL; >>> >>> Hi Akihiko, >>> >>> You are right, the word "owner" is not correct. The issue observed >>> was due to the references kept in flatview ranges and the fact that >>> flatview_destroy() is asynchronous and was called after memory >>> region's destruction. >>> >>> If I replace the word "owner" with "memory subsystem" in the commit >>> message and drop the comment, would that be ok with you? or do want >>> to suggest something else? >> >> This will extend the lifetime of the memory region, but the underlying >> memory is still synchronously freed. Can you show that the flatview >> range will not be used to read the freed memory? > > Yes, the intention of this patch is to delay the mr object finalization > until all memory_region_unref() on this mr have been taken place. > > What do you mean by "the underlying memory is still synchronously freed"? > A pointer is passed to memory_region_init_ram_ptr() with the ptr parameter when initializing the memory region and the memory region keeps the pointer. In virtio_gpu_virgl_resource_unmap(), the memory pointed with the pointer is unmapped with virgl_renderer_resource_unmap() and makes the pointer kept by the memory region dangling though the lifetime of the memory region is extended with this patch. Can you show that the dangling pointer the memory region has will never be referenced?
On 20/9/23 01:18, Akihiko Odaki wrote: > On 2023/09/19 23:21, Xenia Ragiadakou wrote: >> >> On 19/9/23 13:44, Akihiko Odaki wrote: >>> On 2023/09/19 19:28, Xenia Ragiadakou wrote: >>>> >>>> On 15/9/23 18:11, Akihiko Odaki wrote: >>>>> On 2023/09/15 20:11, Huang Rui wrote: >>>>>> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com> >>>>>> >>>>>> When the memory region has a different life-cycle from that of >>>>>> her parent, >>>>>> could be automatically released, once has been unparent and once >>>>>> all of her >>>>>> references have gone away, via the object's free callback. >>>>>> >>>>>> However, currently, references to the memory region are held by >>>>>> its owner >>>>>> without first incrementing the memory region object's reference >>>>>> count. >>>>>> As a result, the automatic deallocation of the object, not taking >>>>>> into >>>>>> account those references, results in use-after-free memory >>>>>> corruption. >>>>>> >>>>>> This patch increases the reference count of an owned memory >>>>>> region object >>>>>> on each memory_region_ref() and decreases it on each >>>>>> memory_region_unref(). >>>>>> >>>>>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> >>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>>>> --- >>>>>> >>>>>> V4 -> V5: >>>>>> - ref/unref only owned memory regions (Akihiko) >>>>>> >>>>>> softmmu/memory.c | 5 +++++ >>>>>> 1 file changed, 5 insertions(+) >>>>>> >>>>>> diff --git a/softmmu/memory.c b/softmmu/memory.c >>>>>> index 7d9494ce70..15e1699750 100644 >>>>>> --- a/softmmu/memory.c >>>>>> +++ b/softmmu/memory.c >>>>>> @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr) >>>>>> /* MMIO callbacks most likely will access data that belongs >>>>>> * to the owner, hence the need to ref/unref the owner >>>>>> whenever >>>>>> * the memory region is in use. >>>>>> + * Likewise, the owner keeps references to the memory region, >>>>>> + * hence the need to ref/unref the memory region object to >>>>>> prevent >>>>>> + * its automatic deallocation while still referenced by its >>>>>> owner. >>>>> >>>>> This comment does not make sense. Traditionally no such automatic >>>>> deallocation happens so the owner has been always required to free >>>>> the memory region when it gets finalized. >>>>> >>>>> "[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands" >>>>> introduces a different kind of memory region, which can be freed >>>>> anytime before the device gets finalized. Even in this case, the >>>>> owner removes the reference to the memory owner by doing >>>>> res->region = NULL; >>>> >>>> Hi Akihiko, >>>> >>>> You are right, the word "owner" is not correct. The issue observed >>>> was due to the references kept in flatview ranges and the fact that >>>> flatview_destroy() is asynchronous and was called after memory >>>> region's destruction. >>>> >>>> If I replace the word "owner" with "memory subsystem" in the commit >>>> message and drop the comment, would that be ok with you? or do want >>>> to suggest something else? >>> >>> This will extend the lifetime of the memory region, but the >>> underlying memory is still synchronously freed. Can you show that >>> the flatview range will not be used to read the freed memory? >> >> Yes, the intention of this patch is to delay the mr object >> finalization until all memory_region_unref() on this mr have been >> taken place. >> >> What do you mean by "the underlying memory is still synchronously >> freed"? >> > > A pointer is passed to memory_region_init_ram_ptr() with the ptr > parameter when initializing the memory region and the memory region > keeps the pointer. > > In virtio_gpu_virgl_resource_unmap(), the memory pointed with the > pointer is unmapped with virgl_renderer_resource_unmap() and makes the > pointer kept by the memory region dangling though the lifetime of the > memory region is extended with this patch. Can you show that the > dangling pointer the memory region has will never be referenced? I see your point but I think it is not directly related to this patch. IMHO, it is related to the implementation of virtio_gpu_virgl_resource_unmap(). Maybe the unmapping should be done in the free callback. However, I would expect the pointer to a disabled memory region to not be used, not sure though.
On 2023/09/20 17:57, Xenia Ragiadakou wrote: > > On 20/9/23 01:18, Akihiko Odaki wrote: >> On 2023/09/19 23:21, Xenia Ragiadakou wrote: >>> >>> On 19/9/23 13:44, Akihiko Odaki wrote: >>>> On 2023/09/19 19:28, Xenia Ragiadakou wrote: >>>>> >>>>> On 15/9/23 18:11, Akihiko Odaki wrote: >>>>>> On 2023/09/15 20:11, Huang Rui wrote: >>>>>>> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com> >>>>>>> >>>>>>> When the memory region has a different life-cycle from that of >>>>>>> her parent, >>>>>>> could be automatically released, once has been unparent and once >>>>>>> all of her >>>>>>> references have gone away, via the object's free callback. >>>>>>> >>>>>>> However, currently, references to the memory region are held by >>>>>>> its owner >>>>>>> without first incrementing the memory region object's reference >>>>>>> count. >>>>>>> As a result, the automatic deallocation of the object, not taking >>>>>>> into >>>>>>> account those references, results in use-after-free memory >>>>>>> corruption. >>>>>>> >>>>>>> This patch increases the reference count of an owned memory >>>>>>> region object >>>>>>> on each memory_region_ref() and decreases it on each >>>>>>> memory_region_unref(). >>>>>>> >>>>>>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> >>>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>>>>> --- >>>>>>> >>>>>>> V4 -> V5: >>>>>>> - ref/unref only owned memory regions (Akihiko) >>>>>>> >>>>>>> softmmu/memory.c | 5 +++++ >>>>>>> 1 file changed, 5 insertions(+) >>>>>>> >>>>>>> diff --git a/softmmu/memory.c b/softmmu/memory.c >>>>>>> index 7d9494ce70..15e1699750 100644 >>>>>>> --- a/softmmu/memory.c >>>>>>> +++ b/softmmu/memory.c >>>>>>> @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr) >>>>>>> /* MMIO callbacks most likely will access data that belongs >>>>>>> * to the owner, hence the need to ref/unref the owner >>>>>>> whenever >>>>>>> * the memory region is in use. >>>>>>> + * Likewise, the owner keeps references to the memory region, >>>>>>> + * hence the need to ref/unref the memory region object to >>>>>>> prevent >>>>>>> + * its automatic deallocation while still referenced by its >>>>>>> owner. >>>>>> >>>>>> This comment does not make sense. Traditionally no such automatic >>>>>> deallocation happens so the owner has been always required to free >>>>>> the memory region when it gets finalized. >>>>>> >>>>>> "[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands" >>>>>> introduces a different kind of memory region, which can be freed >>>>>> anytime before the device gets finalized. Even in this case, the >>>>>> owner removes the reference to the memory owner by doing >>>>>> res->region = NULL; >>>>> >>>>> Hi Akihiko, >>>>> >>>>> You are right, the word "owner" is not correct. The issue observed >>>>> was due to the references kept in flatview ranges and the fact that >>>>> flatview_destroy() is asynchronous and was called after memory >>>>> region's destruction. >>>>> >>>>> If I replace the word "owner" with "memory subsystem" in the commit >>>>> message and drop the comment, would that be ok with you? or do want >>>>> to suggest something else? >>>> >>>> This will extend the lifetime of the memory region, but the >>>> underlying memory is still synchronously freed. Can you show that >>>> the flatview range will not be used to read the freed memory? >>> >>> Yes, the intention of this patch is to delay the mr object >>> finalization until all memory_region_unref() on this mr have been >>> taken place. >>> >>> What do you mean by "the underlying memory is still synchronously >>> freed"? >>> >> >> A pointer is passed to memory_region_init_ram_ptr() with the ptr >> parameter when initializing the memory region and the memory region >> keeps the pointer. >> >> In virtio_gpu_virgl_resource_unmap(), the memory pointed with the >> pointer is unmapped with virgl_renderer_resource_unmap() and makes the >> pointer kept by the memory region dangling though the lifetime of the >> memory region is extended with this patch. Can you show that the >> dangling pointer the memory region has will never be referenced? > > I see your point but I think it is not directly related to this patch. > IMHO, it is related to the implementation of > virtio_gpu_virgl_resource_unmap(). Maybe the unmapping should be done in > the free callback. However, I would expect the pointer to a disabled > memory region to not be used, not sure though. Unmapping in the free callback sounds good.
diff --git a/softmmu/memory.c b/softmmu/memory.c index 7d9494ce70..15e1699750 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr) /* MMIO callbacks most likely will access data that belongs * to the owner, hence the need to ref/unref the owner whenever * the memory region is in use. + * Likewise, the owner keeps references to the memory region, + * hence the need to ref/unref the memory region object to prevent + * its automatic deallocation while still referenced by its owner. * * The memory region is a child of its owner. As long as the * owner doesn't call unparent itself on the memory region, @@ -1808,6 +1811,7 @@ void memory_region_ref(MemoryRegion *mr) * we do not ref/unref them because it slows down DMA sensibly. */ if (mr && mr->owner) { + object_ref(OBJECT(mr)); object_ref(mr->owner); } } @@ -1816,6 +1820,7 @@ void memory_region_unref(MemoryRegion *mr) { if (mr && mr->owner) { object_unref(mr->owner); + object_unref(OBJECT(mr)); } }