V4L/DVB: ir: avoid race conditions at device disconnect - was: Fwd: Re: *buntu 10.10 rc

PATCH In review Mauro Carvalho Chehab 2 participants 2 messages Oct 14, 2010 Media (V4L/DVB)

lore

Share & tools

lore

Mauro Carvalho Chehab

author

2010-10-14 16:30 UTC #0 lore raw

PATCH 1/1 V4L/DVB: ir: avoid race conditions at device disconnect - was: Fwd: Re: *buntu 10.10 rc

This patch were sent only to lirc ML, due to a problem reported there.

Resending it to linux-media ML.

Cheers,
Mauro

-------- Mensagem original --------
Assunto: Re: *buntu 10.10 rc
Data: Thu, 14 Oct 2010 12:06:39 -0300
De: Mauro Carvalho Chehab <[email protected]>
Para: Jarod Wilson <[email protected]>
CC: Greg Oliver <[email protected]>, Douglas Pearless <[email protected]>, LIRC Users <[email protected]>

Em 14-10-2010 11:47, Jarod Wilson escreveu:

Quoted 5 lines · Greg Oliver

...
Quoted 2 lines
OK, with the patches, I get 2 key presses played for every 1 real press,

Quoted 92 lines

Is it really 2 key presses, or is it press + repeat? ir-core doesn't do any repeat filtering, and there was actually a repeat bug fixed in those patches, so this isn't entirely unexpected.

Quoted 2 lines

plus the same oops eventually with more debugs in the modules though..

Okay, so it seems the usb disconnect is what's triggering this. And I think I'm starting to get a decent idea of why I'm not seeing this myself. The ubuntu lirc packages have some udev bits that re-run the lirc initscript on device plug/unplug, and one of the things the initscript does is poke the protocols sysfs node, which is what triggers the call to store_protocols. I think we're racing with the disconnect here. I'm not sure if we need to add some locking, or just bail from store_protocols if ir_dev is NULL and call it good.

Quoted 82 lines

[ 3165.068079] ir_rc6_decode: RC6(6A) scancode 0x800f0420 (toggle: 0)
[ 3165.068088] ir_g_keycode_from_table: Media Center Ed. eHome
Infrared Remote Transceiver (1784:0008): scancode 0x800f0420 keycode
0x69
[ 3165.068114] ir_keydown: Media Center Ed. eHome Infrared Remote
Transceiver (1784:0008): key down event, key 0x0069, scancode
0x800f0420
[ 3165.071059] ir_rc5_decode: RC5(x) decode failed at state 1 (1761us pulse)
[ 3165.072055] ir_rc5_decode: RC5(x) decode failed at state 0 (900us space)
[ 3165.073059] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.074072] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.081112] ir_rc5_decode: RC5(x) decode failed at state 2 (400us space)
[ 3165.083086] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.084065] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.086064] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.089088] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.090061] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.093131] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.100057] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.103071] ir_rc5_decode: RC5(x) decode failed at state 2 (400us pulse)
[ 3165.104060] ir_rc5_decode: RC5(x) decode failed at state 0 (500us space)
[ 3165.105048] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.110588] usb 5-1: USB disconnect, address 2
[ 3165.111120] ir_input_unregister: Freed keycode table
[ 3165.145273] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000048
[ 3165.145286] IP: [<ffffffffa0044fca>] store_protocols+0x20a/0x2d0 [ir_core]
[ 3165.145305] PGD d848067 PUD d934067 PMD 0
[ 3165.145315] Oops: 0000 [#1] SMP
[ 3165.145322] last sysfs file: /sys/devices/virtual/rc/rc0/protocols
[ 3165.145328] CPU 2
[ 3165.145331] Modules linked in: nfsd exportfs nfs lockd fscache

nfs_acl auth_rpcgss snd_hda_codec_nvhdmi snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq snd_timer snd_seq_device nvidia(P) snd soundcore snd_page_alloc shpchp sunrpc ir_lirc_codec lirc_dev ir_sony_decoder ir_jvc_decoder ir_rc6_decoder usb_debug edac_core edac_mce_amd ir_rc5_decoder i2c_piix4 usbserial ir_nec_decoder rc_rc6_mce k10temp psmouse mceusb serio_raw ir_core lp parport usbhid hid pata_atiixp ahci r8169 mii libahci

[ 3165.145408]
[ 3165.145416] Pid: 3069, comm: lirc Tainted: P
2.6.36-020636rc7-generic #201010070908 TA880GB+/TA880GB+
[ 3165.145423] RIP: 0010:[<ffffffffa0044fca>]  [<ffffffffa0044fca>]
store_protocols+0x20a/0x2d0 [ir_core]
[ 3165.145439] RSP: 0018:ffff880037b33e18  EFLAGS: 00010202
[ 3165.145445] RAX: 0000000000000000 RBX: ffff88022dac79b0 RCX: 0000000000000005
[ 3165.145451] RDX: ffff88022ad12400 RSI: ffffffffa0046d80 RDI: ffff88022ad12400
[ 3165.145456] RBP: ffff880037b33e78 R08: 0000000000000001 R09: ffffea00002f4af0
[ 3165.145462] R10: 0000000000000000 R11: 0000000000000246 R12: ffff88021f4239c0
[ 3165.145468] R13: 00000000ffffffed R14: ffffffff8164b520 R15: ffff88022ad12410
[ 3165.145475] FS:  00007fa4cb4e7700(0000) GS:ffff880001e80000(0000)
knlGS:0000000000000000
[ 3165.145481] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3165.145486] CR2: 0000000000000048 CR3: 000000000da52000 CR4: 00000000000006e0
[ 3165.145492] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 3165.145498] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 3165.145504] Process lirc (pid: 3069, threadinfo ffff880037b32000,
task ffff8801932fc4a0)
[ 3165.145509] Stack:
[ 3165.145512]  ffffffff81a48f60 0000000000000005 ffff88022ad12400
0000000001883ad0
[ 3165.145521] <0> ffff880037b33e68 ffffffff8113cee3 ffff88000d832000
ffff88022dac79b0
[ 3165.145531] <0> ffff88021f4239c0 00000000ffffffed ffffffff8164b520
ffff88022ad12410
[ 3165.145542] Call Trace:
[ 3165.145556]  [<ffffffff8113cee3>] ? alloc_pages_current+0xa3/0x110
[ 3165.145567]  [<ffffffff81388390>] dev_attr_store+0x20/0x30
[ 3165.145576]  [<ffffffff811c1d32>] flush_write_buffer+0x62/0x90
[ 3165.145584]  [<ffffffff811c1e76>] sysfs_write_file+0x66/0xa0
[ 3165.145592]  [<ffffffff81156a7c>] vfs_write+0xcc/0x190
[ 3165.145600]  [<ffffffff81157485>] sys_write+0x55/0x90
[ 3165.145609]  [<ffffffff8100b0f2>] system_call_fastpath+0x16/0x1b
[ 3165.145614] Code: c7 c7 78 65 04 a0 31 c0 e8 94 ea 01 e1 48 8b 45
a8 48 83 c4 38 5b 41 5c 41 5d 41 5e 41 5f c9 c3 48 8b 55 b0 48 8b 82
98 02 00 00 <48> 8b 40 48 48 89 45 c0 e9 31 fe ff ff 48 8b 5d b0 48 81
c3 80
[ 3165.145687] RIP  [<ffffffffa0044fca>] store_protocols+0x20a/0x2d0 [ir_core]
[ 3165.145699]  RSP <ffff880037b33e18>
[ 3165.145703] CR2: 0000000000000048
[ 3165.145709] ---[ end trace 0241a6f763d5403b ]---

Hmm... see if the enclosed patch helps.

---

V4L/DVB: ir: avoid race conditions at device disconnect

It is possible that, while ir_unregister_class() is handling, some application could try to access the sysfs nodes, causing an OOPS.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/IR/ir-sysfs.c +9 -0

index dab074e..949b055 100644
@@ -68,6 +68,10 @@ static ssize_t show_protocols(struct device *d,
 	char *tmp = buf;
 	int i;
 
+	/* Device is being removed */
+	if (!ir_dev)
+		return -EINVAL;
+
 	if (ir_dev->props->driver_type == RC_DRIVER_SCANCODE) {
 		enabled = ir_dev->rc_tab.ir_type;
 		allowed = ir_dev->props->allowed_protos;

@@ -122,6 +126,10 @@ static ssize_t store_protocols(struct device *d,
 	int rc, i, count = 0;
 	unsigned long flags;
 
+	/* Device is being removed */
+	if (!ir_dev)
+		return -EINVAL;
+
 	if (ir_dev->props && ir_dev->props->driver_type == RC_DRIVER_SCANCODE)
 		type = ir_dev->rc_tab.ir_type;
 	else

@@ -305,6 +313,7 @@ void ir_unregister_class(struct input_dev *input_dev)
 {
 	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
 
+	input_set_drvdata(input_dev, NULL);
 	clear_bit(ir_dev->devno, &ir_core_dev_number);
 	input_unregister_device(input_dev);
 	device_del(&ir_dev->dev);

Jarod Wilson

Acked-by Reviewed-by on v1 1/1

2010-10-14 18:51 UTC #1 lore raw

Quoted 115 lines · Mauro Carvalho Chehab

This patch were sent only to lirc ML, due to a problem reported there.
Resending it to linux-media ML.
Cheers,
Mauro
-------- Mensagem original --------
Assunto: Re: *buntu 10.10 rc
Data: Thu, 14 Oct 2010 12:06:39 -0300
De: Mauro Carvalho Chehab <[email protected]>
Para: Jarod Wilson <[email protected]>
CC: Greg Oliver <[email protected]>, Douglas Pearless <[email protected]>, LIRC Users <[email protected]>
Em 14-10-2010 11:47, Jarod Wilson escreveu:
Quoted 5 lines · Greg Oliver
...
Quoted 2 lines
OK, with the patches, I get 2 key presses played for every 1 real press,
Quoted 92 lines
Is it really 2 key presses, or is it press + repeat? ir-core doesn't do any repeat filtering, and there was actually a repeat bug fixed in those patches, so this isn't entirely unexpected.
Quoted 2 lines
plus the same oops eventually with more debugs in the modules though..
Okay, so it seems the usb disconnect is what's triggering this. And I think I'm starting to get a decent idea of why I'm not seeing this myself. The ubuntu lirc packages have some udev bits that re-run the lirc initscript on device plug/unplug, and one of the things the initscript does is poke the protocols sysfs node, which is what triggers the call to store_protocols. I think we're racing with the disconnect here. I'm not sure if we need to add some locking, or just bail from store_protocols if ir_dev is NULL and call it good.
Quoted 82 lines
[ 3165.068079] ir_rc6_decode: RC6(6A) scancode 0x800f0420 (toggle: 0)
[ 3165.068088] ir_g_keycode_from_table: Media Center Ed. eHome
Infrared Remote Transceiver (1784:0008): scancode 0x800f0420 keycode
0x69
[ 3165.068114] ir_keydown: Media Center Ed. eHome Infrared Remote
Transceiver (1784:0008): key down event, key 0x0069, scancode
0x800f0420
[ 3165.071059] ir_rc5_decode: RC5(x) decode failed at state 1 (1761us pulse)
[ 3165.072055] ir_rc5_decode: RC5(x) decode failed at state 0 (900us space)
[ 3165.073059] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.074072] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.081112] ir_rc5_decode: RC5(x) decode failed at state 2 (400us space)
[ 3165.083086] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.084065] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.086064] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.089088] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.090061] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.093131] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.100057] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.103071] ir_rc5_decode: RC5(x) decode failed at state 2 (400us pulse)
[ 3165.104060] ir_rc5_decode: RC5(x) decode failed at state 0 (500us space)
[ 3165.105048] ir_rc5_decode: RC5(x) decode failed at state 1 (400us space)
[ 3165.110588] usb 5-1: USB disconnect, address 2
[ 3165.111120] ir_input_unregister: Freed keycode table
[ 3165.145273] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000048
[ 3165.145286] IP: [<ffffffffa0044fca>] store_protocols+0x20a/0x2d0 [ir_core]
[ 3165.145305] PGD d848067 PUD d934067 PMD 0
[ 3165.145315] Oops: 0000 [#1] SMP
[ 3165.145322] last sysfs file: /sys/devices/virtual/rc/rc0/protocols
[ 3165.145328] CPU 2
[ 3165.145331] Modules linked in: nfsd exportfs nfs lockd fscache
nfs_acl auth_rpcgss snd_hda_codec_nvhdmi snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq snd_timer snd_seq_device nvidia(P) snd soundcore snd_page_alloc shpchp sunrpc ir_lirc_codec lirc_dev ir_sony_decoder ir_jvc_decoder ir_rc6_decoder usb_debug edac_core edac_mce_amd ir_rc5_decoder i2c_piix4 usbserial ir_nec_decoder rc_rc6_mce k10temp psmouse mceusb serio_raw ir_core lp parport usbhid hid pata_atiixp ahci r8169 mii libahci
[ 3165.145408]
[ 3165.145416] Pid: 3069, comm: lirc Tainted: P
2.6.36-020636rc7-generic #201010070908 TA880GB+/TA880GB+
[ 3165.145423] RIP: 0010:[<ffffffffa0044fca>]  [<ffffffffa0044fca>]
store_protocols+0x20a/0x2d0 [ir_core]
[ 3165.145439] RSP: 0018:ffff880037b33e18  EFLAGS: 00010202
[ 3165.145445] RAX: 0000000000000000 RBX: ffff88022dac79b0 RCX: 0000000000000005
[ 3165.145451] RDX: ffff88022ad12400 RSI: ffffffffa0046d80 RDI: ffff88022ad12400
[ 3165.145456] RBP: ffff880037b33e78 R08: 0000000000000001 R09: ffffea00002f4af0
[ 3165.145462] R10: 0000000000000000 R11: 0000000000000246 R12: ffff88021f4239c0
[ 3165.145468] R13: 00000000ffffffed R14: ffffffff8164b520 R15: ffff88022ad12410
[ 3165.145475] FS:  00007fa4cb4e7700(0000) GS:ffff880001e80000(0000)
knlGS:0000000000000000
[ 3165.145481] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3165.145486] CR2: 0000000000000048 CR3: 000000000da52000 CR4: 00000000000006e0
[ 3165.145492] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 3165.145498] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 3165.145504] Process lirc (pid: 3069, threadinfo ffff880037b32000,
task ffff8801932fc4a0)
[ 3165.145509] Stack:
[ 3165.145512]  ffffffff81a48f60 0000000000000005 ffff88022ad12400
0000000001883ad0
[ 3165.145521] <0> ffff880037b33e68 ffffffff8113cee3 ffff88000d832000
ffff88022dac79b0
[ 3165.145531] <0> ffff88021f4239c0 00000000ffffffed ffffffff8164b520
ffff88022ad12410
[ 3165.145542] Call Trace:
[ 3165.145556]  [<ffffffff8113cee3>] ? alloc_pages_current+0xa3/0x110
[ 3165.145567]  [<ffffffff81388390>] dev_attr_store+0x20/0x30
[ 3165.145576]  [<ffffffff811c1d32>] flush_write_buffer+0x62/0x90
[ 3165.145584]  [<ffffffff811c1e76>] sysfs_write_file+0x66/0xa0
[ 3165.145592]  [<ffffffff81156a7c>] vfs_write+0xcc/0x190
[ 3165.145600]  [<ffffffff81157485>] sys_write+0x55/0x90
[ 3165.145609]  [<ffffffff8100b0f2>] system_call_fastpath+0x16/0x1b
[ 3165.145614] Code: c7 c7 78 65 04 a0 31 c0 e8 94 ea 01 e1 48 8b 45
a8 48 83 c4 38 5b 41 5c 41 5d 41 5e 41 5f c9 c3 48 8b 55 b0 48 8b 82
98 02 00 00 <48> 8b 40 48 48 89 45 c0 e9 31 fe ff ff 48 8b 5d b0 48 81
c3 80
[ 3165.145687] RIP  [<ffffffffa0044fca>] store_protocols+0x20a/0x2d0 [ir_core]
[ 3165.145699]  RSP <ffff880037b33e18>
[ 3165.145703] CR2: 0000000000000048
[ 3165.145709] ---[ end trace 0241a6f763d5403b ]---
Hmm... see if the enclosed patch helps.

Quoted 8 lines

---
V4L/DVB: ir: avoid race conditions at device disconnect
It is possible that, while ir_unregister_class() is handling, some application could try to access the sysfs nodes, causing an OOPS.
Signed-off-by Mauro Carvalho Chehab <[email protected]>

Same reply I sent to the patch on the lirc list:

Yeah, I'm reasonably sure that'll do the trick, since clearly, store_protocols is getting called after we've already started working our way through ir_input_unregister, so if drvdata is set to NULL before we enter ir_input_unregister and we check for NULL ir_dev in {store,show}_protocols, we should be good.

Acked-by Jarod Wilson <[email protected]>

and/or

Reviewed-by Jarod Wilson <[email protected]>

Signature · 2 lines

Jarod Wilson
[email protected]