Re: [PATCH v4 1/2] drm/xe: Userptr invalidation race with binds fixes

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Feb 25, 2025 at 03:30:54PM +0100, Thomas Hellström wrote:
> Hi, Matt,
> 
> On Mon, 2025-02-24 at 09:01 -0800, Matthew Brost wrote:
> > Always wait on dma-resv bookkeep slots if userptr invalidation has
> > raced
> > with a bind ensuring PTEs temporally setup to invalidated pages are
> > never accessed.
> > 
> > Fixup initial bind handling always add VMAs to invalidation list and
> > wait dma-resv bookkeep slots.
> > 
> > Always hold notifier across TLB invalidation in notifier to prevent a
> > UAF if an unbind races.
> > 
> > Including all of the above changes for Fixes patch in hopes of an
> > easier
> > backport which fix a single patch.
> > 
> > v2:
> >  - Wait dma-resv bookkeep before issuing PTE zap (Thomas)
> >  - Support scratch page on invalidation (Thomas)
> > v3:
> >  - Drop clear of PTEs (Thomas)
> 
> This was what I actually meant.
> 

Ok, I presented this as option and it wasn't clear to me this was
preferred.

> https://patchwork.freedesktop.org/patch/639489/?series=145409&rev=1
>

This patch is doesn't work.
xe_vm.munmap-style-unbind-userptr-one-partial hangs due the error
injection always firing on a single user bind, so we'd have to fix the
error injection too.

Matt
 
> /Thomas
> 
> > v4:
> >  - Remove double dma-resv wait
> > 
> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Cc: <stable@vger.kernel.org>
> > Fixes: e8babb280b5e ("drm/xe: Convert multiple bind ops into single
> > job")
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_pt.c | 21 ++++++++++++---------
> >  drivers/gpu/drm/xe/xe_vm.c |  4 ++--
> >  2 files changed, 14 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> > index 1ddcc7e79a93..ffd23c3564c5 100644
> > --- a/drivers/gpu/drm/xe/xe_pt.c
> > +++ b/drivers/gpu/drm/xe/xe_pt.c
> > @@ -1215,9 +1215,6 @@ static int vma_check_userptr(struct xe_vm *vm,
> > struct xe_vma *vma,
> >  	uvma = to_userptr_vma(vma);
> >  	notifier_seq = uvma->userptr.notifier_seq;
> >  
> > -	if (uvma->userptr.initial_bind && !xe_vm_in_fault_mode(vm))
> > -		return 0;
> > -
> >  	if (!mmu_interval_read_retry(&uvma->userptr.notifier,
> >  				     notifier_seq) &&
> >  	    !xe_pt_userptr_inject_eagain(uvma))
> > @@ -1226,6 +1223,8 @@ static int vma_check_userptr(struct xe_vm *vm,
> > struct xe_vma *vma,
> >  	if (xe_vm_in_fault_mode(vm)) {
> >  		return -EAGAIN;
> >  	} else {
> > +		long err;
> > +
> >  		spin_lock(&vm->userptr.invalidated_lock);
> >  		list_move_tail(&uvma->userptr.invalidate_link,
> >  			       &vm->userptr.invalidated);
> > @@ -1234,19 +1233,23 @@ static int vma_check_userptr(struct xe_vm
> > *vm, struct xe_vma *vma,
> >  		if (xe_vm_in_preempt_fence_mode(vm)) {
> >  			struct dma_resv_iter cursor;
> >  			struct dma_fence *fence;
> > -			long err;
> >  
> >  			dma_resv_iter_begin(&cursor, xe_vm_resv(vm),
> >  					   
> > DMA_RESV_USAGE_BOOKKEEP);
> >  			dma_resv_for_each_fence_unlocked(&cursor,
> > fence)
> >  				dma_fence_enable_sw_signaling(fence)
> > ;
> >  			dma_resv_iter_end(&cursor);
> > -
> > -			err = dma_resv_wait_timeout(xe_vm_resv(vm),
> > -						   
> > DMA_RESV_USAGE_BOOKKEEP,
> > -						    false,
> > MAX_SCHEDULE_TIMEOUT);
> > -			XE_WARN_ON(err <= 0);
> >  		}
> > +
> > +		/*
> > +		 * We are temporally installing PTEs pointing to
> > invalidated
> > +		 * pages, ensure VM is idle to avoid data
> > corruption. PTEs fixed
> > +		 * up upon next exec or in rebind worker.
> > +		 */
> > +		err = dma_resv_wait_timeout(xe_vm_resv(vm),
> > +					    DMA_RESV_USAGE_BOOKKEEP,
> > +					    false,
> > MAX_SCHEDULE_TIMEOUT);
> > +		XE_WARN_ON(err <= 0);
> >  	}
> >  
> >  	return 0;
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index 996000f2424e..9b2acb069a77 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -623,8 +623,6 @@ static bool vma_userptr_invalidate(struct
> > mmu_interval_notifier *mni,
> >  		spin_unlock(&vm->userptr.invalidated_lock);
> >  	}
> >  
> > -	up_write(&vm->userptr.notifier_lock);
> > -
> >  	/*
> >  	 * Preempt fences turn into schedule disables, pipeline
> > these.
> >  	 * Note that even in fault mode, we need to wait for binds
> > and
> > @@ -647,6 +645,8 @@ static bool vma_userptr_invalidate(struct
> > mmu_interval_notifier *mni,
> >  		XE_WARN_ON(err);
> >  	}
> >  
> > +	up_write(&vm->userptr.notifier_lock);
> > +
> >  	trace_xe_vma_userptr_invalidate_complete(vma);
> >  
> >  	return true;
>