From: Jason Gunthorpe <j...@mellanox.com>

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 <j...@mellanox.com>
Tested-by: Philip Yang <philip.y...@amd.com>
---
 mm/hmm.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/mm/hmm.c b/mm/hmm.c
index 26af511cbdd075..c0d43302fd6b2f 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -137,26 +137,16 @@ static void hmm_release(struct mmu_notifier *mn, struct 
mm_struct *mm)
        WARN_ON(!list_empty(&hmm->ranges));
        mutex_unlock(&hmm->lock);
 
-       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);
 }
@@ -286,7 +276,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);
        memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm));
-- 
2.21.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Reply via email to