CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Sat Apr 22 15:32:49 UTC 2023 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: KNF. No functional change intended. To generate a diff of this commit: cvs rdiff -u -r1.217 -r1.218 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.217 src/sys/miscfs/specfs/spec_vnops.c:1.218 --- src/sys/miscfs/specfs/spec_vnops.c:1.217 Sat Apr 22 14:30:16 2023 +++ src/sys/miscfs/specfs/spec_vnops.c Sat Apr 22 15:32:49 2023 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.217 2023/04/22 14:30:16 hannken Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.218 2023/04/22 15:32:49 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.217 2023/04/22 14:30:16 hannken Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.218 2023/04/22 15:32:49 riastradh Exp $"); #ifdef _KERNEL_OPT #include "opt_ddb.h" @@ -184,7 +184,9 @@ const struct vnodeopv_desc spec_vnodeop_ static kauth_listener_t rawio_listener; static struct kcondvar specfs_iocv; -/* Returns true if vnode is /dev/mem or /dev/kmem. */ +/* + * Returns true if vnode is /dev/mem or /dev/kmem. + */ bool iskmemvp(struct vnode *vp) { @@ -478,7 +480,7 @@ top: mutex_enter(&device_lock); } mutex_exit(&device_lock); error = vcache_vget(vp); - if (error != 0) + if (error) return error; *vpp = vp; @@ -513,7 +515,7 @@ spec_node_lookup_by_mount(struct mount * mutex_enter(vq->v_interlock); mutex_exit(&device_lock); error = vcache_vget(vq); - if (error != 0) + if (error) return error; *vpp = vq; @@ -701,7 +703,7 @@ spec_lookup(void *v) } */ *ap = v; *ap->a_vpp = NULL; - return (ENOTDIR); + return ENOTDIR; } typedef int (*spec_ioctl_t)(dev_t, u_long, void *, int, struct lwp *); @@ -743,7 +745,7 @@ spec_open(void *v) * Don't allow open if fs is mounted -nodev. */ if (vp->v_mount && (vp->v_mount->mnt_flag & MNT_NODEV)) - return (ENXIO); + return ENXIO; switch (ap->a_mode & (FREAD | FWRITE)) { case FREAD | FWRITE: @@ -757,8 +759,8 @@ spec_open(void *v) break; } error = kauth_authorize_device_spec(ap->a_cred, req, vp); - if (error != 0) - return (error); + if (error) + return error; /* * Acquire an open reference -- as long as we hold onto it, and @@ -877,7 +879,7 @@ spec_open(void *v) error = cdev_open(dev, ap->a_mode, S_IFCHR, l); if (error != ENXIO) break; - + /* Check if we already have a valid driver */ mutex_enter(&device_lock); cdev = cdevsw_lookup(dev); @@ -888,9 +890,9 @@ spec_open(void *v) /* Get device name from devsw_conv array */ if ((name = cdevsw_getname(major(dev))) == NULL) break; - + /* Try to autoload device module */ - (void) module_autoload(name, MODULE_CLASS_DRIVER); + (void)module_autoload(name, MODULE_CLASS_DRIVER); } while (gen != module_gen); break; @@ -914,8 +916,8 @@ spec_open(void *v) if ((name = bdevsw_getname(major(dev))) == NULL) break; -/* Try to autoload device module */ - (void) module_autoload(name, MODULE_CLASS_DRIVER); + /* Try to autoload device module */ + (void)module_autoload(name, MODULE_CLASS_DRIVER); } while (gen != module_gen); break; @@ -1062,7 +1064,7 @@ spec_read(void *v) } */ *ap = v; struct vnode *vp = ap->a_vp; struct uio *uio = ap->a_uio; - struct lwp *l = curlwp; + struct lwp *l = curlwp; struct specnode *sn; dev_t dev; struct buf *bp; @@ -1077,12 +1079,12 @@ spec_read(void *v) int nrablks, ratogo; KASSERT(uio->uio_rw == UIO_READ); - KASSERTMSG(VMSPACE_IS_KERNEL_P(uio->uio_vmspace) || - uio->uio_vmspace == curproc->p_vmspace, - "vmspace belongs to neither kernel nor curproc"); + KASSERTMSG((VMSPACE_IS_KERNEL_P(uio->uio_vmspace) || + uio->uio_vmspace == curproc->p_vmspace), + "vmspace belongs to neither kernel nor curproc"); if (uio->uio_resid == 0) - return (0); + return 0; switch (vp->v_type) { @@ -1109,12 +,12 @@ spec_read(void *v) spec_io_exit(vp, sn); out: /* XXX What if the caller held an exclusive lock? */ vn_lock(vp, LK_SHARED | LK_RETRY); - return (error); + return error; case VBLK: KASSERT(vp == vp->v_specnode->sn_dev->sd_bdevvp); if (uio->uio_offset < 0) - return (EINVAL); + return EINVAL; if (bdev_ioctl(vp->v_rdev, DIOCGPARTINFO, &pi, FREAD, l) == 0) bsize = imin(imax(pi.pi_bsize, DEV_BSIZE), MAXBSIZE); @@ -1144,8 +1146,8 @@ out: /* XXX What if the caller held an } error = breadn(vp, bn, bsize, - rablks, rasizes, nrablks, - 0, &bp); +rablks, rasizes, nrablks, +0, &bp); } else { if (ratogo > 0) --ratogo; @@ -1161,7 +1163,7
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Sat Apr 22 15:32:49 UTC 2023 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: KNF. No functional change intended. To generate a diff of this commit: cvs rdiff -u -r1.217 -r1.218 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: hannken Date: Sat Apr 22 14:30:17 UTC 2023 Modified Files: src/sys/miscfs/specfs: spec_vnops.c specdev.h Log Message: Remove unused specdev member sd_rdev. Ride 10.99.4 To generate a diff of this commit: cvs rdiff -u -r1.216 -r1.217 src/sys/miscfs/specfs/spec_vnops.c cvs rdiff -u -r1.53 -r1.54 src/sys/miscfs/specfs/specdev.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.216 src/sys/miscfs/specfs/spec_vnops.c:1.217 --- src/sys/miscfs/specfs/spec_vnops.c:1.216 Sat Oct 15 15:20:46 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Sat Apr 22 14:30:16 2023 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.216 2022/10/15 15:20:46 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.217 2023/04/22 14:30:16 hannken Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.216 2022/10/15 15:20:46 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.217 2023/04/22 14:30:16 hannken Exp $"); #ifdef _KERNEL_OPT #include "opt_ddb.h" @@ -397,7 +397,6 @@ spec_node_init(vnode_t *vp, dev_t rdev) } if (vp2 == NULL) { /* No existing record, create a new one. */ - sd->sd_rdev = rdev; sd->sd_mountpoint = NULL; sd->sd_lockf = NULL; sd->sd_refcnt = 1; Index: src/sys/miscfs/specfs/specdev.h diff -u src/sys/miscfs/specfs/specdev.h:1.53 src/sys/miscfs/specfs/specdev.h:1.54 --- src/sys/miscfs/specfs/specdev.h:1.53 Wed Oct 26 23:40:08 2022 +++ src/sys/miscfs/specfs/specdev.h Sat Apr 22 14:30:16 2023 @@ -1,4 +1,4 @@ -/* $NetBSD: specdev.h,v 1.53 2022/10/26 23:40:08 riastradh Exp $ */ +/* $NetBSD: specdev.h,v 1.54 2023/04/22 14:30:16 hannken Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -77,7 +77,6 @@ typedef struct specdev { vnode_t *sd_bdevvp; u_int sd_opencnt; /* # of opens; close when ->0 */ u_int sd_refcnt; /* # of specnodes referencing this */ - dev_t sd_rdev; volatile u_int sd_iocnt; /* # bdev/cdev_* operations active */ bool sd_opened; /* true if successfully opened */ bool sd_closing; /* true when bdev/cdev_close ongoing */
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: hannken Date: Sat Apr 22 14:30:17 UTC 2023 Modified Files: src/sys/miscfs/specfs: spec_vnops.c specdev.h Log Message: Remove unused specdev member sd_rdev. Ride 10.99.4 To generate a diff of this commit: cvs rdiff -u -r1.216 -r1.217 src/sys/miscfs/specfs/spec_vnops.c cvs rdiff -u -r1.53 -r1.54 src/sys/miscfs/specfs/specdev.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Sat Oct 15 15:20:46 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs(9): Attribute blame by stack trace for write to r/o medium. To generate a diff of this commit: cvs rdiff -u -r1.215 -r1.216 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Sat Oct 15 15:20:46 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs(9): Attribute blame by stack trace for write to r/o medium. To generate a diff of this commit: cvs rdiff -u -r1.215 -r1.216 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.215 src/sys/miscfs/specfs/spec_vnops.c:1.216 --- src/sys/miscfs/specfs/spec_vnops.c:1.215 Wed Sep 21 10:59:10 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Sat Oct 15 15:20:46 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.215 2022/09/21 10:59:10 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.216 2022/10/15 15:20:46 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,11 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.215 2022/09/21 10:59:10 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.216 2022/10/15 15:20:46 riastradh Exp $"); + +#ifdef _KERNEL_OPT +#include "opt_ddb.h" +#endif #include #include @@ -86,6 +90,10 @@ __KERNEL_RCSID(0, "$NetBSD: spec_vnops.c #include #include +#ifdef DDB +#include +#endif + /* * Lock order: * @@ -1485,6 +1493,9 @@ spec_strategy(void *v) if (mp && (mp->mnt_flag & MNT_RDONLY)) { printf("%s blk %"PRId64" written while ro!\n", mp->mnt_stat.f_mntonname, bp->b_blkno); +#ifdef DDB +db_stacktrace(); +#endif } } #endif /* DIAGNOSTIC */
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Wed Sep 21 10:59:10 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs(9): XXX comment: what if read downgrades lock? To generate a diff of this commit: cvs rdiff -u -r1.214 -r1.215 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Wed Sep 21 10:59:10 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs(9): XXX comment: what if read downgrades lock? To generate a diff of this commit: cvs rdiff -u -r1.214 -r1.215 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.214 src/sys/miscfs/specfs/spec_vnops.c:1.215 --- src/sys/miscfs/specfs/spec_vnops.c:1.214 Fri Aug 12 21:25:39 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Wed Sep 21 10:59:10 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.214 2022/08/12 21:25:39 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.215 2022/09/21 10:59:10 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.214 2022/08/12 21:25:39 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.215 2022/09/21 10:59:10 riastradh Exp $"); #include #include @@ -1100,7 +1100,8 @@ spec_read(void *v) goto out; error = cdev_read(dev, uio, ap->a_ioflag); spec_io_exit(vp, sn); -out: vn_lock(vp, LK_SHARED | LK_RETRY); +out: /* XXX What if the caller held an exclusive lock? */ + vn_lock(vp, LK_SHARED | LK_RETRY); return (error); case VBLK:
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Fri Aug 12 21:25:39 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Refuse to open a closing-in-progress block device. We could wait for close to complete, but if this happened ever so slightly earlier it would lead to EBUSY anyway, so there's no point in adding logic for that -- either way the caller neglected to wait for the last close to finish before trying to open it the device again. https://mail-index.netbsd.org/current-users/2022/08/09/msg042800.html Reported-by: syzbot+4388f20706ec8a4c8...@syzkaller.appspotmail.com https://syzkaller.appspot.com/bug?id=47c67ab6d3a87514d0707882a9ad6671beaa8642 Reported-by: syzbot+0f1756652dce4cb34...@syzkaller.appspotmail.com https://syzkaller.appspot.com/bug?id=a632ce762d64241fc82a9bc57230b7b7c7095d1a To generate a diff of this commit: cvs rdiff -u -r1.213 -r1.214 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.213 src/sys/miscfs/specfs/spec_vnops.c:1.214 --- src/sys/miscfs/specfs/spec_vnops.c:1.213 Fri Aug 12 17:06:01 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Fri Aug 12 21:25:39 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.213 2022/08/12 17:06:01 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.214 2022/08/12 21:25:39 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.213 2022/08/12 17:06:01 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.214 2022/08/12 21:25:39 riastradh Exp $"); #include #include @@ -789,8 +789,13 @@ spec_open(void *v) * * Treat zero opencnt with non-NULL mountpoint as open. * This may happen after forced detach of a mounted device. + * + * Also treat sd_closing, meaning there is a concurrent + * close in progress, as still open. */ - if (sd->sd_opencnt != 0 || sd->sd_mountpoint != NULL) { + if (sd->sd_opencnt != 0 || + sd->sd_mountpoint != NULL || + sd->sd_closing) { error = EBUSY; break; }
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Fri Aug 12 21:25:39 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Refuse to open a closing-in-progress block device. We could wait for close to complete, but if this happened ever so slightly earlier it would lead to EBUSY anyway, so there's no point in adding logic for that -- either way the caller neglected to wait for the last close to finish before trying to open it the device again. https://mail-index.netbsd.org/current-users/2022/08/09/msg042800.html Reported-by: syzbot+4388f20706ec8a4c8...@syzkaller.appspotmail.com https://syzkaller.appspot.com/bug?id=47c67ab6d3a87514d0707882a9ad6671beaa8642 Reported-by: syzbot+0f1756652dce4cb34...@syzkaller.appspotmail.com https://syzkaller.appspot.com/bug?id=a632ce762d64241fc82a9bc57230b7b7c7095d1a To generate a diff of this commit: cvs rdiff -u -r1.213 -r1.214 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Fri Aug 12 17:06:01 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Assert !closing on successful open. - If there's a prior concurrent close, it must have interrupted this open. - If there's a new concurrent close, it must wait until this open has released device_lock before it can revoke. To generate a diff of this commit: cvs rdiff -u -r1.212 -r1.213 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.212 src/sys/miscfs/specfs/spec_vnops.c:1.213 --- src/sys/miscfs/specfs/spec_vnops.c:1.212 Fri Aug 12 17:05:49 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Fri Aug 12 17:06:01 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.212 2022/08/12 17:05:49 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.213 2022/08/12 17:06:01 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.212 2022/08/12 17:05:49 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.213 2022/08/12 17:06:01 riastradh Exp $"); #include #include @@ -967,6 +967,7 @@ spec_open(void *v) KASSERTMSG(sn->sn_opencnt <= sd->sd_opencnt, "sn_opencnt=%u > sd_opencnt=%u", sn->sn_opencnt, sd->sd_opencnt); + KASSERT(!sd->sd_closing); sd->sd_opened = true; } else if (sd->sd_opencnt == 1 && sd->sd_opened) { /*
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Fri Aug 12 17:06:01 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Assert !closing on successful open. - If there's a prior concurrent close, it must have interrupted this open. - If there's a new concurrent close, it must wait until this open has released device_lock before it can revoke. To generate a diff of this commit: cvs rdiff -u -r1.212 -r1.213 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Fri Aug 12 17:05:49 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Assert opencnt>0 on successful open. To generate a diff of this commit: cvs rdiff -u -r1.211 -r1.212 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.211 src/sys/miscfs/specfs/spec_vnops.c:1.212 --- src/sys/miscfs/specfs/spec_vnops.c:1.211 Thu Aug 11 12:52:24 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Fri Aug 12 17:05:49 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.211 2022/08/11 12:52:24 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.212 2022/08/12 17:05:49 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.211 2022/08/11 12:52:24 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.212 2022/08/12 17:05:49 riastradh Exp $"); #include #include @@ -956,6 +956,17 @@ spec_open(void *v) if (error == 0) error = EBADF; } else if (error == 0) { + /* + * Device has not been revoked, so our opencnt can't + * have gone away at this point -- transition to + * sn_gone=true happens before transition to + * sn_opencnt=0 in spec_node_revoke. + */ + KASSERT(sd->sd_opencnt); + KASSERT(sn->sn_opencnt); + KASSERTMSG(sn->sn_opencnt <= sd->sd_opencnt, + "sn_opencnt=%u > sd_opencnt=%u", + sn->sn_opencnt, sd->sd_opencnt); sd->sd_opened = true; } else if (sd->sd_opencnt == 1 && sd->sd_opened) { /*
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Fri Aug 12 17:05:49 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Assert opencnt>0 on successful open. To generate a diff of this commit: cvs rdiff -u -r1.211 -r1.212 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Thu Aug 11 12:52:24 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Sprinkle opencnt/opened/closing assertions. There seems to be a bug here but I'm not sure what it is yet: https://mail-index.netbsd.org/current-users/2022/08/09/msg042800.html https://syzkaller.appspot.com/bug?id=47c67ab6d3a87514d0707882a9ad6671beaa8642 The decision to actually invoke d_close is serialized under device_lock, so it should not be possible for more than one process to close at the same time, but syzbot and kre found a way for sd_closing to be false later in spec_close. Let's make sure it's false when we're making what should be the exclusive decision to close. We can't assert !sd_opened before cancel and spec_io_drain, because those are necessary to interrupt and wait for pending opens that might later set sd_opened, but we can assert !sd_opened afterward because once sd_closing is true nothing should set sd_opened. To generate a diff of this commit: cvs rdiff -u -r1.210 -r1.211 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.210 src/sys/miscfs/specfs/spec_vnops.c:1.211 --- src/sys/miscfs/specfs/spec_vnops.c:1.210 Mon Mar 28 12:39:10 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Thu Aug 11 12:52:24 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.210 2022/03/28 12:39:10 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.211 2022/08/11 12:52:24 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.210 2022/03/28 12:39:10 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.211 2022/08/11 12:52:24 riastradh Exp $"); #include #include @@ -603,7 +603,9 @@ spec_node_revoke(vnode_t *vp) KASSERT(sn->sn_gone == false); mutex_enter(&device_lock); - KASSERT(sn->sn_opencnt <= sd->sd_opencnt); + KASSERTMSG(sn->sn_opencnt <= sd->sd_opencnt, + "sn_opencnt=%u > sd_opencnt=%u", + sn->sn_opencnt, sd->sd_opencnt); sn->sn_gone = true; if (sn->sn_opencnt != 0) { sd->sd_opencnt -= (sn->sn_opencnt - 1); @@ -775,6 +777,9 @@ spec_open(void *v) break; sd->sd_opencnt++; sn->sn_opencnt++; + KASSERTMSG(sn->sn_opencnt <= sd->sd_opencnt, + "sn_opencnt=%u > sd_opencnt=%u", + sn->sn_opencnt, sd->sd_opencnt); break; case VBLK: /* @@ -789,7 +794,8 @@ spec_open(void *v) error = EBUSY; break; } - KASSERTMSG(sn->sn_opencnt == 0, "%u", sn->sn_opencnt); + KASSERTMSG(sn->sn_opencnt == 0, "sn_opencnt=%u", + sn->sn_opencnt); sn->sn_opencnt = 1; sd->sd_opencnt = 1; sd->sd_bdevvp = vp; @@ -963,6 +969,9 @@ spec_open(void *v) } else { KASSERT(sd->sd_opencnt); KASSERT(sn->sn_opencnt); + KASSERTMSG(sn->sn_opencnt <= sd->sd_opencnt, + "sn_opencnt=%u > sd_opencnt=%u", + sn->sn_opencnt, sd->sd_opencnt); sd->sd_opencnt--; sn->sn_opencnt--; if (vp->v_type == VBLK) @@ -1664,6 +1673,9 @@ spec_close(void *v) mutex_enter(&device_lock); KASSERT(sn->sn_opencnt); KASSERT(sd->sd_opencnt); + KASSERTMSG(sn->sn_opencnt <= sd->sd_opencnt, + "sn_opencnt=%u > sd_opencnt=%u", + sn->sn_opencnt, sd->sd_opencnt); sn->sn_opencnt--; count = --sd->sd_opencnt; if (vp->v_type == VBLK) { @@ -1672,6 +1684,9 @@ spec_close(void *v) sd->sd_bdevvp = NULL; } if (count == 0) { + KASSERTMSG(sn->sn_opencnt == 0, "sn_opencnt=%u", + sn->sn_opencnt); + KASSERT(!sd->sd_closing); sd->sd_opened = false; sd->sd_closing = true; } @@ -1722,6 +1737,7 @@ spec_close(void *v) * reacquiring the lock would deadlock. */ mutex_enter(&device_lock); + KASSERT(!sd->sd_opened); KASSERT(sd->sd_closing); sd->sd_closing = false; cv_broadcast(&specfs_iocv);
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Thu Aug 11 12:52:24 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Sprinkle opencnt/opened/closing assertions. There seems to be a bug here but I'm not sure what it is yet: https://mail-index.netbsd.org/current-users/2022/08/09/msg042800.html https://syzkaller.appspot.com/bug?id=47c67ab6d3a87514d0707882a9ad6671beaa8642 The decision to actually invoke d_close is serialized under device_lock, so it should not be possible for more than one process to close at the same time, but syzbot and kre found a way for sd_closing to be false later in spec_close. Let's make sure it's false when we're making what should be the exclusive decision to close. We can't assert !sd_opened before cancel and spec_io_drain, because those are necessary to interrupt and wait for pending opens that might later set sd_opened, but we can assert !sd_opened afterward because once sd_closing is true nothing should set sd_opened. To generate a diff of this commit: cvs rdiff -u -r1.210 -r1.211 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:38:04 UTC 2022 Modified Files: src/sys/miscfs/specfs: specdev.h Log Message: specfs: Reorder struct specnode members to save padding. Shrinks from 40 bytes to 32 bytes on LP64 systems this way. To generate a diff of this commit: cvs rdiff -u -r1.51 -r1.52 src/sys/miscfs/specfs/specdev.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/specdev.h diff -u src/sys/miscfs/specfs/specdev.h:1.51 src/sys/miscfs/specfs/specdev.h:1.52 --- src/sys/miscfs/specfs/specdev.h:1.51 Mon Mar 28 12:37:46 2022 +++ src/sys/miscfs/specfs/specdev.h Mon Mar 28 12:38:04 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: specdev.h,v 1.51 2022/03/28 12:37:46 riastradh Exp $ */ +/* $NetBSD: specdev.h,v 1.52 2022/03/28 12:38:04 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -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;
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:38:04 UTC 2022 Modified Files: src/sys/miscfs/specfs: specdev.h Log Message: specfs: Reorder struct specnode members to save padding. Shrinks from 40 bytes to 32 bytes on LP64 systems this way. To generate a diff of this commit: cvs rdiff -u -r1.51 -r1.52 src/sys/miscfs/specfs/specdev.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:37:35 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Assert opencnt is nonzero before decrementing. To generate a diff of this commit: cvs rdiff -u -r1.206 -r1.207 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.206 src/sys/miscfs/specfs/spec_vnops.c:1.207 --- src/sys/miscfs/specfs/spec_vnops.c:1.206 Mon Mar 28 12:37:26 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:37:35 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.206 2022/03/28 12:37:26 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.207 2022/03/28 12:37:35 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.206 2022/03/28 12:37:26 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.207 2022/03/28 12:37:35 riastradh Exp $"); #include #include @@ -944,6 +944,8 @@ spec_open(void *v) KASSERT(sn->sn_opencnt == 1); needclose = true; } else { + KASSERT(sd->sd_opencnt); + KASSERT(sn->sn_opencnt); sd->sd_opencnt--; sn->sn_opencnt--; if (vp->v_type == VBLK) @@ -1643,6 +1645,8 @@ spec_close(void *v) * between open and close can use fd_clone. */ mutex_enter(&device_lock); + KASSERT(sn->sn_opencnt); + KASSERT(sd->sd_opencnt); sn->sn_opencnt--; count = --sd->sd_opencnt; if (vp->v_type == VBLK) {
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:37:35 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Assert opencnt is nonzero before decrementing. To generate a diff of this commit: cvs rdiff -u -r1.206 -r1.207 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:37:27 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Take an I/O reference across bdev/cdev_open. - Revoke is used to invalidate all prior access control checks when device permissions are changing, so it must wait for .d_open to exit so any new access must go through new access control checks. - Revoke is used by vdevgone in xyz_detach to wait until all use of the driver's data structures have completed before xyz_detach frees them. So we need to make sure spec_close waits for .d_open too. To generate a diff of this commit: cvs rdiff -u -r1.205 -r1.206 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.205 src/sys/miscfs/specfs/spec_vnops.c:1.206 --- src/sys/miscfs/specfs/spec_vnops.c:1.205 Mon Mar 28 12:37:18 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:37:26 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.205 2022/03/28 12:37:18 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.206 2022/03/28 12:37:26 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.205 2022/03/28 12:37:18 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.206 2022/03/28 12:37:26 riastradh Exp $"); #include #include @@ -694,10 +694,10 @@ spec_open(void *v) } */ *ap = v; struct lwp *l = curlwp; struct vnode *vp = ap->a_vp; - dev_t dev; + dev_t dev, dev1; int error; enum kauth_device_req req; - specnode_t *sn; + specnode_t *sn, *sn1; specdev_t *sd; spec_ioctl_t ioctl; u_int gen = 0; @@ -805,18 +805,34 @@ spec_open(void *v) } /* - * Open the device. If .d_open returns ENXIO (device not - * configured), the driver may not be loaded, so try - * autoloading a module and then try .d_open again if anything - * got loaded. - * * Because opening the device may block indefinitely, e.g. when * opening a tty, and loading a module may cross into many * other subsystems, we must not hold the vnode lock while * calling .d_open, so release it now and reacquire it when * done. + * + * 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 must interrupt + * .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); + if (error) { + vn_lock(vp, LK_EXCLUSIVE | LK_RETRY); + return error; + } + KASSERT(sn1 == sn); + KASSERT(dev1 == dev); + + /* + * Open the device. If .d_open returns ENXIO (device not + * configured), the driver may not be loaded, so try + * autoloading a module and then try .d_open again if anything + * got loaded. + */ switch (vp->v_type) { case VCHR: do { @@ -871,7 +887,16 @@ spec_open(void *v) default: __unreachable(); } + + /* + * Release the I/O reference now that we have called .d_open, + * and reacquire the vnode lock. At this point, the device may + * have been revoked, so we must tread carefully. However, sn + * and sd remain valid pointers until we drop our reference. + */ + spec_io_exit(vp, sn); vn_lock(vp, LK_EXCLUSIVE | LK_RETRY); + KASSERT(vp->v_specnode == sn); /* * If it has been revoked since we released the vnode lock and
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:37:27 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Take an I/O reference across bdev/cdev_open. - Revoke is used to invalidate all prior access control checks when device permissions are changing, so it must wait for .d_open to exit so any new access must go through new access control checks. - Revoke is used by vdevgone in xyz_detach to wait until all use of the driver's data structures have completed before xyz_detach frees them. So we need to make sure spec_close waits for .d_open too. To generate a diff of this commit: cvs rdiff -u -r1.205 -r1.206 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:37:18 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Wait for last close in spec_node_revoke. Otherwise, revoke -- and vdevgone, in the detach path of removable devices -- may complete while I/O operations are still running concurrently. To generate a diff of this commit: cvs rdiff -u -r1.204 -r1.205 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.204 src/sys/miscfs/specfs/spec_vnops.c:1.205 --- src/sys/miscfs/specfs/spec_vnops.c:1.204 Mon Mar 28 12:37:09 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:37:18 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.204 2022/03/28 12:37:09 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.205 2022/03/28 12:37:18 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.204 2022/03/28 12:37:09 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.205 2022/03/28 12:37:18 riastradh Exp $"); #include #include @@ -597,6 +597,16 @@ spec_node_revoke(vnode_t *vp) mutex_enter(&device_lock); KASSERT(sn->sn_opencnt == 0); } + + /* + * We may have revoked the vnode in this thread while another + * thread was in the middle of spec_close, in the window when + * spec_close releases the vnode lock to call .d_close for the + * last close. In that case, wait for the concurrent + * spec_close to complete. + */ + while (sd->sd_closing) + cv_wait(&specfs_iocv, &device_lock); mutex_exit(&device_lock); }
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:37:18 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Wait for last close in spec_node_revoke. Otherwise, revoke -- and vdevgone, in the detach path of removable devices -- may complete while I/O operations are still running concurrently. To generate a diff of this commit: cvs rdiff -u -r1.204 -r1.205 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:37:09 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c specdev.h Log Message: specfs: Prevent new opens while close is waiting to drain. Otherwise, bdev/cdev_close could have cancelled all _existing_ opens, and waited for them to complete (and freed resources used by them) -- but a new one could start, and hang (e.g., a tty), at the same time spec_close tries to drain all pending I/O operations, one of which (the new open) is now hanging indefinitely. Preventing the new open from even starting until bdev/cdev_close is finished and all I/O operations have drained avoids this deadlock. To generate a diff of this commit: cvs rdiff -u -r1.203 -r1.204 src/sys/miscfs/specfs/spec_vnops.c cvs rdiff -u -r1.49 -r1.50 src/sys/miscfs/specfs/specdev.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.203 src/sys/miscfs/specfs/spec_vnops.c:1.204 --- src/sys/miscfs/specfs/spec_vnops.c:1.203 Mon Mar 28 12:37:01 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:37:09 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.203 2022/03/28 12:37:01 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.204 2022/03/28 12:37:09 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.203 2022/03/28 12:37:01 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.204 2022/03/28 12:37:09 riastradh Exp $"); #include #include @@ -397,6 +397,7 @@ spec_node_init(vnode_t *vp, dev_t rdev) sd->sd_bdevvp = NULL; sd->sd_iocnt = 0; sd->sd_opened = false; + sd->sd_closing = false; sn->sn_dev = sd; sd = NULL; } else { @@ -734,8 +735,17 @@ spec_open(void *v) case VCHR: /* * Character devices can accept opens from multiple - * vnodes. + * vnodes. But first, wait for any close to finish. + * Wait under the vnode lock so we don't have to worry + * about the vnode being revoked while we wait. */ + while (sd->sd_closing) { + error = cv_wait_sig(&specfs_iocv, &device_lock); + if (error) +break; + } + if (error) + break; sd->sd_opencnt++; sn->sn_opencnt++; break; @@ -1605,8 +1615,10 @@ spec_close(void *v) count + 1); sd->sd_bdevvp = NULL; } - if (count == 0) + if (count == 0) { sd->sd_opened = false; + sd->sd_closing = true; + } mutex_exit(&device_lock); if (count != 0) @@ -1631,6 +1643,18 @@ spec_close(void *v) */ spec_io_drain(sd); + /* + * Wake any spec_open calls waiting for close to finish -- do + * this before reacquiring the vnode lock, because spec_open + * holds the vnode lock while waiting, so doing this after + * reacquiring the lock would deadlock. + */ + mutex_enter(&device_lock); + KASSERT(sd->sd_closing); + sd->sd_closing = false; + cv_broadcast(&specfs_iocv); + mutex_exit(&device_lock); + if (!(flags & FNONBLOCK)) vn_lock(vp, LK_EXCLUSIVE | LK_RETRY); Index: src/sys/miscfs/specfs/specdev.h diff -u src/sys/miscfs/specfs/specdev.h:1.49 src/sys/miscfs/specfs/specdev.h:1.50 --- src/sys/miscfs/specfs/specdev.h:1.49 Mon Mar 28 12:36:51 2022 +++ src/sys/miscfs/specfs/specdev.h Mon Mar 28 12:37:09 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: specdev.h,v 1.49 2022/03/28 12:36:51 riastradh Exp $ */ +/* $NetBSD: specdev.h,v 1.50 2022/03/28 12:37:09 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -80,6 +80,7 @@ typedef struct specdev { dev_t sd_rdev; volatile u_int sd_iocnt; /* # bdev/cdev_* operations active */ bool sd_opened; /* true if successfully opened */ + bool sd_closing; /* true when bdev/cdev_close ongoing */ } specdev_t; /*
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:37:09 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c specdev.h Log Message: specfs: Prevent new opens while close is waiting to drain. Otherwise, bdev/cdev_close could have cancelled all _existing_ opens, and waited for them to complete (and freed resources used by them) -- but a new one could start, and hang (e.g., a tty), at the same time spec_close tries to drain all pending I/O operations, one of which (the new open) is now hanging indefinitely. Preventing the new open from even starting until bdev/cdev_close is finished and all I/O operations have drained avoids this deadlock. To generate a diff of this commit: cvs rdiff -u -r1.203 -r1.204 src/sys/miscfs/specfs/spec_vnops.c cvs rdiff -u -r1.49 -r1.50 src/sys/miscfs/specfs/specdev.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:37:01 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Take an I/O reference in spec_node_setmountedfs. This is not quite correct. We _should_ require the caller to hold a vnode lock around spec_node_getmountedfs, and an exclusive vnode lock around spec_node_setmountedfs, so that it is only necessary to check whether revoke has already happened, not hold an I/O reference. Unfortunately, various callers in various file systems don't follow this sensible rule. So let's at least make sure the vnode can't be revoked in spec_node_setmountedfs, while we're in bdev_ioctl, and leave a comment explaining what the sorry state of affairs is and how to fix it later. To generate a diff of this commit: cvs rdiff -u -r1.202 -r1.203 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.202 src/sys/miscfs/specfs/spec_vnops.c:1.203 --- src/sys/miscfs/specfs/spec_vnops.c:1.202 Mon Mar 28 12:36:51 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:37:01 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.202 2022/03/28 12:36:51 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.203 2022/03/28 12:37:01 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.202 2022/03/28 12:36:51 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.203 2022/03/28 12:37:01 riastradh Exp $"); #include #include @@ -498,6 +498,11 @@ spec_node_lookup_by_mount(struct mount * /* * 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) @@ -512,23 +517,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); - devvp->v_specnode->sn_dev->sd_mountpoint = mp; - if (mp == NULL) - return; - if (bdev_ioctl(devvp->v_rdev, DIOCGWEDGEINFO, &dkw, FREAD, curlwp) != 0) + error = spec_io_enter(devvp, &sn, &dev); + if (error) 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); } /*
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:37:01 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Take an I/O reference in spec_node_setmountedfs. This is not quite correct. We _should_ require the caller to hold a vnode lock around spec_node_getmountedfs, and an exclusive vnode lock around spec_node_setmountedfs, so that it is only necessary to check whether revoke has already happened, not hold an I/O reference. Unfortunately, various callers in various file systems don't follow this sensible rule. So let's at least make sure the vnode can't be revoked in spec_node_setmountedfs, while we're in bdev_ioctl, and leave a comment explaining what the sorry state of affairs is and how to fix it later. To generate a diff of this commit: cvs rdiff -u -r1.202 -r1.203 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:36:51 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c specdev.h Log Message: specfs: Drain all I/O operations after last .d_close call. New kind of I/O reference on specdevs, sd_iocnt. This could be done with psref instead; I chose a reference count instead for now because we already have to take a per-object lock anyway, v_interlock, for vdead_check, so another atomic is not likely to hurt much more. We can always change the mechanism inside spec_io_enter/exit/drain later on. Make sure every access to vp->v_rdev or vp->v_specnode and every call to a devsw operation is protected either: - by the vnode lock (with vdead_check if we unlocked/relocked), - by positive sd_opencnt, - by spec_io_enter/exit, or - by sd_opencnt management in open/close. To generate a diff of this commit: cvs rdiff -u -r1.201 -r1.202 src/sys/miscfs/specfs/spec_vnops.c cvs rdiff -u -r1.48 -r1.49 src/sys/miscfs/specfs/specdev.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.201 src/sys/miscfs/specfs/spec_vnops.c:1.202 --- src/sys/miscfs/specfs/spec_vnops.c:1.201 Mon Mar 28 12:36:42 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:36:51 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.201 2022/03/28 12:36:42 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.202 2022/03/28 12:36:51 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.201 2022/03/28 12:36:42 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.202 2022/03/28 12:36:51 riastradh Exp $"); #include #include @@ -81,6 +81,7 @@ __KERNEL_RCSID(0, "$NetBSD: spec_vnops.c #include #include #include +#include #include #include @@ -173,6 +174,7 @@ const struct vnodeopv_desc spec_vnodeop_ { &spec_vnodeop_p, spec_vnodeop_entries }; static kauth_listener_t rawio_listener; +static struct kcondvar specfs_iocv; /* Returns true if vnode is /dev/mem or /dev/kmem. */ bool @@ -218,6 +220,141 @@ spec_init(void) rawio_listener = kauth_listen_scope(KAUTH_SCOPE_DEVICE, rawio_listener_cb, NULL); + cv_init(&specfs_iocv, "specio"); +} + +/* + * spec_io_enter(vp, &sn, &dev) + * + * Enter an operation that may not hold vp's vnode lock or an + * fstrans on vp's mount. Until spec_io_exit, the vnode will not + * be revoked. + * + * On success, set sn to the specnode pointer and dev to the dev_t + * number and return zero. Caller must later call spec_io_exit + * when done. + * + * On failure, return ENXIO -- the device has been revoked and no + * longer exists. + */ +static int +spec_io_enter(struct vnode *vp, struct specnode **snp, dev_t *devp) +{ + dev_t dev; + struct specnode *sn; + unsigned iocnt; + int error = 0; + + mutex_enter(vp->v_interlock); + + /* + * Extract all the info we need from the vnode, unless the + * vnode has already been reclaimed. This can happen if the + * underlying device has been removed and all the device nodes + * for it have been revoked. The caller may not hold a vnode + * lock or fstrans to prevent this from happening before it has + * had an opportunity to notice the vnode is dead. + */ + if (vdead_check(vp, VDEAD_NOWAIT) != 0 || + (sn = vp->v_specnode) == NULL || + (dev = vp->v_rdev) == NODEV) { + error = ENXIO; + goto out; + } + + /* + * 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 + * already have to take the interlock for vdead_check, so + * there's not much more cost here to another atomic operation. + */ + 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; + *devp = dev; + error = 0; + +out: mutex_exit(vp->v_interlock); + return error; +} + +/* + * spec_io_exit(vp, sn) + * + * Exit an operation entered with a successful spec_io_enter -- + * allow concurrent spec_node_revoke to proceed. The argument sn + * must match the struct specnode pointer returned by spec_io_exit + * for vp. + */ +static void +spec_io_exit(s
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:36:51 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c specdev.h Log Message: specfs: Drain all I/O operations after last .d_close call. New kind of I/O reference on specdevs, sd_iocnt. This could be done with psref instead; I chose a reference count instead for now because we already have to take a per-object lock anyway, v_interlock, for vdead_check, so another atomic is not likely to hurt much more. We can always change the mechanism inside spec_io_enter/exit/drain later on. Make sure every access to vp->v_rdev or vp->v_specnode and every call to a devsw operation is protected either: - by the vnode lock (with vdead_check if we unlocked/relocked), - by positive sd_opencnt, - by spec_io_enter/exit, or - by sd_opencnt management in open/close. To generate a diff of this commit: cvs rdiff -u -r1.201 -r1.202 src/sys/miscfs/specfs/spec_vnops.c cvs rdiff -u -r1.48 -r1.49 src/sys/miscfs/specfs/specdev.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:36:42 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c specdev.h Log Message: specfs: Resolve a race between close and a failing reopen. To generate a diff of this commit: cvs rdiff -u -r1.200 -r1.201 src/sys/miscfs/specfs/spec_vnops.c cvs rdiff -u -r1.47 -r1.48 src/sys/miscfs/specfs/specdev.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.200 src/sys/miscfs/specfs/spec_vnops.c:1.201 --- src/sys/miscfs/specfs/spec_vnops.c:1.200 Mon Mar 28 12:36:26 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:36:42 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.200 2022/03/28 12:36:26 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.201 2022/03/28 12:36:42 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.200 2022/03/28 12:36:26 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.201 2022/03/28 12:36:42 riastradh Exp $"); #include #include @@ -258,6 +258,7 @@ spec_node_init(vnode_t *vp, dev_t rdev) sd->sd_refcnt = 1; sd->sd_opencnt = 0; sd->sd_bdevvp = NULL; + sd->sd_opened = false; sn->sn_dev = sd; sd = NULL; } else { @@ -518,6 +519,7 @@ spec_open(void *v) spec_ioctl_t ioctl; u_int gen; const char *name; + bool needclose = false; struct partinfo pi; l = curlwp; @@ -688,12 +690,9 @@ spec_open(void *v) * must fail with EBADF. * * Otherwise, if opening it failed, back out and release the - * open reference. - * - * XXX This is wrong -- we might release the last open - * reference here, but we don't close the device. If only this - * thread's call to open failed, that's fine, but we might - * have: + * open reference. If it was ever successfully opened and we + * got the last reference this way, it's now our job to close + * it. This might happen in the following scenario: * * Thread 1 Thread 2 * VOP_OPEN @@ -710,21 +709,54 @@ spec_open(void *v) * release vnode lock * acquire vnode lock * --sd_opencnt == 0 - * but no .d_close (***) + * + * We can't resolve this by making spec_close wait for .d_open + * to complete before examining sd_opencnt, because .d_open can + * hang indefinitely, e.g. for a tty. */ mutex_enter(&device_lock); if (sn->sn_gone) { if (error == 0) error = EBADF; - } else if (error != 0) { + } else if (error == 0) { + sd->sd_opened = true; + } else if (sd->sd_opencnt == 1 && sd->sd_opened) { + /* + * We're the last reference to a _previous_ open even + * though this one failed, so we have to close it. + * Don't decrement the reference count here -- + * spec_close will do that. + */ + KASSERT(sn->sn_opencnt == 1); + needclose = true; + } else { sd->sd_opencnt--; sn->sn_opencnt--; if (vp->v_type == VBLK) sd->sd_bdevvp = NULL; - } mutex_exit(&device_lock); + /* + * If this open failed, but the device was previously opened, + * and another thread concurrently closed the vnode while we + * were in the middle of reopening it, the other thread will + * see sd_opencnt > 0 and thus decide not to call .d_close -- + * it is now our responsibility to do so. + * + * XXX The flags passed to VOP_CLOSE here are wrong, but + * drivers can't rely on FREAD|FWRITE anyway -- e.g., consider + * a device opened by thread 0 with O_READ, then opened by + * thread 1 with O_WRITE, then closed by thread 0, and finally + * closed by thread 1; the last .d_close call will have FWRITE + * but not FREAD. We should just eliminate the FREAD/FWRITE + * parameter to .d_close altogether. + */ + if (needclose) { + KASSERT(error); + VOP_CLOSE(vp, FNONBLOCK, NOCRED); + } + /* If anything went wrong, we're done. */ if (error) return error; @@ -1341,6 +1373,25 @@ spec_close(void *v) * device. For block devices, the open reference count must be * 1 at this point. If the device's open reference count goes * to zero, we're the last one out so get the lights. + * + * We may find --sd->sd_opencnt gives zero, and yet + * sd->sd_opened is false. This happens if the vnode is + * revoked at the same time as it is being opened, which can + * happen when opening a tty blocks indefinitely. In that + * case, we still must call close -- it is the job of close to + * interrupt the open. Either way, the device will be no + * longer opened, so we have to clear sd->sd_opened; subsequent + * opens will have responsibility for issuing close. + * + * This has the side effect that the sequence of opens might + * happen out of order -- we might end up doing open, open, + * close, close, instead of open, close, open, close. This is + * unavoidable with the current devsw API, wher
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:36:42 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c specdev.h Log Message: specfs: Resolve a race between close and a failing reopen. To generate a diff of this commit: cvs rdiff -u -r1.200 -r1.201 src/sys/miscfs/specfs/spec_vnops.c cvs rdiff -u -r1.47 -r1.48 src/sys/miscfs/specfs/specdev.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:36:34 UTC 2022 Modified Files: src/sys/miscfs/specfs: specdev.h Log Message: specfs: Document sn_opencnt, sd_opencnt, sd_refcnt. To generate a diff of this commit: cvs rdiff -u -r1.46 -r1.47 src/sys/miscfs/specfs/specdev.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/specdev.h diff -u src/sys/miscfs/specfs/specdev.h:1.46 src/sys/miscfs/specfs/specdev.h:1.47 --- src/sys/miscfs/specfs/specdev.h:1.46 Sun Jul 18 23:57:15 2021 +++ src/sys/miscfs/specfs/specdev.h Mon Mar 28 12:36:34 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: specdev.h,v 1.46 2021/07/18 23:57:15 dholland Exp $ */ +/* $NetBSD: specdev.h,v 1.47 2022/03/28 12:36:34 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -66,7 +66,7 @@ typedef struct specnode { vnode_t *sn_next; struct specdev *sn_dev; - u_int sn_opencnt; + u_int sn_opencnt; /* # of opens, share of sd_opencnt */ dev_t sn_rdev; bool sn_gone; } specnode_t; @@ -75,8 +75,8 @@ typedef struct specdev { struct mount *sd_mountpoint; struct lockf *sd_lockf; vnode_t *sd_bdevvp; - u_int sd_opencnt; - u_int sd_refcnt; + u_int sd_opencnt; /* # of opens; close when ->0 */ + u_int sd_refcnt; /* # of specnodes referencing this */ dev_t sd_rdev; } specdev_t;
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:36:34 UTC 2022 Modified Files: src/sys/miscfs/specfs: specdev.h Log Message: specfs: Document sn_opencnt, sd_opencnt, sd_refcnt. To generate a diff of this commit: cvs rdiff -u -r1.46 -r1.47 src/sys/miscfs/specfs/specdev.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:36:27 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Paranoia: Assert opencnt is zero on reclaim. To generate a diff of this commit: cvs rdiff -u -r1.199 -r1.200 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.199 src/sys/miscfs/specfs/spec_vnops.c:1.200 --- src/sys/miscfs/specfs/spec_vnops.c:1.199 Mon Mar 28 12:36:18 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:36:26 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.199 2022/03/28 12:36:18 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.200 2022/03/28 12:36:26 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.199 2022/03/28 12:36:18 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.200 2022/03/28 12:36:26 riastradh Exp $"); #include #include @@ -1204,6 +1204,8 @@ spec_reclaim(void *v) } */ *ap = v; struct vnode *vp = ap->a_vp; + KASSERT(vp->v_specnode->sn_opencnt == 0); + VOP_UNLOCK(vp); KASSERT(vp->v_mount == dead_rootmount);
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:36:27 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Paranoia: Assert opencnt is zero on reclaim. To generate a diff of this commit: cvs rdiff -u -r1.199 -r1.200 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:36:18 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Omit needless vdead_check in spec_fdiscard. The vnode lock is held, so the vnode cannot be revoked without also changing v_op so subsequent uses under the vnode lock will go to deadfs's VOP_FDISCARD instead (which is genfs_eopnotsupp). To generate a diff of this commit: cvs rdiff -u -r1.198 -r1.199 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.198 src/sys/miscfs/specfs/spec_vnops.c:1.199 --- src/sys/miscfs/specfs/spec_vnops.c:1.198 Mon Mar 28 12:36:09 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:36:18 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.198 2022/03/28 12:36:09 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.199 2022/03/28 12:36:18 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.198 2022/03/28 12:36:09 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.199 2022/03/28 12:36:18 riastradh Exp $"); #include #include @@ -945,17 +945,7 @@ spec_fdiscard(void *v) dev_t dev; vp = ap->a_vp; - dev = NODEV; - - mutex_enter(vp->v_interlock); - if (vdead_check(vp, VDEAD_NOWAIT) == 0 && vp->v_specnode != NULL) { - dev = vp->v_rdev; - } - mutex_exit(vp->v_interlock); - - if (dev == NODEV) { - return ENXIO; - } + dev = vp->v_rdev; switch (vp->v_type) { case VCHR:
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:36:18 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Omit needless vdead_check in spec_fdiscard. The vnode lock is held, so the vnode cannot be revoked without also changing v_op so subsequent uses under the vnode lock will go to deadfs's VOP_FDISCARD instead (which is genfs_eopnotsupp). To generate a diff of this commit: cvs rdiff -u -r1.198 -r1.199 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:36:09 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Add a comment and assertion to spec_close about refcnts. To generate a diff of this commit: cvs rdiff -u -r1.197 -r1.198 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:36:09 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Add a comment and assertion to spec_close about refcnts. To generate a diff of this commit: cvs rdiff -u -r1.197 -r1.198 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.197 src/sys/miscfs/specfs/spec_vnops.c:1.198 --- src/sys/miscfs/specfs/spec_vnops.c:1.197 Mon Mar 28 12:36:00 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:36:09 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.197 2022/03/28 12:36:00 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.198 2022/03/28 12:36:09 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.197 2022/03/28 12:36:00 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.198 2022/03/28 12:36:09 riastradh Exp $"); #include #include @@ -1344,11 +1344,20 @@ spec_close(void *v) panic("spec_close: not special"); } + /* + * Decrement the open reference count of this node and the + * device. For block devices, the open reference count must be + * 1 at this point. If the device's open reference count goes + * to zero, we're the last one out so get the lights. + */ mutex_enter(&device_lock); sn->sn_opencnt--; count = --sd->sd_opencnt; - if (vp->v_type == VBLK) + if (vp->v_type == VBLK) { + KASSERTMSG(count == 0, "block device with %u opens", + count + 1); sd->sd_bdevvp = NULL; + } mutex_exit(&device_lock); if (count != 0)
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:36:01 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: If sd_opencnt is zero, sn_opencnt had better be zero. To generate a diff of this commit: cvs rdiff -u -r1.196 -r1.197 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.196 src/sys/miscfs/specfs/spec_vnops.c:1.197 --- src/sys/miscfs/specfs/spec_vnops.c:1.196 Mon Mar 28 12:35:52 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:36:00 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.196 2022/03/28 12:35:52 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.197 2022/03/28 12:36:00 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.196 2022/03/28 12:35:52 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.197 2022/03/28 12:36:00 riastradh Exp $"); #include #include @@ -581,6 +581,7 @@ spec_open(void *v) error = EBUSY; break; } + KASSERTMSG(sn->sn_opencnt == 0, "%u", sn->sn_opencnt); sn->sn_opencnt = 1; sd->sd_opencnt = 1; sd->sd_bdevvp = vp;
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:36:01 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: If sd_opencnt is zero, sn_opencnt had better be zero. To generate a diff of this commit: cvs rdiff -u -r1.196 -r1.197 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:35:52 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Factor KASSERT out of switch in spec_open. No functional change. To generate a diff of this commit: cvs rdiff -u -r1.195 -r1.196 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.195 src/sys/miscfs/specfs/spec_vnops.c:1.196 --- src/sys/miscfs/specfs/spec_vnops.c:1.195 Mon Mar 28 12:35:44 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:35:52 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.195 2022/03/28 12:35:44 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.196 2022/03/28 12:35:52 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.195 2022/03/28 12:35:44 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.196 2022/03/28 12:35:52 riastradh Exp $"); #include #include @@ -558,13 +558,13 @@ spec_open(void *v) * can't be revoked until we release the vnode lock. */ mutex_enter(&device_lock); + KASSERT(!sn->sn_gone); switch (vp->v_type) { case VCHR: /* * Character devices can accept opens from multiple * vnodes. */ - KASSERT(!sn->sn_gone); sd->sd_opencnt++; sn->sn_opencnt++; break; @@ -577,7 +577,6 @@ spec_open(void *v) * Treat zero opencnt with non-NULL mountpoint as open. * This may happen after forced detach of a mounted device. */ - KASSERT(!sn->sn_gone); if (sd->sd_opencnt != 0 || sd->sd_mountpoint != NULL) { error = EBUSY; break;
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:35:52 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Factor KASSERT out of switch in spec_open. No functional change. To generate a diff of this commit: cvs rdiff -u -r1.195 -r1.196 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:35:44 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: sn_gone cannot be set while we hold the vnode lock. Revoke runs with the vnode lock too, which is exclusive. Add an assertion to this effect in spec_node_revoke to make it clear. To generate a diff of this commit: cvs rdiff -u -r1.194 -r1.195 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.194 src/sys/miscfs/specfs/spec_vnops.c:1.195 --- src/sys/miscfs/specfs/spec_vnops.c:1.194 Mon Mar 28 12:35:35 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:35:44 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.194 2022/03/28 12:35:35 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.195 2022/03/28 12:35:44 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.194 2022/03/28 12:35:35 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.195 2022/03/28 12:35:44 riastradh Exp $"); #include #include @@ -402,6 +402,8 @@ spec_node_revoke(vnode_t *vp) specnode_t *sn; specdev_t *sd; + KASSERT(VOP_ISLOCKED(vp) == LK_EXCLUSIVE); + sn = vp->v_specnode; sd = sn->sn_dev; @@ -552,11 +554,8 @@ spec_open(void *v) /* * Acquire an open reference -- as long as we hold onto it, and - * the vnode isn't revoked, it can't be closed. - * - * But first check whether it has been revoked -- if so, we - * can't acquire more open references and we must fail - * immediately with EBADF. + * the vnode isn't revoked, it can't be closed, and the vnode + * can't be revoked until we release the vnode lock. */ mutex_enter(&device_lock); switch (vp->v_type) { @@ -565,10 +564,7 @@ spec_open(void *v) * Character devices can accept opens from multiple * vnodes. */ - if (sn->sn_gone) { - error = EBADF; - break; - } + KASSERT(!sn->sn_gone); sd->sd_opencnt++; sn->sn_opencnt++; break; @@ -581,10 +577,7 @@ spec_open(void *v) * Treat zero opencnt with non-NULL mountpoint as open. * This may happen after forced detach of a mounted device. */ - if (sn->sn_gone) { - error = EBADF; - break; - } + KASSERT(!sn->sn_gone); if (sd->sd_opencnt != 0 || sd->sd_mountpoint != NULL) { error = EBUSY; break;
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:35:44 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: sn_gone cannot be set while we hold the vnode lock. Revoke runs with the vnode lock too, which is exclusive. Add an assertion to this effect in spec_node_revoke to make it clear. To generate a diff of this commit: cvs rdiff -u -r1.194 -r1.195 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:35:35 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Reorganize D_DISK tail of spec_open and explain what's up. No functional change intended. To generate a diff of this commit: cvs rdiff -u -r1.193 -r1.194 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.193 src/sys/miscfs/specfs/spec_vnops.c:1.194 --- src/sys/miscfs/specfs/spec_vnops.c:1.193 Mon Mar 28 12:35:26 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:35:35 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.193 2022/03/28 12:35:26 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.194 2022/03/28 12:35:35 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.193 2022/03/28 12:35:26 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.194 2022/03/28 12:35:35 riastradh Exp $"); #include #include @@ -732,15 +732,27 @@ spec_open(void *v) } mutex_exit(&device_lock); - if (cdev_type(dev) != D_DISK || error != 0) + /* If anything went wrong, we're done. */ + if (error) return error; - - ioctl = vp->v_type == VCHR ? cdev_ioctl : bdev_ioctl; - error = (*ioctl)(vp->v_rdev, DIOCGPARTINFO, &pi, FREAD, curlwp); - if (error == 0) - uvm_vnp_setsize(vp, (voff_t)pi.pi_secsize * pi.pi_size); + /* + * For disk devices, automagically set the vnode size to the + * partition size, if we can. This applies to block devices + * and character devices alike -- every block device must have + * a corresponding character device. And if the module is + * loaded it will remain loaded until we're done here (it is + * forbidden to devsw_detach until closed). So it is safe to + * query cdev_type unconditionally here. + */ + if (cdev_type(dev) == D_DISK) { + ioctl = vp->v_type == VCHR ? cdev_ioctl : bdev_ioctl; + if ((*ioctl)(dev, DIOCGPARTINFO, &pi, FREAD, curlwp) == 0) + uvm_vnp_setsize(vp, + (voff_t)pi.pi_secsize * pi.pi_size); + } + /* Success! */ return 0; }
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:35:35 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Reorganize D_DISK tail of spec_open and explain what's up. No functional change intended. To generate a diff of this commit: cvs rdiff -u -r1.193 -r1.194 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:35:26 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Factor VOP_UNLOCK/vn_lock out of switch for clarity. No functional change. To generate a diff of this commit: cvs rdiff -u -r1.192 -r1.193 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.192 src/sys/miscfs/specfs/spec_vnops.c:1.193 --- src/sys/miscfs/specfs/spec_vnops.c:1.192 Mon Mar 28 12:35:17 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:35:26 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.192 2022/03/28 12:35:17 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.193 2022/03/28 12:35:26 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.192 2022/03/28 12:35:17 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.193 2022/03/28 12:35:26 riastradh Exp $"); #include #include @@ -632,9 +632,9 @@ spec_open(void *v) * calling .d_open, so release it now and reacquire it when * done. */ + VOP_UNLOCK(vp); switch (vp->v_type) { case VCHR: - VOP_UNLOCK(vp); do { const struct cdevsw *cdev; @@ -657,12 +657,9 @@ spec_open(void *v) /* Try to autoload device module */ (void) module_autoload(name, MODULE_CLASS_DRIVER); } while (gen != module_gen); - - vn_lock(vp, LK_EXCLUSIVE | LK_RETRY); break; case VBLK: - VOP_UNLOCK(vp); do { const struct bdevsw *bdev; @@ -685,13 +682,12 @@ spec_open(void *v) /* Try to autoload device module */ (void) module_autoload(name, MODULE_CLASS_DRIVER); } while (gen != module_gen); - vn_lock(vp, LK_EXCLUSIVE | LK_RETRY); - break; default: __unreachable(); } + vn_lock(vp, LK_EXCLUSIVE | LK_RETRY); /* * If it has been revoked since we released the vnode lock and
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:35:26 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Factor VOP_UNLOCK/vn_lock out of switch for clarity. No functional change. To generate a diff of this commit: cvs rdiff -u -r1.192 -r1.193 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:35:17 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Factor common device_lock out of switch for clarity. No functional change. To generate a diff of this commit: cvs rdiff -u -r1.191 -r1.192 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.191 src/sys/miscfs/specfs/spec_vnops.c:1.192 --- src/sys/miscfs/specfs/spec_vnops.c:1.191 Mon Mar 28 12:35:08 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:35:17 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.191 2022/03/28 12:35:08 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.192 2022/03/28 12:35:17 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.191 2022/03/28 12:35:08 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.192 2022/03/28 12:35:17 riastradh Exp $"); #include #include @@ -558,20 +558,19 @@ spec_open(void *v) * can't acquire more open references and we must fail * immediately with EBADF. */ + mutex_enter(&device_lock); switch (vp->v_type) { case VCHR: /* * Character devices can accept opens from multiple * vnodes. */ - mutex_enter(&device_lock); if (sn->sn_gone) { - mutex_exit(&device_lock); - return (EBADF); + error = EBADF; + break; } sd->sd_opencnt++; sn->sn_opencnt++; - mutex_exit(&device_lock); break; case VBLK: /* @@ -582,23 +581,24 @@ spec_open(void *v) * Treat zero opencnt with non-NULL mountpoint as open. * This may happen after forced detach of a mounted device. */ - mutex_enter(&device_lock); if (sn->sn_gone) { - mutex_exit(&device_lock); - return (EBADF); + error = EBADF; + break; } if (sd->sd_opencnt != 0 || sd->sd_mountpoint != NULL) { - mutex_exit(&device_lock); - return EBUSY; + error = EBUSY; + break; } sn->sn_opencnt = 1; sd->sd_opencnt = 1; sd->sd_bdevvp = vp; - mutex_exit(&device_lock); break; default: panic("invalid specfs vnode type: %d", vp->v_type); } + mutex_exit(&device_lock); + if (error) + return error; /* * Set VV_ISTTY if this is a tty cdev.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:35:17 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Factor common device_lock out of switch for clarity. No functional change. To generate a diff of this commit: cvs rdiff -u -r1.191 -r1.192 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:35:08 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Delete bogus comment about .d_open/.d_close at same time. Annoying as it is that .d_open and .d_close can run at the same time, it is also necessary for tty semantics, where open can block indefinitely, and it is the responsibility of close (called via revoke) necessary to interrupt it. To generate a diff of this commit: cvs rdiff -u -r1.190 -r1.191 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.190 src/sys/miscfs/specfs/spec_vnops.c:1.191 --- src/sys/miscfs/specfs/spec_vnops.c:1.190 Mon Mar 28 12:34:59 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:35:08 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.190 2022/03/28 12:34:59 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.191 2022/03/28 12:35:08 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.190 2022/03/28 12:34:59 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.191 2022/03/28 12:35:08 riastradh Exp $"); #include #include @@ -557,12 +557,6 @@ spec_open(void *v) * But first check whether it has been revoked -- if so, we * can't acquire more open references and we must fail * immediately with EBADF. - * - * XXX This races with revoke: once we release the vnode lock, - * the vnode may be revoked, and the .d_close callback run, at - * the same time as we're calling .d_open here. Drivers - * shouldn't have to contemplate this scenario; .d_open and - * .d_close should be prevented from running concurrently. */ switch (vp->v_type) { case VCHR:
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:35:08 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Delete bogus comment about .d_open/.d_close at same time. Annoying as it is that .d_open and .d_close can run at the same time, it is also necessary for tty semantics, where open can block indefinitely, and it is the responsibility of close (called via revoke) necessary to interrupt it. To generate a diff of this commit: cvs rdiff -u -r1.190 -r1.191 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:34:59 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Split spec_open switch into three sections. The sections are now: 1. Acquire open reference. 1a (intermezzo). Set VV_ISTTY. 2. Drop the vnode lock to call .d_open and autoload modules if necessary. 3. Handle concurrent revoke if it happenend, or release open reference if .d_open failed. No functional change. Sprinkle comments about problems. To generate a diff of this commit: cvs rdiff -u -r1.189 -r1.190 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.189 src/sys/miscfs/specfs/spec_vnops.c:1.190 --- src/sys/miscfs/specfs/spec_vnops.c:1.189 Mon Mar 28 12:34:51 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:34:59 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.189 2022/03/28 12:34:51 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.190 2022/03/28 12:34:59 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.189 2022/03/28 12:34:51 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.190 2022/03/28 12:34:59 riastradh Exp $"); #include #include @@ -550,6 +550,20 @@ spec_open(void *v) if (error != 0) return (error); + /* + * Acquire an open reference -- as long as we hold onto it, and + * the vnode isn't revoked, it can't be closed. + * + * But first check whether it has been revoked -- if so, we + * can't acquire more open references and we must fail + * immediately with EBADF. + * + * XXX This races with revoke: once we release the vnode lock, + * the vnode may be revoked, and the .d_close callback run, at + * the same time as we're calling .d_open here. Drivers + * shouldn't have to contemplate this scenario; .d_open and + * .d_close should be prevented from running concurrently. + */ switch (vp->v_type) { case VCHR: /* @@ -564,8 +578,68 @@ spec_open(void *v) sd->sd_opencnt++; sn->sn_opencnt++; mutex_exit(&device_lock); + break; + case VBLK: + /* + * For block devices, permit only one open. The buffer + * cache cannot remain self-consistent with multiple + * vnodes holding a block device open. + * + * Treat zero opencnt with non-NULL mountpoint as open. + * This may happen after forced detach of a mounted device. + */ + mutex_enter(&device_lock); + if (sn->sn_gone) { + mutex_exit(&device_lock); + return (EBADF); + } + if (sd->sd_opencnt != 0 || sd->sd_mountpoint != NULL) { + mutex_exit(&device_lock); + return EBUSY; + } + sn->sn_opencnt = 1; + sd->sd_opencnt = 1; + sd->sd_bdevvp = vp; + mutex_exit(&device_lock); + break; + default: + panic("invalid specfs vnode type: %d", vp->v_type); + } + + /* + * Set VV_ISTTY if this is a tty cdev. + * + * XXX This does the wrong thing if the module has to be + * autoloaded. We should maybe set this after autoloading + * modules and calling .d_open successfully, except (a) we need + * the vnode lock to touch it, and (b) once we acquire the + * vnode lock again, the vnode may have been revoked, and + * deadfs's dead_read needs VV_ISTTY to be already set in order + * to return the right answer. So this needs some additional + * synchronization to be made to work correctly with tty driver + * module autoload. For now, let's just hope it doesn't cause + * too much trouble for a tty from an autoloaded driver module + * to fail with EIO instead of returning EOF. + */ + if (vp->v_type == VCHR) { if (cdev_type(dev) == D_TTY) vp->v_vflag |= VV_ISTTY; + } + + /* + * Open the device. If .d_open returns ENXIO (device not + * configured), the driver may not be loaded, so try + * autoloading a module and then try .d_open again if anything + * got loaded. + * + * Because opening the device may block indefinitely, e.g. when + * opening a tty, and loading a module may cross into many + * other subsystems, we must not hold the vnode lock while + * calling .d_open, so release it now and reacquire it when + * done. + */ + switch (vp->v_type) { + case VCHR: VOP_UNLOCK(vp); do { const struct cdevsw *cdev; @@ -594,27 +668,6 @@ spec_open(void *v) break; case VBLK: - /* - * For block devices, permit only one open. The buffer - * cache cannot remain self-consistent with multiple - * vnodes holding a block device open. - * - * Treat zero opencnt with non-NULL mountpoint as open. - * This may happen after forced detach of a mounted device. - */ - mutex_enter(&device_lock); - if (sn->sn_gone) { - mutex_exit(&device_lock); - return (EBADF); - } - if (sd->sd_opencnt != 0 || sd->sd_mountpoint != NULL) { - mutex_exit(&device_lock); -
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:34:59 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Split spec_open switch into three sections. The sections are now: 1. Acquire open reference. 1a (intermezzo). Set VV_ISTTY. 2. Drop the vnode lock to call .d_open and autoload modules if necessary. 3. Handle concurrent revoke if it happenend, or release open reference if .d_open failed. No functional change. Sprinkle comments about problems. To generate a diff of this commit: cvs rdiff -u -r1.189 -r1.190 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:34:51 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Factor common kauth check out of switch in spec_open. No functional change. To generate a diff of this commit: cvs rdiff -u -r1.188 -r1.189 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.188 src/sys/miscfs/specfs/spec_vnops.c:1.189 --- src/sys/miscfs/specfs/spec_vnops.c:1.188 Mon Mar 28 12:34:42 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:34:51 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.188 2022/03/28 12:34:42 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.189 2022/03/28 12:34:51 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.188 2022/03/28 12:34:42 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.189 2022/03/28 12:34:51 riastradh Exp $"); #include #include @@ -546,13 +546,12 @@ spec_open(void *v) req = KAUTH_REQ_DEVICE_RAWIO_SPEC_READ; break; } + error = kauth_authorize_device_spec(ap->a_cred, req, vp); + if (error != 0) + return (error); switch (vp->v_type) { case VCHR: - error = kauth_authorize_device_spec(ap->a_cred, req, vp); - if (error != 0) - return (error); - /* * Character devices can accept opens from multiple * vnodes. @@ -595,10 +594,6 @@ spec_open(void *v) break; case VBLK: - error = kauth_authorize_device_spec(ap->a_cred, req, vp); - if (error != 0) - return (error); - /* * For block devices, permit only one open. The buffer * cache cannot remain self-consistent with multiple
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:34:51 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Factor common kauth check out of switch in spec_open. No functional change. To generate a diff of this commit: cvs rdiff -u -r1.188 -r1.189 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:34:42 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Assert v_type is VBLK or VCHR in spec_open. Nothing else makes sense. Prune dead branches (and replace default case by panic). To generate a diff of this commit: cvs rdiff -u -r1.187 -r1.188 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.187 src/sys/miscfs/specfs/spec_vnops.c:1.188 --- src/sys/miscfs/specfs/spec_vnops.c:1.187 Mon Mar 28 12:34:34 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:34:42 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.187 2022/03/28 12:34:34 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.188 2022/03/28 12:34:42 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.187 2022/03/28 12:34:34 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.188 2022/03/28 12:34:42 riastradh Exp $"); #include #include @@ -525,7 +525,10 @@ spec_open(void *v) sd = sn->sn_dev; name = NULL; gen = 0; - + + KASSERTMSG(vp->v_type == VBLK || vp->v_type == VCHR, "type=%d", + vp->v_type); + /* * Don't allow open if fs is mounted -nodev. */ @@ -644,15 +647,8 @@ spec_open(void *v) break; - case VNON: - case VLNK: - case VDIR: - case VREG: - case VBAD: - case VFIFO: - case VSOCK: default: - return 0; + panic("invalid specfs vnode type: %d", vp->v_type); } mutex_enter(&device_lock);
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:34:42 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Assert v_type is VBLK or VCHR in spec_open. Nothing else makes sense. Prune dead branches (and replace default case by panic). To generate a diff of this commit: cvs rdiff -u -r1.187 -r1.188 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:34:34 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Call bdev_open without the vnode lock. There is no need for it to serialize opens, because they are already serialized by sd_opencnt which for block devices is always either 0 or 1. There's not obviously any other reason why the vnode lock should be held across bdev_open, other than that it might be nice to avoid dropping it if not necessary. For character devices we always have to drop the vnode lock because open might hang indefinitely, when opening a tty, which is not allowed while holding the vnode lock. To generate a diff of this commit: cvs rdiff -u -r1.186 -r1.187 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.186 src/sys/miscfs/specfs/spec_vnops.c:1.187 --- src/sys/miscfs/specfs/spec_vnops.c:1.186 Mon Mar 28 12:34:25 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:34:34 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.186 2022/03/28 12:34:25 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.187 2022/03/28 12:34:34 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.186 2022/03/28 12:34:25 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.187 2022/03/28 12:34:34 riastradh Exp $"); #include #include @@ -617,6 +617,7 @@ spec_open(void *v) sd->sd_opencnt = 1; sd->sd_bdevvp = vp; mutex_exit(&device_lock); + VOP_UNLOCK(vp); do { const struct bdevsw *bdev; @@ -636,13 +637,10 @@ spec_open(void *v) if ((name = bdevsw_getname(major(dev))) == NULL) break; - VOP_UNLOCK(vp); - /* Try to autoload device module */ (void) module_autoload(name, MODULE_CLASS_DRIVER); - - vn_lock(vp, LK_EXCLUSIVE | LK_RETRY); } while (gen != module_gen); + vn_lock(vp, LK_EXCLUSIVE | LK_RETRY); break;
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:34:34 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Call bdev_open without the vnode lock. There is no need for it to serialize opens, because they are already serialized by sd_opencnt which for block devices is always either 0 or 1. There's not obviously any other reason why the vnode lock should be held across bdev_open, other than that it might be nice to avoid dropping it if not necessary. For character devices we always have to drop the vnode lock because open might hang indefinitely, when opening a tty, which is not allowed while holding the vnode lock. To generate a diff of this commit: cvs rdiff -u -r1.186 -r1.187 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:34:26 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Note lock order for vnode lock, device_lock, v_interlock. To generate a diff of this commit: cvs rdiff -u -r1.185 -r1.186 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/miscfs/specfs/spec_vnops.c diff -u src/sys/miscfs/specfs/spec_vnops.c:1.185 src/sys/miscfs/specfs/spec_vnops.c:1.186 --- src/sys/miscfs/specfs/spec_vnops.c:1.185 Mon Mar 28 12:34:17 2022 +++ src/sys/miscfs/specfs/spec_vnops.c Mon Mar 28 12:34:25 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: spec_vnops.c,v 1.185 2022/03/28 12:34:17 riastradh Exp $ */ +/* $NetBSD: spec_vnops.c,v 1.186 2022/03/28 12:34:25 riastradh Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -58,7 +58,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.185 2022/03/28 12:34:17 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.186 2022/03/28 12:34:25 riastradh Exp $"); #include #include @@ -85,6 +85,14 @@ __KERNEL_RCSID(0, "$NetBSD: spec_vnops.c #include #include +/* + * Lock order: + * + * vnode lock + * -> device_lock + * -> struct vnode::v_interlock + */ + /* symbolic sleep message strings for devices */ const char devopn[] = "devopn"; const char devio[] = "devio";
CVS commit: src/sys/miscfs/specfs
Module Name:src Committed By: riastradh Date: Mon Mar 28 12:34:26 UTC 2022 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: specfs: Note lock order for vnode lock, device_lock, v_interlock. To generate a diff of this commit: cvs rdiff -u -r1.185 -r1.186 src/sys/miscfs/specfs/spec_vnops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
re: CVS commit: src/sys/miscfs/specfs
On Thu, 8 Sep 2016, matthew green wrote: "Paul Goyette" writes: Module Name:src Committed By: pgoyette Date: Thu Sep 8 00:07:48 UTC 2016 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: if_config processing wants to auto-load modules named with an if_ prefix, while specfc wants to auto-load modules without the prefix. For modules which can be loaded both ways (ie, if_tap and if_tun), provide a simple conversion table for specfs so it can auto-load the if_ module. This table should always be quite small, and the auto-load operation is relatively infrequent, so the additional overhead of comparing names should be tolerable. would you mind reverting this and implementing the "dependant" module model mlelstv proposed? the above is a hack and doesn't scale or work if a new module with the same "problem" is introduced, as it requires the base kernel to be patched, where as a pair of modules can be added much more easily. Sure, I can do that. Point well taken re requiring base kernel mods to add new table entries. It will take me a couple of days to complete and test. I've got a lot of issues dealing with my new machine... +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
re: CVS commit: src/sys/miscfs/specfs
"Paul Goyette" writes: > Module Name: src > Committed By: pgoyette > Date: Thu Sep 8 00:07:48 UTC 2016 > > Modified Files: > src/sys/miscfs/specfs: spec_vnops.c > > Log Message: > if_config processing wants to auto-load modules named with an if_ prefix, > while specfc wants to auto-load modules without the prefix. For modules > which can be loaded both ways (ie, if_tap and if_tun), provide a simple > conversion table for specfs so it can auto-load the if_ module. > > This table should always be quite small, and the auto-load operation is > relatively infrequent, so the additional overhead of comparing names should > be tolerable. would you mind reverting this and implementing the "dependant" module model mlelstv proposed? the above is a hack and doesn't scale or work if a new module with the same "problem" is introduced, as it requires the base kernel to be patched, where as a pair of modules can be added much more easily. thanks. .mrg.
Re: CVS commit: src/sys/miscfs/specfs
On Wed, 23 Dec 2015, Christos Zoulas wrote: In article <2015135437.b205bf...@cvs.netbsd.org>, Paul Goyette wrote: -=-=-=-=-=- Module Name:src Committed By: pgoyette Date: Tue Dec 22 23:54:37 UTC 2015 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: If we attempt to autoload a driver module, make sure we return an error if it fails. Otherwise we might end up calling a builtin-but-disabled driver module and that can generate all sorts of issues... Is there a reason to override the error? No. And since the error is already being reported, the change was unneeded. The bug I was following is elsewhere, so this change was backed out. +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/miscfs/specfs
In article <2015135437.b205bf...@cvs.netbsd.org>, Paul Goyette wrote: >-=-=-=-=-=- > >Module Name: src >Committed By: pgoyette >Date: Tue Dec 22 23:54:37 UTC 2015 > >Modified Files: > src/sys/miscfs/specfs: spec_vnops.c > >Log Message: >If we attempt to autoload a driver module, make sure we return an error >if it fails. Otherwise we might end up calling a builtin-but-disabled >driver module and that can generate all sorts of issues... Is there a reason to override the error? christos
Re: CVS commit: src/sys/miscfs/specfs
On 29 Jun 2015, at 18:47, David Holland wrote: > On Mon, Jun 29, 2015 at 12:25:49PM -0400, Christos Zoulas wrote: >> Module Name: src >> Committed By:christos >> Date:Mon Jun 29 16:25:49 UTC 2015 >> >> Modified Files: >> src/sys/miscfs/specfs: spec_vnops.c >> >> Log Message: >> Revert previous, and explain why. > > Hrm, shouldn't it be &vp? > > (as it is, it's using the uvm_object lock pointer as the key) Yes, just changed the key to the address of vp->v_specdev which is invariant over the lifetime of the vnode. Coverity is quite cool ... -- J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)
Re: CVS commit: src/sys/miscfs/specfs
On Mon, Jun 29, 2015 at 12:25:49PM -0400, Christos Zoulas wrote: > Module Name: src > Committed By:christos > Date:Mon Jun 29 16:25:49 UTC 2015 > > Modified Files: > src/sys/miscfs/specfs: spec_vnops.c > > Log Message: > Revert previous, and explain why. Hrm, shouldn't it be &vp? (as it is, it's using the uvm_object lock pointer as the key) -- David A. Holland dholl...@netbsd.org
Re: CVS commit: src/sys/miscfs/specfs
On Tue, Apr 13, 2010 at 10:47 PM, Antti Kantee wrote: > On Tue Apr 13 2010 at 13:25:24 +, Andrew Doran wrote: >> So the result of our teeth-pulling so far is: >> >> 1. You are concerned about namespace conflicts. I am too. >> 2. I would be happy to see these managed through documentation and >> a straightforward approval process for adding modules. > > There is an additional pitfall with this. We don't control all modules > and cannot be sure potential 3rd parties use the same processes. > >> 3. You suggest that it would be better for the computer to manage it. > > Maybe not manage it, but at least detect it and fail gracefully. > >> Can you suggest an alternative mechanism that will (a) allow us to >> autoload things that are not anointed device drivers and (b) satisfy >> your concerns? > > devfs ;) > > IOW, Given that we don't know where we most likely are in 6 months, > I'm not too keen on trying to spend energy to solve this right now ... > >> As an example of something that wants autoloading both as a file >> system and a driver, see ZFS. > > ... if zfs is the only use case, since IIUC it's broken anyway. > > Meanwhile, if there is something else which is catastrophically broken > due to lack of holy christening, we can revisit this and see if just > flipping the switch and maybe writing a simple audit script to verify > no collisions is the path of least wailing (this would be close to "2"). I like module names to become longer and unambiguous by making them match the relative paths in src/sys, like "sys/ufs/ffs/ffs.kmod". I don't think of any reason why module (driver, filesystem, ...) names need to be very short. Masao
Re: CVS commit: src/sys/miscfs/specfs
On Tue Apr 13 2010 at 13:25:24 +, Andrew Doran wrote: > So the result of our teeth-pulling so far is: > > 1. You are concerned about namespace conflicts. I am too. > 2. I would be happy to see these managed through documentation and >a straightforward approval process for adding modules. There is an additional pitfall with this. We don't control all modules and cannot be sure potential 3rd parties use the same processes. > 3. You suggest that it would be better for the computer to manage it. Maybe not manage it, but at least detect it and fail gracefully. > Can you suggest an alternative mechanism that will (a) allow us to > autoload things that are not anointed device drivers and (b) satisfy > your concerns? devfs ;) IOW, Given that we don't know where we most likely are in 6 months, I'm not too keen on trying to spend energy to solve this right now ... > As an example of something that wants autoloading both as a file > system and a driver, see ZFS. ... if zfs is the only use case, since IIUC it's broken anyway. Meanwhile, if there is something else which is catastrophically broken due to lack of holy christening, we can revisit this and see if just flipping the switch and maybe writing a simple audit script to verify no collisions is the path of least wailing (this would be close to "2").
Re: CVS commit: src/sys/miscfs/specfs
On Tue, Apr 13, 2010 at 03:40:24PM +0300, Antti Kantee wrote: > On Tue Apr 13 2010 at 12:32:38 +, Andrew Doran wrote: > > So the kernel of the problem is namespace collisions, correct? > > Mostly. Though I still think it's not expected that opening a /dev > node will load e.g. an exec package or secmodel even if that's what some > programmer wants. > > > Would you agree that's it's the kernel programmers responsibility > > to avoid conflicts? > > > > If so how about sprinkling a little process in order to make it harder to > > screw up? > > I'd say computers do conflict detection better. So the result of our teeth-pulling so far is: 1. You are concerned about namespace conflicts. I am too. 2. I would be happy to see these managed through documentation and a straightforward approval process for adding modules. 3. You suggest that it would be better for the computer to manage it. Can you suggest an alternative mechanism that will (a) allow us to autoload things that are not anointed device drivers and (b) satisfy your concerns? As an example of something that wants autoloading both as a file system and a driver, see ZFS.
Re: CVS commit: src/sys/miscfs/specfs
On Tue Apr 13 2010 at 12:32:38 +, Andrew Doran wrote: > So the kernel of the problem is namespace collisions, correct? Mostly. Though I still think it's not expected that opening a /dev node will load e.g. an exec package or secmodel even if that's what some programmer wants. > Would you agree that's it's the kernel programmers responsibility > to avoid conflicts? > > If so how about sprinkling a little process in order to make it harder to > screw up? I'd say computers do conflict detection better.
Re: CVS commit: src/sys/miscfs/specfs
On Tue, Apr 13, 2010 at 03:27:11PM +0300, Antti Kantee wrote: > On Tue Apr 13 2010 at 12:18:38 +, Andrew Doran wrote: > > On Tue, Apr 13, 2010 at 10:47:51AM +0300, Antti Kantee wrote: > > > On Tue Apr 13 2010 at 01:15:56 +, Adam Hoka wrote: > > > > Module Name:src > > > > Committed By: ahoka > > > > Date: Tue Apr 13 01:15:56 UTC 2010 > > > > > > > > Modified Files: > > > > src/sys/miscfs/specfs: spec_vnops.c > > > > > > > > Log Message: > > > > Autoload modules with any class. > > > > > > > > This fixes autoloading of pf, zfs and possibly others. > > > > > > What is wrong with making drivers MODULE_CLASS_DRIVER? I think we > > > should limit autoload aiming for safety, not splatter it all over. > > > > > > Eventually everything will of course be fixed by, *tadaa*, devfs. > > > Meanwhile, please back this out and if you don't think calling a driver > > > a driver is the right thing to do, have a discussion. > > > > What's the problem with autoloading any class? Is it just a cosmetic thing? > > I already mentioned my opinion above: "aim for safety". Since there > is currently no common place to manage the different names we have, > I don't want to discover some day that e.g. some secmodel name matches > a devsw name. So the kernel of the problem is namespace collisions, correct? Would you agree that's it's the kernel programmers responsibility to avoid conflicts? If so how about sprinkling a little process in order to make it harder to screw up?
Re: CVS commit: src/sys/miscfs/specfs
On Tue Apr 13 2010 at 12:18:38 +, Andrew Doran wrote: > On Tue, Apr 13, 2010 at 10:47:51AM +0300, Antti Kantee wrote: > > On Tue Apr 13 2010 at 01:15:56 +, Adam Hoka wrote: > > > Module Name: src > > > Committed By: ahoka > > > Date: Tue Apr 13 01:15:56 UTC 2010 > > > > > > Modified Files: > > > src/sys/miscfs/specfs: spec_vnops.c > > > > > > Log Message: > > > Autoload modules with any class. > > > > > > This fixes autoloading of pf, zfs and possibly others. > > > > What is wrong with making drivers MODULE_CLASS_DRIVER? I think we > > should limit autoload aiming for safety, not splatter it all over. > > > > Eventually everything will of course be fixed by, *tadaa*, devfs. > > Meanwhile, please back this out and if you don't think calling a driver > > a driver is the right thing to do, have a discussion. > > What's the problem with autoloading any class? Is it just a cosmetic thing? I already mentioned my opinion above: "aim for safety". Since there is currently no common place to manage the different names we have, I don't want to discover some day that e.g. some secmodel name matches a devsw name.
Re: CVS commit: src/sys/miscfs/specfs
On Tue, Apr 13, 2010 at 10:47:51AM +0300, Antti Kantee wrote: > On Tue Apr 13 2010 at 01:15:56 +, Adam Hoka wrote: > > Module Name:src > > Committed By: ahoka > > Date: Tue Apr 13 01:15:56 UTC 2010 > > > > Modified Files: > > src/sys/miscfs/specfs: spec_vnops.c > > > > Log Message: > > Autoload modules with any class. > > > > This fixes autoloading of pf, zfs and possibly others. > > What is wrong with making drivers MODULE_CLASS_DRIVER? I think we > should limit autoload aiming for safety, not splatter it all over. > > Eventually everything will of course be fixed by, *tadaa*, devfs. > Meanwhile, please back this out and if you don't think calling a driver > a driver is the right thing to do, have a discussion. What's the problem with autoloading any class? Is it just a cosmetic thing? One doesn't have to be an anointed driver to have an entry in /dev. (It implies that the modules and devsw entries inhabit the same namespace.)
Re: CVS commit: src/sys/miscfs/specfs
On Tue Apr 13 2010 at 01:15:56 +, Adam Hoka wrote: > Module Name: src > Committed By: ahoka > Date: Tue Apr 13 01:15:56 UTC 2010 > > Modified Files: > src/sys/miscfs/specfs: spec_vnops.c > > Log Message: > Autoload modules with any class. > > This fixes autoloading of pf, zfs and possibly others. What is wrong with making drivers MODULE_CLASS_DRIVER? I think we should limit autoload aiming for safety, not splatter it all over. Eventually everything will of course be fixed by, *tadaa*, devfs. Meanwhile, please back this out and if you don't think calling a driver a driver is the right thing to do, have a discussion.