debugfs vs. device removal

DISCUSSION Omar Sandoval 3 participants 6 messages Jan 19, 2017

Share & tools

Omar Sandoval

author

2017-01-19 15:49 UTC #0 lore raw

Hi,

In the block layer, we abuse sysfs to export some per-device debugging information. I was looking into moving this to debugfs, but I realized that debugfs doesn't have a mechanism to ensure that a file associated with a device is safe to use when the device is removed.

At a quick glance, HID has some per-device information in debugfs. However, I don't see any sort of protection against a device being removed. I was easily able to trigger an oops by reading from /sys/kernel/debug/hid/*/rdesc in a loop and removing the USB device (trace attached).

How can I safely export per-device debugging information to debugfs?

Thanks,
Omar

[ 1409.427480] general protection fault: 0000 [#1] PREEMPT SMP
[ 1409.428078] Modules linked in: hid_generic usbhid hid cfg80211 rfkill joydev input_leds led_class mousedev ppdev uhci_hcd evdev mac_hid ehci_pci ehci_hcd psmouse serio_raw atkbd usbcore libps2 pcspkr parport_pc parport i2c_piix4 i6300esb acpi_cpufreq usb_common intel_agp tpm_tis intel_gtt tpm_tis_core qemu_fw_cfg tpm button sch_fq_codel ip_tables x_tables ext4 crc16 jbd2 fscrypto mbcache ata_generic pata_acpi virtio_blk virtio_net ata_piix i8042 libata virtio_pci serio virtio_ring floppy virtio scsi_mod
[ 1409.429869] CPU: 2 PID: 25689 Comm: cat Not tainted 4.8.13-1-ARCH #1
[ 1409.429869] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.1-20161122_114906-anatol 04/01/2014
[ 1409.429869] task: ffff88007ba25b00 task.stack: ffff8800780d8000
[ 1409.429869] RIP: 0010:[<ffffffffa035713c>]  [<ffffffffa035713c>] hid_dump_field+0x1c/0x5e0 [hid]
[ 1409.429869] RSP: 0018:ffff8800780dbc98  EFLAGS: 00010292
[ 1409.429869] RAX: 0000000000000000 RBX: ffff8800374e6b40 RCX: 0000000000000002
[ 1409.429869] RDX: ffff8800374e6b40 RSI: 0000000000000006 RDI: 7273752f00000000
[ 1409.429869] RBP: ffff8800780dbcd8 R08: 0000000000002000 R09: 000000000000000a
[ 1409.429869] R10: ffff880037a65974 R11: ffff880037a6597f R12: ffff8800374e6b40
[ 1409.429869] R13: 7273752f00000000 R14: ffff8800780f7000 R15: ffffffffa036251d
[ 1409.429869] FS:  00007f9824c49480(0000) GS:ffff88007fd00000(0000) knlGS:0000000000000000
[ 1409.429869] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1409.429869] CR2: 000055e0a7b014f7 CR3: 000000007800e000 CR4: 00000000000006e0
[ 1409.429869] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1409.429869] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 1409.429869] Stack:
[ 1409.429869]  ffff8800780dbca8 0000000017a5ac78 000000067b9e1800 0000000000000001
[ 1409.429869]  ffff8800374e6b40 ffff8800780f7020 ffff8800780f7000 ffffffffa036251d
[ 1409.429869]  ffff8800780dbd28 ffffffffa0357804 0000000000000000 ffffffffa03626e4
[ 1409.429869] Call Trace:
[ 1409.429869]  [<ffffffffa0357804>] hid_dump_device+0x104/0x160 [hid]
[ 1409.429869]  [<ffffffffa03578dd>] hid_debug_rdesc_show+0x7d/0x1d0 [hid]
[ 1409.429869]  [<ffffffff8122d940>] ? seq_buf_alloc+0x20/0x40
[ 1409.429869]  [<ffffffff8122dd42>] seq_read+0x102/0x3c0
[ 1409.429869]  [<ffffffff81299460>] full_proxy_read+0x60/0xa0
[ 1409.429869]  [<ffffffff81208667>] __vfs_read+0x37/0x130
[ 1409.429869]  [<ffffffff812b5013>] ? security_file_permission+0xa3/0xc0
[ 1409.429869]  [<ffffffff81209416>] vfs_read+0x96/0x130
[ 1409.429869]  [<ffffffff8120a925>] SyS_read+0x55/0xc0
[ 1409.429869]  [<ffffffff81067747>] ? trace_do_page_fault+0x37/0xf0
[ 1409.429869]  [<ffffffff815f8032>] entry_SYSCALL_64_fastpath+0x1a/0xa4
[ 1409.429869] Code: 36 a0 48 63 f0 e8 85 27 fb e0 e9 0d fe ff ff 0f 1f 44 00 00 55 48 89 e5 41 5741 56 41 55 41 54 49 89 fd 53 48 89 d3 48 83 ec 18 <44> 8b 07 89 75 d4 45 85 c0 0f 85 69 05 00 00 41 8b 7d 04 85 ff
[ 1409.429869] RIP  [<ffffffffa035713c>] hid_dump_field+0x1c/0x5e0 [hid]
[ 1409.429869]  RSP <ffff8800780dbc98>
[ 1409.473184] ---[ end trace 451c7935d39fca87 ]---

Greg Kroah-Hartman

2017-01-19 15:54 UTC #1 lore raw

Quoted 6 lines · Omar Sandoval

Hi,
In the block layer, we abuse sysfs to export some per-device debugging information. I was looking into moving this to debugfs, but I realized that debugfs doesn't have a mechanism to ensure that a file associated with a device is safe to use when the device is removed.

What do you mean by "safe"? The race conditions where you remove a file and still have it open should all now be resolved in 4.8 and 4.9, di dwe miss something?

Quoted 7 lines

At a quick glance, HID has some per-device information in debugfs. However, I don't see any sort of protection against a device being removed. I was easily able to trigger an oops by reading from /sys/kernel/debug/hid/*/rdesc in a loop and removing the USB device (trace attached).
How can I safely export per-device debugging information to debugfs?

Try 4.9 and see if you can still reproduce this, it should be fixed. If not, we might have forgotten to include the .owner field for the hid debugfs file...

thanks,

greg k-h

Jiri Kosina

2017-01-19 16:04 UTC #2 lore raw

Quoted 8 lines · Greg Kroah-Hartman

Quoted 4 lines
In the block layer, we abuse sysfs to export some per-device debugging information. I was looking into moving this to debugfs, but I realized that debugfs doesn't have a mechanism to ensure that a file associated with a device is safe to use when the device is removed.
What do you mean by "safe"? The race conditions where you remove a file and still have it open should all now be resolved in 4.8 and 4.9, di dwe miss something?

This is something else -- Omar is right, hid-debugfs interface is buggy. It basically doesn't synchronize the data dumping with device removal, so if device is removed and deallocated and the race is hit, it tries to dereference struct hid_device which has already been freed.

I'll look into fixing this later today or tomorrow. Basically we'd need to synchronize between hid_remove_device() and anything in hid-debug and whenever removal is pending, not to try to get any data out of it any more and bail immediately. Something like rwlock (debugfs being the reader and device removal being the writer) should work.

Thanks,

Signature · 2 lines

Jiri Kosina
SUSE Labs

Omar Sandoval

author

2017-01-19 17:34 UTC #3 lore raw

Quoted 7 lines · Jiri Kosina

Quoted 5 lines · Greg Kroah-Hartman
Quoted 4 lines
In the block layer, we abuse sysfs to export some per-device debugging information. I was looking into moving this to debugfs, but I realized that debugfs doesn't have a mechanism to ensure that a file associated with a device is safe to use when the device is removed.

Quoted 8 lines

Quoted 3 lines
What do you mean by "safe"? The race conditions where you remove a file and still have it open should all now be resolved in 4.8 and 4.9, di dwe miss something?
This is something else -- Omar is right, hid-debugfs interface is buggy. It basically doesn't synchronize the data dumping with device removal, so if device is removed and deallocated and the race is hit, it tries to dereference struct hid_device which has already been freed.

Yup, I'm talking about the case where I create a debugfs file and the data pointer is, say, a struct request_queue. If userspace calls open() on a debugfs file, then the device goes away, the struct request_queue is going to get freed and read() will blow up.

If we're talking about objects with a struct kobject (like struct request_queue), can we just grab an extra reference in open() and drop it in release()? This allows userspace to keep stuff pinned indefinitely, but debugfs is root-only and the use-case is usually just cat.

Quoted 12 lines

I'll look into fixing this later today or tomorrow. Basically we'd need to synchronize between hid_remove_device() and anything in hid-debug and whenever removal is pending, not to try to get any data out of it any more and bail immediately. Something like rwlock (debugfs being the reader and device removal being the writer) should work.
Thanks,
Signature · 2 lines
Jiri Kosina
SUSE Labs

Greg Kroah-Hartman

2017-01-19 18:29 UTC #4 lore raw

Quoted 22 lines · Omar Sandoval

Quoted 7 lines · Jiri Kosina
Quoted 5 lines · Greg Kroah-Hartman
Quoted 4 lines
In the block layer, we abuse sysfs to export some per-device debugging information. I was looking into moving this to debugfs, but I realized that debugfs doesn't have a mechanism to ensure that a file associated with a device is safe to use when the device is removed.
Quoted 8 lines
Quoted 3 lines
What do you mean by "safe"? The race conditions where you remove a file and still have it open should all now be resolved in 4.8 and 4.9, di dwe miss something?
This is something else -- Omar is right, hid-debugfs interface is buggy. It basically doesn't synchronize the data dumping with device removal, so if device is removed and deallocated and the race is hit, it tries to dereference struct hid_device which has already been freed.
Yup, I'm talking about the case where I create a debugfs file and the data pointer is, say, a struct request_queue. If userspace calls open() on a debugfs file, then the device goes away, the struct request_queue is going to get freed and read() will blow up.

Quoted 5 lines

If we're talking about objects with a struct kobject (like struct request_queue), can we just grab an extra reference in open() and drop it in release()? This allows userspace to keep stuff pinned indefinitely, but debugfs is root-only and the use-case is usually just cat.

Again, debugfs got a bunch of changes in the 4.8 and 4.9 timeframe to resolve this issue. Try it and see with just a "normal" debugfs file and see how it works.

thanks,

greg k-h

Omar Sandoval

author

2017-01-19 19:47 UTC #5 lore raw

Quoted 29 lines · Greg Kroah-Hartman

Quoted 22 lines · Omar Sandoval
Quoted 7 lines · Jiri Kosina
Quoted 5 lines · Greg Kroah-Hartman
Quoted 4 lines
In the block layer, we abuse sysfs to export some per-device debugging information. I was looking into moving this to debugfs, but I realized that debugfs doesn't have a mechanism to ensure that a file associated with a device is safe to use when the device is removed.
Quoted 8 lines
Quoted 3 lines
What do you mean by "safe"? The race conditions where you remove a file and still have it open should all now be resolved in 4.8 and 4.9, di dwe miss something?
This is something else -- Omar is right, hid-debugfs interface is buggy. It basically doesn't synchronize the data dumping with device removal, so if device is removed and deallocated and the race is hit, it tries to dereference struct hid_device which has already been freed.
Yup, I'm talking about the case where I create a debugfs file and the data pointer is, say, a struct request_queue. If userspace calls open() on a debugfs file, then the device goes away, the struct request_queue is going to get freed and read() will blow up.
Quoted 5 lines
If we're talking about objects with a struct kobject (like struct request_queue), can we just grab an extra reference in open() and drop it in release()? This allows userspace to keep stuff pinned indefinitely, but debugfs is root-only and the use-case is usually just cat.

Quoted 3 lines

Again, debugfs got a bunch of changes in the 4.8 and 4.9 timeframe to resolve this issue. Try it and see with just a "normal" debugfs file and see how it works.

The change in this area that I see is 49d200deaa68 ("debugfs: prevent access to removed files' private data"). That went in for 4.7. I'm pretty confused now since I can't reproduce the oops anymore on either 4.8 or 4.10-rc4. If I see it again I'll be sure to report it, but it seems like debugfs should just work for what I need. Thanks for the help, Greg.