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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1529661856.7034.404.camel@padovan.org>
Date:   Fri, 22 Jun 2018 07:04:16 -0300
From:   Gustavo Padovan <gustavo@...ovan.org>
To:     Akhil P Oommen <akhilpo@...eaurora.org>, sumit.semwal@...aro.org,
        jcrouse@...eaurora.org, linux-media@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org,
        linux-kernel@...r.kernel.org
Cc:     smasetty@...eaurora.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2] dma-buf/fence: Take refcount on the module that owns
 the fence

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@...eaurora.org>
> ---
> 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@...labora.com>

Gustavo