> -----Original Message-----
> From: Brost, Matthew <matthew.brost@xxxxxxxxx>
> Sent: Tuesday, April 30, 2024 11:42 AM
> To: Zeng, Oak <oak.zeng@xxxxxxxxx>
> Cc: intel-xe@xxxxxxxxxxxxxxxxxxxxx; Thomas Hellström
> <thomas.hellstrom@xxxxxxxxxxxxxxx>; stable@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] drm/xe: Unmap userptr in MMU invalidation notifier
>
> On Tue, Apr 30, 2024 at 09:11:42AM -0600, Zeng, Oak wrote:
> >
> >
> > > -----Original Message-----
> > > From: Brost, Matthew <matthew.brost@xxxxxxxxx>
> > > Sent: Monday, April 29, 2024 1:18 PM
> > > To: Zeng, Oak <oak.zeng@xxxxxxxxx>
> > > Cc: intel-xe@xxxxxxxxxxxxxxxxxxxxx; Thomas Hellström
> > > <thomas.hellstrom@xxxxxxxxxxxxxxx>; stable@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH] drm/xe: Unmap userptr in MMU invalidation
> notifier
> > >
> > > On Mon, Apr 29, 2024 at 07:55:22AM -0600, Zeng, Oak wrote:
> > > > Hi Matt
> > > >
> > > > > -----Original Message-----
> > > > > From: Intel-xe <intel-xe-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf
> Of
> > > > > Matthew Brost
> > > > > Sent: Friday, April 26, 2024 7:33 PM
> > > > > To: intel-xe@xxxxxxxxxxxxxxxxxxxxx
> > > > > Cc: Brost, Matthew <matthew.brost@xxxxxxxxx>; Thomas Hellström
> > > > > <thomas.hellstrom@xxxxxxxxxxxxxxx>; stable@xxxxxxxxxxxxxxx
> > > > > Subject: [PATCH] drm/xe: Unmap userptr in MMU invalidation
> notifier
> > > > >
> > > > > To be secure, when a userptr is invalidated the pages should be dma
> > > > > unmapped ensuring the device can no longer touch the invalidated
> pages.
> > > > >
> > > > > Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel
> > > GPUs")
> > > > > Fixes: 12f4b58a37f4 ("drm/xe: Use hmm_range_fault to populate
> user
> > > > > pages")
> > > > > Cc: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> > > > > Cc: stable@xxxxxxxxxxxxxxx # 6.8
> > > > > Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> > > > > ---
> > > > > drivers/gpu/drm/xe/xe_vm.c | 3 +++
> > > > > 1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/xe/xe_vm.c
> b/drivers/gpu/drm/xe/xe_vm.c
> > > > > index dfd31b346021..964a5b4d47d8 100644
> > > > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > > > @@ -637,6 +637,9 @@ static bool vma_userptr_invalidate(struct
> > > > > mmu_interval_notifier *mni,
> > > > > XE_WARN_ON(err);
> > > > > }
> > > > >
> > > > > + if (userptr->sg)
> > > > > + xe_hmm_userptr_free_sg(uvma);
> > > >
> > > > Just some thoughts here. I think when we introduce system allocator,
> > > above should be made conditional. We should dma unmap userptr only
> for
> > > normal userptr but not for userptr created for system allocator (fault
> usrptr
> > > in the system allocator series). Because for system allocator the dma-
> > > unmapping would be part of the garbage collector and vma destroy
> process.
> > > Right?
> > > >
> > >
> > > I don't think it should be conditional. In any case when a CPU address
> > > is invalidated we need to ensure the dma mapping (IOMMU mapping) is
> > > also invalid to ensure no path to the old (invalidate) pages exists.
> >
> > I understand for both normal userptr and fault userptr we need to dma
> unmap.
> >
> > I was saying, for fault userptr, the dma unmap would be done in the
> garbage collector codes (we destroy fault userptr vma there and dma unmap
> along with vam destroy), so we don't need dma unmap in your above codes.
> It would something like this:
> >
>
> I understand what you are suggesting, but no this is always needed.
>
> > If (userptr && not fault userptr)
> > Dma-unmap sg
> >
>
> With what you suggest, there is a window between the MMU notifier
> completing and garbage collector running in which the dma-mapping is
> valid. This is not allowed per the security model. Thus we need always
> invalidate dma-addresses in the notifier.
That make sense. Thanks for explain.
Oak
>
> Matt
>
> > If (fault userptr)
> > Trigger garbage collector - this will deal with dma-unmap
> >
> >
> > Oak
> >
> >
> > > This is an extra security that must be enforced. With removing the dma
> > > mapping, in theory rouge accesses from the GPU could still access the
> > > old pages.
> > >
> > > Matt
> > >
> > > > Oak
> > > >
> > > > > +
> > > > > trace_xe_vma_userptr_invalidate_complete(vma);
> > > > >
> > > > > return true;
> > > > > --
> > > > > 2.34.1
> > > >
