Re: race condition between (specfs) device open and close

["J. Hannken-Illjes" <hannken%eis.cs.tu-bs.de@localhost>]

On 30 Dec 2014, at 09:57, matthew green <mrg%eterna.com.au@localhost> wrote:

> 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.
<snip>
> @@ -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;

Will this work if we revoke a device whose cdev_open() blocks,
a TTY waiting for carrier for example?

--
J. Hannken-Illjes - hannken%eis.cs.tu-bs.de@localhost - TU Braunschweig (Germany)