diff mbox

hid-sony: Prevent crash when rumble effects are still loaded at USB disconnect

Message ID d9be52d5-7aed-4852-a339-18eea3d346ed@m-reimer.de (mailing list archive)
State New, archived
Delegated to: Jiri Kosina
Headers show

Commit Message

Manuel Reimer May 30, 2016, 7:15 p.m. UTC 
On 05/29/2016 07:11 PM, Cameron Gutman wrote:
> What prevents one of the send_output_report() functions from accessing sc->output_report_dmabuf after it’s been passed to kfree() but before you’ve set it to NULL? Why not simply wait to free output_report_dmabuf until after hid_hw_stop returns?

Just moving the kfree to the end doesn't seem to be a good idea.

I did the following:

it seems to be a good idea to move sony_cancel_work_sync below that, too 
and kfree after that.

But now I have:

[  391.170091] BUG: unable to handle kernel NULL pointer dereference at 
0000000000000030
[  391.303404] IP: [<ffffffffa0387eaf>] ml_effect_timer+0x1f/0x170 
[ff_memless]
[  391.303404] PGD 3d3a5067 PUD 3d3a4067 PMD 0
[  391.303404] Oops: 0000 [#1] PREEMPT SMP
[  391.303404] Modules linked in: hid_sony ff_memless cfg80211 rfkill 
crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel aes_x86_64 
lrw gf128mul glue_helper ablk_helper cryptd input_leds psmouse ppdev 
acpi_cpufreq pcspkr led_class joydev serio_raw mousedev tpm_tis tpm 
parport_pc mac_hid fjes parport intel_agp intel_gtt e1000 battery video 
evdev button processor i2c_piix4 ac sch_fq_codel vboxvideo(O) ttm 
drm_kms_helper drm syscopyarea sysfillrect sysimgblt fb_sys_fops 
vboxsf(O) vboxguest(O) ip_tables x_tables xfs libcrc32c hid_generic 
usbhid hid sr_mod cdrom sd_mod ata_generic pata_acpi atkbd libps2 
crc32c_intel ahci libahci ohci_pci ohci_hcd ata_piix usbcore usb_common 
libata scsi_mod i8042 serio
[  391.303404] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G           O 
4.5.4-1-ARCH #1
[  391.303404] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS 
VirtualBox 12/01/2006
[  391.303404] task: ffffffff81811500 ti: ffffffff81800000 task.ti: 
ffffffff81800000
[  391.303404] RIP: 0010:[<ffffffffa0387eaf>]  [<ffffffffa0387eaf>] 
ml_effect_timer+0x1f/0x170 [ff_memless]
[  391.303404] RSP: 0018:ffff88003fc03e60  EFLAGS: 00010246
[  421.393551] ata2: lost interrupt (Status 0x58)
[  421.393601] ata2: drained 8 bytes to clear DRQ
[  421.393611] ata2.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 
frozen
[  421.393623] ata2.00: cmd a0/00:00:00:08:00/00:00:00:00:00/a0 tag 0 
pio 16392 in
[  421.393623]          opcode=0x4a 4a 01 00 00 10 00 00 00 08 00res 
40/00:02:00:08:00/00:00:00:00:00/a0 Emask 0x4 (timeout)
[  421.393627] ata2.00: status: { DRDY }
[  421.393789] ata2: soft resetting link
[  421.550864] ata2.00: configured for UDMA/33
[  426.550330] ata2.00: qc timeout (cmd 0xa0)
[  426.550342] ata2.00: TEST_UNIT_READY failed (err_mask=0x5)
[  426.550519] ata2: soft resetting link
[  426.705035] ata2.00: configured for UDMA/33
[  431.703725] ata2.00: qc timeout (cmd 0xa0)
[  431.703736] ata2.00: TEST_UNIT_READY failed (err_mask=0x5)
[  431.703744] ata2.00: limiting speed to UDMA/33:PIO3
[  431.703994] ata2: soft resetting link
[  431.858270] ata2.00: configured for UDMA/33
[  436.856832] ata2.00: qc timeout (cmd 0xa0)
[  436.856840] ata2.00: TEST_UNIT_READY failed (err_mask=0x5)
[  436.856846] ata2.00: disabled
[  436.857022] ata2: soft resetting link
[  437.010167] ata2: EH complete
[  421.547203] RAX: 0000000000000000 RBX: ffff88003d08f000 RCX: 
ffff88003fc03ed8
[  421.547203] RDX: ffff88003d08f000 RSI: ffffffffa0387e90 RDI: 
ffff88003d08f000
[  421.547203] RBP: ffff88003fc03e78 R08: ffff88003ceaee00 R09: 
0000000000000000
[  421.547203] R10: 0000000000000020 R11: 0000000000000000 R12: 
ffff880037a47710
[  421.547203] R13: 0000000000000101 R14: ffffffffa0387e90 R15: 
ffff88003d08f000
[  421.547203] FS:  00007fba4be63700(0000) GS:ffff88003fc00000(0000) 
knlGS:0000000000000000
[  421.547203] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  421.547203] CR2: 0000000000000030 CR3: 000000003d3a2000 CR4: 
00000000000406f0
[  421.547203] Stack:
[  421.547203]  ffff88003fc0dbc0 ffff880037a47710 0000000000000101 
ffff88003fc03eb0
[  421.547203]  ffffffff810e3cc5 ffff88003fc0dbc0 ffff880037a47710 
ffffffffa0387e90
[  421.547203]  0000000000000000 0000000000000001 ffff88003fc03f10 
ffffffff810e4016
[  421.547203] Call Trace:
[  421.547203]  <IRQ>
[  421.547203]  [<ffffffff810e3cc5>] call_timer_fn+0x35/0x150
[  421.547203]  [<ffffffffa0387e90>] ? ml_ff_playback+0x100/0x100 
[ff_memless]
[  421.547203]  [<ffffffff810e4016>] run_timer_softirq+0x236/0x2e0
[  421.547203]  [<ffffffff8107d3e6>] __do_softirq+0xe6/0x2f0
[  421.547203]  [<ffffffff8107d763>] irq_exit+0xa3/0xb0
[  421.547203]  [<ffffffff815b8112>] smp_apic_timer_interrupt+0x42/0x50
[  421.547203]  [<ffffffff815b63f2>] apic_timer_interrupt+0x82/0x90
[  421.547203]  <EOI>
[  421.547203]  [<ffffffff8105f896>] ? native_safe_halt+0x6/0x10
[  421.547203]  [<ffffffff81021c30>] default_idle+0x20/0x110
[  421.547203]  [<ffffffff8102246f>] arch_cpu_idle+0xf/0x20
[  421.547203]  [<ffffffff810baf0a>] default_idle_call+0x2a/0x40
[  421.547203]  [<ffffffff810bb264>] cpu_startup_entry+0x344/0x3b0
[  421.547203]  [<ffffffff815a8c19>] rest_init+0x89/0x90
[  421.547203]  [<ffffffff8190c012>] start_kernel+0x46a/0x48b
[  421.547203]  [<ffffffff8190b120>] ? early_idt_handler_array+0x120/0x120
[  421.547203]  [<ffffffff8190b332>] x86_64_start_reservations+0x2a/0x2c
[  421.547203]  [<ffffffff8190b480>] x86_64_start_kernel+0x14c/0x16f
[  421.547203] Code: a0 e8 06 fa f7 e0 eb a3 0f 1f 40 00 0f 1f 44 00 00 
55 f6 05 d5 11 00 00 04 48 89 e5 41 55 41 54 53 48 8b 87 f8 00 00 00 48 
89 fb <4c> 8b 68 30 75 2c 48 81 c3 10 02 00 00 48 89 df e8 bc d1 22 e1
[  421.547203] RIP  [<ffffffffa0387eaf>] ml_effect_timer+0x1f/0x170 
[ff_memless]
[  421.547203]  RSP <ffff88003fc03e60>
[  421.547203] CR2: 0000000000000030
[  421.547203] ---[ end trace 7a664affc45468fa ]---
[  421.547203] Kernel panic - not syncing: Fatal exception in interrupt
[  421.547203] Kernel Offset: disabled
[  421.547203] ---[ end Kernel panic - not syncing: Fatal exception in 
interrupt

This was some kind of "brute force attempt" to check the stability.

I opened fftest, then I started effect "4" over and over again to get 
the queue in ff_memless filled up. Then I pulled the USB plug.

It may now actually be possible that the memory has been freed again and 
some event in the queue kicked in after that. It may also be possible 
that the hid driver subsystem doesn't really like this case, that the 
device is stopped, but someone still sends to it.

Same with xpad driver:
[   64.957349] xpad 1-2:1.0: xpad_try_sending_next_out_packet - 
usb_submit_urb failed with result -19
[   66.148685] xpad 1-2:1.0: xpad_try_sending_next_out_packet - 
usb_submit_urb failed with result -19
[   66.292505] xpad 1-2:1.0: xpad_try_sending_next_out_packet - 
usb_submit_urb failed with result -19
[   66.336855] xpad 1-2:1.0: xpad_try_sending_next_out_packet - 
usb_submit_urb failed with result -19



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

Patch

--- a/drivers/hid/hid-sony.c	2016-05-13 16:13:00.339346161 +0200
+++ b/drivers/hid/hid-sony.c	2016-05-30 20:28:39.897849164 +0200
@@ -2423,15 +2423,15 @@  static void sony_remove(struct hid_devic
  		sony_battery_remove(sc);
  	}

-	sony_cancel_work_sync(sc);
-
-	kfree(sc->output_report_dmabuf);
-
  	sony_remove_dev_list(sc);

  	sony_release_device_id(sc);

  	hid_hw_stop(hdev);
+
+	sony_cancel_work_sync(sc);
+
+	kfree(sc->output_report_dmabuf);
  }

Idea was: We know that hid_hw_stop will schedule work into our queue. So