kern/51785: Accessing speaker device does not prevent its removal

[paul%whooppee.com@localhost]

>Number:         51785
>Category:       kern
>Synopsis:       Accessing speaker device does not prevent its removal
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Jan 06 03:25:00 +0000 2017
>Originator:     Paul Goyette
>Release:        NetBSD 7.99.53
>Organization:
+------------------+--------------------------+------------------------+
| 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 |
+------------------+--------------------------+------------------------+
>Environment:
	
	
System: NetBSD speedy.whooppee.com 7.99.53 NetBSD 7.99.53 (SPEEDY 2016-12-31 23:00:24) #1: Sun Jan 1 01:39:34 UTC 2017 paul%speedy.whooppee.com@localhost:/build/netbsd-local/obj/amd64/sys/arch/amd64/compile/SPEEDY amd64
Architecture: x86_64
Machine: amd64
>Description:
I started to write a MODULE_CMD_FINI routine for the spkr module.  It
seems that if the spkr device is busy playing something, it is still
possible to call config_fini_component() for the driver with a success
return code.  As a result, the driver assumes that it is safe to be
unloaded.

After the current play-string finishes, I/O completion code returns to
memory that no longer contains the driver, resulting in a panic.
	
>How-To-Repeat:
1. Build a kernel that does NOT have the spkr driver built-in, and
   with the following patch (with appropriate substitution of tab
   characters for white-space):

diff -u -p -r1.5 spkr.c
--- spkr.c      15 Dec 2016 06:55:55 -0000      1.5
+++ spkr.c      6 Jan 2017 03:11:05 -0000
@@ -493,20 +493,21 @@ spkr_modcmd(modcmd_t cmd, void *arg)
                        break;
 
                error = config_init_component(cfdriver_ioconf_spkr,
-                       cfattach_ioconf_spkr, cfdata_ioconf_spkr);
+                   cfattach_ioconf_spkr, cfdata_ioconf_spkr);
                if (error) {
                        devsw_detach(NULL, &spkr_cdevsw);
                }
                break;
 
        case MODULE_CMD_FINI:
-               return EBUSY;
-#ifdef notyet
-               error = config_fini_component(cfdriver_ioconf_spkr,
-                       cfattach_ioconf_spkr, cfdata_ioconf_spkr);
                devsw_detach(NULL, &spkr_cdevsw);
+               error = config_fini_component(cfdriver_ioconf_spkr,
+                   cfattach_ioconf_spkr, cfdata_ioconf_spkr);
+               if (error)
+                       devsw_attach(spkr_cd.cd_name, NULL, &bmajor,
+                           &spkr_cdevsw, &cmajor);
                break;
-#endif
+
        default:
                error = ENOTTY;
                break;

2. play a string to the /dev/speaker in the background:

	echo T60L1CP1CP1C > /dev/speaker &

3. while it is still playing, modunload spkr
	
>Fix:
Some sort of interlock would normally be held while the device is open
(the "echo" command above does not complete until the playstring is
finished).  This interlock would result in a failure to detach the
driver's data structures;  ie, you would get a failure from one or more
of config_{cfdata,cfattach,cfdriver}_detach().  This would cause the
call to config_fini_component() to fail, preventing the module from
being unloaded.
	

>Unformatted: