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
