Re: [PATCH v7 07/10] virtio-gpu: Handle resource blob commands

[Dmitry Osipenko]

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