Commit 66d2a5952eab875f1286e04f738ef029afdaf013

Authored by Dmitry Torokhov
1 parent 6ee88d713f

Input: keyboard - fix lack of locking when traversing handler->h_list

Keyboard handler should not attempt to traverse handler->h_list on
its own, without any locking, otherwise it races with registering
and unregistering of input handles which leads to crashes.

Introduce input_handler_for_each_handle() helper that allows safely
iterate over all handles attached to a particular handler and switch
keyboard handler to use it.

Reported-by: Jim Paradis <jparadis@redhat.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>

Showing 3 changed files with 148 additions and 101 deletions Side-by-side Diff

drivers/char/keyboard.c
... ... @@ -46,8 +46,6 @@
46 46  
47 47 extern void ctrl_alt_del(void);
48 48  
49   -#define to_handle_h(n) container_of(n, struct input_handle, h_node)
50   -
51 49 /*
52 50 * Exported functions/variables
53 51 */
54 52  
55 53  
56 54  
57 55  
58 56  
59 57  
60 58  
61 59  
62 60  
63 61  
64 62  
65 63  
66 64  
67 65  
68 66  
... ... @@ -191,78 +189,85 @@
191 189 * etc.). So this means that scancodes for the extra function keys won't
192 190 * be valid for the first event device, but will be for the second.
193 191 */
  192 +
  193 +struct getset_keycode_data {
  194 + unsigned int scancode;
  195 + unsigned int keycode;
  196 + int error;
  197 +};
  198 +
  199 +static int getkeycode_helper(struct input_handle *handle, void *data)
  200 +{
  201 + struct getset_keycode_data *d = data;
  202 +
  203 + d->error = input_get_keycode(handle->dev, d->scancode, &d->keycode);
  204 +
  205 + return d->error == 0; /* stop as soon as we successfully get one */
  206 +}
  207 +
194 208 int getkeycode(unsigned int scancode)
195 209 {
196   - struct input_handle *handle;
197   - int keycode;
198   - int error = -ENODEV;
  210 + struct getset_keycode_data d = { scancode, 0, -ENODEV };
199 211  
200   - list_for_each_entry(handle, &kbd_handler.h_list, h_node) {
201   - error = input_get_keycode(handle->dev, scancode, &keycode);
202   - if (!error)
203   - return keycode;
204   - }
  212 + input_handler_for_each_handle(&kbd_handler, &d, getkeycode_helper);
205 213  
206   - return error;
  214 + return d.error ?: d.keycode;
207 215 }
208 216  
  217 +static int setkeycode_helper(struct input_handle *handle, void *data)
  218 +{
  219 + struct getset_keycode_data *d = data;
  220 +
  221 + d->error = input_set_keycode(handle->dev, d->scancode, d->keycode);
  222 +
  223 + return d->error == 0; /* stop as soon as we successfully set one */
  224 +}
  225 +
209 226 int setkeycode(unsigned int scancode, unsigned int keycode)
210 227 {
211   - struct input_handle *handle;
212   - int error = -ENODEV;
  228 + struct getset_keycode_data d = { scancode, keycode, -ENODEV };
213 229  
214   - list_for_each_entry(handle, &kbd_handler.h_list, h_node) {
215   - error = input_set_keycode(handle->dev, scancode, keycode);
216   - if (!error)
217   - break;
218   - }
  230 + input_handler_for_each_handle(&kbd_handler, &d, setkeycode_helper);
219 231  
220   - return error;
  232 + return d.error;
221 233 }
222 234  
223 235 /*
224 236 * Making beeps and bells.
225 237 */
226   -static void kd_nosound(unsigned long ignored)
  238 +
  239 +static int kd_sound_helper(struct input_handle *handle, void *data)
227 240 {
228   - struct input_handle *handle;
  241 + unsigned int *hz = data;
  242 + struct input_dev *dev = handle->dev;
229 243  
230   - list_for_each_entry(handle, &kbd_handler.h_list, h_node) {
231   - if (test_bit(EV_SND, handle->dev->evbit)) {
232   - if (test_bit(SND_TONE, handle->dev->sndbit))
233   - input_inject_event(handle, EV_SND, SND_TONE, 0);
234   - if (test_bit(SND_BELL, handle->dev->sndbit))
235   - input_inject_event(handle, EV_SND, SND_BELL, 0);
236   - }
  244 + if (test_bit(EV_SND, dev->evbit)) {
  245 + if (test_bit(SND_TONE, dev->sndbit))
  246 + input_inject_event(handle, EV_SND, SND_TONE, *hz);
  247 + if (test_bit(SND_BELL, handle->dev->sndbit))
  248 + input_inject_event(handle, EV_SND, SND_BELL, *hz ? 1 : 0);
237 249 }
  250 +
  251 + return 0;
238 252 }
239 253  
  254 +static void kd_nosound(unsigned long ignored)
  255 +{
  256 + static unsigned int zero;
  257 +
  258 + input_handler_for_each_handle(&kbd_handler, &zero, kd_sound_helper);
  259 +}
  260 +
240 261 static DEFINE_TIMER(kd_mksound_timer, kd_nosound, 0, 0);
241 262  
242 263 void kd_mksound(unsigned int hz, unsigned int ticks)
243 264 {
244   - struct list_head *node;
  265 + del_timer_sync(&kd_mksound_timer);
245 266  
246   - del_timer(&kd_mksound_timer);
  267 + input_handler_for_each_handle(&kbd_handler, &hz, kd_sound_helper);
247 268  
248   - if (hz) {
249   - list_for_each_prev(node, &kbd_handler.h_list) {
250   - struct input_handle *handle = to_handle_h(node);
251   - if (test_bit(EV_SND, handle->dev->evbit)) {
252   - if (test_bit(SND_TONE, handle->dev->sndbit)) {
253   - input_inject_event(handle, EV_SND, SND_TONE, hz);
254   - break;
255   - }
256   - if (test_bit(SND_BELL, handle->dev->sndbit)) {
257   - input_inject_event(handle, EV_SND, SND_BELL, 1);
258   - break;
259   - }
260   - }
261   - }
262   - if (ticks)
263   - mod_timer(&kd_mksound_timer, jiffies + ticks);
264   - } else
265   - kd_nosound(0);
  269 + if (hz && ticks)
  270 + mod_timer(&kd_mksound_timer, jiffies + ticks);
266 271 }
267 272 EXPORT_SYMBOL(kd_mksound);
268 273  
269 274  
270 275  
271 276  
272 277  
273 278  
... ... @@ -270,30 +275,37 @@
270 275 * Setting the keyboard rate.
271 276 */
272 277  
273   -int kbd_rate(struct kbd_repeat *rep)
  278 +static int kbd_rate_helper(struct input_handle *handle, void *data)
274 279 {
275   - struct list_head *node;
276   - unsigned int d = 0;
277   - unsigned int p = 0;
  280 + struct input_dev *dev = handle->dev;
  281 + struct kbd_repeat *rep = data;
278 282  
279   - list_for_each(node, &kbd_handler.h_list) {
280   - struct input_handle *handle = to_handle_h(node);
281   - struct input_dev *dev = handle->dev;
  283 + if (test_bit(EV_REP, dev->evbit)) {
282 284  
283   - if (test_bit(EV_REP, dev->evbit)) {
284   - if (rep->delay > 0)
285   - input_inject_event(handle, EV_REP, REP_DELAY, rep->delay);
286   - if (rep->period > 0)
287   - input_inject_event(handle, EV_REP, REP_PERIOD, rep->period);
288   - d = dev->rep[REP_DELAY];
289   - p = dev->rep[REP_PERIOD];
290   - }
  285 + if (rep[0].delay > 0)
  286 + input_inject_event(handle,
  287 + EV_REP, REP_DELAY, rep[0].delay);
  288 + if (rep[0].period > 0)
  289 + input_inject_event(handle,
  290 + EV_REP, REP_PERIOD, rep[0].period);
  291 +
  292 + rep[1].delay = dev->rep[REP_DELAY];
  293 + rep[1].period = dev->rep[REP_PERIOD];
291 294 }
292   - rep->delay = d;
293   - rep->period = p;
  295 +
294 296 return 0;
295 297 }
296 298  
  299 +int kbd_rate(struct kbd_repeat *rep)
  300 +{
  301 + struct kbd_repeat data[2] = { *rep };
  302 +
  303 + input_handler_for_each_handle(&kbd_handler, data, kbd_rate_helper);
  304 + *rep = data[1]; /* Copy currently used settings */
  305 +
  306 + return 0;
  307 +}
  308 +
297 309 /*
298 310 * Helper Functions.
299 311 */
300 312  
301 313  
302 314  
303 315  
304 316  
... ... @@ -998,36 +1010,36 @@
998 1010 return leds;
999 1011 }
1000 1012  
  1013 +static int kbd_update_leds_helper(struct input_handle *handle, void *data)
  1014 +{
  1015 + unsigned char leds = *(unsigned char *)data;
  1016 +
  1017 + if (test_bit(EV_LED, handle->dev->evbit)) {
  1018 + input_inject_event(handle, EV_LED, LED_SCROLLL, !!(leds & 0x01));
  1019 + input_inject_event(handle, EV_LED, LED_NUML, !!(leds & 0x02));
  1020 + input_inject_event(handle, EV_LED, LED_CAPSL, !!(leds & 0x04));
  1021 + input_inject_event(handle, EV_SYN, SYN_REPORT, 0);
  1022 + }
  1023 +
  1024 + return 0;
  1025 +}
  1026 +
1001 1027 /*
1002   - * This routine is the bottom half of the keyboard interrupt
1003   - * routine, and runs with all interrupts enabled. It does
1004   - * console changing, led setting and copy_to_cooked, which can
1005   - * take a reasonably long time.
1006   - *
1007   - * Aside from timing (which isn't really that important for
1008   - * keyboard interrupts as they happen often), using the software
1009   - * interrupt routines for this thing allows us to easily mask
1010   - * this when we don't want any of the above to happen.
1011   - * This allows for easy and efficient race-condition prevention
1012   - * for kbd_start => input_inject_event(dev, EV_LED, ...) => ...
  1028 + * This is the tasklet that updates LED state on all keyboards
  1029 + * attached to the box. The reason we use tasklet is that we
  1030 + * need to handle the scenario when keyboard handler is not
  1031 + * registered yet but we already getting updates form VT to
  1032 + * update led state.
1013 1033 */
1014   -
1015 1034 static void kbd_bh(unsigned long dummy)
1016 1035 {
1017   - struct list_head *node;
1018 1036 unsigned char leds = getleds();
1019 1037  
1020 1038 if (leds != ledstate) {
1021   - list_for_each(node, &kbd_handler.h_list) {
1022   - struct input_handle *handle = to_handle_h(node);
1023   - input_inject_event(handle, EV_LED, LED_SCROLLL, !!(leds & 0x01));
1024   - input_inject_event(handle, EV_LED, LED_NUML, !!(leds & 0x02));
1025   - input_inject_event(handle, EV_LED, LED_CAPSL, !!(leds & 0x04));
1026   - input_inject_event(handle, EV_SYN, SYN_REPORT, 0);
1027   - }
  1039 + input_handler_for_each_handle(&kbd_handler, &leds,
  1040 + kbd_update_leds_helper);
  1041 + ledstate = leds;
1028 1042 }
1029   -
1030   - ledstate = leds;
1031 1043 }
1032 1044  
1033 1045 DECLARE_TASKLET_DISABLED(keyboard_tasklet, kbd_bh, 0);
1034 1046  
... ... @@ -1370,15 +1382,11 @@
1370 1382 */
1371 1383 static void kbd_start(struct input_handle *handle)
1372 1384 {
1373   - unsigned char leds = ledstate;
1374   -
1375 1385 tasklet_disable(&keyboard_tasklet);
1376   - if (leds != 0xff) {
1377   - input_inject_event(handle, EV_LED, LED_SCROLLL, !!(leds & 0x01));
1378   - input_inject_event(handle, EV_LED, LED_NUML, !!(leds & 0x02));
1379   - input_inject_event(handle, EV_LED, LED_CAPSL, !!(leds & 0x04));
1380   - input_inject_event(handle, EV_SYN, SYN_REPORT, 0);
1381   - }
  1386 +
  1387 + if (ledstate != 0xff)
  1388 + kbd_update_leds_helper(handle, &ledstate);
  1389 +
1382 1390 tasklet_enable(&keyboard_tasklet);
1383 1391 }
1384 1392  
drivers/input/input.c
... ... @@ -1651,6 +1651,38 @@
1651 1651 EXPORT_SYMBOL(input_unregister_handler);
1652 1652  
1653 1653 /**
  1654 + * input_handler_for_each_handle - handle iterator
  1655 + * @handler: input handler to iterate
  1656 + * @data: data for the callback
  1657 + * @fn: function to be called for each handle
  1658 + *
  1659 + * Iterate over @bus's list of devices, and call @fn for each, passing
  1660 + * it @data and stop when @fn returns a non-zero value. The function is
  1661 + * using RCU to traverse the list and therefore may be usind in atonic
  1662 + * contexts. The @fn callback is invoked from RCU critical section and
  1663 + * thus must not sleep.
  1664 + */
  1665 +int input_handler_for_each_handle(struct input_handler *handler, void *data,
  1666 + int (*fn)(struct input_handle *, void *))
  1667 +{
  1668 + struct input_handle *handle;
  1669 + int retval = 0;
  1670 +
  1671 + rcu_read_lock();
  1672 +
  1673 + list_for_each_entry_rcu(handle, &handler->h_list, h_node) {
  1674 + retval = fn(handle, data);
  1675 + if (retval)
  1676 + break;
  1677 + }
  1678 +
  1679 + rcu_read_unlock();
  1680 +
  1681 + return retval;
  1682 +}
  1683 +EXPORT_SYMBOL(input_handler_for_each_handle);
  1684 +
  1685 +/**
1654 1686 * input_register_handle - register a new input handle
1655 1687 * @handle: handle to register
1656 1688 *
... ... @@ -1683,7 +1715,7 @@
1683 1715 * we can't be racing with input_unregister_handle()
1684 1716 * and so separate lock is not needed here.
1685 1717 */
1686   - list_add_tail(&handle->h_node, &handler->h_list);
  1718 + list_add_tail_rcu(&handle->h_node, &handler->h_list);
1687 1719  
1688 1720 if (handler->start)
1689 1721 handler->start(handle);
... ... @@ -1706,7 +1738,7 @@
1706 1738 {
1707 1739 struct input_dev *dev = handle->dev;
1708 1740  
1709   - list_del_init(&handle->h_node);
  1741 + list_del_rcu(&handle->h_node);
1710 1742  
1711 1743 /*
1712 1744 * Take dev->mutex to prevent race with input_release_device().
... ... @@ -1714,6 +1746,7 @@
1714 1746 mutex_lock(&dev->mutex);
1715 1747 list_del_rcu(&handle->d_node);
1716 1748 mutex_unlock(&dev->mutex);
  1749 +
1717 1750 synchronize_rcu();
1718 1751 }
1719 1752 EXPORT_SYMBOL(input_unregister_handle);
include/linux/input.h
... ... @@ -1021,9 +1021,12 @@
1021 1021 * @keycodesize: size of elements in keycode table
1022 1022 * @keycode: map of scancodes to keycodes for this device
1023 1023 * @setkeycode: optional method to alter current keymap, used to implement
1024   - * sparse keymaps. If not supplied default mechanism will be used
  1024 + * sparse keymaps. If not supplied default mechanism will be used.
  1025 + * The method is being called while holding event_lock and thus must
  1026 + * not sleep
1025 1027 * @getkeycode: optional method to retrieve current keymap. If not supplied
1026   - * default mechanism will be used
  1028 + * default mechanism will be used. The method is being called while
  1029 + * holding event_lock and thus must not sleep
1027 1030 * @ff: force feedback structure associated with the device if device
1028 1031 * supports force feedback effects
1029 1032 * @repeat_key: stores key code of the last key pressed; used to implement
... ... @@ -1294,6 +1297,9 @@
1294 1297  
1295 1298 int __must_check input_register_handler(struct input_handler *);
1296 1299 void input_unregister_handler(struct input_handler *);
  1300 +
  1301 +int input_handler_for_each_handle(struct input_handler *, void *data,
  1302 + int (*fn)(struct input_handle *, void *));
1297 1303  
1298 1304 int input_register_handle(struct input_handle *);
1299 1305 void input_unregister_handle(struct input_handle *);