From 6dfb0593272d9b56a72bc5c5f286ebcc5fafd427 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 1 Feb 2022 02:00:29 +0000 Subject: [PATCH 01/13] fixup! driver(9): Fix synchronization of devsw_attach/lookup/detach. --- sys/kern/subr_devsw.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sys/kern/subr_devsw.c b/sys/kern/subr_devsw.c index 9da96cf29e0f..3e8ed1b17dbf 100644 --- a/sys/kern/subr_devsw.c +++ b/sys/kern/subr_devsw.c @@ -286,8 +286,8 @@ bdevsw_attach(const struct bdevsw *devsw, devmajor_t *devmajor) KASSERT(bdevsw == bdevsw0); newbdevsw = kmem_zalloc(MAXDEVSW * sizeof(newbdevsw[0]), KM_NOSLEEP); - if (newbdevsw == NULL) - return ENOMEM; + if (newbdevsw == NULL) + return ENOMEM; memcpy(newbdevsw, bdevsw, max_bdevsws * sizeof(bdevsw[0])); atomic_store_release(&bdevsw, newbdevsw); atomic_store_release(&max_bdevsws, MAXDEVSW); @@ -350,8 +350,8 @@ cdevsw_attach(const struct cdevsw *devsw, devmajor_t *devmajor) KASSERT(cdevsw == cdevsw0); newcdevsw = kmem_zalloc(MAXDEVSW * sizeof(newcdevsw[0]), KM_NOSLEEP); - if (newcdevsw == NULL) - return ENOMEM; + if (newcdevsw == NULL) + return ENOMEM; memcpy(newcdevsw, cdevsw, max_cdevsws * sizeof(cdevsw[0])); atomic_store_release(&cdevsw, newcdevsw); atomic_store_release(&max_cdevsws, MAXDEVSW); From 9d326f986025ff7aaf96c273284c1f1906df42d7 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 1 Feb 2022 11:21:26 +0000 Subject: [PATCH 02/13] squash! autoconf(9): New localcount-based device instance references. - Add ASSERT_SLEEPABLE() to device_lookup_acquire. - Note possible sleep in comments. --- sys/kern/subr_autoconf.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sys/kern/subr_autoconf.c b/sys/kern/subr_autoconf.c index 39774727aeee..f6d443e9f1f6 100644 --- a/sys/kern/subr_autoconf.c +++ b/sys/kern/subr_autoconf.c @@ -2631,6 +2631,9 @@ config_alldevs_exit(struct alldevs_foray *af) * node and ensuring *_detach calls vdevgone). * * XXX Find a way to assert this. + * + * Safe for use up to and including interrupt context at IPL_VM. + * Never sleeps. */ device_t device_lookup(cfdriver_t cd, int unit) @@ -2672,12 +2675,16 @@ device_lookup_private(cfdriver_t cd, int unit) * returned. May succeed or fail in that case, depending on * whether *_detach has backed out (EBUSY) or committed to * detaching. + * + * May sleep. */ device_t device_lookup_acquire(cfdriver_t cd, int unit) { device_t dv; + ASSERT_SLEEPABLE(); + /* XXX This should have a pserialized fast path -- TBD. */ mutex_enter(&config_misc_lock); mutex_enter(&alldevs_lock); From dad305a03a36aae739241fec3fb67e5ba121e5dc Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 1 Feb 2022 11:46:20 +0000 Subject: [PATCH 03/13] squash! driver(9): Fix synchronization of devsw_attach/lookup/detach. - Omit needless boolean indirection. --- sys/kern/subr_devsw.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/sys/kern/subr_devsw.c b/sys/kern/subr_devsw.c index 3e8ed1b17dbf..9d8c29d19fb2 100644 --- a/sys/kern/subr_devsw.c +++ b/sys/kern/subr_devsw.c @@ -104,7 +104,6 @@ __KERNEL_RCSID(0, "$NetBSD: subr_devsw.c,v 1.38 2017/11/07 18:35:57 christos Exp struct devswref { struct localcount *dr_lc; - bool dr_dynamic; }; /* XXX bdevsw, cdevsw, max_bdevsws, and max_cdevsws should be volatile */ @@ -300,7 +299,6 @@ bdevsw_attach(const struct bdevsw *devsw, devmajor_t *devmajor) lc = kmem_zalloc(sizeof(*lc), KM_SLEEP); localcount_init(lc); bdevswref[*devmajor].dr_lc = lc; - bdevswref[*devmajor].dr_dynamic = true; atomic_store_release(&bdevsw[*devmajor], devsw); @@ -364,7 +362,6 @@ cdevsw_attach(const struct cdevsw *devsw, devmajor_t *devmajor) lc = kmem_zalloc(sizeof(*lc), KM_SLEEP); localcount_init(lc); cdevswref[*devmajor].dr_lc = lc; - cdevswref[*devmajor].dr_dynamic = true; atomic_store_release(&cdevsw[*devmajor], devsw); @@ -424,7 +421,6 @@ devsw_detach_locked(const struct bdevsw *bdev, const struct cdevsw *cdev) localcount_fini(bdevswref[bi].dr_lc); kmem_free(bdevswref[bi].dr_lc, sizeof(*bdevswref[bi].dr_lc)); bdevswref[bi].dr_lc = NULL; - bdevswref[bi].dr_dynamic = false; } if (cdev != NULL) { localcount_drain(cdevswref[ci].dr_lc, @@ -432,7 +428,6 @@ devsw_detach_locked(const struct bdevsw *bdev, const struct cdevsw *cdev) localcount_fini(cdevswref[ci].dr_lc); kmem_free(cdevswref[ci].dr_lc, sizeof(*cdevswref[ci].dr_lc)); cdevswref[ci].dr_lc = NULL; - cdevswref[ci].dr_dynamic = false; } } @@ -494,10 +489,9 @@ bdevsw_lookup_acquire(dev_t dev, struct localcount **lcp) goto out; curbdevswref = atomic_load_consume(&bdevswref); - if (curbdevswref == NULL || !curbdevswref[bmajor].dr_dynamic) { + if (curbdevswref == NULL) { *lcp = NULL; - } else { - *lcp = curbdevswref[bmajor].dr_lc; + } else if ((*lcp = curbdevswref[bmajor].dr_lc) != NULL) { localcount_acquire(*lcp); } @@ -563,10 +557,9 @@ cdevsw_lookup_acquire(dev_t dev, struct localcount **lcp) goto out; curcdevswref = atomic_load_consume(&cdevswref); - if (curcdevswref == NULL || !curcdevswref[cmajor].dr_dynamic) { + if (curcdevswref == NULL) { *lcp = NULL; - } else { - *lcp = curcdevswref[cmajor].dr_lc; + } else if ((*lcp = curcdevswref[cmajor].dr_lc) != NULL) { localcount_acquire(*lcp); } From 96fd048710b89a9df257e0420624f561987842e2 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 1 Feb 2022 12:50:13 +0000 Subject: [PATCH 04/13] autoconf(9): Use dv_detached, not dv_del_gen, for device_lookup. Not necessary to use dv_del_gen because the value of dv_del_gen is relevant to iteration but not lookup -- we only care about whether it's zero or not. And any time dv_del_gen is nonzero, dv_detached is set. Preferable to use dv_detached because if we want to put a pserialized fast path around these lookups, we need to set anything the fast path relies on atomically, but dv_del_gen is a 64-bit integer which we can't use atomics on in MI code. --- sys/kern/subr_autoconf.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sys/kern/subr_autoconf.c b/sys/kern/subr_autoconf.c index f6d443e9f1f6..0a87bc954838 100644 --- a/sys/kern/subr_autoconf.c +++ b/sys/kern/subr_autoconf.c @@ -2643,7 +2643,7 @@ device_lookup(cfdriver_t cd, int unit) mutex_enter(&alldevs_lock); if (unit < 0 || unit >= cd->cd_ndevs) dv = NULL; - else if ((dv = cd->cd_devs[unit]) != NULL && dv->dv_del_gen != 0) + else if ((dv = cd->cd_devs[unit]) != NULL && dv->detached) dv = NULL; mutex_exit(&alldevs_lock); @@ -2690,7 +2690,6 @@ device_lookup_acquire(cfdriver_t cd, int unit) mutex_enter(&alldevs_lock); retry: if (unit < 0 || unit >= cd->cd_ndevs || (dv = cd->cd_devs[unit]) == NULL || - dv->dv_del_gen != 0 || dv->dv_detached) { dv = NULL; } else { From 3fdb699668b8ba613eee15a2e6571a01039389d0 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 1 Feb 2022 12:57:16 +0000 Subject: [PATCH 05/13] autoconf(9): Fast paths for device_lookup and device_lookup_acquire. --- sys/kern/subr_autoconf.c | 92 +++++++++++++++++++++++++++++++++------- 1 file changed, 77 insertions(+), 15 deletions(-) diff --git a/sys/kern/subr_autoconf.c b/sys/kern/subr_autoconf.c index 0a87bc954838..3223494bd0e5 100644 --- a/sys/kern/subr_autoconf.c +++ b/sys/kern/subr_autoconf.c @@ -109,6 +109,9 @@ __KERNEL_RCSID(0, "$NetBSD: subr_autoconf.c,v 1.291 2021/12/31 14:19:57 riastrad #include #include #include +#include +#include +#include #include @@ -1364,8 +1367,16 @@ config_makeroom(int n, struct cfdriver *cd) if (ondevs != 0) memcpy(nsp, cd->cd_devs, sizeof(device_t) * ondevs); - cd->cd_ndevs = nndevs; - cd->cd_devs = nsp; + /* + * Publish the new array first, then publish the new + * length, so pserialized readers reading the length + * first will always see it at most as long as is valid + * to access. + */ + KASSERT(cd->cd_ndevs <= nndevs); + atomic_store_release(&cd->cd_devs, nsp); + atomic_store_release(&cd->cd_ndevs, nndevs); + if (ondevs != 0) { mutex_exit(&alldevs_lock); kmem_free(osp, sizeof(device_t) * ondevs); @@ -1425,7 +1436,7 @@ config_devunlink(device_t dev, struct devicelist *garbage) TAILQ_INSERT_TAIL(garbage, dev, dv_list); /* Remove from cfdriver's array. */ - cd->cd_devs[dev->dv_unit] = NULL; + atomic_store_relaxed(&cd->cd_devs[dev->dv_unit], NULL); /* * If the device now has no units in use, unlink its softc array. @@ -1438,8 +1449,8 @@ config_devunlink(device_t dev, struct devicelist *garbage) if (i == cd->cd_ndevs) { dg->dg_ndevs = cd->cd_ndevs; dg->dg_devs = cd->cd_devs; - cd->cd_devs = NULL; - cd->cd_ndevs = 0; + atomic_store_relaxed(&cd->cd_devs, NULL); + atomic_store_relaxed(&cd->cd_ndevs, 0); } } @@ -1507,8 +1518,8 @@ config_unit_alloc(device_t dev, cfdriver_t cd, cfdata_t cf) if (unit == -1) break; if (unit < cd->cd_ndevs) { - cd->cd_devs[unit] = dev; dev->dv_unit = unit; + atomic_store_release(&cd->cd_devs[unit], dev); break; } config_makeroom(unit, cd); @@ -1914,6 +1925,15 @@ config_dump_garbage(struct devicelist *garbage) { device_t dv; + if (TAILQ_EMPTY(garbage)) + return; + + /* + * Wait for all pserialize readers to notice that devices have + * been deleted before we free them. + */ + xc_barrier(0); + while ((dv = TAILQ_FIRST(garbage)) != NULL) { TAILQ_REMOVE(garbage, dv, dv_list); config_devdelete(dv); @@ -2178,7 +2198,7 @@ config_detach_commit(device_t dev) mutex_enter(&config_misc_lock); KASSERT(dev->dv_detaching == curlwp); - dev->dv_detached = true; + atomic_store_relaxed(&dev->dv_detached, true); cv_broadcast(&config_misc_cv); mutex_exit(&config_misc_lock); } @@ -2638,14 +2658,34 @@ config_alldevs_exit(struct alldevs_foray *af) device_t device_lookup(cfdriver_t cd, int unit) { - device_t dv; + device_t dv = NULL, *devs; + int s; - mutex_enter(&alldevs_lock); - if (unit < 0 || unit >= cd->cd_ndevs) - dv = NULL; - else if ((dv = cd->cd_devs[unit]) != NULL && dv->detached) + s = pserialize_read_enter(); + /* + * We can safely use atomic_load_relaxed here, instead of the + * costlier atomic_load_acquire, because the caller is + * obligated to ensure the device is stable. We still need to + * use pserialize and atomic_load_consume in case the array of + * devices is being updated, so that if we see the old array, + * it doesn't go away before we're done, and if we see the new + * array, we also see its initialized content. + * + * XXX But let's leave it as atomic_load_acquire until we've + * distributed .d_cfdriver/devtounit to all devsw instances + * that might need it, particularly removables like USB. + */ + if (unit < 0 || unit >= atomic_load_acquire(&cd->cd_ndevs)) + goto out; + if ((devs = atomic_load_consume(&cd->cd_devs)) == NULL) + goto out; /* raced with config_devunlink */ + if ((dv = atomic_load_consume(&devs[unit])) == NULL) + goto out; + if (atomic_load_relaxed(&dv->dv_detached) != 0) { dv = NULL; - mutex_exit(&alldevs_lock); + goto out; + } +out: pserialize_read_exit(s); return dv; } @@ -2681,11 +2721,33 @@ device_lookup_private(cfdriver_t cd, int unit) device_t device_lookup_acquire(cfdriver_t cd, int unit) { - device_t dv; + device_t dv = NULL, *devs; + int s; ASSERT_SLEEPABLE(); - /* XXX This should have a pserialized fast path -- TBD. */ + s = pserialize_read_enter(); + if (unit < 0 || unit >= atomic_load_acquire(&cd->cd_ndevs)) + goto out; + if ((devs = atomic_load_consume(&cd->cd_devs)) == NULL) + goto out; /* raced with config_devunlink */ + if ((dv = atomic_load_consume(&devs[unit])) == NULL) + goto out; + if (__predict_false(atomic_load_relaxed(&dv->dv_detached))) { + dv = NULL; + goto out; + } + if (__predict_false(dv->dv_attaching != NULL) && + dv->dv_attaching != curlwp) + goto slow; + if (__predict_false(dv->dv_detaching != NULL)) + goto slow; + localcount_acquire(dv->dv_localcount); +out: pserialize_read_exit(s); + return dv; + +slow: dv = NULL; + pserialize_read_exit(s); mutex_enter(&config_misc_lock); mutex_enter(&alldevs_lock); retry: if (unit < 0 || unit >= cd->cd_ndevs || From 8415a1126b37b1d01edca5148434aee751e7e05c Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Wed, 2 Feb 2022 01:09:07 +0000 Subject: [PATCH 06/13] squash! specfs: Assert specnode is open before we bdev_ioctl. - Don't assert vnode is open -- udf doesn't guarantee it (and maybe it's not important if we're unmounting, but maybe it is, TBD). - Leave an essay about why this is broken and what needs to be done to fix it. --- sys/miscfs/specfs/spec_vnops.c | 43 +++++++++++++++++++++++++++++----- 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/sys/miscfs/specfs/spec_vnops.c b/sys/miscfs/specfs/spec_vnops.c index dfe22506e794..c57ec62f3bbf 100644 --- a/sys/miscfs/specfs/spec_vnops.c +++ b/sys/miscfs/specfs/spec_vnops.c @@ -499,6 +499,11 @@ spec_node_lookup_by_mount(struct mount *mp, vnode_t **vpp) /* * Get the file system mounted on this block device. + * + * XXX Caller should hold the vnode lock -- shared or exclusive -- so + * that this can't changed, and the vnode can't be revoked while we + * examine it. But not all callers do, and they're scattered through a + * lot of file systems, so we can't assert this yet. */ struct mount * spec_node_getmountedfs(vnode_t *devvp) @@ -513,25 +518,51 @@ spec_node_getmountedfs(vnode_t *devvp) /* * Set the file system mounted on this block device. + * + * XXX Caller should hold the vnode lock exclusively so this can't be + * changed or assumed by spec_node_getmountedfs while we change it, and + * the vnode can't be revoked while we handle it. But not all callers + * do, and they're scattered through a lot of file systems, so we can't + * assert this yet. Instead, for now, we'll take an I/O reference so + * at least the ioctl doesn't race with revoke/detach. + * + * If you do change this to assert an exclusive vnode lock, you must + * also do vdead_check before trying bdev_ioctl, because the vnode may + * have been revoked by the time the caller locked it, and this is + * _not_ a vop -- calls to spec_node_setmountedfs don't go through + * v_op, so revoking the vnode doesn't prevent further calls. + * + * XXX Caller should additionally have the vnode open, at least if mp + * is nonnull, but I'm not sure all callers do that -- need to audit. + * Currently udf closes the vnode before clearing the mount. */ void spec_node_setmountedfs(vnode_t *devvp, struct mount *mp) { struct dkwedge_info dkw; + struct specnode *sn; + dev_t dev; + int error; KASSERT(devvp->v_type == VBLK); - KASSERT(devvp->v_specnode->sn_dev->sd_mountpoint == NULL || mp == NULL); - KASSERT(devvp->v_specnode->sn_opencnt); - devvp->v_specnode->sn_dev->sd_mountpoint = mp; - if (mp == NULL) + error = spec_io_enter(devvp, &sn, &dev); + if (error) return; - if (bdev_ioctl(devvp->v_rdev, DIOCGWEDGEINFO, &dkw, FREAD, curlwp) != 0) - return; + KASSERT(sn->sn_dev->sd_mountpoint == NULL || mp == NULL); + sn->sn_dev->sd_mountpoint = mp; + if (mp == NULL) + goto out; + + error = bdev_ioctl(dev, DIOCGWEDGEINFO, &dkw, FREAD, curlwp); + if (error) + goto out; strlcpy(mp->mnt_stat.f_mntfromlabel, dkw.dkw_wname, sizeof(mp->mnt_stat.f_mntfromlabel)); + +out: spec_io_exit(devvp, sn); } /* From 13e64a8a5e5a4fd9015bdbcc1963d300c70aa9a1 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Wed, 2 Feb 2022 01:20:55 +0000 Subject: [PATCH 07/13] specfs: Reorder struct specnode members to save padding. Shrinks from 40 bytes to 32 bytes on LP64 systems this way. XXX kernel ABI change --- sys/miscfs/specfs/specdev.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sys/miscfs/specfs/specdev.h b/sys/miscfs/specfs/specdev.h index c095cf24d2d5..8bb9b4de2b2a 100644 --- a/sys/miscfs/specfs/specdev.h +++ b/sys/miscfs/specfs/specdev.h @@ -66,8 +66,8 @@ typedef struct specnode { vnode_t *sn_next; struct specdev *sn_dev; - u_int sn_opencnt; /* # of opens, share of sd_opencnt */ dev_t sn_rdev; + u_int sn_opencnt; /* # of opens, share of sd_opencnt */ bool sn_gone; } specnode_t; From 1a0ffbeac3342bea36628d4f06de3578364e3cc8 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Wed, 2 Feb 2022 01:26:08 +0000 Subject: [PATCH 08/13] fixup! specfs: Drain all I/O operations after last .d_close call. --- sys/miscfs/specfs/spec_vnops.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sys/miscfs/specfs/spec_vnops.c b/sys/miscfs/specfs/spec_vnops.c index c57ec62f3bbf..137a69a229a0 100644 --- a/sys/miscfs/specfs/spec_vnops.c +++ b/sys/miscfs/specfs/spec_vnops.c @@ -263,8 +263,8 @@ spec_io_enter(struct vnode *vp, struct specnode **snp, dev_t *devp) } /* - * Notify spec_node_revoke that we are doing an I/O operation - * which may not be not bracketed by fstrans(9) and thus is not + * Notify spec_close that we are doing an I/O operation which + * may not be not bracketed by fstrans(9) and thus is not * blocked by vfs suspension. * * We could hold this reference with psref(9) instead, but we @@ -301,9 +301,9 @@ spec_io_exit(struct vnode *vp, struct specnode *sn) KASSERT(vp->v_specnode == sn); /* - * We are done. Notify spec_node_revoke if appropriate. The + * We are done. Notify spec_close if appropriate. The * transition of 1 -> 0 must happen under device_lock so - * spec_node_revoke doesn't miss a wakeup. + * spec_close doesn't miss a wakeup. */ do { iocnt = atomic_load_relaxed(&sd->sd_iocnt); From 51cbf4d83a0b4e38927b225b977ee0fed7d75329 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Wed, 2 Feb 2022 01:26:53 +0000 Subject: [PATCH 09/13] fixup! specfs: Take an I/O reference across bdev/cdev_open. --- sys/miscfs/specfs/spec_vnops.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sys/miscfs/specfs/spec_vnops.c b/sys/miscfs/specfs/spec_vnops.c index 137a69a229a0..33cd09d8fbba 100644 --- a/sys/miscfs/specfs/spec_vnops.c +++ b/sys/miscfs/specfs/spec_vnops.c @@ -806,12 +806,12 @@ spec_open(void *v) * calling .d_open, so release it now and reacquire it when * done. * - * Take an I/O reference so that any concurrent + * Take an I/O reference so that any concurrent spec_close via * spec_node_revoke will wait for us to finish calling .d_open. * The vnode can't be dead at this point because we have it * locked. Note that if revoked, the driver will interrupt - * .d_open before spec_node_revoke starts waiting for I/O to - * drain so this doesn't deadlock. + * .d_open before spec_close starts waiting for I/O to drain so + * this doesn't deadlock. */ VOP_UNLOCK(vp); error = spec_io_enter(vp, &sn1, &dev1); From e9da4d1a66332b8407fdafea85104843c00052dc Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Thu, 3 Feb 2022 22:44:35 +0000 Subject: [PATCH 10/13] fixup! ucom(4): Rework open/close/attach/detach logic. --- sys/dev/usb/ucom.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sys/dev/usb/ucom.c b/sys/dev/usb/ucom.c index 9dd654202f16..53aaeba9d2b6 100644 --- a/sys/dev/usb/ucom.c +++ b/sys/dev/usb/ucom.c @@ -798,10 +798,15 @@ ucomclose(dev_t dev, int flag, int mode, struct lwp *l) /* * We're now closed. Can reopen after this point, so resume * transfers, mark us no longer closing, and notify anyone - * waiting in open. + * waiting in open. The state may be OPEN or ATTACHED at this + * point -- OPEN if the device was already open when we closed + * it, ATTACHED if we interrupted it in the process of opening. */ mutex_enter(&sc->sc_lock); + KASSERTMSG(sc->sc_state == UCOM_ATTACHED || sc->sc_state == UCOM_OPEN, + "%s sc=%p state=%d", device_xname(sc->sc_dev), sc, sc->sc_state); KASSERT(sc->sc_closing); + sc->sc_state = UCOM_ATTACHED; sc->sc_closing = false; cv_broadcast(&sc->sc_statecv); mutex_exit(&sc->sc_lock); From ed9054af9a194164091da9564a0543e8ab16bff8 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Thu, 3 Feb 2022 22:57:58 +0000 Subject: [PATCH 11/13] fixup! specfs: Drain all I/O operations after last .d_close call. --- sys/miscfs/specfs/spec_vnops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sys/miscfs/specfs/spec_vnops.c b/sys/miscfs/specfs/spec_vnops.c index 33cd09d8fbba..a41ecbac89ff 100644 --- a/sys/miscfs/specfs/spec_vnops.c +++ b/sys/miscfs/specfs/spec_vnops.c @@ -809,7 +809,7 @@ spec_open(void *v) * Take an I/O reference so that any concurrent spec_close via * spec_node_revoke will wait for us to finish calling .d_open. * The vnode can't be dead at this point because we have it - * locked. Note that if revoked, the driver will interrupt + * locked. Note that if revoked, the driver must interrupt * .d_open before spec_close starts waiting for I/O to drain so * this doesn't deadlock. */ From c2a6ddbd60154ee7769e298d561c5a359c30e18e Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 4 Feb 2022 23:27:17 +0000 Subject: [PATCH 12/13] squash! specfs: Drain all I/O operations after last .d_close call. - Avoid sd_iocnt overflow in spec_io_enter -- unlikely but possible if pud(4) is involved. - Assert no underflow in spec_io_exit. --- sys/miscfs/specfs/spec_vnops.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/sys/miscfs/specfs/spec_vnops.c b/sys/miscfs/specfs/spec_vnops.c index a41ecbac89ff..8e2b7d042ecb 100644 --- a/sys/miscfs/specfs/spec_vnops.c +++ b/sys/miscfs/specfs/spec_vnops.c @@ -271,9 +271,25 @@ spec_io_enter(struct vnode *vp, struct specnode **snp, dev_t *devp) * already have to take the interlock for vdead_check, so * there's not much more cost here to another atomic operation. */ - iocnt = atomic_inc_uint_nv(&sn->sn_dev->sd_iocnt); - CTASSERT(MAXLWP < UINT_MAX); - KASSERT(iocnt < UINT_MAX); + do { + iocnt = atomic_load_relaxed(&sn->sn_dev->sd_iocnt); + if (__predict_false(iocnt == UINT_MAX)) { + /* + * The I/O count is limited by the number of + * LWPs (which will never overflow this) -- + * unless one driver uses another driver via + * specfs, which is rather unusual, but which + * could happen via pud(4) userspace drivers. + * We could use a 64-bit count, but can't use + * atomics for that on all platforms. + * (Probably better to switch to psref or + * localcount instead.) + */ + error = EBUSY; + goto out; + } + } while (atomic_cas_uint(&sn->sn_dev->sd_iocnt, iocnt, iocnt + 1) + != iocnt); /* Success! */ *snp = sn; @@ -307,6 +323,7 @@ spec_io_exit(struct vnode *vp, struct specnode *sn) */ do { iocnt = atomic_load_relaxed(&sd->sd_iocnt); + KASSERT(iocnt > 0); if (iocnt == 1) { mutex_enter(&device_lock); if (atomic_dec_uint_nv(&sd->sd_iocnt) == 0) From 67599ce5dc2bd0724d01dac2a707cf7de1420358 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 4 Feb 2022 23:28:30 +0000 Subject: [PATCH 13/13] squash! specfs: Drain all I/O operations after last .d_close call. - Fix typo in spec_read copypasta comment. --- sys/miscfs/specfs/spec_vnops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sys/miscfs/specfs/spec_vnops.c b/sys/miscfs/specfs/spec_vnops.c index 8e2b7d042ecb..3111b5b41de8 100644 --- a/sys/miscfs/specfs/spec_vnops.c +++ b/sys/miscfs/specfs/spec_vnops.c @@ -1062,7 +1062,7 @@ spec_read(void *v) * * But before we issue the read, take an I/O reference * to the specnode so close will know when we're done - * writing. Note that the moment we release the lock, + * reading. Note that the moment we release the lock, * the vnode's identity may change; hence spec_io_enter * may fail, and the caller may have a dead vnode on * their hands, if the file system on which vp lived