Re: [RFC PATCH 00/11] mm/hmm: Various revisions from a locking/code review

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, May 23, 2019 at 12:34:25PM -0300, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
> 
> 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.
> 
> Overall this brings in a simplified locking scheme and easy to explain
> lifetime model:
> 
>  If a hmm_range is valid, then the hmm is valid, if a hmm is valid then the mm
>  is allocated memory.
> 
>  If the mm needs to still be alive (ie to lock the mmap_sem, find a vma, etc)
>  then the mmget must be obtained via mmget_not_zero().
> 
> Locking of mm->hmm is shifted to use the mmap_sem consistently for all
> read/write and unlocked accesses are removed.
> 
> The use unlocked reads on 'hmm->dead' are also eliminated in favour of using
> standard mmget() locking to prevent the mm from being released. Many of the
> debugging checks of !range->hmm and !hmm->mm are dropped in favour of poison -
> which is much clearer as to the lifetime intent.
> 
> The trailing patches are just some random cleanups I noticed when reviewing
> this code.
> 
> I expect Jerome & Ralph will have some design notes so this is just RFC, and
> it still needs a matching edit to nouveau. It is only compile tested.
> 
> Regards,
> Jason
> 
> Jason Gunthorpe (11):
>   mm/hmm: Fix use after free with struct hmm in the mmu notifiers
>   mm/hmm: Use hmm_mirror not mm as an argument for hmm_register_range
>   mm/hmm: Hold a mmgrab from hmm to mm
>   mm/hmm: Simplify hmm_get_or_create and make it reliable
>   mm/hmm: Improve locking around hmm->dead
>   mm/hmm: Remove duplicate condition test before wait_event_timeout
>   mm/hmm: Delete hmm_mirror_mm_is_alive()
>   mm/hmm: Use lockdep instead of comments
>   mm/hmm: Remove racy protection against double-unregistration
>   mm/hmm: Poison hmm_range during unregister
>   mm/hmm: Do not use list*_rcu() for hmm->ranges
> 
>  include/linux/hmm.h |  50 ++----------
>  kernel/fork.c       |   1 -
>  mm/hmm.c            | 184 +++++++++++++++++++-------------------------
>  3 files changed, 88 insertions(+), 147 deletions(-)

Jerome, I was doing some more checking of this and noticed lockdep
doesn't compile test if it is turned off, since you took and revised
the series can you please fold in these hunks to fix compile failures
with lockdep on. Thanks

commit f0653c4d4c1dadeaf58d49f1c949ab1d2fda05d3
diff --git a/mm/hmm.c b/mm/hmm.c
index 836adf613f81c8..2a08b78550b90d 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -56,7 +56,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
 {
 	struct hmm *hmm;
 
-	lockdep_assert_held_exclusive(mm->mmap_sem);
+	lockdep_assert_held_exclusive(&mm->mmap_sem);
 
 	if (mm->hmm) {
 		if (kref_get_unless_zero(&mm->hmm->kref))
@@ -262,7 +262,7 @@ static const struct mmu_notifier_ops hmm_mmu_notifier_ops = {
  */
 int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm)
 {
-	lockdep_assert_held_exclusive(mm->mmap_sem);
+	lockdep_assert_held_exclusive(&mm->mmap_sem);
 
 	/* Sanity check */
 	if (!mm || !mirror || !mirror->ops)
@@ -987,7 +987,7 @@ long hmm_range_snapshot(struct hmm_range *range)
 	struct mm_walk mm_walk;
 
 	/* Caller must hold the mmap_sem, and range hold a reference on mm. */
-	lockdep_assert_held(hmm->mm->mmap_sem);
+	lockdep_assert_held(&hmm->mm->mmap_sem);
 	if (WARN_ON(!atomic_read(&hmm->mm->mm_users)))
 		return -EINVAL;
 
@@ -1086,7 +1086,7 @@ long hmm_range_fault(struct hmm_range *range, bool block)
 	int ret;
 
 	/* Caller must hold the mmap_sem, and range hold a reference on mm. */
-	lockdep_assert_held(hmm->mm->mmap_sem);
+	lockdep_assert_held(&hmm->mm->mmap_sem);
 	if (WARN_ON(!atomic_read(&hmm->mm->mm_users)))
 		return -EINVAL;