[src/trunk]: src/sys/uvm PR kern/54783: t_mmap crahes the kernel

[ad <ad%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/3bd977458477
branches:  trunk
changeset: 967701:3bd977458477
user:      ad <ad%NetBSD.org@localhost>
date:      Wed Dec 18 20:38:14 2019 +0000

description:
PR kern/54783: t_mmap crahes the kernel

- Fix various locking & sequencing errors with breaking loans.

- Don't call uvm_pageremove_tree() while holding pg->interlock as radixtree
  can take further locks when freeing nodes.

diffstat:

 sys/uvm/uvm_loan.c |  91 ++++++++++++++++++++++++++---------------------------
 sys/uvm/uvm_page.c |  72 +++++++++++++++++-------------------------
 2 files changed, 73 insertions(+), 90 deletions(-)

diffs (truncated from 312 to 300 lines):

diff -r 01fccb7dd87d -r 3bd977458477 sys/uvm/uvm_loan.c
--- a/sys/uvm/uvm_loan.c        Wed Dec 18 19:40:34 2019 +0000
+++ b/sys/uvm/uvm_loan.c        Wed Dec 18 20:38:14 2019 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: uvm_loan.c,v 1.91 2019/12/15 21:11:35 ad Exp $ */
+/*     $NetBSD: uvm_loan.c,v 1.92 2019/12/18 20:38:14 ad Exp $ */
 
 /*
  * Copyright (c) 1997 Charles D. Cranor and Washington University.
@@ -32,7 +32,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: uvm_loan.c,v 1.91 2019/12/15 21:11:35 ad Exp $");
+__KERNEL_RCSID(0, "$NetBSD: uvm_loan.c,v 1.92 2019/12/18 20:38:14 ad Exp $");
 
 #include <sys/param.h>
 #include <sys/systm.h>
@@ -1121,7 +1121,7 @@
         * force a reload of the old page by clearing it from all
         * pmaps.
         * transfer dirtiness of the old page to the new page.
-        * then lock pg->interlock to rename the pages.
+        * then rename the pages.
         */
 
        uvm_pagecopy(uobjpage, pg);     /* old -> new */
@@ -1143,14 +1143,6 @@
        UVM_PAGE_OWN(uobjpage, NULL);
 
        /*
-        * replace uobjpage with new page.
-        */
-
-       mutex_enter(&uobjpage->interlock);
-       uvm_pagereplace(uobjpage, pg);
-       mutex_exit(&uobjpage->interlock);
-
-       /*
         * if the page is no longer referenced by
         * an anon (i.e. we are breaking an O->K
         * loan), then remove it from any pageq's.
@@ -1159,6 +1151,12 @@
                uvm_pagedequeue(uobjpage);
 
        /*
+        * replace uobjpage with new page.
+        */
+
+       uvm_pagereplace(uobjpage, pg);
+
+       /*
         * at this point we have absolutely no
         * control over uobjpage
         */
@@ -1176,59 +1174,58 @@
 int
 uvm_loanbreak_anon(struct vm_anon *anon, struct uvm_object *uobj)
 {
-       struct vm_page *pg, *dequeuepg;
+       struct vm_page *newpg, *oldpg;
 
        KASSERT(mutex_owned(anon->an_lock));
        KASSERT(uobj == NULL || mutex_owned(uobj->vmobjlock));
 
        /* get new un-owned replacement page */
-       pg = uvm_pagealloc(NULL, 0, NULL, 0);
-       if (pg == NULL) {
+       newpg = uvm_pagealloc(NULL, 0, NULL, 0);
+       if (newpg == NULL) {
                return ENOMEM;
        }
 
-       /* copy old -> new */
-       uvm_pagecopy(anon->an_page, pg);
-
-       /* force reload */
-       pmap_page_protect(anon->an_page, VM_PROT_NONE);
-       if (pg < anon->an_page) {
-               mutex_enter(&pg->interlock);
-               mutex_enter(&anon->an_page->interlock);
-       } else {
-               mutex_enter(&anon->an_page->interlock);
-               mutex_enter(&pg->interlock);
-       }
-       anon->an_page->uanon = NULL;
-       /* in case we owned */
-       anon->an_page->flags &= ~PG_ANON;
-
-       if (uobj) {
-               /* if we were receiver of loan */
-               anon->an_page->loan_count--;
-               dequeuepg = NULL;
-       } else {
+       oldpg = anon->an_page;
+       if (uobj == NULL) {
                /*
                 * we were the lender (A->K); need to remove the page from
                 * pageq's.
                 */
-               dequeuepg = anon->an_page;
+               uvm_pagedequeue(oldpg);
+       }
+
+       /* copy old -> new */
+       uvm_pagecopy(oldpg, newpg);
+
+       /* force reload */
+       pmap_page_protect(oldpg, VM_PROT_NONE);
+       if (newpg < oldpg) {
+               mutex_enter(&newpg->interlock);
+               mutex_enter(&oldpg->interlock);
+       } else {
+               mutex_enter(&oldpg->interlock);
+               mutex_enter(&newpg->interlock);
+       }
+       oldpg->uanon = NULL;
+       /* in case we owned */
+       oldpg->flags &= ~PG_ANON;
+
+       if (uobj) {
+               /* if we were receiver of loan */
+               oldpg->loan_count--;
        }
 
        /* install new page in anon */
-       anon->an_page = pg;
-       pg->uanon = anon;
-       pg->flags |= PG_ANON;
+       anon->an_page = newpg;
+       newpg->uanon = anon;
+       newpg->flags |= PG_ANON;
 
-       mutex_exit(&pg->interlock);
-       mutex_exit(&anon->an_page->interlock);
-       uvm_pageactivate(pg);
-       if (dequeuepg != NULL) {
-               uvm_pagedequeue(anon->an_page);
-       }
+       mutex_exit(&newpg->interlock);
+       mutex_exit(&oldpg->interlock);
+       uvm_pageactivate(newpg);
 
-       pg->flags &= ~(PG_BUSY|PG_FAKE);
-       UVM_PAGE_OWN(pg, NULL);
+       newpg->flags &= ~(PG_BUSY|PG_FAKE);
+       UVM_PAGE_OWN(newpg, NULL);
 
        if (uobj) {
                mutex_exit(uobj->vmobjlock);
diff -r 01fccb7dd87d -r 3bd977458477 sys/uvm/uvm_page.c
--- a/sys/uvm/uvm_page.c        Wed Dec 18 19:40:34 2019 +0000
+++ b/sys/uvm/uvm_page.c        Wed Dec 18 20:38:14 2019 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: uvm_page.c,v 1.205 2019/12/16 22:47:55 ad Exp $        */
+/*     $NetBSD: uvm_page.c,v 1.206 2019/12/18 20:38:14 ad Exp $        */
 
 /*
  * Copyright (c) 1997 Charles D. Cranor and Washington University.
@@ -66,7 +66,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: uvm_page.c,v 1.205 2019/12/16 22:47:55 ad Exp $");
+__KERNEL_RCSID(0, "$NetBSD: uvm_page.c,v 1.206 2019/12/18 20:38:14 ad Exp $");
 
 #include "opt_ddb.h"
 #include "opt_uvm.h"
@@ -148,13 +148,6 @@
 struct vm_page *uvm_physseg_seg_alloc_from_slab(uvm_physseg_t, size_t);
 
 /*
- * local prototypes
- */
-
-static int uvm_pageinsert(struct uvm_object *, struct vm_page *);
-static void uvm_pageremove(struct uvm_object *, struct vm_page *);
-
-/*
  * inline functions
  */
 
@@ -205,22 +198,6 @@
        return 0;
 }
 
-static inline int
-uvm_pageinsert(struct uvm_object *uobj, struct vm_page *pg)
-{
-       int error;
-
-       KDASSERT(uobj != NULL);
-       KDASSERT(uobj == pg->uobject);
-       error = uvm_pageinsert_tree(uobj, pg);
-       if (error != 0) {
-               KASSERT(error == ENOMEM);
-               return error;
-       }
-       uvm_pageinsert_object(uobj, pg);
-       return error;
-}
-
 /*
  * uvm_page_remove: remove page from object.
  *
@@ -265,16 +242,6 @@
        KASSERT(pg == opg);
 }
 
-static inline void
-uvm_pageremove(struct uvm_object *uobj, struct vm_page *pg)
-{
-
-       KDASSERT(uobj != NULL);
-       KASSERT(uobj == pg->uobject);
-       uvm_pageremove_object(uobj, pg);
-       uvm_pageremove_tree(uobj, pg);
-}
-
 static void
 uvm_page_init_buckets(struct pgfreelist *pgfl)
 {
@@ -1043,9 +1010,10 @@
                pg->flags |= PG_ANON;
                cpu_count(CPU_COUNT_ANONPAGES, 1);
        } else if (obj) {
-               error = uvm_pageinsert(obj, pg);
+               uvm_pageinsert_object(obj, pg);
+               error = uvm_pageinsert_tree(obj, pg);
                if (error != 0) {
-                       pg->uobject = NULL;
+                       uvm_pageremove_object(obj, pg);
                        uvm_pagefree(pg);
                        return NULL;
                }
@@ -1077,7 +1045,6 @@
  * uvm_pagereplace: replace a page with another
  *
  * => object must be locked
- * => interlock must be held
  */
 
 void
@@ -1091,13 +1058,23 @@
        KASSERT(newpg->uobject == NULL);
        KASSERT(mutex_owned(uobj->vmobjlock));
 
-       newpg->uobject = uobj;
        newpg->offset = oldpg->offset;
-
        uvm_pageremove_tree(uobj, oldpg);
        uvm_pageinsert_tree(uobj, newpg);
+
+       /* take page interlocks during rename */
+       if (oldpg < newpg) {
+               mutex_enter(&oldpg->interlock);
+               mutex_enter(&newpg->interlock);
+       } else {
+               mutex_enter(&newpg->interlock);
+               mutex_enter(&oldpg->interlock);
+       }
+       newpg->uobject = uobj;
        uvm_pageinsert_object(uobj, newpg);
        uvm_pageremove_object(uobj, oldpg);
+       mutex_exit(&oldpg->interlock);
+       mutex_exit(&newpg->interlock);
 }
 
 /*
@@ -1115,7 +1092,8 @@
         */
 
        if (pg->uobject) {
-               uvm_pageremove(pg->uobject, pg);
+               uvm_pageremove_tree(pg->uobject, pg);
+               uvm_pageremove_object(pg->uobject, pg);
        }
 
        /*
@@ -1197,6 +1175,14 @@
                mutex_owned(pg->uanon->an_lock));
 
        /*
+        * remove the page from the object's tree beore acquiring any page
+        * interlocks: this can acquire locks to free radixtree nodes.
+        */
+       if (pg->uobject != NULL) {
+               uvm_pageremove_tree(pg->uobject, pg);
+       }
+
+       /*
         * if the page is loaned, resolve the loan instead of freeing.
         */
 
@@ -1217,7 +1203,7 @@
                mutex_enter(&pg->interlock);
                locked = true;
                if (pg->uobject != NULL) {
-                       uvm_pageremove(pg->uobject, pg);
+                       uvm_pageremove_object(pg->uobject, pg);