drm/i915/gem: Tighten checks and acquiring the mmap object

Make sure we hold the rcu lock as we acquire the rcu protected reference
of the object when looking it up from the associated mmap vma.

Closes: https://gitlab.freedesktop.org/drm/intel/issues/1083
Fixes: cc662126b413 ("drm/i915: Introduce DRM_I915_GEM_MMAP_OFFSET")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20200130143931.1906301-1-chris@chris-wilson.co.uk
(cherry picked from commit 280d14a69da2e71f43408537c008f2775d5e5360)
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index e9be2508c04fbf84595e986da8654f3491c2a0c7..0b6a442108de0ee14805438aacc59c919ce4e26e 100644 (file)
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -807,60 +807,43 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
        struct drm_vma_offset_node *node;
        struct drm_file *priv = filp->private_data;
        struct drm_device *dev = priv->minor->dev;
+       struct drm_i915_gem_object *obj = NULL;
        struct i915_mmap_offset *mmo = NULL;
-       struct drm_gem_object *obj = NULL;
        struct file *anon;
 
        if (drm_dev_is_unplugged(dev))
                return -ENODEV;
 
+       rcu_read_lock();
        drm_vma_offset_lock_lookup(dev->vma_offset_manager);
        node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
                                                  vma->vm_pgoff,
                                                  vma_pages(vma));
-       if (likely(node)) {
-               mmo = container_of(node, struct i915_mmap_offset,
-                                  vma_node);
-               /*
-                * In our dependency chain, the drm_vma_offset_node
-                * depends on the validity of the mmo, which depends on
-                * the gem object. However the only reference we have
-                * at this point is the mmo (as the parent of the node).
-                * Try to check if the gem object was at least cleared.
-                */
-               if (!mmo || !mmo->obj) {
-                       drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
-                       return -EINVAL;
-               }
+       if (node && drm_vma_node_is_allowed(node, priv)) {
                /*
                 * Skip 0-refcnted objects as it is in the process of being
                 * destroyed and will be invalid when the vma manager lock
                 * is released.
                 */
-               obj = &mmo->obj->base;
-               if (!kref_get_unless_zero(&obj->refcount))
-                       obj = NULL;
+               mmo = container_of(node, struct i915_mmap_offset, vma_node);
+               obj = i915_gem_object_get_rcu(mmo->obj);
        }
        drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
+       rcu_read_unlock();
        if (!obj)
-               return -EINVAL;
-
-       if (!drm_vma_node_is_allowed(node, priv)) {
-               drm_gem_object_put_unlocked(obj);
-               return -EACCES;
-       }
+               return node ? -EACCES : -EINVAL;
 
-       if (i915_gem_object_is_readonly(to_intel_bo(obj))) {
+       if (i915_gem_object_is_readonly(obj)) {
                if (vma->vm_flags & VM_WRITE) {
-                       drm_gem_object_put_unlocked(obj);
+                       i915_gem_object_put(obj);
                        return -EINVAL;
                }
                vma->vm_flags &= ~VM_MAYWRITE;
        }
 
-       anon = mmap_singleton(to_i915(obj->dev));
+       anon = mmap_singleton(to_i915(dev));
        if (IS_ERR(anon)) {
-               drm_gem_object_put_unlocked(obj);
+               i915_gem_object_put(obj);
                return PTR_ERR(anon);
        }
 

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index db70a3306e5939e97907b3f2e3997fd82f760b14..9c86f2dea947b415de4524718c174adbc30f5e44 100644 (file)
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -69,6 +69,15 @@ i915_gem_object_lookup_rcu(struct drm_file *file, u32 handle)
        return idr_find(&file->object_idr, handle);
 }
 
+static inline struct drm_i915_gem_object *
+i915_gem_object_get_rcu(struct drm_i915_gem_object *obj)
+{
+       if (obj && !kref_get_unless_zero(&obj->base.refcount))
+               obj = NULL;
+
+       return obj;
+}
+
 static inline struct drm_i915_gem_object *
 i915_gem_object_lookup(struct drm_file *file, u32 handle)
 {
@@ -76,8 +85,7 @@ i915_gem_object_lookup(struct drm_file *file, u32 handle)
 
        rcu_read_lock();
        obj = i915_gem_object_lookup_rcu(file, handle);
-       if (obj && !kref_get_unless_zero(&obj->base.refcount))
-               obj = NULL;
+       obj = i915_gem_object_get_rcu(obj);
        rcu_read_unlock();
 
        return obj;