[PATCH 08/16] drm/vmwgfx: implement mmap access managament

[David Herrmann]

Hi

On Wed, Aug 14, 2013 at 7:35 PM, Thomas Hellstrom <thellstrom at vmware.com> wrote:
> (CC'ing the proper people since I'm still on parental leave).
>
> On 08/13/2013 11:44 PM, David Herrmann wrote:
>
> Please see inline comments.
>
>
>> Hi
>>
>> On Tue, Aug 13, 2013 at 9:38 PM, David Herrmann <dh.herrmann at gmail.com>
>> wrote:
>>>
>>> Correctly allow and revoke buffer access on each open/close via the new
>>> VMA offset manager.
>
>
> I haven't yet had time to check the access policies of the new VMA offset
> manager, but anything that is identical or stricter than the current vmwgfx
> verify_access() would be fine. If it's stricter however, we need to
> double-check backwards user-space compatibility.

My patches make vmw_dmabuf_alloc_ioctl() add the caller's open-file
(file*) to the list of allowed users of the new bo.
vmw_dmabuf_unref_ioctl() removes it again. I haven't seen any way to
pass a user-dmabuf to another user so there is currently at most one
user for a vmw_dmabuf. vmw_user_dmabuf_reference() looks like it is
intended exactly for this case so it would have to add the file* of
the caller to the list of allowed users. I will change that in v2.
This means every user who gets a handle for the buffer (like gem_open)
will be added to the allowed users. For TTM-object currently only a
single user is allowed.

So I replace vmw_user_bo->base.tfile with a list (actually rbtree) of
allowed files. So more than one user can have access. This, however,
breaks the "shareable" attribute which I wasn't aware of. As far as I
can see, "shareable" is only used by vmwgfx_surface.c and can be set
by userspace to allow arbitrary processes to map this buffer (sounds
like a security issue similar to gem flink).
I actually think we can replace the "shareable" attribute with proper
access-management in the vma-manager. But first I'd need to know
whether "shareable = true" is actually used by vmwgfx user-space and
how buffers are shared? Do you simply pass the mmap offset between
processes? Or do you pass some handle?

If you really pass mmap offsets in user-space and rely on this, I
guess there is no way I can make vmwgfx use the vma-manager access
management. I will have to find a way to work around it or move the
"shareable" flag to ttm_bo.

>
>>>   We also need to make vmw_user_dmabuf_reference()
>>> correctly increase the vma-allow counter, but it is unused so remove it
>>> instead.
>
> IIRC this function or a derivative thereof is used heavily in an upcoming
> version driver, so if possible, please add a corrected version rather than
> remove the (currently) unused code. This will trigger a merge error and the
> upcoming code can be more easily corrected.

I will do so.

>
>>>
>>> Cc: Thomas Hellstrom <thellstrom at vmware.com>
>>> Signed-off-by: David Herrmann <dh.herrmann at gmail.com>
>>
>> Just as a hint, this patch would allow to remove the
>> "->access_verify()" callback in vmwgfx. No other driver uses it,
>> afaik. I will try to add this in v2.
>>
>> Regards
>> David
>>
>>> ---
>>>   drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 29
>>> +++++++++++++++++------------
>>>   1 file changed, 17 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
>>> b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
>>> index 0e67cf4..4d3f0ae 100644
>>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
>>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
>>> @@ -499,6 +499,12 @@ int vmw_dmabuf_alloc_ioctl(struct drm_device *dev,
>>> void *data,
>>>          if (unlikely(ret != 0))
>>>                  goto out_no_dmabuf;
>>>
>>> +       ret = drm_vma_node_allow(&dma_buf->base.vma_node,
>>> file_priv->filp);
>>> +       if (ret) {
>>> +               vmw_dmabuf_unreference(&dma_buf);
>>> +               goto out_no_dmabuf;
>>> +       }
>>> +
>>>          rep->handle = handle;
>>>          rep->map_handle =
>>> drm_vma_node_offset_addr(&dma_buf->base.vma_node);
>>>          rep->cur_gmr_id = handle;
>>> @@ -517,7 +523,18 @@ int vmw_dmabuf_unref_ioctl(struct drm_device *dev,
>>> void *data,
>>>   {
>>>          struct drm_vmw_unref_dmabuf_arg *arg =
>>>              (struct drm_vmw_unref_dmabuf_arg *)data;
>>> +       struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
>>> +       struct vmw_dma_buffer *dma_buf;
>>> +       int ret;
>>> +
>>> +       ret = vmw_user_dmabuf_lookup(tfile, arg->handle, &dma_buf);
>>> +       if (ret)
>>> +               return -EINVAL;
>>>
>>> +       drm_vma_node_revoke(&dma_buf->base.vma_node, file_priv->filp);
>>> +       vmw_dmabuf_unreference(&dma_buf);
>>> +
>>> +       /* FIXME: is this equivalent to vmw_dmabuf_unreference(dma_buf)?
>>> */
>
>
> No. A ttm ref object is rather similar to a generic GEM object, only that
> it's generic in the sense that it is not restricted to buffers, and can make
> any desired object visible to user-space. So translated the below code
> removes a reference that the arg->handle holds on the "gem" object,
> potentially destroying the whole object in which the "gem" object is
> embedded.

So I actually need both lookups, vmw_user_dmabuf_lookup() and the
lookup in ttm_ref_object_base_unref()? Ugh.. but ok, I will leave the
function then as it is now but remove the comment.

>
>>>          return ttm_ref_object_base_unref(vmw_fpriv(file_priv)->tfile,
>>>                                           arg->handle,
>>>                                           TTM_REF_USAGE);
>>> @@ -551,18 +568,6 @@ int vmw_user_dmabuf_lookup(struct ttm_object_file
>>> *tfile,
>>>          return 0;
>>>   }
>>>
>>> -int vmw_user_dmabuf_reference(struct ttm_object_file *tfile,
>>> -                             struct vmw_dma_buffer *dma_buf)
>>> -{
>>> -       struct vmw_user_dma_buffer *user_bo;
>>> -
>>> -       if (dma_buf->base.destroy != vmw_user_dmabuf_destroy)
>>> -               return -EINVAL;
>>> -
>>> -       user_bo = container_of(dma_buf, struct vmw_user_dma_buffer, dma);
>>> -       return ttm_ref_object_add(tfile, &user_bo->base, TTM_REF_USAGE,
>>> NULL);
>>> -}
>>> -
>>>   /*
>>>    * Stream management
>>>    */
>>> --
>>> 1.8.3.4
>>>
>
> Otherwise looks OK to me.

Thanks!
David