git: 67864268da53 - main - devfs: add integrity asserts for cdevp_list

The branch main has been updated by jah:

URL: https://cgit.FreeBSD.org/src/commit/?id=67864268da53b792836f13be10299de8cd62997e

commit 67864268da53b792836f13be10299de8cd62997e
Author:     Jason A. Harmening <jah@FreeBSD.org>
AuthorDate: 2023-09-19 13:44:34 +0000
Commit:     Jason A. Harmening <jah@FreeBSD.org>
CommitDate: 2023-09-21 16:51:12 +0000

    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
---
 sys/fs/devfs/devfs_devs.c  | 12 +++++++++++-
 sys/fs/devfs/devfs_int.h   |  1 +
 sys/fs/devfs/devfs_vnops.c |  4 ++++
 sys/kern/kern_conf.c       |  2 ++
 4 files changed, 18 insertions(+), 1 deletion(-)

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);
 }