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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Tue, 2025-02-25 at 12:06 -0800, Matthew Brost wrote:
> On Tue, Feb 25, 2025 at 11:23:26AM -0800, Matthew Brost wrote:
> > On Tue, Feb 25, 2025 at 07:56:36PM +0100, Thomas Hellström wrote:
> > > On Tue, 2025-02-25 at 09:45 -0800, Matthew Brost wrote:
> > > > 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.
> > > 
> > > Well, I think the more special cases we can get rid of in the
> > > code, the
> > 
> > Sure, not opposed this direction.
> > 
> > > better? Or at least, like in this case, split out what's common
> > > with
> > > the vm notifier into an xe_vm function and call that, making it
> > > more
> > > clear to the reader that we force an invalidation.
> > > 
> > > > 
> > > > > 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.
> > > 
> > > I have a follow up patch that splits out a part of the notifier
> > > like
> > > described above and calls that for each inject, also invalidating
> > > the
> > > userptr's seqno, and that fixes the above problem, but then the
> > > code
> > > hangs in 
> > > 
> > 
> > Ok, that sounds good.
> >  
> > > xe_exec_fault_mode --r once-userptr-prefetch
> > > 
> > > but that's a different failure mode. Apparently the prefetch code
> > > doesn't repin an invalid userptr and returns -EAGAIN forever...
> > > 
> > 
> > I see the issue, we only call xe_vma_userptr_pin_pages in new_vma
> > which
> > the prefetch code bypasses. If the error inject messes with userptr
> > seqno it makes sense this would start to show up. I suppose this
> > needs
> > fixing too.
> > 
> 
> Something like this shoud do the trick:
> 
> @@ -2299,8 +2299,16 @@ static int vm_bind_ioctl_ops_parse(struct
> xe_vm *vm, struct drm_gpuva_ops *ops,
>                         break;
>                 }
>                 case DRM_GPUVA_OP_UNMAP:
> +                       xe_vma_ops_incr_pt_update_ops(vops, op-
> >tile_mask);
> +                       break;
>                 case DRM_GPUVA_OP_PREFETCH:
> -                       /* FIXME: Need to skip some prefetch ops */
> +                       vma = gpuva_to_vma(op->base.prefetch.va);
> +
> +                       if (xe_vma_is_userptr(vma)) {
> +                               err = xe_vma_is_userptr(vma);
> +                               if (err)
> +                                       return err;
> +                       }
>                         xe_vma_ops_incr_pt_update_ops(vops, op-
> >tile_mask);
>                         break;
>                 default:
> 
> Matt
> 
> > Matt 

Yeah, I'll try to put something together based on this + your staging
tree patch.

/Thomas