Commit f17b8980 authored by Chris Wilson's avatar Chris Wilson

drm/i915/gem: Drop local vma->vm_file reference

We use the global device inode, shared amongst all files, and not the
user's device filp to provide the backing storage for the mmap. The
vma->vm_file provides a redundant reference that breaks existing
expected behaviour that closing the user's device fd will release the
resources bound to it, if a mmap persists. (Even without the
vma->vm_file, the mmap will persist past the user's fd as the storage is
bound to the device, i.e. our reference is on the object not file.)

Fixes: cc662126 ("drm/i915: Introduce DRM_I915_GEM_MMAP_OFFSET")
Closes: https://gitlab.freedesktop.org/drm/intel/issues/919Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
Acked-by: default avatarAbdiel Janulgue <abdiel.janulgue@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20200101141007.755429-1-chris@chris-wilson.co.uk
parent 1cd21a7c
...@@ -4,6 +4,7 @@ ...@@ -4,6 +4,7 @@
* Copyright © 2014-2016 Intel Corporation * Copyright © 2014-2016 Intel Corporation
*/ */
#include <linux/anon_inodes.h>
#include <linux/mman.h> #include <linux/mman.h>
#include <linux/pfn_t.h> #include <linux/pfn_t.h>
#include <linux/sizes.h> #include <linux/sizes.h>
...@@ -686,6 +687,46 @@ static const struct vm_operations_struct vm_ops_cpu = { ...@@ -686,6 +687,46 @@ static const struct vm_operations_struct vm_ops_cpu = {
.close = vm_close, .close = vm_close,
}; };
static int singleton_release(struct inode *inode, struct file *file)
{
struct drm_i915_private *i915 = file->private_data;
cmpxchg(&i915->gem.mmap_singleton, file, NULL);
drm_dev_put(&i915->drm);
return 0;
}
static const struct file_operations singleton_fops = {
.owner = THIS_MODULE,
.release = singleton_release,
};
static struct file *mmap_singleton(struct drm_i915_private *i915)
{
struct file *file;
rcu_read_lock();
file = i915->gem.mmap_singleton;
if (file && !get_file_rcu(file))
file = NULL;
rcu_read_unlock();
if (file)
return file;
file = anon_inode_getfile("i915.gem", &singleton_fops, i915, O_RDWR);
if (IS_ERR(file))
return file;
/* Everyone shares a single global address space */
file->f_mapping = i915->drm.anon_inode->i_mapping;
smp_store_mb(i915->gem.mmap_singleton, file);
drm_dev_get(&i915->drm);
return file;
}
/* /*
* This overcomes the limitation in drm_gem_mmap's assignment of a * This overcomes the limitation in drm_gem_mmap's assignment of a
* drm_gem_object as the vma->vm_private_data. Since we need to * drm_gem_object as the vma->vm_private_data. Since we need to
...@@ -699,6 +740,7 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma) ...@@ -699,6 +740,7 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
struct drm_device *dev = priv->minor->dev; struct drm_device *dev = priv->minor->dev;
struct i915_mmap_offset *mmo = NULL; struct i915_mmap_offset *mmo = NULL;
struct drm_gem_object *obj = NULL; struct drm_gem_object *obj = NULL;
struct file *anon;
if (drm_dev_is_unplugged(dev)) if (drm_dev_is_unplugged(dev))
return -ENODEV; return -ENODEV;
...@@ -747,9 +789,26 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma) ...@@ -747,9 +789,26 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
vma->vm_flags &= ~VM_MAYWRITE; vma->vm_flags &= ~VM_MAYWRITE;
} }
anon = mmap_singleton(to_i915(obj->dev));
if (IS_ERR(anon)) {
drm_gem_object_put_unlocked(obj);
return PTR_ERR(anon);
}
vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
vma->vm_private_data = mmo; vma->vm_private_data = mmo;
/*
* We keep the ref on mmo->obj, not vm_file, but we require
* vma->vm_file->f_mapping, see vma_link(), for later revocation.
* Our userspace is accustomed to having per-file resource cleanup
* (i.e. contexts, objects and requests) on their close(fd), which
* requires avoiding extraneous references to their filp, hence why
* we prefer to use an anonymous file for their mmaps.
*/
fput(vma->vm_file);
vma->vm_file = anon;
switch (mmo->mmap_type) { switch (mmo->mmap_type) {
case I915_MMAP_TYPE_WC: case I915_MMAP_TYPE_WC:
vma->vm_page_prot = vma->vm_page_prot =
......
...@@ -1252,6 +1252,16 @@ struct drm_i915_private { ...@@ -1252,6 +1252,16 @@ struct drm_i915_private {
struct llist_head free_list; struct llist_head free_list;
struct work_struct free_work; struct work_struct free_work;
} contexts; } contexts;
/*
* We replace the local file with a global mappings as the
* backing storage for the mmap is on the device and not
* on the struct file, and we do not want to prolong the
* lifetime of the local fd. To minimise the number of
* anonymous inodes we create, we use a global singleton to
* share the global mapping.
*/
struct file *mmap_singleton;
} gem; } gem;
u8 pch_ssc_use; u8 pch_ssc_use;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment