Re: [PATCH] drm/i915: Disable kmem_caches when KASAN is enabled

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> kasan is very good at detecting use-after-free. However, our requests
> are allocated from a rcu-typesafe slab that is important for performance
> but makes kasan less effective. When enabling kasan we are intentionally
> looking for memory errors, disable the use of our caches to improve the
> likelihood of kasan spotting a bug.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/Kconfig.debug      | 13 +++++++++++++
>  drivers/gpu/drm/i915/i915_gem.c         | 11 +++++++++--
>  drivers/gpu/drm/i915/i915_gem_request.c | 25 ++++++++++++++++++++-----
>  drivers/gpu/drm/i915/i915_vma.c         | 15 ++++++++++++---
>  4 files changed, 54 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
> index e091809a9a9e..bd8e90e4dfb9 100644
> --- a/drivers/gpu/drm/i915/Kconfig.debug
> +++ b/drivers/gpu/drm/i915/Kconfig.debug
> @@ -48,6 +48,19 @@ config DRM_I915_DEBUG_GEM
>  
>            If in doubt, say "N".
>  
> +config DRM_I915_DEBUG_KASAN
> +        bool "Insert extra checks when using KASAN"
> +        default n
> +        depends on DRM_I915_WERROR
> +        depends on KASAN
> +        help
> +	  Turns off use of kmem_caches to improve KASAN error detection,
> +          and inserts extra santiy checks.

s/santiy/sanity.

Would i915.kmem_cache=0/1 be too costly?

The name could also be DEBUG_NO_KMEM_CACHE
-Mika

> +
> +          Recommended for driver developers only.
> +
> +          If in doubt, say "N".
> +
>  config DRM_I915_SW_FENCE_DEBUG_OBJECTS
>          bool "Enable additional driver debugging for fence objects"
>          depends on DRM_I915
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 202bb850f260..0195468531f7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -639,13 +639,20 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
>  
>  void *i915_gem_object_alloc(struct drm_i915_private *dev_priv)
>  {
> -	return kmem_cache_zalloc(dev_priv->objects, GFP_KERNEL);
> +	if (IS_ENABLED(CONFIG_DRM_I915_KASAN))
> +		return kzalloc(kmem_cache_size(dev_priv->objects), GFP_KERNEL);
> +	else
> +		return kmem_cache_zalloc(dev_priv->objects, GFP_KERNEL);
>  }
>  
>  void i915_gem_object_free(struct drm_i915_gem_object *obj)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> -	kmem_cache_free(dev_priv->objects, obj);
> +
> +	if (IS_ENABLED(CONFIG_DRM_I915_KASAN))
> +		kfree(obj);
> +	else
> +		kmem_cache_free(dev_priv->objects, obj);
>  }
>  
>  static int
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index 1e1d9f2072cd..715760aa5aa1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -73,7 +73,10 @@ static void i915_fence_release(struct dma_fence *fence)
>  	 */
>  	i915_sw_fence_fini(&req->submit);
>  
> -	kmem_cache_free(req->i915->requests, req);
> +	if (IS_ENABLED(CONFIG_KASAN))
> +		dma_fence_free(fence);
> +	else
> +		kmem_cache_free(req->i915->requests, req);
>  }
>  
>  const struct dma_fence_ops i915_fence_ops = {
> @@ -105,14 +108,20 @@ i915_gem_request_remove_from_client(struct drm_i915_gem_request *request)
>  static struct i915_dependency *
>  i915_dependency_alloc(struct drm_i915_private *i915)
>  {
> -	return kmem_cache_alloc(i915->dependencies, GFP_KERNEL);
> +	if (IS_ENABLED(CONFIG_DRM_I915_KASAN))
> +		return kmalloc(kmem_cache_size(i915->dependencies), GFP_KERNEL);
> +	else
> +		return kmem_cache_alloc(i915->dependencies, GFP_KERNEL);
>  }
>  
>  static void
>  i915_dependency_free(struct drm_i915_private *i915,
>  		     struct i915_dependency *dep)
>  {
> -	kmem_cache_free(i915->dependencies, dep);
> +	if (IS_ENABLED(CONFIG_DRM_I915_KASAN))
> +		kfree(dep);
> +	else
> +		kmem_cache_free(i915->dependencies, dep);
>  }
>  
>  static void
> @@ -571,7 +580,10 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
>  	 *
>  	 * Do not use kmem_cache_zalloc() here!
>  	 */
> -	req = kmem_cache_alloc(dev_priv->requests, GFP_KERNEL);
> +	if (IS_ENABLED(CONFIG_DRM_I915_KASAN))
> +		req = kmalloc(sizeof(*req), GFP_KERNEL);
> +	else
> +		req = kmem_cache_alloc(dev_priv->requests, GFP_KERNEL);
>  	if (!req) {
>  		ret = -ENOMEM;
>  		goto err_unreserve;
> @@ -634,7 +646,10 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
>  	GEM_BUG_ON(!list_empty(&req->priotree.signalers_list));
>  	GEM_BUG_ON(!list_empty(&req->priotree.waiters_list));
>  
> -	kmem_cache_free(dev_priv->requests, req);
> +	if (IS_ENABLED(CONFIG_DRM_I915_KASAN))
> +		kfree(req);
> +	else
> +		kmem_cache_free(dev_priv->requests, req);
>  err_unreserve:
>  	unreserve_seqno(engine);
>  err_unpin:
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 1aba47024656..684d529f6b91 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -81,7 +81,10 @@ vma_create(struct drm_i915_gem_object *obj,
>  	/* The aliasing_ppgtt should never be used directly! */
>  	GEM_BUG_ON(vm == &vm->i915->mm.aliasing_ppgtt->base);
>  
> -	vma = kmem_cache_zalloc(vm->i915->vmas, GFP_KERNEL);
> +	if (IS_ENABLED(CONFIG_DRM_I915_KASAN))
> +		vma = kzalloc(kmem_cache_size(vm->i915->vmas), GFP_KERNEL);
> +	else
> +		vma = kmem_cache_zalloc(vm->i915->vmas, GFP_KERNEL);
>  	if (vma == NULL)
>  		return ERR_PTR(-ENOMEM);
>  
> @@ -159,7 +162,10 @@ vma_create(struct drm_i915_gem_object *obj,
>  	return vma;
>  
>  err_vma:
> -	kmem_cache_free(vm->i915->vmas, vma);
> +	if (IS_ENABLED(CONFIG_DRM_I915_KASAN))
> +		kfree(vma);
> +	else
> +		kmem_cache_free(vm->i915->vmas, vma);
>  	return ERR_PTR(-E2BIG);
>  }
>  
> @@ -588,7 +594,10 @@ void i915_vma_destroy(struct i915_vma *vma)
>  	if (!i915_vma_is_ggtt(vma))
>  		i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm));
>  
> -	kmem_cache_free(to_i915(vma->obj->base.dev)->vmas, vma);
> +	if (IS_ENABLED(CONFIG_DRM_I915_KASAN))
> +		kfree(vma);
> +	else
> +		kmem_cache_free(to_i915(vma->obj->base.dev)->vmas, vma);
>  }
>  
>  void i915_vma_close(struct i915_vma *vma)
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx