NetBSD Problem Report #51785

From paul@whooppee.com  Fri Jan  6 03:21:53 2017
Return-Path: <paul@whooppee.com>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 39D437A21A
	for <gnats-bugs@gnats.NetBSD.org>; Fri,  6 Jan 2017 03:21:53 +0000 (UTC)
Message-Id: <20170106032150.534E816E62@speedy.whooppee.com>
Date: Fri,  6 Jan 2017 11:21:50 +0800 (PHT)
From: paul@whooppee.com
Reply-To: paul@whooppee.com
To: gnats-bugs@NetBSD.org
Subject: Accessing speaker device does not prevent its removal
X-Send-Pr-Version: 3.95

>Number:         51785
>Category:       kern
>Synopsis:       Accessing speaker device does not prevent its removal
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    pgoyette
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Jan 06 03:25:00 +0000 2017
>Closed-Date:    Fri Jan 06 09:33:09 +0000 2017
>Last-Modified:  Fri Jan 06 09:35:01 +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:/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.


>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: kern-bug-people->pgoyette
Responsible-Changed-By: pgoyette@NetBSD.org
Responsible-Changed-When: Fri, 06 Jan 2017 09:33:09 +0000
Responsible-Changed-Why:
It's mine.


State-Changed-From-To: open->closed
State-Changed-By: pgoyette@NetBSD.org
State-Changed-When: Fri, 06 Jan 2017 09:33:09 +0000
State-Changed-Why:
Fix committed.


From: "Paul Goyette" <pgoyette@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51785 CVS commit: src/sys/dev
Date: Fri, 6 Jan 2017 09:32:08 +0000

 Module Name:	src
 Committed By:	pgoyette
 Date:		Fri Jan  6 09:32:08 UTC 2017

 Modified Files:
 	src/sys/dev: spkr.c spkr_audio.c spkrvar.h
 	src/sys/dev/isa: spkr_pcppi.c

 Log Message:
 Implement a common spkr_detach() function and call it from the
 attachment-specific detach functions.  Returns EBUSY if the
 device instance is busy, based on whether or not a sc->sc_inbuf
 is allocated.  The buffer is malloc()d at spkropen time, and is
 free()d in spkrclose().

 Now we can actually implement the MODULE_CMD_FINI command and
 unload the driver at will.

 Addresses my PR kern/51785


 To generate a diff of this commit:
 cvs rdiff -u -r1.5 -r1.6 src/sys/dev/spkr.c src/sys/dev/spkrvar.h
 cvs rdiff -u -r1.2 -r1.3 src/sys/dev/spkr_audio.c
 cvs rdiff -u -r1.8 -r1.9 src/sys/dev/isa/spkr_pcppi.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

>Unformatted:

Home	PR Database Search