Re: [PATCH] dma-fence: add comment for WARN_ON in dma_fence_release()

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jan 29, 2018 at 05:40:02PM +0200, Oded Gabbay wrote:
> In dma_fence_release() there is a WARN_ON which could be triggered by
> several cases of wrong dma-fence usage. This patch adds a comment to
> explain two use-cases to help driver developers that use dma-fence
> and trigger that WARN_ON to better understand the reasons for it.
> 
> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>

Not sure how useful this is, trying to do anything with a dma_fence while
not holding a reference is just plain buggy. If you do that, then all
kinds of use-after-free hilarity can happen. Trying to enumerate all the
ways you can get refcounting wrong seems futile.

What we maybe could do is a simple one-line like

	/* Failed to signal before release, could be a refcounting issue. */

I think this is generally a true statement and more useful hint for the
next convoluted scenario (which in all likelihood will be different from
yours).
-Daniel

> ---
>  drivers/dma-buf/dma-fence.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 5d101c4053e0..a7170ab23ec0 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -171,6 +171,39 @@ void dma_fence_release(struct kref *kref)
>  
>  	trace_dma_fence_destroy(fence);
>  
> +	/*
> +	 * If the WARN_ON below is triggered it could be because the dma fence
> +	 * was not signaled and therefore, the cb list is still not empty
> +	 * because the cb functions were not called.
> +	 *
> +	 * A more subtle case is where the fence got signaled by a thread that
> +	 * didn't hold a ref to the fence. The following describes the scenario:
> +	 *
> +	 *      Thread A                            Thread B
> +	 *--------------------------        --------------------------
> +	 * calls dma_fence_signal() {
> +	 *      set signal bit
> +	 *
> +	 *            scheduled out
> +	 *      ---------------------------> calls dma_fence_wait_timeout() and
> +	 *                                   returns immediately
> +	 *
> +	 *                                   calls dma_fence_put()
> +	 *                                         |
> +	 *                                         |thread A doesn't hold ref
> +	 *                                         |to fence so ref goes to 0
> +	 *                                         |and release is called
> +	 *                                         |
> +	 *                                         -> dma_fence_release()
> +	 *                                            |
> +	 *                                            -> WARN_ON triggered
> +	 *
> +	 *      go over CB list,
> +	 *      call each CB and remove it
> +	 *      }
> +	 *
> +	 *
> +	 */
>  	WARN_ON(!list_empty(&fence->cb_list));
>  
>  	if (fence->ops->release)
> -- 
> 2.14.3
> 

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