drm/prime: keep a reference from the handle to exported dma-buf (v6)

Currently we have a problem with this:
1. i915: create gem object
2. i915: export gem object to prime
3. radeon: import gem object
4. close prime fd
5. radeon: unref object
6. i915: unref object

i915 has an imported object reference in its file priv, that isn't
cleaned up properly until fd close. The reference gets added at step 2,
but at step 6 we don't have enough info to clean it up.

The solution is to take a reference on the dma-buf when we export it,
and drop the reference when the gem handle goes away.

So when we export a dma_buf from a gem object, we keep track of it
with the handle, we take a reference to the dma_buf. When we close
the handle (i.e. userspace is finished with the buffer), we drop
the reference to the dma_buf, and it gets collected.

This patch isn't meant to fix any other problem or bikesheds, and it doesn't
fix any races with other scenarios.

v1.1: move export symbol line back up.

v2: okay I had to do a bit more, as the first patch showed a leak
on one of my tests, that I found using the dma-buf debugfs support,
the problem case is exporting a buffer twice with the same handle,
we'd add another export handle for it unnecessarily, however
we now fail if we try to export the same object with a different gem handle,
however I'm not sure if that is a case I want to support, and I've
gotten the code to WARN_ON if we hit something like that.

v2.1: rebase this patch, write better commit msg.
v3: cleanup error handling, track import vs export in linked list,
these two patches were separate previously, but seem to work better
like this.
v4: danvet is correct, this code is no longer useful, since the buffer
better exist, so remove it.
v5: always take a reference to the dma buf object, import or export.
(Imre Deak contributed this originally)
v6: square the circle, remove import vs export tracking now
that there is no difference

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
Signed-off-by: Dave Airlie <airlied@redhat.com>

2 files changed, 42 insertions, 38 deletions

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index af779ae19ebf..cf919e36e8ae 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -205,11 +205,11 @@ static void
 drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
 {
         if (obj->import_attach) {
-                drm_prime_remove_imported_buf_handle(&filp->prime,
+                drm_prime_remove_buf_handle(&filp->prime,
                                 obj->import_attach->dmabuf);
         }
         if (obj->export_dma_buf) {
-                drm_prime_remove_imported_buf_handle(&filp->prime,
+                drm_prime_remove_buf_handle(&filp->prime,
                                 obj->export_dma_buf);
         }
 }
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 25d02187067e..7830d8e1f212 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -62,6 +62,7 @@ struct drm_prime_member {
         struct dma_buf *dma_buf;
         uint32_t handle;
 };
+static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle);
 
 static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
                 enum dma_data_direction dir)
@@ -200,7 +201,8 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 {
         struct drm_gem_object *obj;
         void *buf;
-        int ret;
+        int ret = 0;
+        struct dma_buf *dmabuf;
 
         obj = drm_gem_object_lookup(dev, file_priv, handle);
         if (!obj)
@@ -209,43 +211,44 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
         mutex_lock(&file_priv->prime.lock);
         /* re-export the original imported object */
         if (obj->import_attach) {
-                get_dma_buf(obj->import_attach->dmabuf);
-                *prime_fd = dma_buf_fd(obj->import_attach->dmabuf, flags);
-                drm_gem_object_unreference_unlocked(obj);
-                mutex_unlock(&file_priv->prime.lock);
-                return 0;
+                dmabuf = obj->import_attach->dmabuf;
+                goto out_have_obj;
         }
 
         if (obj->export_dma_buf) {
-                get_dma_buf(obj->export_dma_buf);
-                *prime_fd = dma_buf_fd(obj->export_dma_buf, flags);
-                drm_gem_object_unreference_unlocked(obj);
-        } else {
-                buf = dev->driver->gem_prime_export(dev, obj, flags);
-                if (IS_ERR(buf)) {
-                        /* normally the created dma-buf takes ownership of the ref,
-                         * but if that fails then drop the ref
-                         */
-                        drm_gem_object_unreference_unlocked(obj);
-                        mutex_unlock(&file_priv->prime.lock);
-                        return PTR_ERR(buf);
-                }
-                obj->export_dma_buf = buf;
-                *prime_fd = dma_buf_fd(buf, flags);
+                dmabuf = obj->export_dma_buf;
+                goto out_have_obj;
         }
+
+        buf = dev->driver->gem_prime_export(dev, obj, flags);
+        if (IS_ERR(buf)) {
+                /* normally the created dma-buf takes ownership of the ref,
+                 * but if that fails then drop the ref
+                 */
+                ret = PTR_ERR(buf);
+                goto out;
+        }
+        obj->export_dma_buf = buf;
+
         /* if we've exported this buffer the cheat and add it to the import list
          * so we get the correct handle back
          */
-        ret = drm_prime_add_imported_buf_handle(&file_priv->prime,
-                        obj->export_dma_buf, handle);
-        if (ret) {
-                drm_gem_object_unreference_unlocked(obj);
-                mutex_unlock(&file_priv->prime.lock);
-                return ret;
-        }
+        ret = drm_prime_add_buf_handle(&file_priv->prime,
+                                       obj->export_dma_buf, handle);
+        if (ret)
+                goto out;
 
+        *prime_fd = dma_buf_fd(buf, flags);
         mutex_unlock(&file_priv->prime.lock);
         return 0;
+
+out_have_obj:
+        get_dma_buf(dmabuf);
+        *prime_fd = dma_buf_fd(dmabuf, flags);
+out:
+        drm_gem_object_unreference_unlocked(obj);
+        mutex_unlock(&file_priv->prime.lock);
+        return ret;
 }
 EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
 
@@ -314,7 +317,7 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 
         mutex_lock(&file_priv->prime.lock);
 
-        ret = drm_prime_lookup_imported_buf_handle(&file_priv->prime,
+        ret = drm_prime_lookup_buf_handle(&file_priv->prime,
                         dma_buf, handle);
         if (!ret) {
                 ret = 0;
@@ -333,7 +336,7 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
         if (ret)
                 goto out_put;
 
-        ret = drm_prime_add_imported_buf_handle(&file_priv->prime,
+        ret = drm_prime_add_buf_handle(&file_priv->prime,
                         dma_buf, *handle);
         if (ret)
                 goto fail;
@@ -487,7 +490,7 @@ void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv)
 }
 EXPORT_SYMBOL(drm_prime_destroy_file_private);
 
-int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
+static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
 {
         struct drm_prime_member *member;
 
@@ -495,14 +498,14 @@ int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv
         if (!member)
                 return -ENOMEM;
 
+        get_dma_buf(dma_buf);
         member->dma_buf = dma_buf;
         member->handle = handle;
         list_add(&member->entry, &prime_fpriv->head);
         return 0;
 }
-EXPORT_SYMBOL(drm_prime_add_imported_buf_handle);
 
-int drm_prime_lookup_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t *handle)
+int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t *handle)
 {
         struct drm_prime_member *member;
 
@@ -514,19 +517,20 @@ int drm_prime_lookup_imported_buf_handle(struct drm_prime_file_private *prime_fp
         }
         return -ENOENT;
 }
-EXPORT_SYMBOL(drm_prime_lookup_imported_buf_handle);
+EXPORT_SYMBOL(drm_prime_lookup_buf_handle);
 
-void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
+void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
 {
         struct drm_prime_member *member, *safe;
 
         mutex_lock(&prime_fpriv->lock);
         list_for_each_entry_safe(member, safe, &prime_fpriv->head, entry) {
                 if (member->dma_buf == dma_buf) {
+                        dma_buf_put(dma_buf);
                         list_del(&member->entry);
                         kfree(member);
                 }
         }
         mutex_unlock(&prime_fpriv->lock);
 }
-EXPORT_SYMBOL(drm_prime_remove_imported_buf_handle);
+EXPORT_SYMBOL(drm_prime_remove_buf_handle);