devfs: add integrity asserts for cdevp_list

It's possible for misuse of cdev KPIs or for bugs in devfs itself to
result in e.g. a cdev object's container being freed while still on the
global list used to populate each devfs mount; see PR 273418 for a
recent example.

Since a node may be marked inactive well before it is reaped from the
list, add a new flag solely to track list membership, and employ it in
some basic list integrity assertions to catch bad actors.

Discussed with:	kib, mjg
MFC after:	1 week

4 files changed, 18 insertions, 1 deletions

diff --git a/sys/fs/devfs/devfs_devs.c b/sys/fs/devfs/devfs_devs.c
index c6efd0d421b1..db879efe803a 100644
--- a/sys/fs/devfs/devfs_devs.c
+++ b/sys/fs/devfs/devfs_devs.c
@@ -175,6 +175,9 @@ devfs_free(struct cdev *cdev)
 	struct cdev_priv *cdp;
 
 	cdp = cdev2priv(cdev);
+	KASSERT((cdp->cdp_flags & (CDP_ACTIVE | CDP_ON_ACTIVE_LIST)) == 0,
+	    ("%s: cdp %p (%s) still on active list",
+	    __func__, cdp, cdev->si_name));
 	if (cdev->si_cred != NULL)
 		crfree(cdev->si_cred);
 	devfs_free_cdp_inode(cdp->cdp_inode);
@@ -516,6 +519,9 @@ devfs_populate_loop(struct devfs_mount *dm, int cleanup)
 	dev_lock();
 	TAILQ_FOREACH(cdp, &cdevp_list, cdp_list) {
 		KASSERT(cdp->cdp_dirents != NULL, ("NULL cdp_dirents"));
+		KASSERT((cdp->cdp_flags & CDP_ON_ACTIVE_LIST) != 0,
+		    ("%s: cdp %p (%s) should not be on active list",
+		    __func__, cdp, cdp->cdp_c.si_name));
 
 		/*
 		 * If we are unmounting, or the device has been destroyed,
@@ -547,6 +553,7 @@ devfs_populate_loop(struct devfs_mount *dm, int cleanup)
 		if (!(cdp->cdp_flags & CDP_ACTIVE)) {
 			if (cdp->cdp_inuse > 0)
 				continue;
+			cdp->cdp_flags &= ~CDP_ON_ACTIVE_LIST;
 			TAILQ_REMOVE(&cdevp_list, cdp, cdp_list);
 			dev_unlock();
 			dev_rel(&cdp->cdp_c);
@@ -698,7 +705,10 @@ devfs_create(struct cdev *dev)
 
 	dev_lock_assert_locked();
 	cdp = cdev2priv(dev);
-	cdp->cdp_flags |= CDP_ACTIVE;
+	KASSERT((cdp->cdp_flags & CDP_ON_ACTIVE_LIST) == 0,
+	    ("%s: cdp %p (%s) already on active list",
+	    __func__, cdp, dev->si_name));
+	cdp->cdp_flags |= (CDP_ACTIVE | CDP_ON_ACTIVE_LIST);
 	cdp->cdp_inode = alloc_unrl(devfs_inos);
 	dev_refl(dev);
 	TAILQ_INSERT_TAIL(&cdevp_list, cdp, cdp_list);
diff --git a/sys/fs/devfs/devfs_int.h b/sys/fs/devfs/devfs_int.h
index 32c6fb414250..916297425b53 100644
--- a/sys/fs/devfs/devfs_int.h
+++ b/sys/fs/devfs/devfs_int.h
@@ -55,6 +55,7 @@ struct cdev_priv {
 #define CDP_ACTIVE		(1 << 0)
 #define CDP_SCHED_DTR		(1 << 1)
 #define	CDP_UNREF_DTR		(1 << 2)
+#define CDP_ON_ACTIVE_LIST	(1 << 3)
 
 	u_int			cdp_inuse;
 	u_int			cdp_maxdirent;
diff --git a/sys/fs/devfs/devfs_vnops.c b/sys/fs/devfs/devfs_vnops.c
index 2f700f9dad25..1df7d13be919 100644
--- a/sys/fs/devfs/devfs_vnops.c
+++ b/sys/fs/devfs/devfs_vnops.c
@@ -1664,6 +1664,10 @@ devfs_revoke(struct vop_revoke_args *ap)
 	dev_lock();
 	cdp->cdp_inuse--;
 	if (!(cdp->cdp_flags & CDP_ACTIVE) && cdp->cdp_inuse == 0) {
+		KASSERT((cdp->cdp_flags & CDP_ON_ACTIVE_LIST) != 0,
+		    ("%s: cdp %p (%s) not on active list",
+		    __func__, cdp, dev->si_name));
+		cdp->cdp_flags &= ~CDP_ON_ACTIVE_LIST;
 		TAILQ_REMOVE(&cdevp_list, cdp, cdp_list);
 		dev_unlock();
 		dev_rel(&cdp->cdp_c);
diff --git a/sys/kern/kern_conf.c b/sys/kern/kern_conf.c
index d6063696c85b..a7c22b7d118a 100644
--- a/sys/kern/kern_conf.c
+++ b/sys/kern/kern_conf.c
@@ -119,6 +119,8 @@ dev_free_devlocked(struct cdev *cdev)
 	cdp = cdev2priv(cdev);
 	KASSERT((cdp->cdp_flags & CDP_UNREF_DTR) == 0,
 	    ("destroy_dev() was not called after delist_dev(%p)", cdev));
+	KASSERT((cdp->cdp_flags & CDP_ON_ACTIVE_LIST) == 0,
+	    ("%s: cdp %p (%s) on active list", __func__, cdp, cdev->si_name));
 	TAILQ_INSERT_HEAD(&cdevp_free_list, cdp, cdp_list);
 }