From 29202881817966622313abba815917e8162058ed Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Wed, 19 Jan 2022 12:10:55 +0000
Subject: [PATCH 22/37] 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.
---
 sys/miscfs/specfs/spec_vnops.c | 43 ++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/sys/miscfs/specfs/spec_vnops.c b/sys/miscfs/specfs/spec_vnops.c
index 36430acb99a9..565827b2236a 100644
--- a/sys/miscfs/specfs/spec_vnops.c
+++ b/sys/miscfs/specfs/spec_vnops.c
@@ -351,6 +351,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)
@@ -365,23 +370,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);
 }
 
 /*