race condition between (specfs) device open and close

[matthew green <mrg%eterna.com.au@localhost>]

hi folks.


while trying to fix any midi/sequencer issues i've seen in the
last year, noticed that any attempt to call 'midiplay' on more
than one midi device at the same time, while not allowed, would
leave the sequencer device always busy.

i tracked this down to an ugly race condition inside the
specfs code.  the problem is that:

	t1		t2
	opens dev
	sd_opencnt++
			tries to open dev
			sd_opencnt++
	closes dev
	since sd_opencnt is != 1, does not call close
	sd_opencnt--
			gets EBUSY
		sd_opencnt--

in this case, the device close routine is never called, and in
at least the midi case, all future opens return EBUSY due to
there already being an active open.

i've spent some time reviewing the specfs code, and i think that
the below patch fixes these specific problems.  it certainly
fixes the problem i have with trying to open /dev/music
concurrently.  it may not be entirely correct or have problems
i've not seen with it, but atf works fine, as does the last
couple of days of random testing.

the main changes are:
	- introduce a "dying" flag to the specnode struture,
	  and use it appropriately
	- add a condvar between open/close and revoke, to
	  ensure any revoke succeeds properly.

i'd like to get this into netbsd 7, so any additional testing
against that branch or against -current would be greatly
appreciated.

thanks.


.mrg.


Index: miscfs/specfs/spec_vnops.c
===================================================================
RCS file: /cvsroot/src/sys/miscfs/specfs/spec_vnops.c,v
retrieving revision 1.145
diff -p -u -r1.145 spec_vnops.c
--- miscfs/specfs/spec_vnops.c	25 Jul 2014 08:20:53 -0000	1.145
+++ miscfs/specfs/spec_vnops.c	30 Dec 2014 08:37:57 -0000
@@ -160,6 +160,12 @@ const struct vnodeopv_entry_desc spec_vn
 const struct vnodeopv_desc spec_vnodeop_opv_desc =
 	{ &spec_vnodeop_p, spec_vnodeop_entries };
 
+/*
+ * This tracks concurrent-open/close with revoke(), and is attached to
+ * device_lock.
+ */
+static kcondvar_t specopenclose_cv;
+
 static kauth_listener_t rawio_listener;
 
 /* Returns true if vnode is /dev/mem or /dev/kmem. */
@@ -206,6 +212,7 @@ spec_init(void)
 
 	rawio_listener = kauth_listen_scope(KAUTH_SCOPE_DEVICE,
 	    rawio_listener_cb, NULL);
+	cv_init(&specopenclose_cv, "specopcl");
 }
 
 /*
@@ -265,6 +272,7 @@ spec_node_init(vnode_t *vp, dev_t rdev)
 	sn->sn_opencnt = 0;
 	sn->sn_rdev = rdev;
 	sn->sn_gone = false;
+	sn->sn_dying = false;
 	vp->v_specnode = sn;
 	vp->v_specnext = *vpp;
 	*vpp = vp;
@@ -397,7 +405,14 @@ spec_node_revoke(vnode_t *vp)
 	KASSERT(sn->sn_gone == false);
 
 	mutex_enter(&device_lock);
-	KASSERT(sn->sn_opencnt <= sd->sd_opencnt);
+	sn->sn_dying = true;
+	while (sn->sn_opencnt > sd->sd_opencnt) {
+		/*
+		 * Open is in progress, but not complete, must wait
+		 * for it to complete.
+		 */
+		cv_wait(&specopenclose_cv, &device_lock);
+	}
 	if (sn->sn_opencnt != 0) {
 		sd->sd_opencnt -= (sn->sn_opencnt - 1);
 		sn->sn_opencnt = 1;
@@ -540,11 +555,10 @@ spec_open(void *v)
 		 * vnodes.
 		 */
 		mutex_enter(&device_lock);
-		if (sn->sn_gone) {
+		if (sn->sn_gone || sn->sn_dying) {
 			mutex_exit(&device_lock);
 			return (EBADF);
 		}
-		sd->sd_opencnt++;
 		sn->sn_opencnt++;
 		mutex_exit(&device_lock);
 		if (cdev_type(dev) == D_TTY)
@@ -573,6 +587,9 @@ spec_open(void *v)
 			(void) module_autoload(name, MODULE_CLASS_DRIVER);
 		} while (gen != module_gen);
 
+		if (error == 0)
+			sd->sd_opencnt++;
+
 		vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
 		break;
 
@@ -587,7 +604,7 @@ spec_open(void *v)
 		 * vnodes holding a block device open.
 		 */
 		mutex_enter(&device_lock);
-		if (sn->sn_gone) {
+		if (sn->sn_gone || sn->sn_dying) {
 			mutex_exit(&device_lock);
 			return (EBADF);
 		}
@@ -596,7 +613,6 @@ spec_open(void *v)
 			return EBUSY;
 		}
 		sn->sn_opencnt = 1;
-		sd->sd_opencnt = 1;
 		sd->sd_bdevvp = vp;
 		mutex_exit(&device_lock);
 		do {
@@ -626,6 +642,9 @@ spec_open(void *v)
 			vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
 		} while (gen != module_gen);
 
+		if (!error)
+			sd->sd_opencnt = 1;
+
 		break;
 
 	case VNON:
@@ -640,16 +659,17 @@ spec_open(void *v)
 	}
 
 	mutex_enter(&device_lock);
-	if (sn->sn_gone) {
+	if (sn->sn_gone || sn->sn_dying) {
 		if (error == 0)
 			error = EBADF;
 	} else if (error != 0) {
-		sd->sd_opencnt--;
 		sn->sn_opencnt--;
 		if (vp->v_type == VBLK)
 			sd->sd_bdevvp = NULL;
 
 	}
+	if (sn->sn_dying)
+		cv_broadcast(&specopenclose_cv);
 	mutex_exit(&device_lock);
 
 	if (cdev_type(dev) != D_DISK || error != 0)
Index: miscfs/specfs/specdev.h
===================================================================
RCS file: /cvsroot/src/sys/miscfs/specfs/specdev.h,v
retrieving revision 1.43
diff -p -u -r1.43 specdev.h
--- miscfs/specfs/specdev.h	25 Jul 2014 08:19:19 -0000	1.43
+++ miscfs/specfs/specdev.h	30 Dec 2014 08:37:57 -0000
@@ -69,6 +69,7 @@ typedef struct specnode {
 	u_int		sn_opencnt;
 	dev_t		sn_rdev;
 	bool		sn_gone;
+	bool		sn_dying;
 } specnode_t;
 
 typedef struct specdev {