On Fri, May 24, 2019 at 01:01:49PM -0400, Jerome Glisse wrote:
> On Fri, May 24, 2019 at 01:59:31PM -0300, Jason Gunthorpe wrote:
> > On Fri, May 24, 2019 at 12:49:02PM -0400, Jerome Glisse wrote:
> > > On Fri, May 24, 2019 at 11:36:49AM -0300, Jason Gunthorpe wrote:
> > > > On Thu, May 23, 2019 at 12:34:25PM -0300, Jason Gunthorpe wrote:
> > > > > From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > > > >
> > > > > This patch series arised out of discussions with Jerome when looking at the
> > > > > ODP changes, particularly informed by use after free races we have already
> > > > > found and fixed in the ODP code (thanks to syzkaller) working with mmu
> > > > > notifiers, and the discussion with Ralph on how to resolve the lifetime model.
> > > >
> > > > So the last big difference with ODP's flow is how 'range->valid'
> > > > works.
> > > >
> > > > In ODP this was done using the rwsem umem->umem_rwsem which is
> > > > obtained for read in invalidate_start and released in invalidate_end.
> > > >
> > > > Then any other threads that wish to only work on a umem which is not
> > > > undergoing invalidation will obtain the write side of the lock, and
> > > > within that lock's critical section the virtual address range is known
> > > > to not be invalidating.
> > > >
> > > > I cannot understand how hmm gets to the same approach. It has
> > > > range->valid, but it is not locked by anything that I can see, so when
> > > > we test it in places like hmm_range_fault it seems useless..
> > > >
> > > > Jerome, how does this work?
> > > >
> > > > I have a feeling we should copy the approach from ODP and use an
> > > > actual lock here.
> > >
> > > range->valid is use as bail early if invalidation is happening in
> > > hmm_range_fault() to avoid doing useless work. The synchronization
> > > is explained in the documentation:
> >
> > That just says the hmm APIs handle locking. I asked how the apis
> > implement that locking internally.
> >
> > Are you trying to say that if I do this, hmm will still work completely
> > correctly?
>
> Yes it will keep working correctly. You would just be doing potentialy
> useless work.
I don't see how it works correctly.
Apply the comment out patch I showed and this trivially happens:
CPU0 CPU1
hmm_invalidate_start()
ops->sync_cpu_device_pagetables()
device_lock()
// Wipe out page tables in device, enable faulting
device_unlock()
DEVICE PAGE FAULT
device_lock()
hmm_range_register()
hmm_range_dma_map()
device_unlock()
hmm_invalidate_end()
The mmu notifier spec says:
* Invalidation of multiple concurrent ranges may be
* optionally permitted by the driver. Either way the
* establishment of sptes is forbidden in the range passed to
* invalidate_range_begin/end for the whole duration of the
* invalidate_range_begin/end critical section.
And I understand "establishment of sptes is forbidden" means
"hmm_range_dmap_map() must fail with EAGAIN".
This is why ODP uses an actual lock held across the critical region
which completely prohibits reading the CPU pages tables, or
establishing new mappings.
So, I still think we need a true lock, not a 'maybe valid' flag.
Jason
