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

[Dmitry Osipenko]

On 4/15/24 13:05, Akihiko Odaki wrote:
...
>> Do you have example of a legit use-case where hostmem MR could outlive
>> resource mapping?
> 
> MR outliving after memory_region_del_subregion() is not a use-case, but
> a situation that occurs due to the limitation of the memory subsystem.
> It is not easy to answer how often such a situation happens.
> 
>>
>> Turning it into a error condition is much more reasonable to do than to
>> to worry about edge case that nobody cares about, which can't be tested
>> easily and that not trivial to support, IMO.
>>
> I'm not sure what you mean by turning into an error condition. I doubt
> it's possible to emit errors when someone touches an unmapped region.

My idea was about failing in virtio_gpu_virgl_unmap_resource_blob()
where we could check whether MR has external reference and fail if it has.

> Reproducing this issue is not easy as it's often cases for
> use-after-free bugs, but fixing it is not that complicated in my opinion
> since you already have an implementation which asynchronously unmaps the
> region in v6. I write my suggestions to fix problems in v6:
> 
> - Remove ref member in virgl_gpu_resource, vres_get_ref(),
> vres_put_ref(), and virgl_resource_unmap().
> 
> - Change virtio_gpu_virgl_process_cmd(),
> virgl_cmd_resource_unmap_blob(), and virgl_cmd_resource_unref() to
> return a bool, which tells if the command was processed or suspended.
> 
> - In virtio_gpu_process_cmdq(), break if the command was suspended.
> 
> - In virgl_resource_blob_async_unmap(), call virtio_gpu_gl_block(g, false).
> 
> - In virgl_cmd_resource_unmap_blob() and virgl_cmd_resource_unref(),
> call memory_region_del_subregion() and virtio_gpu_gl_block(g, true), and
> tell that the command was suspended if the reference counter of
> MemoryRegion > 0. Free and unmap the MR otherwise.

Your suggestion works, I'll proceed with it in v8.

Thanks for the review

-- 
Best regards,
Dmitry