[v3,12/20] virtio-gpu: fix hang under TCG when unmapping blob

From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

This commit fixes an indefinite hang when using VIRTIO GPU blob objects
under TCG in certain conditions.

The VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB VIRTIO command creates a
MemoryRegion and attaches it to an offset on a PCI BAR of the
VirtIOGPUdevice. The VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB command unmaps
it.

Because virglrenderer commands are not thread-safe they are only
called on the main context and QEMU performs the cleanup in three steps
to prevent a use-after-free scenario where the guest can access the
region after it’s unmapped:

1. From the main context, the region’s field finish_unmapping is false
   by default, so it sets a variable cmd_suspended, increases the
   renderer_blocked variable, deletes the blob subregion, and unparents
   the blob subregion causing its reference count to decrement.

2. From an RCU context, the MemoryView gets freed, the FlatView gets
   recalculated, the free callback of the blob region
   virtio_gpu_virgl_hostmem_region_free is called which sets the
   region’s field finish_unmapping to true, allowing the main thread
   context to finish replying to the command

3. From the main context, the command is processed again, but this time
   finish_unmapping is true, so virgl_renderer_resource_unmap can be
   called and a response is sent to the guest.

It happens so that under TCG, if the guest has no timers configured (and
thus no interrupt will cause the CPU to exit), the RCU thread does not
have enough time to grab the locks and recalculate the FlatView.

That’s not a big problem in practice since most guests will assume a
response will happen later in time and go on to do different things,
potentially triggering interrupts and allowing the RCU context to run.
If the guest waits for the unmap command to complete though, it blocks
indefinitely. Attaching to the QEMU monitor and force quitting the guest
allows the cleanup to continue.

There's no reason why the FlatView recalculation can't occur right away
when we delete the blob subregion, however. It does not, because when we
create the subregion we set the object as its own parent:

    memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);

The extra reference is what prevents freeing the memory region object in
the memory transaction of deleting the subregion.

This commit changes the owner object to the device, which removes the
extra owner reference in the memory region and causes the MR to be
freed right away in the main context.

Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20250410122643.1747913-3-manos.pitsidianakis@linaro.org>
Cc: qemu-stable@nongnu.org
---
 hw/display/virtio-gpu-virgl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On 2025/05/22 1:42, Alex Bennée wrote:
> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> 
> This commit fixes an indefinite hang when using VIRTIO GPU blob objects
> under TCG in certain conditions.
> 
> The VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB VIRTIO command creates a
> MemoryRegion and attaches it to an offset on a PCI BAR of the
> VirtIOGPUdevice. The VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB command unmaps
> it.
> 
> Because virglrenderer commands are not thread-safe they are only
> called on the main context and QEMU performs the cleanup in three steps
> to prevent a use-after-free scenario where the guest can access the
> region after it’s unmapped:
> 
> 1. From the main context, the region’s field finish_unmapping is false
>     by default, so it sets a variable cmd_suspended, increases the
>     renderer_blocked variable, deletes the blob subregion, and unparents
>     the blob subregion causing its reference count to decrement.
> 
> 2. From an RCU context, the MemoryView gets freed, the FlatView gets
>     recalculated, the free callback of the blob region
>     virtio_gpu_virgl_hostmem_region_free is called which sets the
>     region’s field finish_unmapping to true, allowing the main thread
>     context to finish replying to the command
> 
> 3. From the main context, the command is processed again, but this time
>     finish_unmapping is true, so virgl_renderer_resource_unmap can be
>     called and a response is sent to the guest.
> 
> It happens so that under TCG, if the guest has no timers configured (and
> thus no interrupt will cause the CPU to exit), the RCU thread does not
> have enough time to grab the locks and recalculate the FlatView.
> 
> That’s not a big problem in practice since most guests will assume a
> response will happen later in time and go on to do different things,
> potentially triggering interrupts and allowing the RCU context to run.
> If the guest waits for the unmap command to complete though, it blocks
> indefinitely. Attaching to the QEMU monitor and force quitting the guest
> allows the cleanup to continue.
> 
> There's no reason why the FlatView recalculation can't occur right away
> when we delete the blob subregion, however. It does not, because when we
> create the subregion we set the object as its own parent:
> 
>      memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
> 
> The extra reference is what prevents freeing the memory region object in
> the memory transaction of deleting the subregion.
> 
> This commit changes the owner object to the device, which removes the
> extra owner reference in the memory region and causes the MR to be
> freed right away in the main context.
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Tested-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20250410122643.1747913-3-manos.pitsidianakis@linaro.org>
> Cc: qemu-stable@nongnu.org
> ---
>   hw/display/virtio-gpu-virgl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 71a7500de9..8fbe4e70cc 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -112,7 +112,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>       vmr->g = g;
>       mr = g_new0(MemoryRegion, 1);
>   
> -    memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
> +    memory_region_init_ram_ptr(mr, OBJECT(g), "blob", size, data);
>       memory_region_add_subregion(&b->hostmem, offset, mr);
>       memory_region_set_enabled(mr, true);
>   

I suggest dropping this patch for now due to the reason I pointed out 
for the first version of this series.

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> On 2025/05/22 1:42, Alex Bennée wrote:
>> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>> This commit fixes an indefinite hang when using VIRTIO GPU blob
>> objects
>> under TCG in certain conditions.
>> The VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB VIRTIO command creates a
>> MemoryRegion and attaches it to an offset on a PCI BAR of the
>> VirtIOGPUdevice. The VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB command unmaps
>> it.
>> Because virglrenderer commands are not thread-safe they are only
>> called on the main context and QEMU performs the cleanup in three steps
>> to prevent a use-after-free scenario where the guest can access the
>> region after it’s unmapped:
>> 1. From the main context, the region’s field finish_unmapping is
>> false
>>     by default, so it sets a variable cmd_suspended, increases the
>>     renderer_blocked variable, deletes the blob subregion, and unparents
>>     the blob subregion causing its reference count to decrement.
>> 2. From an RCU context, the MemoryView gets freed, the FlatView gets
>>     recalculated, the free callback of the blob region
>>     virtio_gpu_virgl_hostmem_region_free is called which sets the
>>     region’s field finish_unmapping to true, allowing the main thread
>>     context to finish replying to the command
>> 3. From the main context, the command is processed again, but this
>> time
>>     finish_unmapping is true, so virgl_renderer_resource_unmap can be
>>     called and a response is sent to the guest.
>> It happens so that under TCG, if the guest has no timers configured
>> (and
>> thus no interrupt will cause the CPU to exit), the RCU thread does not
>> have enough time to grab the locks and recalculate the FlatView.
>> That’s not a big problem in practice since most guests will assume a
>> response will happen later in time and go on to do different things,
>> potentially triggering interrupts and allowing the RCU context to run.
>> If the guest waits for the unmap command to complete though, it blocks
>> indefinitely. Attaching to the QEMU monitor and force quitting the guest
>> allows the cleanup to continue.
>> There's no reason why the FlatView recalculation can't occur right
>> away
>> when we delete the blob subregion, however. It does not, because when we
>> create the subregion we set the object as its own parent:
>>      memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
>> The extra reference is what prevents freeing the memory region
>> object in
>> the memory transaction of deleting the subregion.
>> This commit changes the owner object to the device, which removes
>> the
>> extra owner reference in the memory region and causes the MR to be
>> freed right away in the main context.
>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-Id: <20250410122643.1747913-3-manos.pitsidianakis@linaro.org>
>> Cc: qemu-stable@nongnu.org
>> ---
>>   hw/display/virtio-gpu-virgl.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/hw/display/virtio-gpu-virgl.c
>> b/hw/display/virtio-gpu-virgl.c
>> index 71a7500de9..8fbe4e70cc 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -112,7 +112,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>       vmr->g = g;
>>       mr = g_new0(MemoryRegion, 1);
>>   -    memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size,
>> data);
>> +    memory_region_init_ram_ptr(mr, OBJECT(g), "blob", size, data);
>>       memory_region_add_subregion(&b->hostmem, offset, mr);
>>       memory_region_set_enabled(mr, true);
>>   
>
> I suggest dropping this patch for now due to the reason I pointed out
> for the first version of this series.

This fixes an actual bug - without it we get a hang.

On 2025/05/22 15:45, Alex Bennée wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> 
>> On 2025/05/22 1:42, Alex Bennée wrote:
>>> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>> This commit fixes an indefinite hang when using VIRTIO GPU blob
>>> objects
>>> under TCG in certain conditions.
>>> The VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB VIRTIO command creates a
>>> MemoryRegion and attaches it to an offset on a PCI BAR of the
>>> VirtIOGPUdevice. The VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB command unmaps
>>> it.
>>> Because virglrenderer commands are not thread-safe they are only
>>> called on the main context and QEMU performs the cleanup in three steps
>>> to prevent a use-after-free scenario where the guest can access the
>>> region after it’s unmapped:
>>> 1. From the main context, the region’s field finish_unmapping is
>>> false
>>>      by default, so it sets a variable cmd_suspended, increases the
>>>      renderer_blocked variable, deletes the blob subregion, and unparents
>>>      the blob subregion causing its reference count to decrement.
>>> 2. From an RCU context, the MemoryView gets freed, the FlatView gets
>>>      recalculated, the free callback of the blob region
>>>      virtio_gpu_virgl_hostmem_region_free is called which sets the
>>>      region’s field finish_unmapping to true, allowing the main thread
>>>      context to finish replying to the command
>>> 3. From the main context, the command is processed again, but this
>>> time
>>>      finish_unmapping is true, so virgl_renderer_resource_unmap can be
>>>      called and a response is sent to the guest.
>>> It happens so that under TCG, if the guest has no timers configured
>>> (and
>>> thus no interrupt will cause the CPU to exit), the RCU thread does not
>>> have enough time to grab the locks and recalculate the FlatView.
>>> That’s not a big problem in practice since most guests will assume a
>>> response will happen later in time and go on to do different things,
>>> potentially triggering interrupts and allowing the RCU context to run.
>>> If the guest waits for the unmap command to complete though, it blocks
>>> indefinitely. Attaching to the QEMU monitor and force quitting the guest
>>> allows the cleanup to continue.
>>> There's no reason why the FlatView recalculation can't occur right
>>> away
>>> when we delete the blob subregion, however. It does not, because when we
>>> create the subregion we set the object as its own parent:
>>>       memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
>>> The extra reference is what prevents freeing the memory region
>>> object in
>>> the memory transaction of deleting the subregion.
>>> This commit changes the owner object to the device, which removes
>>> the
>>> extra owner reference in the memory region and causes the MR to be
>>> freed right away in the main context.
>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>>> Message-Id: <20250410122643.1747913-3-manos.pitsidianakis@linaro.org>
>>> Cc: qemu-stable@nongnu.org
>>> ---
>>>    hw/display/virtio-gpu-virgl.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>> diff --git a/hw/display/virtio-gpu-virgl.c
>>> b/hw/display/virtio-gpu-virgl.c
>>> index 71a7500de9..8fbe4e70cc 100644
>>> --- a/hw/display/virtio-gpu-virgl.c
>>> +++ b/hw/display/virtio-gpu-virgl.c
>>> @@ -112,7 +112,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>>        vmr->g = g;
>>>        mr = g_new0(MemoryRegion, 1);
>>>    -    memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size,
>>> data);
>>> +    memory_region_init_ram_ptr(mr, OBJECT(g), "blob", size, data);
>>>        memory_region_add_subregion(&b->hostmem, offset, mr);
>>>        memory_region_set_enabled(mr, true);
>>>    
>>
>> I suggest dropping this patch for now due to the reason I pointed out
>> for the first version of this series.
> 
> This fixes an actual bug - without it we get a hang.
> 

I understand that but it also introduces a regression; "[PATCH v3 14/20] 
ui/gtk-gl-area: Remove extra draw call in refresh" is also a similar case.

Ideally such a bug should be fixed without regression, but I understand 
it is sometimes difficult to do that and postponing the bug resolution 
until figuring out the correct way does not make sense.

In such a case, a bug should be fixed minimizing the regression and the 
documentation of the regression should be left in the code.

In particular, this patch can cause use-after-free whether TCG is used 
or not. Instead, I suggest to avoid freeing memory regions at all on 
TCG. It will surely leak memory, but won't result in use-after-free at 
least and the other accelerators will be unaffected.

Regards,
Akihiko Odaki

On Thu, May 22, 2025 at 10:03 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2025/05/22 15:45, Alex Bennée wrote:
> > Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> >
> >> On 2025/05/22 1:42, Alex Bennée wrote:
> >>> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> >>> This commit fixes an indefinite hang when using VIRTIO GPU blob
> >>> objects
> >>> under TCG in certain conditions.
> >>> The VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB VIRTIO command creates a
> >>> MemoryRegion and attaches it to an offset on a PCI BAR of the
> >>> VirtIOGPUdevice. The VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB command unmaps
> >>> it.
> >>> Because virglrenderer commands are not thread-safe they are only
> >>> called on the main context and QEMU performs the cleanup in three steps
> >>> to prevent a use-after-free scenario where the guest can access the
> >>> region after it’s unmapped:
> >>> 1. From the main context, the region’s field finish_unmapping is
> >>> false
> >>>      by default, so it sets a variable cmd_suspended, increases the
> >>>      renderer_blocked variable, deletes the blob subregion, and unparents
> >>>      the blob subregion causing its reference count to decrement.
> >>> 2. From an RCU context, the MemoryView gets freed, the FlatView gets
> >>>      recalculated, the free callback of the blob region
> >>>      virtio_gpu_virgl_hostmem_region_free is called which sets the
> >>>      region’s field finish_unmapping to true, allowing the main thread
> >>>      context to finish replying to the command
> >>> 3. From the main context, the command is processed again, but this
> >>> time
> >>>      finish_unmapping is true, so virgl_renderer_resource_unmap can be
> >>>      called and a response is sent to the guest.
> >>> It happens so that under TCG, if the guest has no timers configured
> >>> (and
> >>> thus no interrupt will cause the CPU to exit), the RCU thread does not
> >>> have enough time to grab the locks and recalculate the FlatView.
> >>> That’s not a big problem in practice since most guests will assume a
> >>> response will happen later in time and go on to do different things,
> >>> potentially triggering interrupts and allowing the RCU context to run.
> >>> If the guest waits for the unmap command to complete though, it blocks
> >>> indefinitely. Attaching to the QEMU monitor and force quitting the guest
> >>> allows the cleanup to continue.
> >>> There's no reason why the FlatView recalculation can't occur right
> >>> away
> >>> when we delete the blob subregion, however. It does not, because when we
> >>> create the subregion we set the object as its own parent:
> >>>       memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
> >>> The extra reference is what prevents freeing the memory region
> >>> object in
> >>> the memory transaction of deleting the subregion.
> >>> This commit changes the owner object to the device, which removes
> >>> the
> >>> extra owner reference in the memory region and causes the MR to be
> >>> freed right away in the main context.
> >>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> >>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> >>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
> >>> Message-Id: <20250410122643.1747913-3-manos.pitsidianakis@linaro.org>
> >>> Cc: qemu-stable@nongnu.org
> >>> ---
> >>>    hw/display/virtio-gpu-virgl.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>> diff --git a/hw/display/virtio-gpu-virgl.c
> >>> b/hw/display/virtio-gpu-virgl.c
> >>> index 71a7500de9..8fbe4e70cc 100644
> >>> --- a/hw/display/virtio-gpu-virgl.c
> >>> +++ b/hw/display/virtio-gpu-virgl.c
> >>> @@ -112,7 +112,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
> >>>        vmr->g = g;
> >>>        mr = g_new0(MemoryRegion, 1);
> >>>    -    memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size,
> >>> data);
> >>> +    memory_region_init_ram_ptr(mr, OBJECT(g), "blob", size, data);
> >>>        memory_region_add_subregion(&b->hostmem, offset, mr);
> >>>        memory_region_set_enabled(mr, true);
> >>>
> >>
> >> I suggest dropping this patch for now due to the reason I pointed out
> >> for the first version of this series.
> >
> > This fixes an actual bug - without it we get a hang.
> >
>
> I understand that but it also introduces a regression; "[PATCH v3 14/20]
> ui/gtk-gl-area: Remove extra draw call in refresh" is also a similar case.
>
> Ideally such a bug should be fixed without regression, but I understand
> it is sometimes difficult to do that and postponing the bug resolution
> until figuring out the correct way does not make sense.
>
> In such a case, a bug should be fixed minimizing the regression and the
> documentation of the regression should be left in the code.
>
> In particular, this patch can cause use-after-free whether TCG is used
> or not. Instead, I suggest to avoid freeing memory regions at all on
> TCG. It will surely leak memory, but won't result in use-after-free at
> least and the other accelerators will be unaffected.
>
> Regards,
> Akihiko Odaki

We tested this fix with ASAN and didn't see anything. Do you have a
test case in mind that can reproduce this use-after-free? It'd help
make a certain decision on whether to drop this patch or not. I'm not
doubting that this can cause a use-after-free by the way, it's just
that it is hypothetical only. If it causes a use-after-free for sure
we should definitely drop it.

> Instead, I suggest to avoid freeing memory regions at all on
> TCG. It will surely leak memory, but won't result in use-after-free at
> least and the other accelerators will be unaffected.

Leaking memory for blob objects is also not ideal, since they are
frequently allocated. It's memory-safe but the leak can accumulate
over time.

On 2025/05/22 16:31, Manos Pitsidianakis wrote:
> On Thu, May 22, 2025 at 10:03 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2025/05/22 15:45, Alex Bennée wrote:
>>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>>>
>>>> On 2025/05/22 1:42, Alex Bennée wrote:
>>>>> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>>>> This commit fixes an indefinite hang when using VIRTIO GPU blob
>>>>> objects
>>>>> under TCG in certain conditions.
>>>>> The VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB VIRTIO command creates a
>>>>> MemoryRegion and attaches it to an offset on a PCI BAR of the
>>>>> VirtIOGPUdevice. The VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB command unmaps
>>>>> it.
>>>>> Because virglrenderer commands are not thread-safe they are only
>>>>> called on the main context and QEMU performs the cleanup in three steps
>>>>> to prevent a use-after-free scenario where the guest can access the
>>>>> region after it’s unmapped:
>>>>> 1. From the main context, the region’s field finish_unmapping is
>>>>> false
>>>>>       by default, so it sets a variable cmd_suspended, increases the
>>>>>       renderer_blocked variable, deletes the blob subregion, and unparents
>>>>>       the blob subregion causing its reference count to decrement.
>>>>> 2. From an RCU context, the MemoryView gets freed, the FlatView gets
>>>>>       recalculated, the free callback of the blob region
>>>>>       virtio_gpu_virgl_hostmem_region_free is called which sets the
>>>>>       region’s field finish_unmapping to true, allowing the main thread
>>>>>       context to finish replying to the command
>>>>> 3. From the main context, the command is processed again, but this
>>>>> time
>>>>>       finish_unmapping is true, so virgl_renderer_resource_unmap can be
>>>>>       called and a response is sent to the guest.
>>>>> It happens so that under TCG, if the guest has no timers configured
>>>>> (and
>>>>> thus no interrupt will cause the CPU to exit), the RCU thread does not
>>>>> have enough time to grab the locks and recalculate the FlatView.
>>>>> That’s not a big problem in practice since most guests will assume a
>>>>> response will happen later in time and go on to do different things,
>>>>> potentially triggering interrupts and allowing the RCU context to run.
>>>>> If the guest waits for the unmap command to complete though, it blocks
>>>>> indefinitely. Attaching to the QEMU monitor and force quitting the guest
>>>>> allows the cleanup to continue.
>>>>> There's no reason why the FlatView recalculation can't occur right
>>>>> away
>>>>> when we delete the blob subregion, however. It does not, because when we
>>>>> create the subregion we set the object as its own parent:
>>>>>        memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
>>>>> The extra reference is what prevents freeing the memory region
>>>>> object in
>>>>> the memory transaction of deleting the subregion.
>>>>> This commit changes the owner object to the device, which removes
>>>>> the
>>>>> extra owner reference in the memory region and causes the MR to be
>>>>> freed right away in the main context.
>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>>>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>>>>> Message-Id: <20250410122643.1747913-3-manos.pitsidianakis@linaro.org>
>>>>> Cc: qemu-stable@nongnu.org
>>>>> ---
>>>>>     hw/display/virtio-gpu-virgl.c | 2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>> diff --git a/hw/display/virtio-gpu-virgl.c
>>>>> b/hw/display/virtio-gpu-virgl.c
>>>>> index 71a7500de9..8fbe4e70cc 100644
>>>>> --- a/hw/display/virtio-gpu-virgl.c
>>>>> +++ b/hw/display/virtio-gpu-virgl.c
>>>>> @@ -112,7 +112,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>>>>         vmr->g = g;
>>>>>         mr = g_new0(MemoryRegion, 1);
>>>>>     -    memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size,
>>>>> data);
>>>>> +    memory_region_init_ram_ptr(mr, OBJECT(g), "blob", size, data);
>>>>>         memory_region_add_subregion(&b->hostmem, offset, mr);
>>>>>         memory_region_set_enabled(mr, true);
>>>>>
>>>>
>>>> I suggest dropping this patch for now due to the reason I pointed out
>>>> for the first version of this series.
>>>
>>> This fixes an actual bug - without it we get a hang.
>>>
>>
>> I understand that but it also introduces a regression; "[PATCH v3 14/20]
>> ui/gtk-gl-area: Remove extra draw call in refresh" is also a similar case.
>>
>> Ideally such a bug should be fixed without regression, but I understand
>> it is sometimes difficult to do that and postponing the bug resolution
>> until figuring out the correct way does not make sense.
>>
>> In such a case, a bug should be fixed minimizing the regression and the
>> documentation of the regression should be left in the code.
>>
>> In particular, this patch can cause use-after-free whether TCG is used
>> or not. Instead, I suggest to avoid freeing memory regions at all on
>> TCG. It will surely leak memory, but won't result in use-after-free at
>> least and the other accelerators will be unaffected.
>>
>> Regards,
>> Akihiko Odaki
> 
> We tested this fix with ASAN and didn't see anything. Do you have a
> test case in mind that can reproduce this use-after-free? It'd help
> make a certain decision on whether to drop this patch or not. I'm not
> doubting that this can cause a use-after-free by the way, it's just
> that it is hypothetical only. If it causes a use-after-free for sure
> we should definitely drop it.

No, I don't have a test case and it should rarely occur. More 
concretely, a UAF occurs if the guest accesses the memory region while 
trying to unmap it. It is just a theory indeed, but the theory says the 
UAF is possible.

> 
>> Instead, I suggest to avoid freeing memory regions at all on
>> TCG. It will surely leak memory, but won't result in use-after-free at
>> least and the other accelerators will be unaffected.
> 
> Leaking memory for blob objects is also not ideal, since they are
> frequently allocated. It's memory-safe but the leak can accumulate
> over time.
 >

Memory safety and leak free cannot be compatible unless RCU is fixed. We 
need to choose either of them.

Regards,
Akihiko Odaki

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> On 2025/05/22 16:31, Manos Pitsidianakis wrote:
>> On Thu, May 22, 2025 at 10:03 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>
>>> On 2025/05/22 15:45, Alex Bennée wrote:
>>>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>>>>
>>>>> On 2025/05/22 1:42, Alex Bennée wrote:
>>>>>> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>>>>> This commit fixes an indefinite hang when using VIRTIO GPU blob
>>>>>> objects
>>>>>> under TCG in certain conditions.
>>>>>> The VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB VIRTIO command creates a
>>>>>> MemoryRegion and attaches it to an offset on a PCI BAR of the
>>>>>> VirtIOGPUdevice. The VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB command unmaps
>>>>>> it.
>>>>>> Because virglrenderer commands are not thread-safe they are only
>>>>>> called on the main context and QEMU performs the cleanup in three steps
>>>>>> to prevent a use-after-free scenario where the guest can access the
>>>>>> region after it’s unmapped:
>>>>>> 1. From the main context, the region’s field finish_unmapping is
>>>>>> false
>>>>>>       by default, so it sets a variable cmd_suspended, increases the
>>>>>>       renderer_blocked variable, deletes the blob subregion, and unparents
>>>>>>       the blob subregion causing its reference count to decrement.
>>>>>> 2. From an RCU context, the MemoryView gets freed, the FlatView gets
>>>>>>       recalculated, the free callback of the blob region
>>>>>>       virtio_gpu_virgl_hostmem_region_free is called which sets the
>>>>>>       region’s field finish_unmapping to true, allowing the main thread
>>>>>>       context to finish replying to the command
>>>>>> 3. From the main context, the command is processed again, but this
>>>>>> time
>>>>>>       finish_unmapping is true, so virgl_renderer_resource_unmap can be
>>>>>>       called and a response is sent to the guest.
>>>>>> It happens so that under TCG, if the guest has no timers configured
>>>>>> (and
>>>>>> thus no interrupt will cause the CPU to exit), the RCU thread does not
>>>>>> have enough time to grab the locks and recalculate the FlatView.
>>>>>> That’s not a big problem in practice since most guests will assume a
>>>>>> response will happen later in time and go on to do different things,
>>>>>> potentially triggering interrupts and allowing the RCU context to run.
>>>>>> If the guest waits for the unmap command to complete though, it blocks
>>>>>> indefinitely. Attaching to the QEMU monitor and force quitting the guest
>>>>>> allows the cleanup to continue.
>>>>>> There's no reason why the FlatView recalculation can't occur right
>>>>>> away
>>>>>> when we delete the blob subregion, however. It does not, because when we
>>>>>> create the subregion we set the object as its own parent:
>>>>>>        memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
>>>>>> The extra reference is what prevents freeing the memory region
>>>>>> object in
>>>>>> the memory transaction of deleting the subregion.
>>>>>> This commit changes the owner object to the device, which removes
>>>>>> the
>>>>>> extra owner reference in the memory region and causes the MR to be
>>>>>> freed right away in the main context.
>>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>>>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>>>>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>>>>>> Message-Id: <20250410122643.1747913-3-manos.pitsidianakis@linaro.org>
>>>>>> Cc: qemu-stable@nongnu.org
>>>>>> ---
>>>>>>     hw/display/virtio-gpu-virgl.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>> diff --git a/hw/display/virtio-gpu-virgl.c
>>>>>> b/hw/display/virtio-gpu-virgl.c
>>>>>> index 71a7500de9..8fbe4e70cc 100644
>>>>>> --- a/hw/display/virtio-gpu-virgl.c
>>>>>> +++ b/hw/display/virtio-gpu-virgl.c
>>>>>> @@ -112,7 +112,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>>>>>         vmr->g = g;
>>>>>>         mr = g_new0(MemoryRegion, 1);
>>>>>>     -    memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size,
>>>>>> data);
>>>>>> +    memory_region_init_ram_ptr(mr, OBJECT(g), "blob", size, data);
>>>>>>         memory_region_add_subregion(&b->hostmem, offset, mr);
>>>>>>         memory_region_set_enabled(mr, true);
>>>>>>
>>>>>
>>>>> I suggest dropping this patch for now due to the reason I pointed out
>>>>> for the first version of this series.
>>>>
>>>> This fixes an actual bug - without it we get a hang.
>>>>
>>>
>>> I understand that but it also introduces a regression; "[PATCH v3 14/20]
>>> ui/gtk-gl-area: Remove extra draw call in refresh" is also a similar case.
>>>
>>> Ideally such a bug should be fixed without regression, but I understand
>>> it is sometimes difficult to do that and postponing the bug resolution
>>> until figuring out the correct way does not make sense.
>>>
>>> In such a case, a bug should be fixed minimizing the regression and the
>>> documentation of the regression should be left in the code.
>>>
>>> In particular, this patch can cause use-after-free whether TCG is used
>>> or not. Instead, I suggest to avoid freeing memory regions at all on
>>> TCG. It will surely leak memory, but won't result in use-after-free at
>>> least and the other accelerators will be unaffected.
>>>
>>> Regards,
>>> Akihiko Odaki
>> We tested this fix with ASAN and didn't see anything. Do you have a
>> test case in mind that can reproduce this use-after-free? It'd help
>> make a certain decision on whether to drop this patch or not. I'm not
>> doubting that this can cause a use-after-free by the way, it's just
>> that it is hypothetical only. If it causes a use-after-free for sure
>> we should definitely drop it.
>
> No, I don't have a test case and it should rarely occur. More
> concretely, a UAF occurs if the guest accesses the memory region while
> trying to unmap it. It is just a theory indeed, but the theory says
> the UAF is possible.

I have a test case this fixes which I think trumps a theoretical UAF
without a test case.

Why would the guest attempt to access after triggering the free itself?
Wouldn't it be correct to fault the guest for violating its own memory
safety rules?

>>> Instead, I suggest to avoid freeing memory regions at all on
>>> TCG. It will surely leak memory, but won't result in use-after-free at
>>> least and the other accelerators will be unaffected.
>> Leaking memory for blob objects is also not ideal, since they are
>> frequently allocated. It's memory-safe but the leak can accumulate
>> over time.
>>
>
> Memory safety and leak free cannot be compatible unless RCU is fixed.
> We need to choose either of them.

How can the guest access something that is now unmapped? The RCU should
only run after the flatview has been updated.

>
> Regards,
> Akihiko Odaki

On 2025/05/22 18:28, Alex Bennée wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> 
>> On 2025/05/22 16:31, Manos Pitsidianakis wrote:
>>> On Thu, May 22, 2025 at 10:03 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> On 2025/05/22 15:45, Alex Bennée wrote:
>>>>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>>>>>
>>>>>> On 2025/05/22 1:42, Alex Bennée wrote:
>>>>>>> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>>>>>> This commit fixes an indefinite hang when using VIRTIO GPU blob
>>>>>>> objects
>>>>>>> under TCG in certain conditions.
>>>>>>> The VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB VIRTIO command creates a
>>>>>>> MemoryRegion and attaches it to an offset on a PCI BAR of the
>>>>>>> VirtIOGPUdevice. The VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB command unmaps
>>>>>>> it.
>>>>>>> Because virglrenderer commands are not thread-safe they are only
>>>>>>> called on the main context and QEMU performs the cleanup in three steps
>>>>>>> to prevent a use-after-free scenario where the guest can access the
>>>>>>> region after it’s unmapped:
>>>>>>> 1. From the main context, the region’s field finish_unmapping is
>>>>>>> false
>>>>>>>        by default, so it sets a variable cmd_suspended, increases the
>>>>>>>        renderer_blocked variable, deletes the blob subregion, and unparents
>>>>>>>        the blob subregion causing its reference count to decrement.
>>>>>>> 2. From an RCU context, the MemoryView gets freed, the FlatView gets
>>>>>>>        recalculated, the free callback of the blob region
>>>>>>>        virtio_gpu_virgl_hostmem_region_free is called which sets the
>>>>>>>        region’s field finish_unmapping to true, allowing the main thread
>>>>>>>        context to finish replying to the command
>>>>>>> 3. From the main context, the command is processed again, but this
>>>>>>> time
>>>>>>>        finish_unmapping is true, so virgl_renderer_resource_unmap can be
>>>>>>>        called and a response is sent to the guest.
>>>>>>> It happens so that under TCG, if the guest has no timers configured
>>>>>>> (and
>>>>>>> thus no interrupt will cause the CPU to exit), the RCU thread does not
>>>>>>> have enough time to grab the locks and recalculate the FlatView.
>>>>>>> That’s not a big problem in practice since most guests will assume a
>>>>>>> response will happen later in time and go on to do different things,
>>>>>>> potentially triggering interrupts and allowing the RCU context to run.
>>>>>>> If the guest waits for the unmap command to complete though, it blocks
>>>>>>> indefinitely. Attaching to the QEMU monitor and force quitting the guest
>>>>>>> allows the cleanup to continue.
>>>>>>> There's no reason why the FlatView recalculation can't occur right
>>>>>>> away
>>>>>>> when we delete the blob subregion, however. It does not, because when we
>>>>>>> create the subregion we set the object as its own parent:
>>>>>>>         memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
>>>>>>> The extra reference is what prevents freeing the memory region
>>>>>>> object in
>>>>>>> the memory transaction of deleting the subregion.
>>>>>>> This commit changes the owner object to the device, which removes
>>>>>>> the
>>>>>>> extra owner reference in the memory region and causes the MR to be
>>>>>>> freed right away in the main context.
>>>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>>>>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>>>>>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>>>>>>> Message-Id: <20250410122643.1747913-3-manos.pitsidianakis@linaro.org>
>>>>>>> Cc: qemu-stable@nongnu.org
>>>>>>> ---
>>>>>>>      hw/display/virtio-gpu-virgl.c | 2 +-
>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>> diff --git a/hw/display/virtio-gpu-virgl.c
>>>>>>> b/hw/display/virtio-gpu-virgl.c
>>>>>>> index 71a7500de9..8fbe4e70cc 100644
>>>>>>> --- a/hw/display/virtio-gpu-virgl.c
>>>>>>> +++ b/hw/display/virtio-gpu-virgl.c
>>>>>>> @@ -112,7 +112,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>>>>>>          vmr->g = g;
>>>>>>>          mr = g_new0(MemoryRegion, 1);
>>>>>>>      -    memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size,
>>>>>>> data);
>>>>>>> +    memory_region_init_ram_ptr(mr, OBJECT(g), "blob", size, data);
>>>>>>>          memory_region_add_subregion(&b->hostmem, offset, mr);
>>>>>>>          memory_region_set_enabled(mr, true);
>>>>>>>
>>>>>>
>>>>>> I suggest dropping this patch for now due to the reason I pointed out
>>>>>> for the first version of this series.
>>>>>
>>>>> This fixes an actual bug - without it we get a hang.
>>>>>
>>>>
>>>> I understand that but it also introduces a regression; "[PATCH v3 14/20]
>>>> ui/gtk-gl-area: Remove extra draw call in refresh" is also a similar case.
>>>>
>>>> Ideally such a bug should be fixed without regression, but I understand
>>>> it is sometimes difficult to do that and postponing the bug resolution
>>>> until figuring out the correct way does not make sense.
>>>>
>>>> In such a case, a bug should be fixed minimizing the regression and the
>>>> documentation of the regression should be left in the code.
>>>>
>>>> In particular, this patch can cause use-after-free whether TCG is used
>>>> or not. Instead, I suggest to avoid freeing memory regions at all on
>>>> TCG. It will surely leak memory, but won't result in use-after-free at
>>>> least and the other accelerators will be unaffected.
>>>>
>>>> Regards,
>>>> Akihiko Odaki
>>> We tested this fix with ASAN and didn't see anything. Do you have a
>>> test case in mind that can reproduce this use-after-free? It'd help
>>> make a certain decision on whether to drop this patch or not. I'm not
>>> doubting that this can cause a use-after-free by the way, it's just
>>> that it is hypothetical only. If it causes a use-after-free for sure
>>> we should definitely drop it.
>>
>> No, I don't have a test case and it should rarely occur. More
>> concretely, a UAF occurs if the guest accesses the memory region while
>> trying to unmap it. It is just a theory indeed, but the theory says
>> the UAF is possible.
> 
> I have a test case this fixes which I think trumps a theoretical UAF
> without a test case.
> 
> Why would the guest attempt to access after triggering the free itself?
> Wouldn't it be correct to fault the guest for violating its own memory
> safety rules?

docs/devel/secure-coding-practices.rst says "Unexpected accesses must 
not cause memory corruption or leaks in QEMU".

I'm not completely sure whether it is safe without concurrent accesses 
either. In particular, KVM does not immediately update the guest memory 
mapping, so it may result in a time window where the guest memory is 
mapped to an unmapped host memory region, and I suspect that could cause 
a problem. That also motivates limiting the scope of the change to TCG.

> 
>>>> Instead, I suggest to avoid freeing memory regions at all on
>>>> TCG. It will surely leak memory, but won't result in use-after-free at
>>>> least and the other accelerators will be unaffected.
>>> Leaking memory for blob objects is also not ideal, since they are
>>> frequently allocated. It's memory-safe but the leak can accumulate
>>> over time.
>>>
>>
>> Memory safety and leak free cannot be compatible unless RCU is fixed.
>> We need to choose either of them.
> 
> How can the guest access something that is now unmapped? The RCU should
> only run after the flatview has been updated.

This patch bypasses RCU. That's why it solves the hang even though the 
RCU itself is not fixed.

Let me summarize the theory and the actual behavior below:

The theory is that RCU satisfies the common requirement of concurrent 
algorithms. More concretely:
1) It is race-free; for RCU, it means it prevents use-after-free.
2) It does not prevent forward progress.

The patch message suggests 2) is not satisfied. A proper fix would be to 
change RCU to satisfy 2).

However, this patch workarounds the problem by circumventing RCU, which 
solves 2) but it regresses 1).

My suggestion is to document and to limit the impact of 1) by:
a) Limiting the scope of the change to TCG.
b) Not freeing memory regions, which will prevent use-after-free while 
leaking memory.

Manos said b) can be problematic because mappings are frequently 
created. Whether b) makes sense or not depends on the probability and 
impact of UAF and memory leak.

Regards,
Akihiko Odaki