dma_buf_export() adds a reference to the owning module to the dmabuf (to prevent the driver from being unloaded whilst a third party still refers to the dmabuf). However, drm_gem_prime_export() was passing its own THIS_MODULE (i.e. drm.ko) rather than the driver. Extract the right owner from the device->fops instead.
v2: Use C99 initializers to zero out unset elements of dma_buf_export_info v3: Extract the right module from dev->fops.
Testcase: igt/vgem_basic/unload Reported-by: Petri Latvala petri.latvala@intel.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Petri Latvala petri.latvala@intel.com Cc: Christian König christian.koenig@amd.com Cc: stable@vger.kernel.org Tested-by: Petri Latvala petri.latvala@intel.com --- drivers/gpu/drm/drm_prime.c | 17 ++++++++++------- include/drm/drmP.h | 3 ++- 2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 57201d68cf61..80907b34d857 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -397,14 +397,17 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = { * using the PRIME helpers. */ struct dma_buf *drm_gem_prime_export(struct drm_device *dev, - struct drm_gem_object *obj, int flags) + struct drm_gem_object *obj, + int flags) { - DEFINE_DMA_BUF_EXPORT_INFO(exp_info); - - exp_info.ops = &drm_gem_prime_dmabuf_ops; - exp_info.size = obj->size; - exp_info.flags = flags; - exp_info.priv = obj; + struct dma_buf_export_info exp_info = { + .exp_name = KBUILD_MODNAME, /* white lie for debug */ + .owner = dev->driver->fops->owner, + .ops = &drm_gem_prime_dmabuf_ops, + .size = obj->size, + .flags = flags, + .priv = obj, + };
if (dev->driver->gem_prime_res_obj) exp_info.resv = dev->driver->gem_prime_res_obj(obj); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 0e99669159c1..81fcd553edf7 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1012,7 +1012,8 @@ static inline int drm_debugfs_remove_files(const struct drm_info_list *files, #endif
extern struct dma_buf *drm_gem_prime_export(struct drm_device *dev, - struct drm_gem_object *obj, int flags); + struct drm_gem_object *obj, + int flags); extern int drm_gem_prime_handle_to_fd(struct drm_device *dev, struct drm_file *file_priv, uint32_t handle, uint32_t flags, int *prime_fd);
dma_buf may live a long time, longer than the last direct user of the driver. We already hold a reference to the owner module (that prevents the object code from disappearing), but there is no reference to the drm_dev - so the pointers to the driver backend themselves may vanish.
Testcase: igt/vgem_basic/unload Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Petri Latvala petri.latvala@intel.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: stable@vger.kernel.org Tested-by: Petri Latvala petri.latvala@intel.com --- drivers/gpu/drm/armada/armada_gem.c | 9 +++++++-- drivers/gpu/drm/drm_prime.c | 10 +++++++++- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 1 + drivers/gpu/drm/tegra/gem.c | 7 ++++++- drivers/gpu/drm/udl/udl_dmabuf.c | 7 ++++++- 5 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index cb8f0347b934..bdd3af043827 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -541,13 +541,18 @@ armada_gem_prime_export(struct drm_device *dev, struct drm_gem_object *obj, int flags) { DEFINE_DMA_BUF_EXPORT_INFO(exp_info); + struct dma_buf *dma_buf;
exp_info.ops = &armada_gem_prime_dmabuf_ops; exp_info.size = obj->size; - exp_info.flags = O_RDWR; + exp_info.flags = flags; exp_info.priv = obj;
- return dma_buf_export(&exp_info); + dma_buf = dma_buf_export(&exp_info); + if (!IS_ERR(dma_buf)) + drm_dev_ref(dev); + + return dma_buf; }
struct drm_gem_object * diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 80907b34d857..5081f187d9e3 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -293,9 +293,12 @@ static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach, void drm_gem_dmabuf_release(struct dma_buf *dma_buf) { struct drm_gem_object *obj = dma_buf->priv; + struct drm_device *dev = obj->dev;
/* drop the reference on the export fd holds */ drm_gem_object_unreference_unlocked(obj); + + drm_dev_unref(dev); } EXPORT_SYMBOL(drm_gem_dmabuf_release);
@@ -408,11 +411,16 @@ struct dma_buf *drm_gem_prime_export(struct drm_device *dev, .flags = flags, .priv = obj, }; + struct dma_buf *dma_buf;
if (dev->driver->gem_prime_res_obj) exp_info.resv = dev->driver->gem_prime_res_obj(obj);
- return dma_buf_export(&exp_info); + dma_buf = dma_buf_export(&exp_info); + if (!IS_ERR(dma_buf)) + drm_dev_ref(dev); + + return dma_buf; } EXPORT_SYMBOL(drm_gem_prime_export);
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index 10265bb35604..b58b2e63add1 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -288,6 +288,7 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev, return dma_buf;
export_fences(obj, dma_buf); + drm_dev_ref(obj->base.dev); return dma_buf; }
diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index aa60d9909ea2..aaae8b6ba668 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -607,13 +607,18 @@ struct dma_buf *tegra_gem_prime_export(struct drm_device *drm, int flags) { DEFINE_DMA_BUF_EXPORT_INFO(exp_info); + struct dma_buf *dma_buf;
exp_info.ops = &tegra_gem_prime_dmabuf_ops; exp_info.size = gem->size; exp_info.flags = flags; exp_info.priv = gem;
- return dma_buf_export(&exp_info); + dma_buf = dma_buf_export(&exp_info); + if (!IS_ERR(dma_buf)) + drm_dev_ref(drm); + + return dma_buf; }
struct drm_gem_object *tegra_gem_prime_import(struct drm_device *drm, diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c index e2243edd1ce3..249cc1072112 100644 --- a/drivers/gpu/drm/udl/udl_dmabuf.c +++ b/drivers/gpu/drm/udl/udl_dmabuf.c @@ -203,13 +203,18 @@ struct dma_buf *udl_gem_prime_export(struct drm_device *dev, struct drm_gem_object *obj, int flags) { DEFINE_DMA_BUF_EXPORT_INFO(exp_info); + struct dma_buf *dma_buf;
exp_info.ops = &udl_dmabuf_ops; exp_info.size = obj->size; exp_info.flags = flags; exp_info.priv = obj;
- return dma_buf_export(&exp_info); + dma_buf = dma_buf_export(&exp_info); + if (!IS_ERR(dma_buf)) + drm_dev_ref(dev); + + return dma_buf; }
static int udl_prime_create(struct drm_device *dev,
In order to keep the dmabuf alive whilst the mmap is, we need to hold a reference to the dmabuf and not the backing object. This is important as the dmabuf not only keeps the object alive, but also the device so that
dmabuf = vgem_create_dmabuf(); ptr = mmap(... dmabuf ...); close(dmabuf);
persists across module-unload as well as device closure.
Testcase: igt/vgem_basic/unload Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Petri Latvala petri.latvala@intel.com --- drivers/gpu/drm/vgem/vgem_drv.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index f36c14729b55..74a83e41efa9 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -198,7 +198,6 @@ static struct drm_ioctl_desc vgem_ioctls[] = {
static int vgem_mmap(struct file *filp, struct vm_area_struct *vma) { - unsigned long flags = vma->vm_flags; int ret;
ret = drm_gem_mmap(filp, vma); @@ -208,7 +207,7 @@ static int vgem_mmap(struct file *filp, struct vm_area_struct *vma) /* Keep the WC mmaping set by drm_gem_mmap() but our pages * are ordinary and not special. */ - vma->vm_flags = flags | VM_DONTEXPAND | VM_DONTDUMP; + vma->vm_flags &= ~(VM_IO | VM_PFNMAP); return 0; }
@@ -281,21 +280,11 @@ static int vgem_prime_mmap(struct drm_gem_object *obj, { int ret;
- if (obj->size < vma->vm_end - vma->vm_start) - return -EINVAL; - - if (!obj->filp) - return -ENODEV; - - ret = obj->filp->f_op->mmap(obj->filp, vma); + ret = drm_gem_mmap_obj(obj, obj->size, vma); if (ret) return ret;
- fput(vma->vm_file); - vma->vm_file = get_file(obj->filp); - vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; - vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); - + vma->vm_flags &= ~(VM_IO | VM_PFNMAP); return 0; }
On Fri, Sep 30, 2016 at 02:59:59PM +0100, Chris Wilson wrote:
In order to keep the dmabuf alive whilst the mmap is, we need to hold a reference to the dmabuf and not the backing object. This is important as the dmabuf not only keeps the object alive, but also the device so that
dmabuf = vgem_create_dmabuf(); ptr = mmap(... dmabuf ...); close(dmabuf);
persists across module-unload as well as device closure.
I don't see where we grab the ref to the dma-buf here instead of the backing storage. And doesn't the exact same issue happen when you use dumb mmap? Or maybe I'm just a bit confused about what's going on here ... -Daniel
Testcase: igt/vgem_basic/unload Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Petri Latvala petri.latvala@intel.com
drivers/gpu/drm/vgem/vgem_drv.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index f36c14729b55..74a83e41efa9 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -198,7 +198,6 @@ static struct drm_ioctl_desc vgem_ioctls[] = {
static int vgem_mmap(struct file *filp, struct vm_area_struct *vma) {
unsigned long flags = vma->vm_flags; int ret;
ret = drm_gem_mmap(filp, vma);
@@ -208,7 +207,7 @@ static int vgem_mmap(struct file *filp, struct vm_area_struct *vma) /* Keep the WC mmaping set by drm_gem_mmap() but our pages * are ordinary and not special. */
- vma->vm_flags = flags | VM_DONTEXPAND | VM_DONTDUMP;
- vma->vm_flags &= ~(VM_IO | VM_PFNMAP); return 0;
}
@@ -281,21 +280,11 @@ static int vgem_prime_mmap(struct drm_gem_object *obj, { int ret;
- if (obj->size < vma->vm_end - vma->vm_start)
return -EINVAL;- if (!obj->filp)
return -ENODEV;- ret = obj->filp->f_op->mmap(obj->filp, vma);
- ret = drm_gem_mmap_obj(obj, obj->size, vma); if (ret) return ret;
- fput(vma->vm_file);
- vma->vm_file = get_file(obj->filp);
- vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
- vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
- vma->vm_flags &= ~(VM_IO | VM_PFNMAP); return 0;
}
-- 2.9.3
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 05, 2016 at 03:11:00PM +0200, Daniel Vetter wrote:
On Fri, Sep 30, 2016 at 02:59:59PM +0100, Chris Wilson wrote:
In order to keep the dmabuf alive whilst the mmap is, we need to hold a reference to the dmabuf and not the backing object. This is important as the dmabuf not only keeps the object alive, but also the device so that
dmabuf = vgem_create_dmabuf(); ptr = mmap(... dmabuf ...); close(dmabuf);
persists across module-unload as well as device closure.
I don't see where we grab the ref to the dma-buf here instead of the backing storage. And doesn't the exact same issue happen when you use dumb mmap? Or maybe I'm just a bit confused about what's going on here ...
The reference to the dmabuf is in vma->vm_file, which was used to keep the module alive. So this might be overzealous, in that there shouldn't be a way to get back from the shmemfs object to the module. However, that does make me aware of a failure path we have in patches that do add callbacks from shmemfs to the driver...
At least using the ptr after closing the module is ok. So this patch is not required. -Chris
On Wed, Oct 05, 2016 at 02:40:09PM +0100, Chris Wilson wrote:
On Wed, Oct 05, 2016 at 03:11:00PM +0200, Daniel Vetter wrote:
On Fri, Sep 30, 2016 at 02:59:59PM +0100, Chris Wilson wrote:
In order to keep the dmabuf alive whilst the mmap is, we need to hold a reference to the dmabuf and not the backing object. This is important as the dmabuf not only keeps the object alive, but also the device so that
dmabuf = vgem_create_dmabuf(); ptr = mmap(... dmabuf ...); close(dmabuf);
persists across module-unload as well as device closure.
I don't see where we grab the ref to the dma-buf here instead of the backing storage. And doesn't the exact same issue happen when you use dumb mmap? Or maybe I'm just a bit confused about what's going on here ...
The reference to the dmabuf is in vma->vm_file, which was used to keep the module alive. So this might be overzealous, in that there shouldn't be a way to get back from the shmemfs object to the module. However, that does make me aware of a failure path we have in patches that do add callbacks from shmemfs to the driver...
At least using the ptr after closing the module is ok. So this patch is not required.
I think if we entirely redirect the mmap to shmem, and not forget to also update vm_file, the driver could get unloaded without ill effects. I'll leave this one out for now, picked up 1&2 from v4 to -misc meanwhile. -Daniel
Am 30.09.2016 um 15:59 schrieb Chris Wilson:
dma_buf_export() adds a reference to the owning module to the dmabuf (to prevent the driver from being unloaded whilst a third party still refers to the dmabuf). However, drm_gem_prime_export() was passing its own THIS_MODULE (i.e. drm.ko) rather than the driver. Extract the right owner from the device->fops instead.
v2: Use C99 initializers to zero out unset elements of dma_buf_export_info v3: Extract the right module from dev->fops.
Testcase: igt/vgem_basic/unload Reported-by: Petri Latvala petri.latvala@intel.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Petri Latvala petri.latvala@intel.com Cc: Christian König christian.koenig@amd.com Cc: stable@vger.kernel.org Tested-by: Petri Latvala petri.latvala@intel.com
Reviewed-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/drm_prime.c | 17 ++++++++++------- include/drm/drmP.h | 3 ++- 2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 57201d68cf61..80907b34d857 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -397,14 +397,17 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = {
- using the PRIME helpers.
*/ struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
struct drm_gem_object *obj, int flags)
struct drm_gem_object *obj,int flags)
- DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
- exp_info.ops = &drm_gem_prime_dmabuf_ops;
- exp_info.size = obj->size;
- exp_info.flags = flags;
- exp_info.priv = obj;
struct dma_buf_export_info exp_info = {
.exp_name = KBUILD_MODNAME, /* white lie for debug */.owner = dev->driver->fops->owner,.ops = &drm_gem_prime_dmabuf_ops,.size = obj->size,.flags = flags,.priv = obj,};
if (dev->driver->gem_prime_res_obj) exp_info.resv = dev->driver->gem_prime_res_obj(obj);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 0e99669159c1..81fcd553edf7 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1012,7 +1012,8 @@ static inline int drm_debugfs_remove_files(const struct drm_info_list *files, #endif
extern struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
struct drm_gem_object *obj, int flags);
struct drm_gem_object *obj,int flags);
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Christian König -
Daniel Vetter