mm/hmm: Remove confusing comment and logic from hmm_release

hmm_release() is called exactly once per hmm. ops->release() cannot
accidentally trigger any action that would recurse back onto
hmm->mirrors_sem.

This fixes a use after-free race of the form:

       CPU0                                   CPU1
                                           hmm_release()
                                             up_write(&hmm->mirrors_sem);
 hmm_mirror_unregister(mirror)
  down_write(&hmm->mirrors_sem);
  up_write(&hmm->mirrors_sem);
  kfree(mirror)
                                             mirror->ops->release(mirror)

The only user we have today for ops->release is an empty function, so this
is unambiguously safe.

As a consequence of plugging this race drivers are not allowed to
register/unregister mirrors from within a release op.

Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Philip Yang <Philip.Yang@amd.com>

1 files changed, 9 insertions, 19 deletions

diff --git a/mm/hmm.c b/mm/hmm.c
index c30aa9403dbe..b224ea635a77 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -130,26 +130,16 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
          */
         WARN_ON(!list_empty_careful(&hmm->ranges));
 
-        down_write(&hmm->mirrors_sem);
-        mirror = list_first_entry_or_null(&hmm->mirrors, struct hmm_mirror,
-                                          list);
-        while (mirror) {
-                list_del_init(&mirror->list);
-                if (mirror->ops->release) {
-                        /*
-                         * Drop mirrors_sem so the release callback can wait
-                         * on any pending work that might itself trigger a
-                         * mmu_notifier callback and thus would deadlock with
-                         * us.
-                         */
-                        up_write(&hmm->mirrors_sem);
+        down_read(&hmm->mirrors_sem);
+        list_for_each_entry(mirror, &hmm->mirrors, list) {
+                /*
+                 * Note: The driver is not allowed to trigger
+                 * hmm_mirror_unregister() from this thread.
+                 */
+                if (mirror->ops->release)
                         mirror->ops->release(mirror);
-                        down_write(&hmm->mirrors_sem);
-                }
-                mirror = list_first_entry_or_null(&hmm->mirrors,
-                                                  struct hmm_mirror, list);
         }
-        up_write(&hmm->mirrors_sem);
+        up_read(&hmm->mirrors_sem);
 
         hmm_put(hmm);
 }
@@ -279,7 +269,7 @@ void hmm_mirror_unregister(struct hmm_mirror *mirror)
         struct hmm *hmm = mirror->hmm;
 
         down_write(&hmm->mirrors_sem);
-        list_del_init(&mirror->list);
+        list_del(&mirror->list);
         up_write(&hmm->mirrors_sem);
         hmm_put(hmm);
 }