[Mesa-dev] [PATCH] virgl: Use right key to insert resource to hash.

[Lepton Wu]

On Wed, Mar 20, 2019 at 3:03 PM Chia-I Wu <olvaffe at gmail.com> wrote:

> On Mon, Mar 18, 2019 at 2:22 PM Lepton Wu <lepton at 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. This triggers use after free. Also, we
>> should only reuse resource from bo_handle hash when the handle type is
>> FD.
>>
> Reuse is not very accurate.  Opening a shared handle (flink name) twice
> gives two GEM handles.  Importing an fd handle (prime fd) twice gives the
> same GEM handle.  In all cases, within a virgl_winsys, we want only one GEM
> handle and only one virgl_resource for each kernel GEM object.
>
> I think the logic should go like:
>
>   if (HANDLE_TYPE_SHARED) {
>     if (bo_names.has(flink_name))
>       return bo_names[flink_name];
>     gem_handle = gem_open(flink_name);
>   } else {
>     gem_handle = drmPrimeFDToHandle(prime_fd);
>   }
>
>
  if (bo_handles.has(gem_handle))
>     return bo_handles[gem_handle];
>   bo_handles[gem_handle] = create_new_resource();
>
> Hi, the current patch did most of what you said with only one difference:
it didn't insert to bo_handles[]   hash when the type
is  HANDLE_TYPE_SHARED.
I think this is reasonable since opening a shared handle always get a new
gem handle very time and I think it doesn't worth to insert it to
bo_handles[] hash.
What do you think?

>
>> Signed-off-by: Lepton Wu <lepton at chromium.org>
>> ---
>>  src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 15 ++++++++-------
>>  1 file changed, 8 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 120e8eda2cd..01811a0e997 100644
>> --- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> +++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> @@ -424,13 +424,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);
>> @@ -469,7 +469,8 @@ 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);
>>
>>  done:
>>     mtx_unlock(&qdws->bo_handles_mutex);
>> --
>> 2.21.0.225.g810b269d1ac-goog
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190401/ba68e5ce/attachment.html>