Date: Thu, 28 Sep 2023 01:42:45 GMT From: "Jason A. Harmening" <jah@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: git: ee596061e5a5 - stable/13 - devfs: add integrity asserts for cdevp_list Message-ID: <202309280142.38S1gj4X005515@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch stable/13 has been updated by jah: URL: https://cgit.FreeBSD.org/src/commit/?id=ee596061e5a5952f1ddc68627492fbe94af8344b commit ee596061e5a5952f1ddc68627492fbe94af8344b 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-28 01:29:52 +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 (cherry picked from commit 67864268da53b792836f13be10299de8cd62997e) --- 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 6d8ce5cc3a63..1e28db5966a7 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); @@ -521,6 +524,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, @@ -552,6 +558,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); @@ -703,7 +710,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 26589e0bded6..9cf50c58018d 100644 --- a/sys/fs/devfs/devfs_int.h +++ b/sys/fs/devfs/devfs_int.h @@ -57,6 +57,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 e8c8956d36fd..a71cfda9fa9a 100644 --- a/sys/fs/devfs/devfs_vnops.c +++ b/sys/fs/devfs/devfs_vnops.c @@ -1676,6 +1676,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 866788530e7f..a3af24a43b61 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); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202309280142.38S1gj4X005515>