Diff below changes pms(4) to make sure only one thread can change the
state, thus generating I/O, of the underlying device at the same time.

It fixes a race easily reproducible when a machine having wsmoused(8)
and X running is resumed.  The problem is that the first program trying
to activate the mouse sleeps in pms_change_state() before changing the
state of the device.  At this moment, the second program also tries to
enable the device and enqueue more commands before going to sleep...

While setting the state before going to sleep would also prevent this
particular race, it won't prevent the (very unlikely) scenario where a
program tries to disable the device while another one tries to enable
it.

Other mouse devices parent of wsmouse(4) might be subject to similar
races, but pms(4) is special because it can be the parent of two
wsmouses pointing to the same softc!  So at this stage of the release
cycle I think it is safer to only fix this particular and problematic
device.

ok?

Index: pms.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/pckbc/pms.c,v
retrieving revision 1.52
diff -u -p -r1.52 pms.c
--- pms.c       12 Jul 2014 18:48:52 -0000      1.52
+++ pms.c       23 Jul 2014 11:00:40 -0000
@@ -26,6 +26,7 @@
 
 #include <sys/param.h>
 #include <sys/systm.h>
+#include <sys/rwlock.h>
 #include <sys/device.h>
 #include <sys/ioctl.h>
 #include <sys/malloc.h>
@@ -148,6 +149,8 @@ struct pms_softc {          /* driver status inf
 #define PMS_STATE_ENABLED      1
 #define PMS_STATE_SUSPENDED    2
 
+       struct rwlock sc_state_lock;
+
        int sc_dev_enable;
 #define PMS_DEV_IGNORE         0x00
 #define PMS_DEV_PRIMARY                0x01
@@ -762,8 +765,13 @@ int
 pms_enable(void *v)
 {
        struct pms_softc *sc = v;
+       int rv;
+
+       rw_enter_write(&sc->sc_state_lock);
+       rv = pms_change_state(sc, PMS_STATE_ENABLED, PMS_DEV_PRIMARY);
+       rw_exit_write(&sc->sc_state_lock);
 
-       return pms_change_state(sc, PMS_STATE_ENABLED, PMS_DEV_PRIMARY);
+       return (rv);
 }
 
 void
@@ -771,7 +779,9 @@ pms_disable(void *v)
 {
        struct pms_softc *sc = v;
 
+       rw_enter_write(&sc->sc_state_lock);
        pms_change_state(sc, PMS_STATE_DISABLED, PMS_DEV_PRIMARY);
+       rw_exit_write(&sc->sc_state_lock);
 }
 
 int
@@ -789,8 +799,13 @@ int
 pms_sec_enable(void *v)
 {
        struct pms_softc *sc = v;
+       int rv;
+
+       rw_enter_write(&sc->sc_state_lock);
+       rv = pms_change_state(sc, PMS_STATE_ENABLED, PMS_DEV_SECONDARY);
+       rw_exit_write(&sc->sc_state_lock);
 
-       return (pms_change_state(sc, PMS_STATE_ENABLED, PMS_DEV_SECONDARY));
+       return (rv);
 }
 
 void
@@ -798,7 +813,9 @@ pms_sec_disable(void *v)
 {
        struct pms_softc *sc = v;
 
+       rw_enter_write(&sc->sc_state_lock);
        pms_change_state(sc, PMS_STATE_DISABLED, PMS_DEV_SECONDARY);
+       rw_exit_write(&sc->sc_state_lock);
 }
 
 int

Reply via email to