Re: [PATCH] drm/prime: keep a reference from the handle to exported dma-buf (v2)

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Dec 20, 2012 at 01:39:07PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This is likely the simplest solution to the problem, and seems
> to work fine.
> 
> 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.
> 
> I'd like to refrain from too much in this patch as I'd like it to
> go to stable, so future cleanups should look at maybe reducing
> the obj storing two pointers etc.
> 
> 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.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

[snip]

> -void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
> +static void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
>  {
>  	struct drm_prime_member *member, *safe;
>  
> @@ -349,4 +380,16 @@ void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_f
>  	}
>  	mutex_unlock(&prime_fpriv->lock);
>  }
> +
> +void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
> +{
> +	drm_prime_remove_buf_handle(prime_fpriv, dma_buf);
> +}
>  EXPORT_SYMBOL(drm_prime_remove_imported_buf_handle);
> +
> +void drm_prime_remove_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
> +{
> +	drm_prime_remove_buf_handle(prime_fpriv, dma_buf);
> +	dma_buf_put(dma_buf);

We need some form of protection to ensure that the removal of the
export_dma_buf and ref dropping is atomic vs. lookups. Atm there's no
protection, which means if you e.g. transfer a gem_bo from one process to
another with dma_buf (dropping the gem_handle right away), we'll get a
leak:
[first proces]
1. handle_to_fd
2. gem_close, dropping the above internal dma-buf ref
3. pass dma-buf to other process
[other process]
4. fd_to_handle see export_dma_buf != NULL, doesn't re-add export
reference
5. close dma_buf fd, freeing it
6. gem_close tries to unref the now free'd dma_buf

For added fun, try to careful race steps 2. against 4. (or for a different
kind of fun, against 5. but that requires more evil afterwards to blow
up).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch