[PATCH v3 1/3] drm/prime: Pass the right module owner through to dma_buf_export()

[PATCH v3 1/3] drm/prime: Pass the right module owner through to dma_buf_export()

[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>
---
 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);
-- 
2.9.3

[PATCH v3 2/3] drm/prime: Take a ref on the drm_dev when exporting a dma_buf

[Chris Wilson]

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,
-- 
2.9.3

[PATCH v3 3/3] drm/vgem: Keep a reference to the dmabuf when mmaped

[Chris Wilson]

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;
 }
 
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/3] drm/vgem: Keep a reference to the dmabuf when mmaped

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/3] drm/vgem: Keep a reference to the dmabuf when mmaped

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/3] drm/vgem: Keep a reference to the dmabuf when mmaped

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/3] drm/prime: Pass the right module owner through to dma_buf_export()

[Christian König]

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);
>   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);


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx