drm/vmwgfx: Do not drop the reference to the handle too soon

v3: Fix vmw_user_bo_lookup which was also dropping the gem reference
before the kernel was done with buffer depending on userspace doing
the right thing. Same bug, different spot.

It is possible for userspace to predict the next buffer handle and
to destroy the buffer while it's still used by the kernel. Delay
dropping the internal reference on the buffers until kernel is done
with them.

Instead of immediately dropping the gem reference in vmw_user_bo_lookup
and vmw_gem_object_create_with_handle let the callers decide when they're
ready give the control back to userspace.

Also fixes the second usage of vmw_gem_object_create_with_handle in
vmwgfx_surface.c which wasn't grabbing an explicit reference
to the gem object which could have been destroyed by the userspace
on the owning surface at any point.

Signed-off-by: Zack Rusin <zackr@vmware.com>
Fixes: 8afa13a0583f ("drm/vmwgfx: Implement DRIVER_GEM")
Reviewed-by: Martin Krastev <krastevm@vmware.com>
Reviewed-by: Maaz Mombasawala <mombasawalam@vmware.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230211050514.2431155-1-zack@kde.org

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index 7a00314882a311f7845c9d5500f650b12806ad19..82094c137855b9b7e6a2c2f6666da6bb58553f36 100644 (file)
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -500,6 +500,7 @@ static int vmw_user_bo_synccpu_release(struct drm_file *filp,
                ttm_bo_put(&vmw_bo->tbo);
        }
 
+       drm_gem_object_put(&vmw_bo->tbo.base);
        return ret;
 }
 
@@ -540,6 +541,7 @@ int vmw_user_bo_synccpu_ioctl(struct drm_device *dev, void *data,
 
                ret = vmw_user_bo_synccpu_grab(vbo, arg->flags);
                vmw_bo_unreference(&vbo);
+               drm_gem_object_put(&vbo->tbo.base);
                if (unlikely(ret != 0)) {
                        if (ret == -ERESTARTSYS || ret == -EBUSY)
                                return -EBUSY;
@@ -596,7 +598,7 @@ int vmw_bo_unref_ioctl(struct drm_device *dev, void *data,
  * struct vmw_bo should be placed.
  * Return: Zero on success, Negative error code on error.
  *
- * The vmw buffer object pointer will be refcounted.
+ * The vmw buffer object pointer will be refcounted (both ttm and gem)
  */
 int vmw_user_bo_lookup(struct drm_file *filp,
                       u32 handle,
@@ -613,7 +615,6 @@ int vmw_user_bo_lookup(struct drm_file *filp,
 
        *out = to_vmw_bo(gobj);
        ttm_bo_get(&(*out)->tbo);
-       drm_gem_object_put(gobj);
 
        return 0;
 }
@@ -693,7 +694,8 @@ int vmw_dumb_create(struct drm_file *file_priv,
        ret = vmw_gem_object_create_with_handle(dev_priv, file_priv,
                                                args->size, &args->handle,
                                                &vbo);
-
+       /* drop reference from allocate - handle holds it now */
+       drm_gem_object_put(&vbo->tbo.base);
        return ret;
 }
 

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index 6d1b46c23719bc170d0a4cce083b8bfb8679179f..6b9aa2b4ef54add2363c69357a446f2bdfb5a460 100644 (file)
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -1165,6 +1165,7 @@ static int vmw_translate_mob_ptr(struct vmw_private *dev_priv,
        vmw_bo_placement_set(vmw_bo, VMW_BO_DOMAIN_MOB, VMW_BO_DOMAIN_MOB);
        ret = vmw_validation_add_bo(sw_context->ctx, vmw_bo);
        ttm_bo_put(&vmw_bo->tbo);
+       drm_gem_object_put(&vmw_bo->tbo.base);
        if (unlikely(ret != 0))
                return ret;
 
@@ -1221,6 +1222,7 @@ static int vmw_translate_guest_ptr(struct vmw_private *dev_priv,
                             VMW_BO_DOMAIN_GMR | VMW_BO_DOMAIN_VRAM);
        ret = vmw_validation_add_bo(sw_context->ctx, vmw_bo);
        ttm_bo_put(&vmw_bo->tbo);
+       drm_gem_object_put(&vmw_bo->tbo.base);
        if (unlikely(ret != 0))
                return ret;
 

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
index 51bd1f8c5cc4512b5b61db2beb4decf4c51c2fcf..d6baf73a64589ab6fdf1e8264907e1d8259bfbaf 100644 (file)
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
@@ -133,8 +133,6 @@ int vmw_gem_object_create_with_handle(struct vmw_private *dev_priv,
        (*p_vbo)->tbo.base.funcs = &vmw_gem_object_funcs;
 
        ret = drm_gem_handle_create(filp, &(*p_vbo)->tbo.base, handle);
-       /* drop reference from allocate - handle holds it now */
-       drm_gem_object_put(&(*p_vbo)->tbo.base);
 out_no_bo:
        return ret;
 }
@@ -161,6 +159,8 @@ int vmw_gem_object_create_ioctl(struct drm_device *dev, void *data,
        rep->map_handle = drm_vma_node_offset_addr(&vbo->tbo.base.vma_node);
        rep->cur_gmr_id = handle;
        rep->cur_gmr_offset = 0;
+       /* drop reference from allocate - handle holds it now */
+       drm_gem_object_put(&vbo->tbo.base);
 out_no_bo:
        return ret;
 }

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 8659de9d23f354be7611cf480169bbabfae645f2..84d6380b98950c1d249de097a1b7c819f06dc52c 100644 (file)
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1725,8 +1725,10 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct drm_device *dev,
 
 err_out:
        /* vmw_user_lookup_handle takes one ref so does new_fb */
-       if (bo)
+       if (bo) {
                vmw_bo_unreference(&bo);
+               drm_gem_object_put(&bo->tbo.base);
+       }
        if (surface)
                vmw_surface_unreference(&surface);
 

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c b/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c
index 7bcda29a2897ddc73f84538af6a71840783543ed..8d171d71cb8a732afcd785f79a9efaaf8e49f856 100644 (file)
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c
@@ -458,6 +458,7 @@ int vmw_overlay_ioctl(struct drm_device *dev, void *data,
        ret = vmw_overlay_update_stream(dev_priv, buf, arg, true);
 
        vmw_bo_unreference(&buf);
+       drm_gem_object_put(&buf->tbo.base);
 
 out_unlock:
        mutex_unlock(&overlay->mutex);

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c b/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
index 6b8e984695ed0dc7466c1b658b511239139a484d..e7226db8b242481fc7a7790ef07ba244f80aad94 100644 (file)
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
@@ -810,6 +810,7 @@ static int vmw_shader_define(struct drm_device *dev, struct drm_file *file_priv,
                                    num_output_sig, tfile, shader_handle);
 out_bad_arg:
        vmw_bo_unreference(&buffer);
+       drm_gem_object_put(&buffer->tbo.base);
        return ret;
 }
 

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index 9d4ae9623a00882fdc3129d4c28e3b0a70b2af4f..5db403ee8261d38dae1477ac414c70c7b3ff689a 100644 (file)
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -686,7 +686,7 @@ static void vmw_user_surface_base_release(struct ttm_base_object **p_base)
            container_of(base, struct vmw_user_surface, prime.base);
        struct vmw_resource *res = &user_srf->srf.res;
 
-       if (base->shareable && res && res->guest_memory_bo)
+       if (res->guest_memory_bo)
                drm_gem_object_put(&res->guest_memory_bo->tbo.base);
 
        *p_base = NULL;
@@ -867,7 +867,11 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data,
                        goto out_unlock;
                }
                vmw_bo_reference(res->guest_memory_bo);
-               drm_gem_object_get(&res->guest_memory_bo->tbo.base);
+               /*
+                * We don't expose the handle to the userspace and surface
+                * already holds a gem reference
+                */
+               drm_gem_handle_delete(file_priv, backup_handle);
        }
 
        tmp = vmw_resource_reference(&srf->res);
@@ -1571,8 +1575,6 @@ vmw_gb_surface_define_internal(struct drm_device *dev,
                        drm_vma_node_offset_addr(&res->guest_memory_bo->tbo.base.vma_node);
                rep->buffer_size = res->guest_memory_bo->tbo.base.size;
                rep->buffer_handle = backup_handle;
-               if (user_srf->prime.base.shareable)
-                       drm_gem_object_get(&res->guest_memory_bo->tbo.base);
        } else {
                rep->buffer_map_handle = 0;
                rep->buffer_size = 0;