From 8415a1126b37b1d01edca5148434aee751e7e05c Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Wed, 2 Feb 2022 01:09:07 +0000
Subject: [PATCH 06/13] squash! specfs: Assert specnode is open before we
 bdev_ioctl.

- Don't assert vnode is open -- udf doesn't guarantee it (and maybe
  it's not important if we're unmounting, but maybe it is, TBD).

- Leave an essay about why this is broken and what needs to be done
  to fix it.
---
 sys/miscfs/specfs/spec_vnops.c | 43 +++++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/sys/miscfs/specfs/spec_vnops.c b/sys/miscfs/specfs/spec_vnops.c
index dfe22506e794..c57ec62f3bbf 100644
--- a/sys/miscfs/specfs/spec_vnops.c
+++ b/sys/miscfs/specfs/spec_vnops.c
@@ -499,6 +499,11 @@ spec_node_lookup_by_mount(struct mount *mp, vnode_t **vpp)
 
 /*
  * Get the file system mounted on this block device.
+ *
+ * XXX Caller should hold the vnode lock -- shared or exclusive -- so
+ * that this can't changed, and the vnode can't be revoked while we
+ * examine it.  But not all callers do, and they're scattered through a
+ * lot of file systems, so we can't assert this yet.
  */
 struct mount *
 spec_node_getmountedfs(vnode_t *devvp)
@@ -513,25 +518,51 @@ spec_node_getmountedfs(vnode_t *devvp)
 
 /*
  * Set the file system mounted on this block device.
+ *
+ * XXX Caller should hold the vnode lock exclusively so this can't be
+ * changed or assumed by spec_node_getmountedfs while we change it, and
+ * the vnode can't be revoked while we handle it.  But not all callers
+ * do, and they're scattered through a lot of file systems, so we can't
+ * assert this yet.  Instead, for now, we'll take an I/O reference so
+ * at least the ioctl doesn't race with revoke/detach.
+ *
+ * If you do change this to assert an exclusive vnode lock, you must
+ * also do vdead_check before trying bdev_ioctl, because the vnode may
+ * have been revoked by the time the caller locked it, and this is
+ * _not_ a vop -- calls to spec_node_setmountedfs don't go through
+ * v_op, so revoking the vnode doesn't prevent further calls.
+ *
+ * XXX Caller should additionally have the vnode open, at least if mp
+ * is nonnull, but I'm not sure all callers do that -- need to audit.
+ * Currently udf closes the vnode before clearing the mount.
  */
 void
 spec_node_setmountedfs(vnode_t *devvp, struct mount *mp)
 {
 	struct dkwedge_info dkw;
+	struct specnode *sn;
+	dev_t dev;
+	int error;
 
 	KASSERT(devvp->v_type == VBLK);
-	KASSERT(devvp->v_specnode->sn_dev->sd_mountpoint == NULL || mp == NULL);
-	KASSERT(devvp->v_specnode->sn_opencnt);
 
-	devvp->v_specnode->sn_dev->sd_mountpoint = mp;
-	if (mp == NULL)
+	error = spec_io_enter(devvp, &sn, &dev);
+	if (error)
 		return;
 
-	if (bdev_ioctl(devvp->v_rdev, DIOCGWEDGEINFO, &dkw, FREAD, curlwp) != 0)
-		return;
+	KASSERT(sn->sn_dev->sd_mountpoint == NULL || mp == NULL);
+	sn->sn_dev->sd_mountpoint = mp;
+	if (mp == NULL)
+		goto out;
+
+	error = bdev_ioctl(dev, DIOCGWEDGEINFO, &dkw, FREAD, curlwp);
+	if (error)
+		goto out;
 
 	strlcpy(mp->mnt_stat.f_mntfromlabel, dkw.dkw_wname,
 	    sizeof(mp->mnt_stat.f_mntfromlabel));
+
+out:	spec_io_exit(devvp, sn);
 }
 
 /*