Hi
On Wed, Aug 14, 2013 at 7:35 PM, Thomas Hellstrom thellstrom@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@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@vmware.com Signed-off-by: David Herrmann dh.herrmann@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