Fix two races in the handling of the d_gianttrick for the D_NEEDGIANT

drivers.

In the giant_XXX wrappers for the device methods of the D_NEEDGIANT
drivers, do not dereference the cdev->si_devsw. It is racing with
the destroy_devl() clearing of the si_devsw. Instead, use the
dev_refthread() and return ENXIO for the destroyed device. [1]

The check for the D_INIT in the prep_cdevsw() was not synchronized with
the call of the fini_cdevsw() in destroy_devl(), that under rapid device
creation/destruction may result in the use of uninitialized cdevsw [2].
Change the protocol for the prep_cdevsw(), requiring it to be called
under dev_mtx, where the check for D_INIT is done.

Do not free the memory allocated for the gianttrick cdevsw while holding
the dev_mtx, put it into the free list to be freed later. Reuse the
d_gianttrick pointer to keep the size and layout of the struct cdevsw
(requested by phk). Free the memory in the dev_unlock_and_free(), and do
all the free after the dev_mtx is dropped (suggested by jhb).

Reported by:	bsdimp + many [1], pho [2]
Reviewed by:	phk, jhb
Tested by:	pho
MFC after:	1 week

1 files changed, 121 insertions, 35 deletions

diff --git a/sys/kern/kern_conf.c b/sys/kern/kern_conf.c
index 1cc8a15..939feea 100644
--- a/sys/kern/kern_conf.c
+++ b/sys/kern/kern_conf.c
@@ -61,6 +61,8 @@ static struct cdev *make_dev_credv(int flags,
 
 static struct cdev_priv_list cdevp_free_list =
     TAILQ_HEAD_INITIALIZER(cdevp_free_list);
+static SLIST_HEAD(free_cdevsw, cdevsw) cdevsw_gt_post_list =
+    SLIST_HEAD_INITIALIZER();
 
 void
 dev_lock(void)
@@ -69,19 +71,41 @@ dev_lock(void)
 	mtx_lock(&devmtx);
 }
 
+/*
+ * Free all the memory collected while the cdev mutex was
+ * locked. Since devmtx is after the system map mutex, free() cannot
+ * be called immediately and is postponed until cdev mutex can be
+ * dropped.
+ */
 static void
 dev_unlock_and_free(void)
 {
+	struct cdev_priv_list cdp_free;
+	struct free_cdevsw csw_free;
 	struct cdev_priv *cdp;
+	struct cdevsw *csw;
 
 	mtx_assert(&devmtx, MA_OWNED);
-	while ((cdp = TAILQ_FIRST(&cdevp_free_list)) != NULL) {
-		TAILQ_REMOVE(&cdevp_free_list, cdp, cdp_list);
-		mtx_unlock(&devmtx);
+
+	/*
+	 * Make the local copy of the list heads while the dev_mtx is
+	 * held. Free it later.
+	 */
+	TAILQ_INIT(&cdp_free);
+	TAILQ_CONCAT(&cdp_free, &cdevp_free_list, cdp_list);
+	csw_free = cdevsw_gt_post_list;
+	SLIST_INIT(&cdevsw_gt_post_list);
+
+	mtx_unlock(&devmtx);
+
+	while ((cdp = TAILQ_FIRST(&cdp_free)) != NULL) {
+		TAILQ_REMOVE(&cdp_free, cdp, cdp_list);
 		devfs_free(&cdp->cdp_c);
-		mtx_lock(&devmtx);
 	}
-	mtx_unlock(&devmtx);
+	while ((csw = SLIST_FIRST(&csw_free)) != NULL) {
+		SLIST_REMOVE_HEAD(&csw_free, d_postfree_list);
+		free(csw, M_DEVT);
+	}
 }
 
 static void
@@ -94,6 +118,14 @@ dev_free_devlocked(struct cdev *cdev)
 	TAILQ_INSERT_HEAD(&cdevp_free_list, cdp, cdp_list);
 }
 
+static void
+cdevsw_free_devlocked(struct cdevsw *csw)
+{
+
+	mtx_assert(&devmtx, MA_OWNED);
+	SLIST_INSERT_HEAD(&cdevsw_gt_post_list, csw, d_postfree_list);
+}
+
 void
 dev_unlock(void)
 {
@@ -297,118 +329,164 @@ no_poll(struct cdev *dev __unused, int events, struct thread *td __unused)
 static int
 giant_open(struct cdev *dev, int oflags, int devtype, struct thread *td)
 {
+	struct cdevsw *dsw;
 	int retval;
 
+	dsw = dev_refthread(dev);
+	if (dsw == NULL)
+		return (ENXIO);
 	mtx_lock(&Giant);
-	retval = dev->si_devsw->d_gianttrick->
-	    d_open(dev, oflags, devtype, td);
+	retval = dsw->d_gianttrick->d_open(dev, oflags, devtype, td);
 	mtx_unlock(&Giant);
+	dev_relthread(dev);
 	return (retval);
 }
 
 static int
 giant_fdopen(struct cdev *dev, int oflags, struct thread *td, struct file *fp)
 {
+	struct cdevsw *dsw;
 	int retval;
 
+	dsw = dev_refthread(dev);
+	if (dsw == NULL)
+		return (ENXIO);
 	mtx_lock(&Giant);
-	retval = dev->si_devsw->d_gianttrick->
-	    d_fdopen(dev, oflags, td, fp);
+	retval = dsw->d_gianttrick->d_fdopen(dev, oflags, td, fp);
 	mtx_unlock(&Giant);
+	dev_relthread(dev);
 	return (retval);
 }
 
 static int
 giant_close(struct cdev *dev, int fflag, int devtype, struct thread *td)
 {
+	struct cdevsw *dsw;
 	int retval;
 
+	dsw = dev_refthread(dev);
+	if (dsw == NULL)
+		return (ENXIO);
 	mtx_lock(&Giant);
-	retval = dev->si_devsw->d_gianttrick->
-	    d_close(dev, fflag, devtype, td);
+	retval = dsw->d_gianttrick->d_close(dev, fflag, devtype, td);
 	mtx_unlock(&Giant);
+	dev_relthread(dev);
 	return (retval);
 }
 
 static void
 giant_strategy(struct bio *bp)
 {
+	struct cdevsw *dsw;
+	struct cdev *dev;
 
+	dev = bp->bio_dev;
+	dsw = dev_refthread(dev);
+	if (dsw == NULL) {
+		biofinish(bp, NULL, ENXIO);
+		return;
+	}
 	mtx_lock(&Giant);
-	bp->bio_dev->si_devsw->d_gianttrick->
-	    d_strategy(bp);
+	dsw->d_gianttrick->d_strategy(bp);
 	mtx_unlock(&Giant);
+	dev_relthread(dev);
 }
 
 static int
 giant_ioctl(struct cdev *dev, u_long cmd, caddr_t data, int fflag, struct thread *td)
 {
+	struct cdevsw *dsw;
 	int retval;
 
+	dsw = dev_refthread(dev);
+	if (dsw == NULL)
+		return (ENXIO);
 	mtx_lock(&Giant);
 	retval = dev->si_devsw->d_gianttrick->
 	    d_ioctl(dev, cmd, data, fflag, td);
 	mtx_unlock(&Giant);
+	dev_relthread(dev);
 	return (retval);
 }
   
 static int
 giant_read(struct cdev *dev, struct uio *uio, int ioflag)
 {
+	struct cdevsw *dsw;
 	int retval;
 
+	dsw = dev_refthread(dev);
+	if (dsw == NULL)
+		return (ENXIO);
 	mtx_lock(&Giant);
 	retval = dev->si_devsw->d_gianttrick->
 	    d_read(dev, uio, ioflag);
 	mtx_unlock(&Giant);
+	dev_relthread(dev);
 	return (retval);
 }
 
 static int
 giant_write(struct cdev *dev, struct uio *uio, int ioflag)
 {
+	struct cdevsw *dsw;
 	int retval;
 
+	dsw = dev_refthread(dev);
+	if (dsw == NULL)
+		return (ENXIO);
 	mtx_lock(&Giant);
-	retval = dev->si_devsw->d_gianttrick->
-		d_write(dev, uio, ioflag);
+	retval = dsw->d_gianttrick->d_write(dev, uio, ioflag);
 	mtx_unlock(&Giant);
+	dev_relthread(dev);
 	return (retval);
 }
 
 static int
 giant_poll(struct cdev *dev, int events, struct thread *td)
 {
+	struct cdevsw *dsw;
 	int retval;
 
+	dsw = dev_refthread(dev);
+	if (dsw == NULL)
+		return (ENXIO);
 	mtx_lock(&Giant);
-	retval = dev->si_devsw->d_gianttrick->
-	    d_poll(dev, events, td);
+	retval = dsw->d_gianttrick->d_poll(dev, events, td);
 	mtx_unlock(&Giant);
+	dev_relthread(dev);
 	return (retval);
 }
 
 static int
 giant_kqfilter(struct cdev *dev, struct knote *kn)
 {
+	struct cdevsw *dsw;
 	int retval;
 
+	dsw = dev_refthread(dev);
+	if (dsw == NULL)
+		return (ENXIO);
 	mtx_lock(&Giant);
-	retval = dev->si_devsw->d_gianttrick->
-	    d_kqfilter(dev, kn);
+	retval = dsw->d_gianttrick->d_kqfilter(dev, kn);
 	mtx_unlock(&Giant);
+	dev_relthread(dev);
 	return (retval);
 }
 
 static int
 giant_mmap(struct cdev *dev, vm_offset_t offset, vm_paddr_t *paddr, int nprot)
 {
+	struct cdevsw *dsw;
 	int retval;
 
+	dsw = dev_refthread(dev);
+	if (dsw == NULL)
+		return (ENXIO);
 	mtx_lock(&Giant);
-	retval = dev->si_devsw->d_gianttrick->
-	    d_mmap(dev, offset, paddr, nprot);
+	retval = dsw->d_gianttrick->d_mmap(dev, offset, paddr, nprot);
 	mtx_unlock(&Giant);
+	dev_relthread(dev);
 	return (retval);
 }
 
@@ -490,7 +568,7 @@ fini_cdevsw(struct cdevsw *devsw)
 	if (devsw->d_gianttrick != NULL) {
 		gt = devsw->d_gianttrick;
 		memcpy(devsw, gt, sizeof *devsw);
-		free(gt, M_DEVT);
+		cdevsw_free_devlocked(gt);
 		devsw->d_gianttrick = NULL;
 	}
 	devsw->d_flags &= ~D_INIT;
@@ -501,11 +579,20 @@ prep_cdevsw(struct cdevsw *devsw)
 {
 	struct cdevsw *dsw2;
 
-	if (devsw->d_flags & D_NEEDGIANT)
+	mtx_assert(&devmtx, MA_OWNED);
+	if (devsw->d_flags & D_INIT)
+		return;
+	if (devsw->d_flags & D_NEEDGIANT) {
+		dev_unlock();
 		dsw2 = malloc(sizeof *dsw2, M_DEVT, M_WAITOK);
-	else
+		dev_lock();
+	} else
 		dsw2 = NULL;
-	dev_lock();
+	if (devsw->d_flags & D_INIT) {
+		if (dsw2 != NULL)
+			cdevsw_free_devlocked(dsw2);
+		return;
+	}
 
 	if (devsw->d_version != D_VERSION_01) {
 		printf(
@@ -536,8 +623,8 @@ prep_cdevsw(struct cdevsw *devsw)
 		if (devsw->d_gianttrick == NULL) {
 			memcpy(dsw2, devsw, sizeof *dsw2);
 			devsw->d_gianttrick = dsw2;
-		} else
-			free(dsw2, M_DEVT);
+			dsw2 = NULL;
+		}
 	}
 
 #define FIXUP(member, noop, giant) 				\
@@ -566,7 +653,8 @@ prep_cdevsw(struct cdevsw *devsw)
 
 	devsw->d_flags |= D_INIT;
 
-	dev_unlock();
+	if (dsw2 != NULL)
+		cdevsw_free_devlocked(dsw2);
 }
 
 struct cdev *
@@ -580,10 +668,9 @@ make_dev_credv(int flags, struct cdevsw *devsw, int minornr,
 	KASSERT((minornr & ~MAXMINOR) == 0,
 	    ("Invalid minor (0x%x) in make_dev", minornr));
 
-	if (!(devsw->d_flags & D_INIT)) 
-		prep_cdevsw(devsw);
 	dev = devfs_alloc();
 	dev_lock();
+	prep_cdevsw(devsw);
 	dev = newdev(devsw, minornr, dev);
 	if (flags & MAKEDEV_REF)
 		dev_refl(dev);
@@ -620,7 +707,7 @@ make_dev_credv(int flags, struct cdevsw *devsw, int minornr,
 
 	devfs_create(dev);
 	clean_unrhdrl(devfs_inos);
-	dev_unlock();
+	dev_unlock_and_free();
 	return (dev);
 }
 
@@ -884,8 +971,6 @@ clone_create(struct clonedevs **cdp, struct cdevsw *csw, int *up, struct cdev **
 	KASSERT(*up <= CLONE_UNITMASK,
 	    ("Too high unit (0x%x) in clone_create", *up));
 
-	if (!(csw->d_flags & D_INIT))
-		prep_cdevsw(csw);
 
 	/*
 	 * Search the list for a lot of things in one go:
@@ -898,6 +983,7 @@ clone_create(struct clonedevs **cdp, struct cdevsw *csw, int *up, struct cdev **
 	unit = *up;
 	ndev = devfs_alloc();
 	dev_lock();
+	prep_cdevsw(csw);
 	low = extra;
 	de = dl = NULL;
 	cd = *cdp;
@@ -977,7 +1063,7 @@ clone_cleanup(struct clonedevs **cdp)
 			destroy_devl(dev);
 		}
 	}
-	dev_unlock();
+	dev_unlock_and_free();
 	free(cd, M_DEVBUF);
 	*cdp = NULL;
 }
@@ -1004,7 +1090,7 @@ destroy_dev_tq(void *ctx, int pending)
 		cb = cp->cdp_dtr_cb;
 		cb_arg = cp->cdp_dtr_cb_arg;
 		destroy_devl(dev);
-		dev_unlock();
+		dev_unlock_and_free();
 		dev_rel(dev);
 		if (cb != NULL)
 			cb(cb_arg);