Commit d0b2c533 authored by Daniel Vetter's avatar Daniel Vetter Committed by Dave Airlie
Browse files

drm/prime: Always add exported buffers to the handle cache 



... not only when the dma-buf is freshly created. In contrived
examples someone else could have exported/imported the dma-buf already
and handed us the gem object with a flink name. If such on object gets
reexported as a dma_buf we won't have it in the handle cache already,
which breaks the guarantee that for dma-buf imports we always hand
back an existing handle if there is one.

This is exercised by igt/prime_self_import/with_one_bo_two_files

Now if we extend the locked sections just a notch more we can also
plug th racy buf/handle cache setup in handle_to_fd:

If evil userspace races a concurrent gem close against a prime export
operation we can end up tearing down the gem handle before the dma buf
handle cache is set up. When handle_to_fd gets around to adding the
handle to the cache there will be no one left to clean it up,
effectily leaking the bo (and the dma-buf, since the handle cache
holds a ref on the dma-buf):

Thread A			Thread B

handle_to_fd:

lookup gem object from handle
creates new dma_buf

				gem_close on the same handle
				obj->dma_buf is set, but file priv buf
				handle cache has no entry

				obj->handle_count drops to 0

drm_prime_add_buf_handle sets up the handle cache

-> We have a dma-buf reference in the handle cache, but since the
handle_count of the gem object already dropped to 0 no on will clean
it up. When closing the drm device fd we'll hit the WARN_ON in
drm_prime_destroy_file_private.

The important change is to extend the critical section of the
filp->prime.lock to cover the gem handle lookup. This serializes with
a concurrent gem handle close.

This leak is exercised by igt/prime_self_import/export-vs-gem_close-race

Signed-off-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: default avatarDave Airlie <airlied@redhat.com>
parent de9564d8

drivers/gpu/drm/drm_gem.c

+4 −2
Original line number Diff line number Diff line
@@ -195,10 +195,12 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) 
	 * Note: obj->dma_buf can't disappear as long as we still hold a

	 * handle reference in obj->handle_count.

	 */

	mutex_lock(&filp->prime.lock);

	if (obj->dma_buf) {

		drm_prime_remove_buf_handle(&filp->prime,

		drm_prime_remove_buf_handle_locked(&filp->prime,

						   obj->dma_buf);

	}

	mutex_unlock(&filp->prime.lock);

}



static void drm_gem_object_ref_bug(struct kref *list_kref)

drivers/gpu/drm/drm_prime.c

+48 −33
Original line number Diff line number Diff line
@@ -83,6 +83,19 @@ static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, 
	return 0;

}



static struct dma_buf *drm_prime_lookup_buf_by_handle(struct drm_prime_file_private *prime_fpriv,

						      uint32_t handle)

{

	struct drm_prime_member *member;



	list_for_each_entry(member, &prime_fpriv->head, entry) {

		if (member->handle == handle)

			return member->dma_buf;

	}



	return NULL;

}



static int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpriv,

				       struct dma_buf *dma_buf,

				       uint32_t *handle)
@@ -146,8 +159,7 @@ static void drm_gem_map_detach(struct dma_buf *dma_buf, 
	attach->priv = NULL;

}



static void drm_prime_remove_buf_handle_locked(

		struct drm_prime_file_private *prime_fpriv,

void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpriv,

					struct dma_buf *dma_buf)

{

	struct drm_prime_member *member, *safe;
@@ -337,6 +349,8 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, 
	 */

	obj->dma_buf = dmabuf;

	get_dma_buf(obj->dma_buf);

	/* Grab a new ref since the callers is now used by the dma-buf */

	drm_gem_object_reference(obj);



	return dmabuf;

}
@@ -349,10 +363,20 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, 
	int ret = 0;

	struct dma_buf *dmabuf;



	mutex_lock(&file_priv->prime.lock);

	obj = drm_gem_object_lookup(dev, file_priv, handle);

	if (!obj)

		return -ENOENT;

	if (!obj)  {

		ret = -ENOENT;

		goto out_unlock;

	}



	dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);

	if (dmabuf) {

		get_dma_buf(dmabuf);

		goto out_have_handle;

	}



	mutex_lock(&dev->object_name_lock);

	/* re-export the original imported object */

	if (obj->import_attach) {

		dmabuf = obj->import_attach->dmabuf;
@@ -360,45 +384,45 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, 
		goto out_have_obj;

	}



	mutex_lock(&dev->object_name_lock);

	if (obj->dma_buf) {

		get_dma_buf(obj->dma_buf);

		dmabuf = obj->dma_buf;

		mutex_unlock(&dev->object_name_lock);

		goto out_have_obj;

	}



	dmabuf = export_and_register_object(dev, obj, flags);

	mutex_unlock(&dev->object_name_lock);

	if (IS_ERR(dmabuf)) {

		/* normally the created dma-buf takes ownership of the ref,

		 * but if that fails then drop the ref

		 */

		ret = PTR_ERR(dmabuf);

		mutex_unlock(&dev->object_name_lock);

		goto out;

	}



	mutex_lock(&file_priv->prime.lock);

	/* if we've exported this buffer the cheat and add it to the import list

	 * so we get the correct handle back

out_have_obj:

	/*

	 * If we've exported this buffer then cheat and add it to the import list

	 * so we get the correct handle back. We must do this under the

	 * protection of dev->object_name_lock to ensure that a racing gem close

	 * ioctl doesn't miss to remove this buffer handle from the cache.

	 */

	ret = drm_prime_add_buf_handle(&file_priv->prime,

				       dmabuf, handle);

	mutex_unlock(&dev->object_name_lock);

	if (ret)

		goto fail_put_dmabuf;



out_have_handle:

	ret = dma_buf_fd(dmabuf, flags);

	if (ret < 0)

		goto fail_rm_handle;



	*prime_fd = ret;

	mutex_unlock(&file_priv->prime.lock);

	return 0;



out_have_obj:

	ret = dma_buf_fd(dmabuf, flags);

	/*

	 * We must _not_ remove the buffer from the handle cache since the newly

	 * created dma buf is already linked in the global obj->dma_buf pointer,

	 * and that is invariant as long as a userspace gem handle exists.

	 * Closing the handle will clean out the cache anyway, so we don't leak.

	 */

	if (ret < 0) {

		dma_buf_put(dmabuf);

		goto fail_put_dmabuf;

	} else {

		*prime_fd = ret;

		ret = 0;
@@ -406,14 +430,13 @@ out_have_obj: 


	goto out;



fail_rm_handle:

	drm_prime_remove_buf_handle_locked(&file_priv->prime,

					   dmabuf);

	mutex_unlock(&file_priv->prime.lock);

fail_put_dmabuf:

	dma_buf_put(dmabuf);

out:

	drm_gem_object_unreference_unlocked(obj);

out_unlock:

	mutex_unlock(&file_priv->prime.lock);



	return ret;

}

EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
@@ -669,11 +692,3 @@ void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv) 
	WARN_ON(!list_empty(&prime_fpriv->head));

}

EXPORT_SYMBOL(drm_prime_destroy_file_private);
 


void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)

{

	mutex_lock(&prime_fpriv->lock);

	drm_prime_remove_buf_handle_locked(prime_fpriv, dma_buf);

	mutex_unlock(&prime_fpriv->lock);

}

EXPORT_SYMBOL(drm_prime_remove_buf_handle);

include/drm/drmP.h

+1 −1
Original line number Diff line number Diff line
@@ -1508,7 +1508,7 @@ int drm_gem_dumb_destroy(struct drm_file *file, 


void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv);

void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv);

void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf);

void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf);



#if DRM_DEBUG_CODE

extern int drm_vma_info(struct seq_file *m, void *data);

CRA Git | Maintained and supported by SUSTech CRA and CCSE