VIGS: Delay surface destruction

We should destroy surfaces on TTM BO destruction
instead of GEM free handler. The latter may cause races
like this:
1. GEM is created
2. GEM is mapped and written to
3. GEM is freed, but not unmapped. Thus, the host gets
   a "destroy_surface" command and target frees up
   an id from IDR.
4. Another GEM is created. Thus, it's assigned a freed id, which
   is id of first GEM, the host gets "create_surface" command
5. First GEM is unmapped, host gets "update_gpu" command with
   wrong data and size and crashes

The race occured on wayland/GBM during window resize

Change-Id: Ifddab147999e450e4b4affd7144d91f29fb39534

diff --git a/drivers/gpu/drm/vigs/vigs_comm.c b/drivers/gpu/drm/vigs/vigs_comm.c
index cc5685b..5ef8ed1 100644 (file)
--- a/drivers/gpu/drm/vigs/vigs_comm.c
+++ b/drivers/gpu/drm/vigs/vigs_comm.c
@@ -224,6 +224,7 @@ void vigs_comm_destroy(struct vigs_comm *comm)
 {
     DRM_DEBUG_DRIVER("enter\n");
 
+    mutex_destroy(&comm->mutex);
     vigs_comm_exit(comm);
     if (comm->execbuffer) {
         drm_gem_object_unreference_unlocked(&comm->execbuffer->gem.base);

diff --git a/drivers/gpu/drm/vigs/vigs_device.c b/drivers/gpu/drm/vigs/vigs_device.c
index a39152b..e2642fe 100644 (file)
--- a/drivers/gpu/drm/vigs/vigs_device.c
+++ b/drivers/gpu/drm/vigs/vigs_device.c
@@ -95,12 +95,20 @@ static struct vigs_surface
 
     mutex_lock(&vigs_dev->drm_dev->struct_mutex);
 
+    mutex_lock(&vigs_dev->surface_idr_mutex);
+
     sfc = idr_find(&vigs_dev->surface_idr, sfc_id);
 
     if (sfc) {
-        drm_gem_object_reference(&sfc->gem.base);
+        if (vigs_gem_freed(&sfc->gem)) {
+            sfc = NULL;
+        } else {
+            drm_gem_object_reference(&sfc->gem.base);
+        }
     }
 
+    mutex_unlock(&vigs_dev->surface_idr_mutex);
+
     mutex_unlock(&vigs_dev->drm_dev->struct_mutex);
 
     return sfc;
@@ -308,6 +316,7 @@ int vigs_device_init(struct vigs_device *vigs_dev,
     vigs_dev->io_size = pci_resource_len(pci_dev, 2);
 
     idr_init(&vigs_dev->surface_idr);
+    mutex_init(&vigs_dev->surface_idr_mutex);
 
     if (!vigs_dev->vram_base || !vigs_dev->ram_base || !vigs_dev->io_base) {
         DRM_ERROR("VRAM, RAM or IO bar not found on device\n");
@@ -404,6 +413,7 @@ fail2:
     drm_rmmap(vigs_dev->drm_dev, vigs_dev->io_map);
 fail1:
     idr_destroy(&vigs_dev->surface_idr);
+    mutex_destroy(&vigs_dev->surface_idr_mutex);
 
     return ret;
 }
@@ -420,6 +430,7 @@ void vigs_device_cleanup(struct vigs_device *vigs_dev)
     vigs_mman_destroy(vigs_dev->mman);
     drm_rmmap(vigs_dev->drm_dev, vigs_dev->io_map);
     idr_destroy(&vigs_dev->surface_idr);
+    mutex_destroy(&vigs_dev->surface_idr_mutex);
 }
 
 int vigs_device_mmap(struct file *filp, struct vm_area_struct *vma)
@@ -444,8 +455,11 @@ int vigs_device_add_surface(struct vigs_device *vigs_dev,
 {
     int ret, tmp_id = 0;
 
+    mutex_lock(&vigs_dev->surface_idr_mutex);
+
     do {
         if (unlikely(idr_pre_get(&vigs_dev->surface_idr, GFP_KERNEL) == 0)) {
+            mutex_unlock(&vigs_dev->surface_idr_mutex);
             return -ENOMEM;
         }
 
@@ -454,13 +468,17 @@ int vigs_device_add_surface(struct vigs_device *vigs_dev,
 
     *id = tmp_id;
 
+    mutex_unlock(&vigs_dev->surface_idr_mutex);
+
     return ret;
 }
 
 void vigs_device_remove_surface(struct vigs_device *vigs_dev,
                                 vigsp_surface_id sfc_id)
 {
+    mutex_lock(&vigs_dev->surface_idr_mutex);
     idr_remove(&vigs_dev->surface_idr, sfc_id);
+    mutex_unlock(&vigs_dev->surface_idr_mutex);
 }
 
 int vigs_device_add_surface_unlocked(struct vigs_device *vigs_dev,

diff --git a/drivers/gpu/drm/vigs/vigs_device.h b/drivers/gpu/drm/vigs/vigs_device.h
index 88e8dde..368b1a9 100644 (file)
--- a/drivers/gpu/drm/vigs/vigs_device.h
+++ b/drivers/gpu/drm/vigs/vigs_device.h
@@ -27,6 +27,7 @@ struct vigs_device
     resource_size_t io_size;
 
     struct idr surface_idr;
+    struct mutex surface_idr_mutex;
 
     /* Map of IO BAR. */
     drm_local_map_t *io_map;
@@ -55,18 +56,16 @@ void vigs_device_cleanup(struct vigs_device *vigs_dev);
 
 int vigs_device_mmap(struct file *filp, struct vm_area_struct *vma);
 
-/*
- * Must be called with drm_device::struct_mutex held.
- * @{
- */
 int vigs_device_add_surface(struct vigs_device *vigs_dev,
                             struct vigs_surface *sfc,
                             vigsp_surface_id* id);
 
 void vigs_device_remove_surface(struct vigs_device *vigs_dev,
                                 vigsp_surface_id sfc_id);
+
 /*
- * @}
+ * Locks drm_device::struct_mutex.
+ * @{
  */
 
 int vigs_device_add_surface_unlocked(struct vigs_device *vigs_dev,
@@ -77,6 +76,10 @@ void vigs_device_remove_surface_unlocked(struct vigs_device *vigs_dev,
                                          vigsp_surface_id sfc_id);
 
 /*
+ * @}
+ */
+
+/*
  * IOCTLs
  * @{
  */

diff --git a/drivers/gpu/drm/vigs/vigs_execbuffer.c b/drivers/gpu/drm/vigs/vigs_execbuffer.c
index 6a65a80..a71c836 100644 (file)
--- a/drivers/gpu/drm/vigs/vigs_execbuffer.c
+++ b/drivers/gpu/drm/vigs/vigs_execbuffer.c
@@ -3,9 +3,6 @@
 
 static void vigs_execbuffer_destroy(struct vigs_gem_object *gem)
 {
-    struct vigs_execbuffer *execbuffer = vigs_gem_to_vigs_execbuffer(gem);
-
-    vigs_gem_cleanup(&execbuffer->gem);
 }
 
 int vigs_execbuffer_create(struct vigs_device *vigs_dev,

diff --git a/drivers/gpu/drm/vigs/vigs_gem.c b/drivers/gpu/drm/vigs/vigs_gem.c
index 39b3837..9cef303 100644 (file)
--- a/drivers/gpu/drm/vigs/vigs_gem.c
+++ b/drivers/gpu/drm/vigs/vigs_gem.c
@@ -9,6 +9,11 @@ static void vigs_gem_bo_destroy(struct ttm_buffer_object *bo)
 {
     struct vigs_gem_object *vigs_gem = bo_to_vigs_gem(bo);
 
+    if (vigs_gem->destroy) {
+        vigs_gem->destroy(vigs_gem);
+    }
+
+    drm_gem_object_release(&vigs_gem->base);
     kfree(vigs_gem);
 }
 
@@ -61,6 +66,13 @@ int vigs_gem_init(struct vigs_gem_object *vigs_gem,
         vigs_dev->mman->bo_dev.dev_mapping = vigs_dev->drm_dev->dev_mapping;
     }
 
+    ret = drm_gem_object_init(vigs_dev->drm_dev, &vigs_gem->base, size);
+
+    if (ret != 0) {
+        kfree(vigs_gem);
+        return ret;
+    }
+
     ret = ttm_bo_init(&vigs_dev->mman->bo_dev, &vigs_gem->bo, size, bo_type,
                       &placement, 0, 0,
                       false, NULL, 0,
@@ -74,14 +86,6 @@ int vigs_gem_init(struct vigs_gem_object *vigs_gem,
     vigs_gem->pin_count = 0;
     vigs_gem->destroy = destroy;
 
-    ret = drm_gem_object_init(vigs_dev->drm_dev, &vigs_gem->base, size);
-
-    if (ret != 0) {
-        struct ttm_buffer_object *bo = &vigs_gem->bo;
-        ttm_bo_unref(&bo);
-        return ret;
-    }
-
     DRM_DEBUG_DRIVER("GEM created (type = %u, off = 0x%llX, sz = %lu)\n",
                      type,
                      vigs_gem_mmap_offset(vigs_gem),
@@ -94,18 +98,6 @@ void vigs_gem_cleanup(struct vigs_gem_object *vigs_gem)
 {
     struct ttm_buffer_object *bo = &vigs_gem->bo;
 
-    vigs_gem_reserve(vigs_gem);
-
-    vigs_gem_kunmap(vigs_gem);
-
-    vigs_gem_unreserve(vigs_gem);
-
-    DRM_DEBUG_DRIVER("GEM destroyed (type = %u, off = 0x%llX, sz = %lu)\n",
-                     vigs_gem->type,
-                     vigs_gem_mmap_offset(vigs_gem),
-                     vigs_gem_size(vigs_gem));
-
-    drm_gem_object_release(&vigs_gem->base);
     ttm_bo_unref(&bo);
 }
 
@@ -258,7 +250,20 @@ void vigs_gem_free_object(struct drm_gem_object *gem)
 {
     struct vigs_gem_object *vigs_gem = gem_to_vigs_gem(gem);
 
-    vigs_gem->destroy(vigs_gem);
+    vigs_gem_reserve(vigs_gem);
+
+    vigs_gem_kunmap(vigs_gem);
+
+    vigs_gem_unreserve(vigs_gem);
+
+    vigs_gem->freed = true;
+
+    DRM_DEBUG_DRIVER("GEM free (type = %u, off = 0x%llX, sz = %lu)\n",
+                     vigs_gem->type,
+                     vigs_gem_mmap_offset(vigs_gem),
+                     vigs_gem_size(vigs_gem));
+
+    vigs_gem_cleanup(vigs_gem);
 }
 
 int vigs_gem_init_object(struct drm_gem_object *gem)

diff --git a/drivers/gpu/drm/vigs/vigs_gem.h b/drivers/gpu/drm/vigs/vigs_gem.h
index 51405c4..0b521bd 100644 (file)
--- a/drivers/gpu/drm/vigs/vigs_gem.h
+++ b/drivers/gpu/drm/vigs/vigs_gem.h
@@ -20,6 +20,11 @@ struct vigs_gem_object
     struct ttm_buffer_object bo;
 
     /*
+     * Indicates that drm_driver::gem_free_object was called.
+     */
+    bool freed;
+
+    /*
      * Use it only when this GEM is reserved. This makes it easier
      * to reserve a set of GEMs and then unreserve them later.
      */
@@ -55,6 +60,20 @@ static inline struct vigs_gem_object *bo_to_vigs_gem(struct ttm_buffer_object *b
 }
 
 /*
+ * Must be called with drm_device::struct_mutex held.
+ * @{
+ */
+
+static inline bool vigs_gem_freed(struct vigs_gem_object *vigs_gem)
+{
+    return vigs_gem->freed;
+}
+
+/*
+ * @}
+ */
+
+/*
  * Initializes a gem object. 'size' is automatically rounded up to page size.
  * 'vigs_gem' is kfree'd on failure.
  */

diff --git a/drivers/gpu/drm/vigs/vigs_surface.c b/drivers/gpu/drm/vigs/vigs_surface.c
index c1b1bf0..a359feb 100644 (file)
--- a/drivers/gpu/drm/vigs/vigs_surface.c
+++ b/drivers/gpu/drm/vigs/vigs_surface.c
@@ -158,11 +158,13 @@ static void vigs_surface_destroy(struct vigs_gem_object *gem)
     struct vigs_surface *sfc = vigs_gem_to_vigs_surface(gem);
     struct vigs_device *vigs_dev = gem->base.dev->dev_private;
 
-    vigs_comm_destroy_surface(vigs_dev->comm, sfc->id);
+    if (sfc->id) {
+        vigs_comm_destroy_surface(vigs_dev->comm, sfc->id);
 
-    vigs_device_remove_surface(vigs_dev, sfc->id);
+        vigs_device_remove_surface(vigs_dev, sfc->id);
 
-    vigs_gem_cleanup(&sfc->gem);
+        DRM_DEBUG_DRIVER("Surface destroyed (id = %u)\n", sfc->id);
+    }
 }
 
 int vigs_surface_create(struct vigs_device *vigs_dev,
@@ -219,6 +221,7 @@ int vigs_surface_create(struct vigs_device *vigs_dev,
 fail3:
     vigs_device_remove_surface_unlocked(vigs_dev, (*sfc)->id);
 fail2:
+    (*sfc)->id = 0;
     vigs_gem_cleanup(&(*sfc)->gem);
 fail1:
     *sfc = NULL;