[v2] virgl: Use right key to insert resource to hash.

Submitted by Lepton Wu on April 8, 2019, 4:34 p.m.

Details

Message ID 20190408163418.93708-1-lepton@chromium.org
State New
Headers show
Series "virgl: Use right key to insert resource to hash." ( rev: 2 ) in Mesa

Not browsing as part of any series.

Commit Message

Lepton Wu April 8, 2019, 4:34 p.m.
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 <lepton@chromium.org>
---
 .../winsys/virgl/drm/virgl_drm_winsys.c       | 24 +++++++++++++------
 1 file changed, 17 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

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;
+      }
    }
 
    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;
 
@@ -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);

Comments


On Mon, Apr 8, 2019 at 11:10 AM Chia-I Wu <olvaffe@gmail.com> wrote:
>
>
>
> On Mon, Apr 8, 2019 at 9:34 AM Lepton Wu <lepton@chromium.org> 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 <lepton@chromium.org>
>> ---
>>  .../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.
>
>>     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
>>

https://gitlab.freedesktop.org/mesa/mesa/merge_requests/605

Can you try that please and maybe discuss there as well? I cleaned up
the code a bit and realigned it with the radeon code.

Dave.

On Tue, 9 Apr 2019 at 04:44, Chia-I Wu <olvaffe@gmail.com> wrote:
>
>
>
> On Mon, Apr 8, 2019 at 11:24 AM Lepton Wu <lepton@chromium.org> wrote:
>>
>> On Mon, Apr 8, 2019 at 11:10 AM Chia-I Wu <olvaffe@gmail.com> wrote:
>> >
>> >
>> >
>> > On Mon, Apr 8, 2019 at 9:34 AM Lepton Wu <lepton@chromium.org> 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 <lepton@chromium.org>
>> >> ---
>> >>  .../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.
>
> 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.
>
>> >
>> >>     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
>> >>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev