Hello,
On 4/13/24 14:57, Akihiko Odaki wrote:
...
>> +static void
>> +virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>> + struct
>> virtio_gpu_simple_resource *res)
>> +{
>> + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>> +
>> + if (!res->mr) {
>> + return;
>> + }
>> +
>> + memory_region_set_enabled(res->mr, false);
>> + memory_region_del_subregion(&b->hostmem, res->mr);
>> +
>> + /* memory region owns res->mr object and frees it when mr is
>> released */
>> + res->mr = NULL;
>> +
>> + virgl_renderer_resource_unmap(res->resource_id);
>
> Hi,
>
> First, thanks for keeping working on this.
>
> This patch has some changes since the previous version, but it is still
> vulnerable to the race condition pointed out. The memory region is
> asynchronously unmapped from the guest address space, but the backing
> memory on the host address space is unmapped synchronously before that.
> This results in use-after-free. The whole unmapping operation needs to
> be implemented in an asynchronous manner.
Thanks for the clarification! I missed this point from the previous
discussion.
Could you please clarify what do you mean by the "asynchronous manner"?
Virglrenderer API works only in the virtio-gpu-gl context, it can't be
accessed from other places.
The memory_region_del_subregion() should remove the region as long as
nobody references it, isn't it? On Linux guest nobody should reference
hostmem regions besides virtio-gpu device on the unmap, don't know about
other guests.
We can claim it a guest's fault if MR lives after the deletion and in
that case exit Qemu with a noisy error msg or leak resource. WDYT?
--
Best regards,
Dmitry
