RE: [PATCH] input: polldev can cause crash in case of polling disabled

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




>-----Original Message-----
>From: linux-input-owner@xxxxxxxxxxxxxxx [mailto:linux-input-
>owner@xxxxxxxxxxxxxxx] On Behalf Of Onkalo Samu.P (Nokia-D/Tampere)
>Sent: 17 February, 2010 10:16
>To: dmitry.torokhov@xxxxxxxxx; oleg@xxxxxxxxxx
>Cc: linux-input@xxxxxxxxxxxxxxx
>Subject: RE: [PATCH] input: polldev can cause crash in case of polling
>disabled
>
>
>
>>-----Original Message-----
>>From: ext Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx]
>>Sent: 16 February, 2010 23:43
>>To: Onkalo Samu.P (Nokia-D/Tampere); oleg@xxxxxxxxxx
>>Cc: linux-input@xxxxxxxxxxxxxxx
>>Subject: Re: [PATCH] input: polldev can cause crash in case of polling
>>disabled
>>
>>On Tue, Feb 16, 2010 at 07:37:49PM +0100, samu.p.onkalo@xxxxxxxxx
>wrote:
>>>
>>>
>>> >-----Original Message-----
>>> >From: ext Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx]
>>> >Sent: 16 February, 2010 19:51
>>> >To: Onkalo Samu.P (Nokia-D/Tampere)
>>> >Cc: linux-input@xxxxxxxxxxxxxxx
>>> >Subject: Re: [PATCH] input: polldev can cause crash in case of
>>polling
>>> >disabled
>>> >
>>> >Hi Samu,
>>> >
>>> >On Tue, Feb 16, 2010 at 04:44:41PM +0200, Samu Onkalo wrote:
>>> >> If polling is set to disabled value and polled input device
>>> >> is opened and closed several times, address to workqueue will
>>probably
>>> >> change at some point. Since nothing is queued (due to polled
>>disabled
>>> >> state), content of the work struct contains pointer to the old and
>>> >non-existent
>>> >> workqueue.
>>> >
>>> >This I do not quite understand. The work struct as far as I can see
>>does
>>> >not reference workqueue at all. There is a list entry but if we do
>>not
>>> >poll the device that entry should be always detached from any lists.
>>We
>>> >properly initialize WQ entry when we create the device and it shoudl
>>> >remain valid until the device is destroyed.
>>>
>>> 'data' entry contains a pointer to per-cpu-workqueue which in turn
>>contains
>>> the workqueue pointer. This 'data' entry is not ok in case of
>failure.
>>I can
>>> Collect more data about this.
>>>
>>> >
>>> >> When the device is closed again, cancel_delayed_work_sync
>>> >> goes crazy due to pointer to nonexisting workqueue.
>>> >>
>>> >
>>> >What kind of failure do you see? Is there a stack trace or
>something?
>>>
>>> Kernel panic while in workqueue handling (paging fault with some
>>crappy address).
>
>It looks like this (in ARM):
>[   88.974884] Unable to handle kernel paging request at virtual address
>732e726f
>[   88.994445] pgd = c0004000
>[   88.997192] [732e726f] *pgd=00000000
>[   89.013610] Internal error: Oops: 5 [#1] PREEMPT
>[   89.079406] CPU: 0    Not tainted  (2.6.32-09063-g2953cf4 #30)
>[   89.085296] PC is at __cancel_work_timer+0xfc/0x1a0
>[   89.090179] LR is at __cancel_work_timer+0xec/0x1a0
>[   89.095092] pc : [<c006f880>]    lr : [<c006f870>]    psr: a0000153
>[   89.095092] sp : cf411c98  ip : cf410000  fp : cf411d1c
>[   89.106628] r10: dfba59fc  r9 : dfba59f0  r8 : dfba59e4
>[   89.111877] r7 : 00000000  r6 : 00000000  r5 : dfba59e0  r4 :
>dfba59c0
>[   89.118438] r3 : 732e726f  r2 : d34a1b00  r1 : 00000001  r0 :
>dfba59f0
>[   89.125000] Flags: NzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM
>Segment user
>[   89.132263] Control: 10c5387d  Table: 91110019  DAC: 00000015
>[   89.144866] Stack: (0xcf411c98 to 0xcf412000)
>[   89.149261] 1c80:
>00000002 00000000
>[   89.157470] 1ca0: c006f824 cf410000 00000000 d34a1b00 dfa3f540
>ded44078 cf411cfc cf411cc8
>[   89.165710] 1cc0: c00861d8 c0084a20 00000002 00000000 00000000
>c02b2428 cf410000 20000153
>[   89.173919] 1ce0: cf411cfc c00813a4 cf410000 20000153 cf411d44
>dfba59c0 dfb618cc dfb2b864
>[   89.182159] 1d00: df67e960 ded44078 dfa3f540 ded44078 cf411d2c
>cf411d20 c006f938 c006f790
>[   89.190368] 1d20: cf411d44 cf411d30 bf10c2fc c006f930 dfb2b0c0
>dfb618cc cf411d64 cf411d48
>[   89.198608] 1d40: c02b2458 bf10c2e8 dfb618c0 d1ca1080 dfb6193c
>df67e960 cf411d84 cf411d68
>[   89.206817] 1d60: c02b4ab8 c02b240c 00000000 cf408200 00000010
>df67e960 cf411dc4 cf411d88
>[   89.215057] 1d80: c00ce2e0 c02b4a38 00000000 00000000 cf408200
>df67e960 cf411db4 cf408200
>[   89.223266] 1da0: 00000000 d3bca000 0000000c d3bca008 00000001
>00000000 cf411dd4 cf411dc8
>[   89.231506] 1dc0: c00ce438 c00ce1bc cf411df4 cf411dd8 c00caa74
>c00ce414 00000041 00000000
>[   89.239715] 1de0: d3bca000 0000000c cf411e1c cf411df8 c005f8dc
>c00caa08 d34a1de0 d34a1b00
>[   89.247955] 1e00: d3bca000 00000002 cf411ee0 d349b684 cf411e3c
>cf411e20 c005f984 c005f85c
>[   89.256164] 1e20: 00000002 d34a1b00 cf410000 00000002 cf411e54
>cf411e40 c0060fa8 c005f950
>[   89.264404] 1e40: cf411e5c cf411e50 cf411e74 cf411e58 c00614ec
>c0060de8 c002e204 00000009
>[   89.272613] 1e60: be95167c cf410000 cf411ec4 cf411e78 c006c218
>c0061460 00000400 dfb618f0
>[   89.280853] 1e80: dfb618f0 20000153 cf411ea4 cf411fb0 d349b184
>d34d1680 cf411ebc c002e204
>[   89.289062] 1ea0: cf411fb0 be95167c 00000003 c002e204 cf410000
>00000003 cf411fac cf411ec8
>[   89.297302] 1ec0: c0030430 c006be94 d1ca1080 cf410000 cf411f3c
>00000001 c02b55a8 c0073430
>[   89.305511] 1ee0: 00000009 00000000 00000000 00000000 00000000
>c00732ec cf411ef8 cf411ef8
>[   89.313751] 1f00: c00ccbfc c01e16e0 00000000 dfae9a10 d353cdd4
>cf408200 be951644 cf411f70
>[   89.321960] 1f20: 00000400 00000400 cf410000 00000003 cf411f6c
>cf411f40 c00cd770 c02b54c4
>[   89.330200] 1f40: 00000000 00000000 cf410000 00000000 00000000
>cf408200 00000400 be951644
>[   89.338409] 1f60: cf411fa4 cf411f70 c00cd8e0 c00cd6c4 cf411f8c
>cf411f80 c0082b24 c0082250
>[   89.346649] 1f80: 00000000 00012d98 00012818 be95167c 00000003
>c002e204 cf410000 00000003
>[   89.354858] 1fa0: 00000000 cf411fb0 c002e098 c00303c0 fffffe00
>be951644 00000400 00000001
>[   89.363098] 1fc0: 00012d98 00012818 be95167c 00000003 be951644
>00000003 00000003 be951cd4
>[   89.371307] 1fe0: 00000002 be950a70 0000cfe4 400fbfcc 60000050
>00000003 00000000 00000000
>[   89.379547] Backtrace:
>[   89.382019] [<c006f784>] (__cancel_work_timer+0x0/0x1a0) from
>[<c006f938>] (cancel_delayed_work_sync+0x14/0x18)
>[   89.392181] [<c006f924>] (cancel_delayed_work_sync+0x0/0x18) from
>[<bf10c2fc>] (input_close_polled_device+0x20/0x74 [input_p)
>[   89.404174] [<bf10c2dc>] (input_close_polled_device+0x0/0x74
>[input_polldev]) from [<c02b2458>] (input_close_device+0x58/0x7)
>[   89.415618]  r5:dfb618cc r4:dfb2b0c0
>[   89.419250] [<c02b2400>] (input_close_device+0x0/0x7c) from
>[<c02b4ab8>] (evdev_release+0x8c/0xa4)
>[   89.428253]  r7:df67e960 r6:dfb6193c r5:d1ca1080 r4:dfb618c0
>[   89.433990] [<c02b4a2c>] (evdev_release+0x0/0xa4) from [<c00ce2e0>]
>(__fput+0x130/0x258)
>[   89.442108]  r7:df67e960 r6:00000010 r5:cf408200 r4:00000000
>[   89.447845] [<c00ce1b0>] (__fput+0x0/0x258) from [<c00ce438>]
>(fput+0x30/0x34)
>[   89.455108] [<c00ce408>] (fput+0x0/0x34) from [<c00caa74>]
>(filp_close+0x78/0x84)
>[   89.462646] [<c00ca9fc>] (filp_close+0x0/0x84) from [<c005f8dc>]
>(put_files_struct+0x8c/0xf4)
>[   89.471221]  r7:0000000c r6:d3bca000 r5:00000000 r4:00000041
>[   89.476928] [<c005f850>] (put_files_struct+0x0/0xf4) from
>[<c005f984>] (exit_files+0x40/0x44)
>[   89.485504] [<c005f944>] (exit_files+0x0/0x44) from [<c0060fa8>]
>(do_exit+0x1cc/0x678)
>[   89.493469]  r7:00000002 r6:cf410000 r5:d34a1b00 r4:00000002
>[   89.499206] [<c0060ddc>] (do_exit+0x0/0x678) from [<c00614ec>]
>(do_group_exit+0x98/0xc8)
>[   89.507354] [<c0061454>] (do_group_exit+0x0/0xc8) from [<c006c218>]
>(get_signal_to_deliver+0x390/0x3d8)
>[   89.516784]  r7:cf410000 r6:be95167c r5:00000009 r4:c002e204
>[   89.522521] [<c006be88>] (get_signal_to_deliver+0x0/0x3d8) from
>[<c0030430>] (do_notify_resume+0x7c/0x594)
>[   89.532226] [<c00303b4>] (do_notify_resume+0x0/0x594) from
>[<c002e098>] (work_pending+0x1c/0x20)
>[   89.541076] Code: e5953000 e3d33003 0a000011 e593304c (e5934000)
>[   90.065185] ---[ end trace d28022cc252c43c3 ]---
>[   90.075866] Kernel panic - not syncing: Fatal exception
>
>I added traces to drivers/input/input-polldev.c
>static int input_open_polled_device(struct input_dev *input)
>{
>	struct input_polled_dev *dev = input_get_drvdata(input);
>	int error;
>	struct cpu_workqueue_struct *cwq;
>
>	printk("INPUT_OPEN_POLLED_DEVICE - enter\n");
>	printk("polldev_wq before start_workqueue: %x\n", polldev_wq);
>	if (polldev_wq != NULL)
>		printk("cpu_wq of wq: %x\n", polldev_wq->cpu_wq);
>
>
>	error = input_polldev_start_workqueue();
>	if (error)
>		return error;
>	printk("polldev_wq after start_workqueue: %x\n", polldev_wq);
>	if (polldev_wq != NULL)
>		printk("cpu_wq of wq: %x\n", polldev_wq->cpu_wq);
>
>	if (dev->open)
>		dev->open(dev);
>
>	printk("poll_interval: %x\n", dev->poll_interval );
>	printk("addr of work: %x\n", &dev->work);
>	cwq = atomic_long_read(&dev->work.work.data) &
>WORK_STRUCT_WQ_DATA_MASK;
>	printk("data (==cwq) at work (before queueing): %x\n", cwq);
>	if (cwq != NULL)
>		printk("wq from cwq: %x\n", cwq->wq);
>
>	/* Only start polling if polling is enabled */
>	if (dev->poll_interval > 0)
>		queue_delayed_work(polldev_wq, &dev->work, 0);
>
>	cwq = atomic_long_read(&dev->work.work.data) &
>WORK_STRUCT_WQ_DATA_MASK;
>	printk("data (==cwq) at work (after queueing): %x\n", cwq);
>	if (cwq != NULL)
>		printk("wq from cwq: %x\n", cwq->wq);
>
>	printk("INPUT_OPEN_POLLED_DEVICE - done\n");
>	return 0;
>}
>
>First time device open:
>[   29.094879] INPUT_OPEN_POLLED_DEVICE - enter
>[   29.099182] polldev_wq before start_workqueue: 0
>[   29.154449] polldev_wq after start_workqueue: ded3c580
>[   29.159637] cpu_wq of wq: dc4b8480
>[   29.255950] poll_interval: 32
>[   29.258941] addr of work: dfbe8f20
>[   29.262359] data (==cwq) at work (before queueing): 0
>[   29.267669] data (==cwq) at work (after queueing): dc4b8480  <-------
>CPU workqueue same as polldev_wq
>[   29.273315] wq from cwq: ded3c580
>[   29.276641] INPUT_OPEN_POLLED_DEVICE - done
>
>there were other devices opened and closed between
>
>Second time device open:
>[  202.372680] INPUT_OPEN_POLLED_DEVICE - enter
>[  202.377258] polldev_wq before start_workqueue: dc57ba00
>[  202.382598] cpu_wq of wq: dc4ea900
>[  202.386077] polldev_wq after start_workqueue: dc57ba00
>[  202.391326] cpu_wq of wq: dc4ea900
>[  202.459259] poll_interval: 0  <-------------------------------- no
>queueing because of this
>[  202.462188] addr of work: dfbe8f20
>[  202.465637] data (==cwq) at work (before queueing): dc4b8480  <------
>----- CPU workqueue not updated.
>[  202.471435] wq from cwq: 6b6b006f
>[  202.474853] data (==cwq) at work (after queueing): dc4b8480 <-----
>queueing not done -> not updated
>[  202.480468] wq from cwq: 6b6b006f <-------------------------- crap
>[  202.483886] INPUT_OPEN_POLLED_DEVICE - done
>
>And when cancel_delayed_work_sync is called, kernel crashes due to crap
>address to per-cpu workqueue.
>
>Actually problem is that "data" field in the work struct points to the
>non-existing per-cpu workqueue entry.
>When this is cancelled at device close, kernel crashes. But what is the
>root cause? In input-polldev,
>workqueue can change from time to time depending on what happens at the
>userspace. Work struct
>can still contain references to the old workqueue. Is that illegal thing
>to do?
>Or should cancellation of the work cleanup the work struct so that it is
>in initial state.
>
>It is said in the workqueue.c:
>* The caller must ensure that workqueue_struct on which this work was
>last
> * queued can't be destroyed before this function returns.
>
>This is not true here. Workqueue has been destroyed since the work has
>never queued to the new workqueue.
>Either cancel_(delayed)_work_sync should clear the data field instead of
>setting it to non-pending or
>input-polldev must clear the work struct in case of no queueing. Or do
>you have other proposals?
>

One solution is not to destroy and recreate workqueue in input-polldev
based on open / close calls. This way references to workqueue stays valid.

-Samu

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux