drm/prime: fixup the dma buf export cache locking

Well, actually add some, because currently there's exactly none:
- in handle_to_fd we only take the file_priv's prime lock, but the
  underlying gem object we're exporting isn't per-file.
- in each driver's dma_buf->release callback we don't grab any locks
  at all.

Also, we're not protected by any object lifetime rules: We can export
and GEM object then fclose that fd _while_ we re-export the same
object again. If we're unlucky, we'll see a non-zero export_dma_buf,
but the file's refcount is already zero.

To fix this, we need two pieces:
- We need to add some locking. I've opted for a new per-device lock to
  not entangle this locking with the already messy dev->struct_mutex
  misery.
- We need to hold a reference onto the dma buf object while
  exported_dma_buf is non-NULL.  Otherwise we'll always have races
  where the dma buf's file refcount is zero already, but
  export_dma_buf is still set.

Now obviously this dma buf reference would lead to a refcount loop, so
we need to break that at the right time. Do the same trick as for the
gem flink name: As soon as all GEM userspace handles are gone (i.e.
obj->handle_count == 0) we drop this dma_buf reference.

This has a few funny complications:
- We could hold the last dma buf reference (but by necessity not the
  last reference to the underlying GEM object). The only thing that
  can therefore happen is that the dma buf object gets freed and we
  call the driver's ->release callback. Which needs to drop its own
  reference to the underlying GEM object.

  To ensure that we don't end up with surprises kill the (already)
  unused handle_unreference variant and only expose the unlocked one
  (which is safer).

- Userspace might race with us when exporting a dma_buf and close the
  last userspace GEM handle. Check for that and re-call handle_free to
  clean up the mess. Note that the current flink name code is
  suffering from the same race but doesn't hanlde it. A follow-up
  patch will fix this.

- Userspace might drop all GEM handles, but still hold a reference
  through a dma buf object. To ensure that an export of this object
  after it has been imported will do the right thing, we also need to
  set up the dma buf cache again. Otherwise we could create a second
  dma buf when exporting it again. Also add some paranoia and check
  whether a re-import gives back the right dma_buf.

  Because this cache is now not only set on export but also on import
  and is always set if there's a dma buf associated for this GEM
  object (and some GEM userspace handles still around), drop the
  export_ prefix and just call it obj->dma_buf.

Note that this patch also fixes another little race on the export
side: As soon as we've called dma_buf_fd, userspace could sneak in and
close the fd, hence reaping the last reference count to the dma_buf
and destroying it. Avoid this use-after-free by doing the
fd-installing last, once everything has been set up correctly.

v2: Be careful to not drop any references while holding locks. This
only really affected -ENOMEM error paths, but could lead to a deadlock
on one of the prime locks.

v3: Chris Wilson complained that a per-obj mutex is a bit much. So
moved to the device again.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

1 files changed, 25 insertions, 21 deletions

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index cb47d128c095..b57a2cfa1760 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -669,10 +669,23 @@ struct drm_gem_object {
 
 	void *driver_private;
 
-	/* dma buf exported from this GEM object */
-	struct dma_buf *export_dma_buf;
+	/** dma_buf - dma buf associated with this GEM object
+	 *
+	 * This is a cache of the dma buf associated to this GEM object to
+	 * ensure that there is only ever one dma buf for a given GEM object.
+	 * Only kept around as long as there are still userspace handles around.
+	 * If only the dma buf holds the last userspace-visible reference on
+	 * this GEM object, dma_buf will be NULL, but set again on import.
+	 * Protected by dev->prime_lock.
+	 */
+	struct dma_buf *dma_buf;
 
-	/* dma buf attachment backing this object */
+	/**
+	 * import_attach - dma buf attachment backing this object.
+	 *
+	 * Invariant over the lifetime
+	 * of an object, hence needs no locking.
+	 */
 	struct dma_buf_attachment *import_attach;
 };
 
@@ -1204,6 +1217,15 @@ struct drm_device {
 	/*@{ */
 	spinlock_t object_name_lock;
 	struct idr object_name_idr;
+
+	/**
+	 * prime_lock - protects dma buf state of exported GEM objects
+	 *
+	 * Specifically obj->dma_buf, but this can be used by drivers for
+	 * other things.
+	 */
+	struct mutex prime_lock;
+
 	/*@} */
 	int switch_power_state;
 
@@ -1662,24 +1684,6 @@ drm_gem_object_handle_reference(struct drm_gem_object *obj)
 }
 
 static inline void
-drm_gem_object_handle_unreference(struct drm_gem_object *obj)
-{
-	if (obj == NULL)
-		return;
-
-	if (atomic_read(&obj->handle_count) == 0)
-		return;
-	/*
-	 * Must bump handle count first as this may be the last
-	 * ref, in which case the object would disappear before we
-	 * checked for a name
-	 */
-	if (atomic_dec_and_test(&obj->handle_count))
-		drm_gem_object_handle_free(obj);
-	drm_gem_object_unreference(obj);
-}
-
-static inline void
 drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
 {
 	if (obj == NULL)