Re: [BrownBag] drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 24/05/2019 07:45, Chris Wilson wrote:
> Having deferred the vma destruction to a worker where we can acquire the
> struct_mutex, we have to avoid chasing back into the now destroyed
> ppgtt. The pd_vma is special in having a custom unbind function to scan
> for unused pages despite the VMA itself being notionally part of the
> GGTT. As such, we need to disable that callback to avoid a
> use-after-free.
> 
> This unfortunately blew up so early during boot that CI declared the
> machine unreachable as opposed to being the major failure it was. Oops.
> 
> Fixes: d3622099c76f ("drm/i915/gtt: Always acquire struct_mutex for gen6_ppgtt_cleanup")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 8d8a4b0ad4d9..266baa11df64 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1847,6 +1847,33 @@ static void gen6_ppgtt_cleanup_work(struct work_struct *wrk)
>   	kfree(work);
>   }
>   
> +static int nop_set_pages(struct i915_vma *vma)
> +{
> +	return -ENODEV;
> +}
> +
> +static void nop_clear_pages(struct i915_vma *vma)
> +{
> +}
> +
> +static int nop_bind(struct i915_vma *vma,
> +		    enum i915_cache_level cache_level,
> +		    u32 unused)
> +{
> +	return -ENODEV;
> +}
> +
> +static void nop_unbind(struct i915_vma *vma)
> +{
> +}
> +
> +static const struct i915_vma_ops nop_vma_ops = {
> +	.set_pages = nop_set_pages,
> +	.clear_pages = nop_clear_pages,
> +	.bind_vma = nop_bind,
> +	.unbind_vma = nop_unbind,
> +};
> +
>   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>   {
>   	struct gen6_hw_ppgtt *ppgtt = to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
> @@ -1855,6 +1882,7 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>   	/* FIXME remove the struct_mutex to bring the locking under control */
>   	INIT_WORK(&work->base, gen6_ppgtt_cleanup_work);
>   	work->vma = ppgtt->vma;
> +	work->vma->ops = &nop_vma_ops;

Could we use some asserts before overriding the vma ops? Like 
GEM_BUG_ON(vma->pages)? And something for still bound?

>   	schedule_work(&work->base);
>   
>   	gen6_ppgtt_free_pd(ppgtt);
> 

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx