Re: [Mesa-dev] [PATCH] virgl: Use right key to insert resource to hash.
The kernel code will create a new gem handle for SHARED handle every
time, see code here:
https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_gem.c#L796
On Mon, Apr 8, 2019 at 11:12 AM Chia-I Wu wrote:
>
>
>
> On Wed, Apr 3, 2019 at 8:17 PM Dave Airlie wrote:
>>
>> On Thu, 4 Apr 2019 at 06:54, Chia-I Wu wrote:
>> >
>> > You could end up having two virgl_hw_res with two different GEM handles
>> > pointing to the same kernel GEM object. That might break some assumptions
>> > about dependency tracking.
>> >
>> > For example, when the cmdbuf being built uses a buffer and you want to
>> > transfer some more data into the buffer, you normally need to submit the
>> > cmdbuf first before starting the transfer. The current code detects that
>> > with virgl_drm_res_is_ref, which assumes each kernel GEM object has a
>> > unique virgl_hw_res.
>> >
>> > On Mon, Apr 1, 2019 at 12:37 PM Lepton Wu wrote:
>> >>
>> >>
>> >>
>> >>
>> >> On Wed, Mar 20, 2019 at 3:03 PM Chia-I Wu wrote:
>> >>>
>> >>> On Mon, Mar 18, 2019 at 2:22 PM Lepton Wu 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?
>>
>> Just to reinforce this, we can only have one GEM handle for a kernel
>> object, validation will go wrong and deadlock if we submit two handles
>> pointing at the same bo.
>>
>> Opening a shared handle should not get a new gem handle, if should
>> return any gem handle that already exists.
>
> I just tried and that is not the case. Sounds like a kernel bug?
The kernel code will create a new gem handle for SHARED handle every
time, see code here:
https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_gem.c#L796
>
>>
>>
>> Dave.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] virgl: Use right key to insert resource to hash.
On Wed, Apr 3, 2019 at 8:17 PM Dave Airlie wrote:
> On Thu, 4 Apr 2019 at 06:54, Chia-I Wu wrote:
> >
> > You could end up having two virgl_hw_res with two different GEM handles
> pointing to the same kernel GEM object. That might break some assumptions
> about dependency tracking.
> >
> > For example, when the cmdbuf being built uses a buffer and you want to
> transfer some more data into the buffer, you normally need to submit the
> cmdbuf first before starting the transfer. The current code detects that
> with virgl_drm_res_is_ref, which assumes each kernel GEM object has a
> unique virgl_hw_res.
> >
> > On Mon, Apr 1, 2019 at 12:37 PM Lepton Wu wrote:
> >>
> >>
> >>
> >>
> >> On Wed, Mar 20, 2019 at 3:03 PM Chia-I Wu wrote:
> >>>
> >>> On Mon, Mar 18, 2019 at 2:22 PM Lepton Wu 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?
>
> Just to reinforce this, we can only have one GEM handle for a kernel
> object, validation will go wrong and deadlock if we submit two handles
> pointing at the same bo.
>
> Opening a shared handle should not get a new gem handle, if should
> return any gem handle that already exists.
>
I just tried and that is not the case. Sounds like a kernel bug?
>
> Dave.
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] virgl: Use right key to insert resource to hash.
On Thu, 4 Apr 2019 at 06:54, Chia-I Wu wrote:
>
> You could end up having two virgl_hw_res with two different GEM handles
> pointing to the same kernel GEM object. That might break some assumptions
> about dependency tracking.
>
> For example, when the cmdbuf being built uses a buffer and you want to
> transfer some more data into the buffer, you normally need to submit the
> cmdbuf first before starting the transfer. The current code detects that
> with virgl_drm_res_is_ref, which assumes each kernel GEM object has a unique
> virgl_hw_res.
>
> On Mon, Apr 1, 2019 at 12:37 PM Lepton Wu wrote:
>>
>>
>>
>>
>> On Wed, Mar 20, 2019 at 3:03 PM Chia-I Wu wrote:
>>>
>>> On Mon, Mar 18, 2019 at 2:22 PM Lepton Wu 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?
Just to reinforce this, we can only have one GEM handle for a kernel
object, validation will go wrong and deadlock if we submit two handles
pointing at the same bo.
Opening a shared handle should not get a new gem handle, if should
return any gem handle that already exists.
Dave.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] virgl: Use right key to insert resource to hash.
You could end up having two virgl_hw_res with two different GEM handles
pointing to the same kernel GEM object. That might break some assumptions
about dependency tracking.
For example, when the cmdbuf being built uses a buffer and you want to
transfer some more data into the buffer, you normally need to submit the
cmdbuf first before starting the transfer. The current code detects that
with virgl_drm_res_is_ref, which assumes each kernel GEM object has a
unique virgl_hw_res.
On Mon, Apr 1, 2019 at 12:37 PM Lepton Wu wrote:
>
>
>
> On Wed, Mar 20, 2019 at 3:03 PM Chia-I Wu wrote:
>
>> On Mon, Mar 18, 2019 at 2:22 PM Lepton Wu 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
>>> ---
>>> 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, , 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, , 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(>bo_handles_mutex);
>>> --
>>> 2.21.0.225.g810b269d1ac-goog
>>>
>>> ___
>>> mesa-dev mailing list
>>> mesa-dev@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] virgl: Use right key to insert resource to hash.
On Wed, Mar 20, 2019 at 3:03 PM Chia-I Wu wrote:
> On Mon, Mar 18, 2019 at 2:22 PM Lepton Wu 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
>> ---
>> 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, , 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, , 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(>bo_handles_mutex);
>> --
>> 2.21.0.225.g810b269d1ac-goog
>>
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] virgl: Use right key to insert resource to hash.
On Mon, Mar 18, 2019 at 2:22 PM Lepton Wu 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();
> Signed-off-by: Lepton Wu
> ---
> 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, , 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, , 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(>bo_handles_mutex);
> --
> 2.21.0.225.g810b269d1ac-goog
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] virgl: Use right key to insert resource to hash.
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.
Signed-off-by: Lepton Wu
---
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, , 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, , 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(>bo_handles_mutex);
--
2.21.0.225.g810b269d1ac-goog
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
