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

[Chia-I Wu]

On Wed, Apr 3, 2019 at 8:17 PM Dave Airlie <airlied at gmail.com> wrote:

> On Thu, 4 Apr 2019 at 06:54, Chia-I Wu <olvaffe at gmail.com> 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 <lepton at chromium.org> wrote:
> >>
> >>
> >>
> >>
> >> 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?
>
> 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190408/a1208cda/attachment.html>