Re: [PATCH v2] dma-buf/fence: Take refcount on the module that owns the fence

[Gustavo Padovan]

Hi Akhil,

On Fri, 2018-06-22 at 15:10 +0530, Akhil P Oommen wrote:
> Each fence object holds function pointers of the module that
> initialized
> it. Allowing the module to unload before this fence's release is
> catastrophic. So, keep a refcount on the module until the fence is
> released.
> 
> Signed-off-by: Akhil P Oommen <akhilpo@xxxxxxxxxxxxxx>
> ---
> Changes in v2:
> - added description for the new function parameter.
> 
>  drivers/dma-buf/dma-fence.c | 16 +++++++++++++---
>  include/linux/dma-fence.h   | 10 ++++++++--
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-
> fence.c
> index 4edb9fd..2aaa44e 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -18,6 +18,7 @@
>   * more details.
>   */
>  
> +#include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/export.h>
>  #include <linux/atomic.h>
> @@ -168,6 +169,7 @@ void dma_fence_release(struct kref *kref)
>  {
>  	struct dma_fence *fence =
>  		container_of(kref, struct dma_fence, refcount);
> +	struct module *module = fence->owner;
>  
>  	trace_dma_fence_destroy(fence);
>  
> @@ -178,6 +180,8 @@ void dma_fence_release(struct kref *kref)
>  		fence->ops->release(fence);
>  	else
>  		dma_fence_free(fence);
> +
> +	module_put(module);
>  }
>  EXPORT_SYMBOL(dma_fence_release);
>  
> @@ -541,6 +545,7 @@ struct default_wait_cb {
>  
>  /**
>   * dma_fence_init - Initialize a custom fence.
> + * @module:	[in]	the module that calls this API
>   * @fence:	[in]	the fence to initialize
>   * @ops:	[in]	the dma_fence_ops for operations on this
> fence
>   * @lock:	[in]	the irqsafe spinlock to use for locking
> this fence
> @@ -556,8 +561,9 @@ struct default_wait_cb {
>   * to check which fence is later by simply using dma_fence_later.
>   */
>  void
> -dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops
> *ops,
> -	       spinlock_t *lock, u64 context, unsigned seqno)
> +_dma_fence_init(struct module *module, struct dma_fence *fence,
> +		const struct dma_fence_ops *ops, spinlock_t *lock,
> +		u64 context, unsigned seqno)
>  {
>  	BUG_ON(!lock);
>  	BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
> @@ -571,7 +577,11 @@ struct default_wait_cb {
>  	fence->seqno = seqno;
>  	fence->flags = 0UL;
>  	fence->error = 0;
> +	fence->owner = module;
> +
> +	if (!try_module_get(module))
> +		fence->owner = NULL;
>  
>  	trace_dma_fence_init(fence);
>  }
> -EXPORT_SYMBOL(dma_fence_init);
> +EXPORT_SYMBOL(_dma_fence_init);

Do we still need to export the symbol, it won't be called from outside
anymore? Other than that looks good to me:

Reviewed-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxx>

Gustavo