<div dir="ltr"><div dir="ltr"> </div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Apr 8, 2019 at 11:24 AM Lepton Wu <<a href="mailto:lepton@chromium.org">lepton@chromium.org</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Apr 8, 2019 at 11:10 AM Chia-I Wu <<a href="mailto:olvaffe@gmail.com" target="_blank">olvaffe@gmail.com</a>> wrote: > > > > On Mon, Apr 8, 2019 at 9:34 AM Lepton Wu <<a href="mailto:lepton@chromium.org" target="_blank">lepton@chromium.org</a>> wrote: >> >> The old code could use gem name as key when inserting it to bo_handles >> hash table while trying to remove it from hash table with bo_handle as >> key in virgl_hw_res_destroy and then it fail to remove it from bo_handles >> hash table. This triggers use after free. Also, we should insert resource >> to bo_names hash table when handle type is SHARED. >> >> Signed-off-by: Lepton Wu <<a href="mailto:lepton@chromium.org" target="_blank">lepton@chromium.org</a>> >> --- >> .../winsys/virgl/drm/virgl_drm_winsys.c | 24 +++++++++++++------ >> 1 file changed, 17 insertions(+), 7 deletions(-) >> >> diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c >> index 2cf8b4ba076..af92b6a98fc 100644 >> --- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c >> +++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c >> @@ -406,6 +406,12 @@ virgl_drm_winsys_resource_create_handle(struct virgl_winsys *qws, >> return NULL; >> } >> >> + if (whandle->type != WINSYS_HANDLE_TYPE_FD && >> + whandle->type != WINSYS_HANDLE_TYPE_SHARED) { >> + fprintf(stderr, "Unexpected handle type: %d\n", whandle->type); >> + return NULL; >> + } >> + >> mtx_lock(&qdws->bo_handles_mutex); >> >> if (whandle->type == WINSYS_HANDLE_TYPE_SHARED) { >> @@ -424,13 +430,13 @@ virgl_drm_winsys_resource_create_handle(struct virgl_winsys *qws, >> res = NULL; >> goto done; >> } >> - } >> >> - res = util_hash_table_get(qdws->bo_handles, (void*)(uintptr_t)handle); >> - if (res) { >> - struct virgl_hw_res *r = NULL; >> - virgl_drm_resource_reference(qdws, &r, res); >> - goto done; >> + res = util_hash_table_get(qdws->bo_handles, (void*)(uintptr_t)handle); >> + if (res) { >> + struct virgl_hw_res *r = NULL; >> + virgl_drm_resource_reference(qdws, &r, res); >> + goto done; >> + } >> } >> > You can still run into troubles when asked to import a buffer by both its prime fd and its flink name. But it seems there is no way to fix it at user space, right? Every time we got a flink name for the first time, kernel will give a new handle and no way to reuse any res from hash table. </blockquote><div>Yeah, I think you are right... but that appears to be a kernel bug. Once kernel is fixed to return the same handle for the same flink name / prime fd, we should be ready for that. Let's see what Dave thinks.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > >> res = CALLOC_STRUCT(virgl_hw_res); >> @@ -448,6 +454,8 @@ virgl_drm_winsys_resource_create_handle(struct virgl_winsys *qws, >> goto done; >> } >> res->bo_handle = open_arg.handle; >> + res->flinked = true; >> + res->flink = whandle->handle; >> } >> res->name = handle; > > res->name has no user. Remove it. > > It is also less confusing to make sure `handle' means GEM handle at this point, by calling either GEM_OPEN or drmPrimeFDToHandle depending on the handle type. > >> >> @@ -469,7 +477,9 @@ virgl_drm_winsys_resource_create_handle(struct virgl_winsys *qws, >> res->num_cs_references = 0; >> res->fence_fd = -1; >> >> - util_hash_table_set(qdws->bo_handles, (void *)(uintptr_t)handle, res); >> + util_hash_table_set(qdws->bo_handles, (void *)(uintptr_t)res->bo_handle, res); >> + if (whandle->type == WINSYS_HANDLE_TYPE_SHARED) >> + util_hash_table_set(qdws->bo_names, (void *)(uintptr_t)res->flink, res); >> >> done: >> mtx_unlock(&qdws->bo_handles_mutex); >> -- >> 2.21.0.392.gf8f6787159e-goog >> </blockquote></div></div>