linux-kernel - Re: [Intel-gfx] [PATCH v2 06/21] drm/i915/gt: Batch TLB invalidations

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: <20220720091304.14b5186b@maurocar-mobl2>
Date:   Wed, 20 Jul 2022 09:13:04 +0200
From:   Mauro Carvalho Chehab <mauro.chehab@...ux.intel.com>
To:     Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        David Airlie <airlied@...ux.ie>,
        dri-devel@...ts.freedesktop.org,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Chris Wilson <chris.p.wilson@...el.com>,
        Dave Airlie <airlied@...hat.com>,
        Tomas Winkler <tomas.winkler@...el.com>,
        Matthew Auld <matthew.auld@...el.com>,
        Thomas Hellström 
        <thomas.hellstrom@...ux.intel.com>,
        Lucas De Marchi <lucas.demarchi@...el.com>,
        intel-gfx@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        linux-media@...r.kernel.org,
        Christian König <christian.koenig@....com>
Subject: Re: [Intel-gfx] [PATCH v2 06/21] drm/i915/gt: Batch TLB
 invalidations

On Mon, 18 Jul 2022 14:52:05 +0100
Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com> wrote:

> 
> On 14/07/2022 13:06, Mauro Carvalho Chehab wrote:
> > From: Chris Wilson <chris.p.wilson@...el.com>
> > 
> > Invalidate TLB in patch, in order to reduce performance regressions.
> 
> "in batches"?

Yeah. Will fix it.

> > diff --git a/drivers/gpu/drm/i915/gt/intel_ppgtt.c b/drivers/gpu/drm/i915/gt/intel_ppgtt.c
> > index d8b94d638559..2da6c82a8bd2 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_ppgtt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_ppgtt.c
> > @@ -206,8 +206,12 @@ void ppgtt_bind_vma(struct i915_address_space *vm,
> >   void ppgtt_unbind_vma(struct i915_address_space *vm,
> >   		      struct i915_vma_resource *vma_res)
> >   {
> > -	if (vma_res->allocated)
> > -		vm->clear_range(vm, vma_res->start, vma_res->vma_size);
> > +	if (!vma_res->allocated)
> > +		return;
> > +
> > +	vm->clear_range(vm, vma_res->start, vma_res->vma_size);
> > +	if (vma_res->tlb)
> > +		vma_invalidate_tlb(vm, *vma_res->tlb);
> 
> The patch is about more than batching? If there is a security hole in 
> this area (unbind) with the current code?

No, I don't think there's a security hole. The rationale for this is
not due to it.

Since commit 2f6b90da9192 ("drm/i915: Use vma resources for async unbinding"),
VMA unbind can happen either sync or async.

So, the logic needs to do TLB invalidate on two places. After this
patch, the code at __i915_vma_evict is:

	struct dma_fence *__i915_vma_evict(struct i915_vma *vma, bool async)
	{
...
		if (async)
			unbind_fence = i915_vma_resource_unbind(vma_res,
								&vma->obj->mm.tlb);
		else
			unbind_fence = i915_vma_resource_unbind(vma_res, NULL);

		vma->resource = NULL;

		atomic_and(~(I915_VMA_BIND_MASK | I915_VMA_ERROR | I915_VMA_GGTT_WRITE),
			   &vma->flags);

		i915_vma_detach(vma);

		if (!async) {
			if (unbind_fence) {
				dma_fence_wait(unbind_fence, false);
				dma_fence_put(unbind_fence);
				unbind_fence = NULL;
			}
			vma_invalidate_tlb(vma->vm, vma->obj->mm.tlb);
		}
...

So, basically, if !async, __i915_vma_evict() will do TLB cache invalidation.

However, when async is used, the actual page release will happen later,
at this function:

	void ppgtt_unbind_vma(struct i915_address_space *vm,
			      struct i915_vma_resource *vma_res)
	{
		if (!vma_res->allocated)
			return;

		vm->clear_range(vm, vma_res->start, vma_res->vma_size);
		if (vma_res->tlb)
			vma_invalidate_tlb(vm, *vma_res->tlb);
	}
	
Regards,
Mauro