<div dir="ltr"><div dir="ltr"><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Mar 20, 2019 at 3:03 PM Chia-I Wu <<a href="mailto:olvaffe@gmail.com">olvaffe@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">On Mon, Mar 18, 2019 at 2:22 PM Lepton Wu <<a href="mailto:lepton@chromium.org" target="_blank">lepton@chromium.org</a>> wrote:<br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The old code could use gem name as key when inserting it to bo_handles<br>
hash table while trying to remove it from hash table with bo_handle as<br>
key in virgl_hw_res_destroy. This triggers use after free. Also, we<br>
should only reuse resource from bo_handle hash when the handle type is<br>
FD.<br></blockquote><div>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.</div><div><br></div><div>I think the logic should go like:</div><div><br></div><div> if (HANDLE_TYPE_SHARED) {</div><div><div> if (bo_names.has(flink_name))</div><div> return bo_names[flink_name];</div></div><div> gem_handle = gem_open(flink_name);</div><div> } else {</div><div> gem_handle = drmPrimeFDToHandle(prime_fd);</div><div> }</div><div> <br></div></div></div></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div></div><div> if (bo_handles.has(gem_handle))</div><div> return bo_handles[gem_handle];</div><div> bo_handles[gem_handle] = create_new_resource();</div><div><br></div></div></div></div></div></div></blockquote><div>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.</div><div>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.</div><div>What do you think?</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Signed-off-by: Lepton Wu <<a href="mailto:lepton@chromium.org" target="_blank">lepton@chromium.org</a>><br>
---<br>
src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 15 ++++++++-------<br>
1 file changed, 8 insertions(+), 7 deletions(-)<br>
<br>
diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c<br>
index 120e8eda2cd..01811a0e997 100644<br>
--- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c<br>
+++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c<br>
@@ -424,13 +424,13 @@ virgl_drm_winsys_resource_create_handle(struct virgl_winsys *qws,<br>
res = NULL;<br>
goto done;<br>
}<br>
- }<br>
<br>
- res = util_hash_table_get(qdws->bo_handles, (void*)(uintptr_t)handle);<br>
- if (res) {<br>
- struct virgl_hw_res *r = NULL;<br>
- virgl_drm_resource_reference(qdws, &r, res);<br>
- goto done;<br>
+ res = util_hash_table_get(qdws->bo_handles, (void*)(uintptr_t)handle);<br>
+ if (res) {<br>
+ struct virgl_hw_res *r = NULL;<br>
+ virgl_drm_resource_reference(qdws, &r, res);<br>
+ goto done;<br>
+ }<br>
}<br>
<br>
res = CALLOC_STRUCT(virgl_hw_res);<br>
@@ -469,7 +469,8 @@ virgl_drm_winsys_resource_create_handle(struct virgl_winsys *qws,<br>
res->num_cs_references = 0;<br>
res->fence_fd = -1;<br>
<br>
- util_hash_table_set(qdws->bo_handles, (void *)(uintptr_t)handle, res);<br>
+ util_hash_table_set(qdws->bo_handles, (void *)(uintptr_t)res->bo_handle,<br>
+ res);<br>
<br>
done:<br>
mtx_unlock(&qdws->bo_handles_mutex);<br>
-- <br>
2.21.0.225.g810b269d1ac-goog<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a></blockquote></div></div></div></div></div>
</blockquote></div></div>