NetBSD Problem Report #52533

From paul@whooppee.com  Sat Sep  9 05:09: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 "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 13D1E7A177
	for <gnats-bugs@gnats.NetBSD.org>; Sat,  9 Sep 2017 05:09:53 +0000 (UTC)
Message-Id: <20170909050916.7143116E41@speedy.whooppee.com>
Date: Sat,  9 Sep 2017 13:09:16 +0800 (+08)
From: paul@whooppee.com
Reply-To: paul@whooppee.com
To: gnats-bugs@NetBSD.org
Subject: swsensor module can panic the machine when unloaded
X-Send-Pr-Version: 3.95

>Number:         52533
>Category:       kern
>Synopsis:       swsensor module can panic when being unloaded
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    pgoyette
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Sep 09 05:10:00 +0000 2017
>Closed-Date:    Mon Oct 23 21:53:10 +0000 2017
>Last-Modified:  Mon Oct 23 21:53:10 +0000 2017
>Originator:     Paul Goyette
>Release:        NetBSD 8.99.2
>Organization:
+------------------+--------------------------+----------------------------+
| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:          |
| (Retired)        | FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com   |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org |
+------------------+--------------------------+----------------------------+
>Environment:


System: NetBSD speedy.whooppee.com 8.99.2 NetBSD 8.99.2 (SPEEDY 2017-08-30 21:55:55 UTC) #0: Thu Aug 31 03:05:47 UTC 2017 paul@speedy.whooppee.com:/build/netbsd-local/obj/amd64/sys/arch/amd64/compile/SPEEDY amd64
Architecture: x86_64
Machine: amd64
>Description:
	With recent changes to the sysmon_envsys event handling, the
	swsensor module can now easily panic the system.  This seems to
	be the result of a call to callout_halt() specifying a callout
	that has never been initialized.

	This currently affects the running of atf tests (which use the
	swsensor(4) device to exercise sysmon_envsys functionality.

>How-To-Repeat:
	Boot a GENERIC kernel, run 'modload swsensor', and then run
	'modunload swsensor'.  BANG!

>Fix:
	(As discussed with msaitoh@ and ozaki-r@ in private Email)

	I really think we need to rework the access-control for the
	callout!  I think we might need a 3-state variable instead of a
	single flag bit:

		SME_CALLOUT_INVALID = 0
		SME_CALLOUT_READY   = 1
		SME_CALLOUT_HALTED  = 2

	INVALID is the initial state, before the call to callout_init().
	After we call callout_destroy(), the state gets reset to INVALID.

	READY means that we have successfully called callout_init() and
	we are prepared to schedule events and to service them.  We
	should test for this state before calling _any_ of the other
	callout_*() routines, (except callout_destroy() - see below).

	HALTED means that there are no events scheduled for this callout,
	but the data structure is still initialized and needs to be
	cleaned up.  We need to set this state before we call
	callout_halt() (or callout_stop() unless we intend to schedule a
	new event).  We should confirm we are in this state before we call
	callout_destroy().

	All accesses to the callout state (both read and write) need to
	be protected by the sme->sme_mtx mutex.


>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: kern-bug-people->pgoyette
Responsible-Changed-By: pgoyette@NetBSD.org
Responsible-Changed-When: Mon, 11 Sep 2017 06:03:53 +0000
Responsible-Changed-Why:
I'm fixing it


State-Changed-From-To: open->needs-pullups
State-Changed-By: pgoyette@NetBSD.org
State-Changed-When: Mon, 11 Sep 2017 06:03:53 +0000
State-Changed-Why:
Fix committed, needs pullups to 8 (and possibly one small part to 7)


From: "Paul Goyette" <pgoyette@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52533 CVS commit: src/sys/dev/sysmon
Date: Mon, 11 Sep 2017 06:02:09 +0000

 Module Name:	src
 Committed By:	pgoyette
 Date:		Mon Sep 11 06:02:09 UTC 2017

 Modified Files:
 	src/sys/dev/sysmon: sysmon_envsys.c sysmon_envsys_events.c sysmonvar.h

 Log Message:
 Improve tracking of the state of an event's callout, and protect all
 queries or modifications of the state with the sme_mtx mutex.

 Detach the rndsrc before re-attaching it (with potentially new values).

 Clean up some lock-ordering issues.

 And a couple of KNF issues for good measure!

 Should address PR kern/52533

 XXX Pullup-8 along with the previous fixes from msaitoh@
 XXX http://mail-index.netbsd.org/source-changes/2017/09/06/msg088028.html
 ~
 ~


 To generate a diff of this commit:
 cvs rdiff -u -r1.140 -r1.141 src/sys/dev/sysmon/sysmon_envsys.c
 cvs rdiff -u -r1.120 -r1.121 src/sys/dev/sysmon/sysmon_envsys_events.c
 cvs rdiff -u -r1.49 -r1.50 src/sys/dev/sysmon/sysmonvar.h

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

State-Changed-From-To: needs-pullups->pending-pullups
State-Changed-By: pgoyette@NetBSD.org
State-Changed-When: Mon, 11 Sep 2017 06:21:26 +0000
State-Changed-Why:
Pull-up 8 ticket #281, pull-up 7 ticket #1511


From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52533 CVS commit: [netbsd-8] src/sys/dev/sysmon
Date: Sat, 23 Sep 2017 17:22:49 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Sat Sep 23 17:22:49 UTC 2017

 Modified Files:
 	src/sys/dev/sysmon [netbsd-8]: sysmon_envsys.c sysmon_envsys_events.c
 	    sysmonvar.h

 Log Message:
 Pull up following revision(s) (requested by pgoyette in ticket #281):
 	sys/dev/sysmon/sysmon_envsys.c: 1.140-1.141
 	sys/dev/sysmon/sysmon_envsys_events.c: 1.120-1.121
 	sys/dev/sysmon/sysmonvar.h: 1.50
 Fixes a problem that some driver(e.g. acpitz(4) or coretemp(5)) which
 use sysmon_envsys sleep waiting at "rndsrc" when "drvctl -d".
 Don't call rnd_detach_source() in sme_remove_event() which is called
 from sme_event_unregister_all(). Instead, call rnd_detach_source() in
 sysmon_envsys_sensor_detach() and call sysmon_envsys_sensor_detach()
 before sme_event_unregister_sensor(). Each sensor(envsys_data) has each
 rnd_src, but some sme_events point to the same rnd_src in a sensor.
 Calling rnd_detach_souce() twice with the same rnd_src brokes a reference
 count in rnd_src. OK'd by pgoyette@.
 --
 Improve tracking of the state of an event's callout, and protect all
 queries or modifications of the state with the sme_mtx mutex.
 Detach the rndsrc before re-attaching it (with potentially new values).
 Clean up some lock-ordering issues.
 And a couple of KNF issues for good measure!
 Should address PR kern/52533


 To generate a diff of this commit:
 cvs rdiff -u -r1.139 -r1.139.10.1 src/sys/dev/sysmon/sysmon_envsys.c
 cvs rdiff -u -r1.119 -r1.119.2.1 src/sys/dev/sysmon/sysmon_envsys_events.c
 cvs rdiff -u -r1.49 -r1.49.10.1 src/sys/dev/sysmon/sysmonvar.h

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

State-Changed-From-To: pending-pullups->closed
State-Changed-By: pgoyette@NetBSD.org
State-Changed-When: Mon, 23 Oct 2017 21:53:10 +0000
State-Changed-Why:
pullup 7-1511 completed


>Unformatted:

Home	PR Database Search