[src/pgoyette-localcount]: src/sys/kern Fix some locking sequences. Thanks t...

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

details:   https://anonhg.NetBSD.org/src/rev/2223415e77c1
branches:  pgoyette-localcount
changeset: 852795:2223415e77c1
user:      pgoyette <pgoyette%NetBSD.org@localhost>
date:      Sat Jul 16 22:35:34 2016 +0000

description:
Fix some locking sequences.  Thanks to ristradh@ for the review!

diffstat:

 sys/kern/subr_devsw.c |  112 +++++++++++++++++++++++++++++++++++--------------
 1 files changed, 79 insertions(+), 33 deletions(-)

diffs (237 lines):

diff -r 0d44b0951cfc -r 2223415e77c1 sys/kern/subr_devsw.c
--- a/sys/kern/subr_devsw.c     Sat Jul 16 22:06:42 2016 +0000
+++ b/sys/kern/subr_devsw.c     Sat Jul 16 22:35:34 2016 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: subr_devsw.c,v 1.34.2.1 2016/07/16 07:54:13 pgoyette Exp $     */
+/*     $NetBSD: subr_devsw.c,v 1.34.2.2 2016/07/16 22:35:34 pgoyette Exp $     */
 
 /*-
  * Copyright (c) 2001, 2002, 2007, 2008 The NetBSD Foundation, Inc.
@@ -69,7 +69,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: subr_devsw.c,v 1.34.2.1 2016/07/16 07:54:13 pgoyette Exp $");
+__KERNEL_RCSID(0, "$NetBSD: subr_devsw.c,v 1.34.2.2 2016/07/16 22:35:34 pgoyette Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_dtrace.h"
@@ -85,8 +85,10 @@
 #include <sys/buf.h>
 #include <sys/reboot.h>
 #include <sys/sdt.h>
+#include <sys/atomic.h>
 #include <sys/condvar.h>
 #include <sys/localcount.h>
+#include <sys/pserialize.h>
 
 #ifdef DEVSW_DEBUG
 #define        DPRINTF(x)      printf x
@@ -139,6 +141,13 @@
 
        mutex_enter(&device_lock);
 
+       if (bdev != NULL) {
+               KASSERT(bdev->d_localcount != NULL);
+               KASSERT(bdev->d_localcount != cdev->d_localcount);
+       }
+       if (cdev != NULL)
+               KASSERT(cdev->d_localcount != NULL);
+
        for (i = 0 ; i < max_devsw_convs ; i++) {
                conv = &devsw_conv[i];
                if (conv->d_name == NULL || strcmp(devname, conv->d_name) != 0)
@@ -164,13 +173,14 @@
                        goto fail;
                }
 
+               /* use membar_producer() to ensure visibility of the xdevsw */
                if (bdev != NULL) {
-                       KASSERT(bdev->d_localcount != NULL);
                        localcount_init(bdev->d_localcount);
+                       membar_producer();
                        bdevsw[*bmajor] = bdev;
                }
-               KASSERT(cdev->d_localcount != NULL);
                localcount_init(cdev->d_localcount);
+               membar_producer();
                cdevsw[*cmajor] = cdev;
 
                mutex_exit(&device_lock);
@@ -278,6 +288,9 @@
        if (bdevsw[*devmajor] != NULL)
                return (EEXIST);
 
+       /* ensure visibility of the bdevsw */
+       membar_producer();
+
        bdevsw[*devmajor] = devsw;
        KASSERT(devsw->d_localcount != NULL);
        localcount_init(devsw->d_localcount);
@@ -327,6 +340,9 @@
        if (cdevsw[*devmajor] != NULL)
                return (EEXIST);
 
+       /* ensure visibility of the bdevsw */
+       membar_producer();
+
        cdevsw[*devmajor] = devsw;
        KASSERT(devsw->d_localcount != NULL);
        localcount_init(devsw->d_localcount);
@@ -335,16 +351,15 @@
 }
 
 /*
- * First, look up both bdev and cdev indices, and confirm that the
- * localcount pointer(s) exist.  Then drain any existing references,
- * deactivate the localcount(s).  Finally, remove the {b,c}devsw[]
- * entries.
+ * First, look up both bdev and cdev indices, and remove the
+ * {b,c]devsw[] entries so no new references can be taken.  Then
+ * drain any existing references.
  */
 
 static void
 devsw_detach_locked(const struct bdevsw *bdev, const struct cdevsw *cdev)
 {
-       int i, j;
+       int i, j, s;
 
        KASSERT(mutex_owned(&device_lock));
 
@@ -370,24 +385,21 @@
                        break;
                }
        }
-       if (i < max_bdevsws) {
+       if (i < max_bdevsws)
+               bdevsw[i] = NULL;
+       if (j < max_cdevsws )
+               cdevsw[j] = NULL;
+
+       s = pserialize_read_enter();
+       if (i < max_bdevsws && bdev->d_localcount != NULL) {
                localcount_drain(bdev->d_localcount, &device_cv, &device_lock);
                localcount_fini(bdev->d_localcount);
-               bdevsw[i] = NULL;
        }
-       if (j < max_cdevsws ) {
-               /*
-                * Take care not to drain/fini the d_localcount if the same
-                * one was used for both cdev and bdev!
-                */
-               if (i >= max_bdevsws ||
-                   bdev->d_localcount != cdev->d_localcount) {
-                       localcount_drain(cdev->d_localcount, &device_cv,
-                           &device_lock);
-                       localcount_fini(cdev->d_localcount);
-               }
-               cdevsw[j] = NULL;
+       if (j < max_cdevsws && cdev->d_localcount != NULL ) {
+               localcount_drain(cdev->d_localcount, &device_cv, &device_lock);
+               localcount_fini(cdev->d_localcount);
        }
+       pserialize_read_exit(s);
 }
 
 int
@@ -423,6 +435,8 @@
 bdevsw_lookup_acquire(dev_t dev)
 {
        devmajor_t bmajor;
+       const struct bdevsw *bdev = NULL;
+       int s;
 
        if (dev == NODEV)
                return (NULL);
@@ -430,20 +444,35 @@
        if (bmajor < 0 || bmajor >= max_bdevsws)
                return (NULL);
 
+       /* Prevent any concurrent attempts to detach the device */
+       mutex_enter(&device_lock);
+
+       /* Start a read transaction to block localcount_drain() */
+       s = pserialize_read_enter();
+
+       /* Get the struct bdevsw pointer */
+       bdev = bdevsw[bmajor];
+       if (bdev == NULL)
+               goto out;
+
+       /* Wait for the content of the struct bdevsw to become visible */
+       membar_datadep_consumer();
+
+       /* If the devsw is not statically linked, acquire a reference */
        if (bdevsw[bmajor]->d_localcount != NULL)
                localcount_acquire(bdevsw[bmajor]->d_localcount);
 
-       return (bdevsw[bmajor]);
+out:   pserialize_read_exit(s);
+       mutex_exit(&device_lock);
+
+       return bdev;
 }
 
 void
 bdevsw_release(const struct bdevsw *bd)
 {
-       devmajor_t bmaj;
 
-       bmaj = bdevsw_lookup_major(bd);
-
-       KASSERTMSG(bmaj != NODEVMAJOR, "%s: no bmajor to release!", __func__);
+       KASSERT(bd != NULL);
        if (bd->d_localcount != NULL)
                localcount_release(bd->d_localcount, &device_cv, &device_lock);
 }
@@ -471,6 +500,8 @@
 cdevsw_lookup_acquire(dev_t dev)
 {
        devmajor_t cmajor;
+       const struct cdevsw *cdev = NULL;
+       int s;
 
        if (dev == NODEV)
                return (NULL);
@@ -478,20 +509,35 @@
        if (cmajor < 0 || cmajor >= max_cdevsws)
                return (NULL);
 
+       /* Prevent any concurrent attempts to detach the device */
+       mutex_enter(&device_lock);
+
+       /* Start a read transaction to block localcount_drain() */
+       s = pserialize_read_enter();
+
+       /* Get the struct bdevsw pointer */
+       cdev = cdevsw[cmajor];
+       if (cdev == NULL)
+               goto out;
+
+       /* Wait for the content of the struct bdevsw to become visible */
+       membar_datadep_consumer();
+
+       /* If the devsw is not statically linked, acquire a reference */
        if (cdevsw[cmajor]->d_localcount != NULL)
                localcount_acquire(cdevsw[cmajor]->d_localcount);
 
-       return (cdevsw[cmajor]);
+out:   pserialize_read_exit(s);
+       mutex_exit(&device_lock);
+
+       return cdev;
 }
 
 void
 cdevsw_release(const struct cdevsw *cd)
 {
-       devmajor_t cmaj;
 
-       cmaj = cdevsw_lookup_major(cd);
-
-       KASSERTMSG(cmaj != NODEVMAJOR, "%s: no cmajor to release!", __func__);
+       KASSERT(cd != NULL);
        if (cd->d_localcount != NULL)
                localcount_release(cd->d_localcount, &device_cv, &device_lock);
 }