Hi,
This is a revised series which also contains a minimal fix to the memory leak
discovered by David Hermann upon which the first NULL-pointer-dereference fix
also depends.
These patches need to get to Linus ASAP as the problems are present in 3.3-rc6
as well as earlier kernels and thus should be backported to the stable trees as
well.
Thanks,
Johan
Johan Hovold (3):
bluetooth: hci_ldisc: fix memory leak on tty_close
bluetooth: hci_ldisc: fix NULL-pointer dereference on tty_close
bluetooth: hci_core: fix NULL-pointer dereference at unregister
drivers/bluetooth/hci_ldisc.c | 4 ++--
include/net/bluetooth/hci.h | 2 ++
net/bluetooth/hci_core.c | 7 +++++++
3 files changed, 11 insertions(+), 2 deletions(-)
--
1.7.8.4
Hi,
On Thu, Mar 15, 2012, Marcel Holtmann wrote:
> Hi Johan,
>
> > Do not close protocol driver until device has been unregistered.
> >
> > This fixes a race between tty_close and hci_dev_open which can result in
> > a NULL-pointer dereference.
> >
> > The line discipline closes the protocol driver while we may still have
> > hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointer
> > dereference when lock is acquired and hci_init_req called.
> >
> > Bug is 100% reproducible using hciattach and a disconnected serial port:
> >
> > 0. # hciattach -n ttyO1 any noflow
> >
> > 1. hci_dev_open called from hci_power_on grabs req lock
> > 2. hci_init_req executes but device fails to initialise (times out
> > eventually)
> > 3. hci_dev_open is called from hci_sock_ioctl and sleeps on req lock
> > 4. hci_uart_tty_close detaches protocol driver and cancels init req
> > 5. hci_dev_open (1) releases req lock
> > 6. hci_dev_open (3) grabs req lock, calls hci_init_req, which triggers oops
> > when request is prepared in hci_uart_send_frame
> >
> > [ 137.201263] Unable to handle kernel NULL pointer dereference at virtual address 00000028
> > [ 137.209838] pgd = c0004000
> > [ 137.212677] [00000028] *pgd=00000000
> > [ 137.216430] Internal error: Oops: 17 [#1]
> > [ 137.220642] Modules linked in:
> > [ 137.223846] CPU: 0 Tainted: G W (3.3.0-rc6-dirty #406)
> > [ 137.230529] PC is at __lock_acquire+0x5c/0x1ab0
> > [ 137.235290] LR is at lock_acquire+0x9c/0x128
> > [ 137.239776] pc : [<c0071490>] lr : [<c00733f8>] psr: 20000093
> > [ 137.239776] sp : cf869dd8 ip : c0529554 fp : c051c730
> > [ 137.251800] r10: 00000000 r9 : cf8673c0 r8 : 00000080
> > [ 137.257293] r7 : 00000028 r6 : 00000002 r5 : 00000000 r4 : c053fd70
> > [ 137.264129] r3 : 00000000 r2 : 00000000 r1 : 00000000 r0 : 00000001
> > [ 137.270965] Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel
> > [ 137.278717] Control: 10c5387d Table: 8f0f4019 DAC: 00000015
> > [ 137.284729] Process kworker/u:1 (pid: 7, stack limit = 0xcf8682e8)
> > [ 137.291229] Stack: (0xcf869dd8 to 0xcf86a000)
> > [ 137.295776] 9dc0: c0529554 00000000
> > [ 137.304351] 9de0: cf8673c0 cf868000 d03ea1ef cf868000 000001ef 00000470 00000000 00000002
> > [ 137.312927] 9e00: cf8673c0 00000001 c051c730 c00716ec 0000000c 00000440 c0529554 00000001
> > [ 137.321533] 9e20: c051c730 cf868000 d03ea1f3 00000000 c053b978 00000000 00000028 cf868000
> > [ 137.330078] 9e40: 00000000 00000000 00000002 00000000 00000000 c00733f8 00000002 00000080
> > [ 137.338684] 9e60: 00000000 c02a1d50 00000000 00000001 60000013 c0969a1c 60000093 c053b96c
> > [ 137.347259] 9e80: 00000002 00000018 20000013 c02a1d50 cf0ac000 00000000 00000002 cf868000
> > [ 137.355834] 9ea0: 00000089 c0374130 00000002 00000000 c02a1d50 cf0ac000 0000000c cf0fc540
> > [ 137.364410] 9ec0: 00000018 c02a1d50 cf0fc540 00000000 cf0fc540 c0282238 c028220c cf178d80
> > [ 137.372985] 9ee0: 127525d8 c02821cc 9a1fa451 c032727c 9a1fa451 127525d8 cf0fc540 cf0ac4ec
> > [ 137.381561] 9f00: cf0ac000 cf0fc540 cf0ac584 c03285f4 c0328580 cf0ac4ec cf85c740 c05510cc
> > [ 137.390136] 9f20: ce825400 c004c914 00000002 00000000 c004c884 ce8254f5 cf869f48 00000000
> > [ 137.398712] 9f40: c0328580 ce825415 c0a7f914 c061af64 00000000 c048cf3c cf8673c0 cf85c740
> > [ 137.407287] 9f60: c05510cc c051a66c c05510ec c05510c4 cf85c750 cf868000 00000089 c004d6ac
> > [ 137.415863] 9f80: 00000000 c0073d14 00000001 cf853ed8 cf85c740 c004d558 00000013 00000000
> > [ 137.424438] 9fa0: 00000000 00000000 00000000 c00516b0 00000000 00000000 cf85c740 00000000
> > [ 137.433013] 9fc0: 00000001 dead4ead ffffffff ffffffff c0551674 00000000 00000000 c0450aa4
> > [ 137.441589] 9fe0: cf869fe0 cf869fe0 cf853ed8 c005162c c0013b30 c0013b30 00ffff00 00ffff00
> > [ 137.450164] [<c0071490>] (__lock_acquire+0x5c/0x1ab0) from [<c00733f8>] (lock_acquire+0x9c/0x128)
> > [ 137.459503] [<c00733f8>] (lock_acquire+0x9c/0x128) from [<c0374130>] (_raw_spin_lock_irqsave+0x44/0x58)
> > [ 137.469360] [<c0374130>] (_raw_spin_lock_irqsave+0x44/0x58) from [<c02a1d50>] (skb_queue_tail+0x18/0x48)
> > [ 137.479339] [<c02a1d50>] (skb_queue_tail+0x18/0x48) from [<c0282238>] (h4_enqueue+0x2c/0x34)
> > [ 137.488189] [<c0282238>] (h4_enqueue+0x2c/0x34) from [<c02821cc>] (hci_uart_send_frame+0x34/0x68)
> > [ 137.497497] [<c02821cc>] (hci_uart_send_frame+0x34/0x68) from [<c032727c>] (hci_send_frame+0x50/0x88)
> > [ 137.507171] [<c032727c>] (hci_send_frame+0x50/0x88) from [<c03285f4>] (hci_cmd_work+0x74/0xd4)
> > [ 137.516204] [<c03285f4>] (hci_cmd_work+0x74/0xd4) from [<c004c914>] (process_one_work+0x1a0/0x4ec)
> > [ 137.525604] [<c004c914>] (process_one_work+0x1a0/0x4ec) from [<c004d6ac>] (worker_thread+0x154/0x344)
> > [ 137.535278] [<c004d6ac>] (worker_thread+0x154/0x344) from [<c00516b0>] (kthread+0x84/0x90)
> > [ 137.543975] [<c00516b0>] (kthread+0x84/0x90) from [<c0013b30>] (kernel_thread_exit+0x0/0x8)
> > [ 137.552734] Code: e59f4e5c e5941000 e3510000 0a000031 (e5971000)
> > [ 137.559234] ---[ end trace 1b75b31a2719ed1e ]---
> >
> > Cc: stable <[email protected]>
> > Signed-off-by: Johan Hovold <[email protected]>
> > ---
> > drivers/bluetooth/hci_ldisc.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
>
> Acked-by: Marcel Holtmann <[email protected]>
Both patches have been applied to my bluetooth-next tree. Thanks.
Johan
Hi Johan,
> Make sure hci_dev_open returns immediately if hci_dev_unregister has
> been called.
>
> This fixes a race between hci_dev_open and hci_dev_unregister which can
> lead to a NULL-pointer dereference.
>
> Bug is 100% reproducible using hciattach and a disconnected serial port:
>
> 0. # hciattach -n /dev/ttyO1 any noflow
>
> 1. hci_dev_open called from hci_power_on grabs req lock
> 2. hci_init_req executes but device fails to initialise (times out
> eventually)
> 3. hci_dev_open is called from hci_sock_ioctl and sleeps on req lock
> 4. hci_uart_tty_close calls hci_dev_unregister and sleeps on req lock in
> hci_dev_do_close
> 5. hci_dev_open (1) releases req lock
> 6. hci_dev_do_close grabs req lock and returns as device is not up
> 7. hci_dev_unregister sleeps in destroy_workqueue
> 8. hci_dev_open (3) grabs req lock, calls hci_init_req and eventually sleeps
> 9. hci_dev_unregister finishes, while hci_dev_open is still running...
>
> [ 79.627136] INFO: trying to register non-static key.
> [ 79.632354] the code is fine but needs lockdep annotation.
> [ 79.638122] turning off the locking correctness validator.
> [ 79.643920] [<c00188bc>] (unwind_backtrace+0x0/0xf8) from [<c00729c4>] (__lock_acquire+0x1590/0x1ab0)
> [ 79.653594] [<c00729c4>] (__lock_acquire+0x1590/0x1ab0) from [<c00733f8>] (lock_acquire+0x9c/0x128)
> [ 79.663085] [<c00733f8>] (lock_acquire+0x9c/0x128) from [<c0040a88>] (run_timer_softirq+0x150/0x3ac)
> [ 79.672668] [<c0040a88>] (run_timer_softirq+0x150/0x3ac) from [<c003a3b8>] (__do_softirq+0xd4/0x22c)
> [ 79.682281] [<c003a3b8>] (__do_softirq+0xd4/0x22c) from [<c003a924>] (irq_exit+0x8c/0x94)
> [ 79.690856] [<c003a924>] (irq_exit+0x8c/0x94) from [<c0013a50>] (handle_IRQ+0x34/0x84)
> [ 79.699157] [<c0013a50>] (handle_IRQ+0x34/0x84) from [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c)
> [ 79.708648] [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c) from [<c037499c>] (__irq_usr+0x3c/0x60)
> [ 79.718048] Exception stack(0xcf281fb0 to 0xcf281ff8)
> [ 79.723358] 1fa0: 0001e6a0 be8dab00 0001e698 00036698
> [ 79.731933] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000000 00000004 00000000
> [ 79.740509] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 ffffffff
> [ 79.747497] Unable to handle kernel NULL pointer dereference at virtual address 00000000
> [ 79.756011] pgd = cf3b4000
> [ 79.758850] [00000000] *pgd=8f0c7831, *pte=00000000, *ppte=00000000
> [ 79.765502] Internal error: Oops: 80000007 [#1]
> [ 79.770294] Modules linked in:
> [ 79.773529] CPU: 0 Tainted: G W (3.3.0-rc6-00002-gb5d5c87 #421)
> [ 79.781066] PC is at 0x0
> [ 79.783721] LR is at run_timer_softirq+0x16c/0x3ac
> [ 79.788787] pc : [<00000000>] lr : [<c0040aa4>] psr: 60000113
> [ 79.788787] sp : cf281ee0 ip : 00000000 fp : cf280000
> [ 79.800903] r10: 00000004 r9 : 00000100 r8 : b6f234d0
> [ 79.806427] r7 : c0519c28 r6 : cf093488 r5 : c0561a00 r4 : 00000000
> [ 79.813323] r3 : 00000000 r2 : c054eee0 r1 : 00000001 r0 : 00000000
> [ 79.820190] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
> [ 79.827728] Control: 10c5387d Table: 8f3b4019 DAC: 00000015
> [ 79.833801] Process gpsd (pid: 1265, stack limit = 0xcf2802e8)
> [ 79.839965] Stack: (0xcf281ee0 to 0xcf282000)
> [ 79.844573] 1ee0: 00000002 00000000 c0040a24 00000000 00000002 cf281f08 00200200 00000000
> [ 79.853210] 1f00: 00000000 cf281f18 cf281f08 00000000 00000000 00000000 cf281f18 cf281f18
> [ 79.861816] 1f20: 00000000 00000001 c056184c 00000000 00000001 b6f234d0 c0561848 00000004
> [ 79.870452] 1f40: cf280000 c003a3b8 c051e79c 00000001 00000000 00000100 3fa9e7b8 0000000a
> [ 79.879089] 1f60: 00000025 cf280000 00000025 00000000 00000000 b6f234d0 00000000 00000004
> [ 79.887756] 1f80: 00000000 c003a924 c053ad38 c0013a50 fa200000 cf281fb0 ffffffff c0008530
> [ 79.896362] 1fa0: 0001e6a0 0000aab8 80000010 c037499c 0001e6a0 be8dab00 0001e698 00036698
> [ 79.904998] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000000 00000004 00000000
> [ 79.913665] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 ffffffff 00fbf700 04ffff00
> [ 79.922302] [<c0040aa4>] (run_timer_softirq+0x16c/0x3ac) from [<c003a3b8>] (__do_softirq+0xd4/0x22c)
> [ 79.931945] [<c003a3b8>] (__do_softirq+0xd4/0x22c) from [<c003a924>] (irq_exit+0x8c/0x94)
> [ 79.940582] [<c003a924>] (irq_exit+0x8c/0x94) from [<c0013a50>] (handle_IRQ+0x34/0x84)
> [ 79.948913] [<c0013a50>] (handle_IRQ+0x34/0x84) from [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c)
> [ 79.958404] [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c) from [<c037499c>] (__irq_usr+0x3c/0x60)
> [ 79.967773] Exception stack(0xcf281fb0 to 0xcf281ff8)
> [ 79.973083] 1fa0: 0001e6a0 be8dab00 0001e698 00036698
> [ 79.981658] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000000 00000004 00000000
> [ 79.990234] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 ffffffff
> [ 79.997161] Code: bad PC value
> [ 80.000396] ---[ end trace 6f6739840475f9ee ]---
> [ 80.005279] Kernel panic - not syncing: Fatal exception in interrupt
>
> Cc: stable <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> include/net/bluetooth/hci.h | 1 +
> net/bluetooth/hci_core.c | 7 +++++++
> 2 files changed, 8 insertions(+), 0 deletions(-)
Acked-by: Marcel Holtmann <[email protected]>
Regards
Marcel
Hi Johan,
> Do not close protocol driver until device has been unregistered.
>
> This fixes a race between tty_close and hci_dev_open which can result in
> a NULL-pointer dereference.
>
> The line discipline closes the protocol driver while we may still have
> hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointer
> dereference when lock is acquired and hci_init_req called.
>
> Bug is 100% reproducible using hciattach and a disconnected serial port:
>
> 0. # hciattach -n ttyO1 any noflow
>
> 1. hci_dev_open called from hci_power_on grabs req lock
> 2. hci_init_req executes but device fails to initialise (times out
> eventually)
> 3. hci_dev_open is called from hci_sock_ioctl and sleeps on req lock
> 4. hci_uart_tty_close detaches protocol driver and cancels init req
> 5. hci_dev_open (1) releases req lock
> 6. hci_dev_open (3) grabs req lock, calls hci_init_req, which triggers oops
> when request is prepared in hci_uart_send_frame
>
> [ 137.201263] Unable to handle kernel NULL pointer dereference at virtual address 00000028
> [ 137.209838] pgd = c0004000
> [ 137.212677] [00000028] *pgd=00000000
> [ 137.216430] Internal error: Oops: 17 [#1]
> [ 137.220642] Modules linked in:
> [ 137.223846] CPU: 0 Tainted: G W (3.3.0-rc6-dirty #406)
> [ 137.230529] PC is at __lock_acquire+0x5c/0x1ab0
> [ 137.235290] LR is at lock_acquire+0x9c/0x128
> [ 137.239776] pc : [<c0071490>] lr : [<c00733f8>] psr: 20000093
> [ 137.239776] sp : cf869dd8 ip : c0529554 fp : c051c730
> [ 137.251800] r10: 00000000 r9 : cf8673c0 r8 : 00000080
> [ 137.257293] r7 : 00000028 r6 : 00000002 r5 : 00000000 r4 : c053fd70
> [ 137.264129] r3 : 00000000 r2 : 00000000 r1 : 00000000 r0 : 00000001
> [ 137.270965] Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel
> [ 137.278717] Control: 10c5387d Table: 8f0f4019 DAC: 00000015
> [ 137.284729] Process kworker/u:1 (pid: 7, stack limit = 0xcf8682e8)
> [ 137.291229] Stack: (0xcf869dd8 to 0xcf86a000)
> [ 137.295776] 9dc0: c0529554 00000000
> [ 137.304351] 9de0: cf8673c0 cf868000 d03ea1ef cf868000 000001ef 00000470 00000000 00000002
> [ 137.312927] 9e00: cf8673c0 00000001 c051c730 c00716ec 0000000c 00000440 c0529554 00000001
> [ 137.321533] 9e20: c051c730 cf868000 d03ea1f3 00000000 c053b978 00000000 00000028 cf868000
> [ 137.330078] 9e40: 00000000 00000000 00000002 00000000 00000000 c00733f8 00000002 00000080
> [ 137.338684] 9e60: 00000000 c02a1d50 00000000 00000001 60000013 c0969a1c 60000093 c053b96c
> [ 137.347259] 9e80: 00000002 00000018 20000013 c02a1d50 cf0ac000 00000000 00000002 cf868000
> [ 137.355834] 9ea0: 00000089 c0374130 00000002 00000000 c02a1d50 cf0ac000 0000000c cf0fc540
> [ 137.364410] 9ec0: 00000018 c02a1d50 cf0fc540 00000000 cf0fc540 c0282238 c028220c cf178d80
> [ 137.372985] 9ee0: 127525d8 c02821cc 9a1fa451 c032727c 9a1fa451 127525d8 cf0fc540 cf0ac4ec
> [ 137.381561] 9f00: cf0ac000 cf0fc540 cf0ac584 c03285f4 c0328580 cf0ac4ec cf85c740 c05510cc
> [ 137.390136] 9f20: ce825400 c004c914 00000002 00000000 c004c884 ce8254f5 cf869f48 00000000
> [ 137.398712] 9f40: c0328580 ce825415 c0a7f914 c061af64 00000000 c048cf3c cf8673c0 cf85c740
> [ 137.407287] 9f60: c05510cc c051a66c c05510ec c05510c4 cf85c750 cf868000 00000089 c004d6ac
> [ 137.415863] 9f80: 00000000 c0073d14 00000001 cf853ed8 cf85c740 c004d558 00000013 00000000
> [ 137.424438] 9fa0: 00000000 00000000 00000000 c00516b0 00000000 00000000 cf85c740 00000000
> [ 137.433013] 9fc0: 00000001 dead4ead ffffffff ffffffff c0551674 00000000 00000000 c0450aa4
> [ 137.441589] 9fe0: cf869fe0 cf869fe0 cf853ed8 c005162c c0013b30 c0013b30 00ffff00 00ffff00
> [ 137.450164] [<c0071490>] (__lock_acquire+0x5c/0x1ab0) from [<c00733f8>] (lock_acquire+0x9c/0x128)
> [ 137.459503] [<c00733f8>] (lock_acquire+0x9c/0x128) from [<c0374130>] (_raw_spin_lock_irqsave+0x44/0x58)
> [ 137.469360] [<c0374130>] (_raw_spin_lock_irqsave+0x44/0x58) from [<c02a1d50>] (skb_queue_tail+0x18/0x48)
> [ 137.479339] [<c02a1d50>] (skb_queue_tail+0x18/0x48) from [<c0282238>] (h4_enqueue+0x2c/0x34)
> [ 137.488189] [<c0282238>] (h4_enqueue+0x2c/0x34) from [<c02821cc>] (hci_uart_send_frame+0x34/0x68)
> [ 137.497497] [<c02821cc>] (hci_uart_send_frame+0x34/0x68) from [<c032727c>] (hci_send_frame+0x50/0x88)
> [ 137.507171] [<c032727c>] (hci_send_frame+0x50/0x88) from [<c03285f4>] (hci_cmd_work+0x74/0xd4)
> [ 137.516204] [<c03285f4>] (hci_cmd_work+0x74/0xd4) from [<c004c914>] (process_one_work+0x1a0/0x4ec)
> [ 137.525604] [<c004c914>] (process_one_work+0x1a0/0x4ec) from [<c004d6ac>] (worker_thread+0x154/0x344)
> [ 137.535278] [<c004d6ac>] (worker_thread+0x154/0x344) from [<c00516b0>] (kthread+0x84/0x90)
> [ 137.543975] [<c00516b0>] (kthread+0x84/0x90) from [<c0013b30>] (kernel_thread_exit+0x0/0x8)
> [ 137.552734] Code: e59f4e5c e5941000 e3510000 0a000031 (e5971000)
> [ 137.559234] ---[ end trace 1b75b31a2719ed1e ]---
>
> Cc: stable <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/bluetooth/hci_ldisc.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
Acked-by: Marcel Holtmann <[email protected]>
Regards
Marcel
Make sure hci_dev_open returns immediately if hci_dev_unregister has
been called.
This fixes a race between hci_dev_open and hci_dev_unregister which can
lead to a NULL-pointer dereference.
Bug is 100% reproducible using hciattach and a disconnected serial port:
0. # hciattach -n /dev/ttyO1 any noflow
1. hci_dev_open called from hci_power_on grabs req lock
2. hci_init_req executes but device fails to initialise (times out
eventually)
3. hci_dev_open is called from hci_sock_ioctl and sleeps on req lock
4. hci_uart_tty_close calls hci_dev_unregister and sleeps on req lock in
hci_dev_do_close
5. hci_dev_open (1) releases req lock
6. hci_dev_do_close grabs req lock and returns as device is not up
7. hci_dev_unregister sleeps in destroy_workqueue
8. hci_dev_open (3) grabs req lock, calls hci_init_req and eventually sleeps
9. hci_dev_unregister finishes, while hci_dev_open is still running...
[ 79.627136] INFO: trying to register non-static key.
[ 79.632354] the code is fine but needs lockdep annotation.
[ 79.638122] turning off the locking correctness validator.
[ 79.643920] [<c00188bc>] (unwind_backtrace+0x0/0xf8) from [<c00729c4>] (__lock_acquire+0x1590/0x1ab0)
[ 79.653594] [<c00729c4>] (__lock_acquire+0x1590/0x1ab0) from [<c00733f8>] (lock_acquire+0x9c/0x128)
[ 79.663085] [<c00733f8>] (lock_acquire+0x9c/0x128) from [<c0040a88>] (run_timer_softirq+0x150/0x3ac)
[ 79.672668] [<c0040a88>] (run_timer_softirq+0x150/0x3ac) from [<c003a3b8>] (__do_softirq+0xd4/0x22c)
[ 79.682281] [<c003a3b8>] (__do_softirq+0xd4/0x22c) from [<c003a924>] (irq_exit+0x8c/0x94)
[ 79.690856] [<c003a924>] (irq_exit+0x8c/0x94) from [<c0013a50>] (handle_IRQ+0x34/0x84)
[ 79.699157] [<c0013a50>] (handle_IRQ+0x34/0x84) from [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c)
[ 79.708648] [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c) from [<c037499c>] (__irq_usr+0x3c/0x60)
[ 79.718048] Exception stack(0xcf281fb0 to 0xcf281ff8)
[ 79.723358] 1fa0: 0001e6a0 be8dab00 0001e698 00036698
[ 79.731933] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000000 00000004 00000000
[ 79.740509] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 ffffffff
[ 79.747497] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[ 79.756011] pgd = cf3b4000
[ 79.758850] [00000000] *pgd=8f0c7831, *pte=00000000, *ppte=00000000
[ 79.765502] Internal error: Oops: 80000007 [#1]
[ 79.770294] Modules linked in:
[ 79.773529] CPU: 0 Tainted: G W (3.3.0-rc6-00002-gb5d5c87 #421)
[ 79.781066] PC is at 0x0
[ 79.783721] LR is at run_timer_softirq+0x16c/0x3ac
[ 79.788787] pc : [<00000000>] lr : [<c0040aa4>] psr: 60000113
[ 79.788787] sp : cf281ee0 ip : 00000000 fp : cf280000
[ 79.800903] r10: 00000004 r9 : 00000100 r8 : b6f234d0
[ 79.806427] r7 : c0519c28 r6 : cf093488 r5 : c0561a00 r4 : 00000000
[ 79.813323] r3 : 00000000 r2 : c054eee0 r1 : 00000001 r0 : 00000000
[ 79.820190] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 79.827728] Control: 10c5387d Table: 8f3b4019 DAC: 00000015
[ 79.833801] Process gpsd (pid: 1265, stack limit = 0xcf2802e8)
[ 79.839965] Stack: (0xcf281ee0 to 0xcf282000)
[ 79.844573] 1ee0: 00000002 00000000 c0040a24 00000000 00000002 cf281f08 00200200 00000000
[ 79.853210] 1f00: 00000000 cf281f18 cf281f08 00000000 00000000 00000000 cf281f18 cf281f18
[ 79.861816] 1f20: 00000000 00000001 c056184c 00000000 00000001 b6f234d0 c0561848 00000004
[ 79.870452] 1f40: cf280000 c003a3b8 c051e79c 00000001 00000000 00000100 3fa9e7b8 0000000a
[ 79.879089] 1f60: 00000025 cf280000 00000025 00000000 00000000 b6f234d0 00000000 00000004
[ 79.887756] 1f80: 00000000 c003a924 c053ad38 c0013a50 fa200000 cf281fb0 ffffffff c0008530
[ 79.896362] 1fa0: 0001e6a0 0000aab8 80000010 c037499c 0001e6a0 be8dab00 0001e698 00036698
[ 79.904998] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000000 00000004 00000000
[ 79.913665] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 ffffffff 00fbf700 04ffff00
[ 79.922302] [<c0040aa4>] (run_timer_softirq+0x16c/0x3ac) from [<c003a3b8>] (__do_softirq+0xd4/0x22c)
[ 79.931945] [<c003a3b8>] (__do_softirq+0xd4/0x22c) from [<c003a924>] (irq_exit+0x8c/0x94)
[ 79.940582] [<c003a924>] (irq_exit+0x8c/0x94) from [<c0013a50>] (handle_IRQ+0x34/0x84)
[ 79.948913] [<c0013a50>] (handle_IRQ+0x34/0x84) from [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c)
[ 79.958404] [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c) from [<c037499c>] (__irq_usr+0x3c/0x60)
[ 79.967773] Exception stack(0xcf281fb0 to 0xcf281ff8)
[ 79.973083] 1fa0: 0001e6a0 be8dab00 0001e698 00036698
[ 79.981658] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000000 00000004 00000000
[ 79.990234] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 ffffffff
[ 79.997161] Code: bad PC value
[ 80.000396] ---[ end trace 6f6739840475f9ee ]---
[ 80.005279] Kernel panic - not syncing: Fatal exception in interrupt
Cc: stable <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
include/net/bluetooth/hci.h | 1 +
net/bluetooth/hci_core.c | 7 +++++++
2 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 344b0f9..8f928f7 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -92,6 +92,7 @@ enum {
HCI_SERVICE_CACHE,
HCI_LINK_KEYS,
HCI_DEBUG_KEYS,
+ HCI_UNREGISTER,
HCI_LE_SCAN,
HCI_SSP_ENABLED,
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 2f3e1bb..2e3f193 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -666,6 +666,11 @@ int hci_dev_open(__u16 dev)
hci_req_lock(hdev);
+ if (test_bit(HCI_UNREGISTER, &hdev->dev_flags)) {
+ ret = -ENODEV;
+ goto done;
+ }
+
if (hdev->rfkill && rfkill_blocked(hdev->rfkill)) {
ret = -ERFKILL;
goto done;
@@ -1850,6 +1855,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
+ set_bit(HCI_UNREGISTER, &hdev->dev_flags);
+
write_lock(&hci_dev_list_lock);
list_del(&hdev->list);
write_unlock(&hci_dev_list_lock);
--
1.7.8.4
Do not close protocol driver until device has been unregistered.
This fixes a race between tty_close and hci_dev_open which can result in
a NULL-pointer dereference.
The line discipline closes the protocol driver while we may still have
hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointer
dereference when lock is acquired and hci_init_req called.
Bug is 100% reproducible using hciattach and a disconnected serial port:
0. # hciattach -n ttyO1 any noflow
1. hci_dev_open called from hci_power_on grabs req lock
2. hci_init_req executes but device fails to initialise (times out
eventually)
3. hci_dev_open is called from hci_sock_ioctl and sleeps on req lock
4. hci_uart_tty_close detaches protocol driver and cancels init req
5. hci_dev_open (1) releases req lock
6. hci_dev_open (3) grabs req lock, calls hci_init_req, which triggers oops
when request is prepared in hci_uart_send_frame
[ 137.201263] Unable to handle kernel NULL pointer dereference at virtual address 00000028
[ 137.209838] pgd = c0004000
[ 137.212677] [00000028] *pgd=00000000
[ 137.216430] Internal error: Oops: 17 [#1]
[ 137.220642] Modules linked in:
[ 137.223846] CPU: 0 Tainted: G W (3.3.0-rc6-dirty #406)
[ 137.230529] PC is at __lock_acquire+0x5c/0x1ab0
[ 137.235290] LR is at lock_acquire+0x9c/0x128
[ 137.239776] pc : [<c0071490>] lr : [<c00733f8>] psr: 20000093
[ 137.239776] sp : cf869dd8 ip : c0529554 fp : c051c730
[ 137.251800] r10: 00000000 r9 : cf8673c0 r8 : 00000080
[ 137.257293] r7 : 00000028 r6 : 00000002 r5 : 00000000 r4 : c053fd70
[ 137.264129] r3 : 00000000 r2 : 00000000 r1 : 00000000 r0 : 00000001
[ 137.270965] Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel
[ 137.278717] Control: 10c5387d Table: 8f0f4019 DAC: 00000015
[ 137.284729] Process kworker/u:1 (pid: 7, stack limit = 0xcf8682e8)
[ 137.291229] Stack: (0xcf869dd8 to 0xcf86a000)
[ 137.295776] 9dc0: c0529554 00000000
[ 137.304351] 9de0: cf8673c0 cf868000 d03ea1ef cf868000 000001ef 00000470 00000000 00000002
[ 137.312927] 9e00: cf8673c0 00000001 c051c730 c00716ec 0000000c 00000440 c0529554 00000001
[ 137.321533] 9e20: c051c730 cf868000 d03ea1f3 00000000 c053b978 00000000 00000028 cf868000
[ 137.330078] 9e40: 00000000 00000000 00000002 00000000 00000000 c00733f8 00000002 00000080
[ 137.338684] 9e60: 00000000 c02a1d50 00000000 00000001 60000013 c0969a1c 60000093 c053b96c
[ 137.347259] 9e80: 00000002 00000018 20000013 c02a1d50 cf0ac000 00000000 00000002 cf868000
[ 137.355834] 9ea0: 00000089 c0374130 00000002 00000000 c02a1d50 cf0ac000 0000000c cf0fc540
[ 137.364410] 9ec0: 00000018 c02a1d50 cf0fc540 00000000 cf0fc540 c0282238 c028220c cf178d80
[ 137.372985] 9ee0: 127525d8 c02821cc 9a1fa451 c032727c 9a1fa451 127525d8 cf0fc540 cf0ac4ec
[ 137.381561] 9f00: cf0ac000 cf0fc540 cf0ac584 c03285f4 c0328580 cf0ac4ec cf85c740 c05510cc
[ 137.390136] 9f20: ce825400 c004c914 00000002 00000000 c004c884 ce8254f5 cf869f48 00000000
[ 137.398712] 9f40: c0328580 ce825415 c0a7f914 c061af64 00000000 c048cf3c cf8673c0 cf85c740
[ 137.407287] 9f60: c05510cc c051a66c c05510ec c05510c4 cf85c750 cf868000 00000089 c004d6ac
[ 137.415863] 9f80: 00000000 c0073d14 00000001 cf853ed8 cf85c740 c004d558 00000013 00000000
[ 137.424438] 9fa0: 00000000 00000000 00000000 c00516b0 00000000 00000000 cf85c740 00000000
[ 137.433013] 9fc0: 00000001 dead4ead ffffffff ffffffff c0551674 00000000 00000000 c0450aa4
[ 137.441589] 9fe0: cf869fe0 cf869fe0 cf853ed8 c005162c c0013b30 c0013b30 00ffff00 00ffff00
[ 137.450164] [<c0071490>] (__lock_acquire+0x5c/0x1ab0) from [<c00733f8>] (lock_acquire+0x9c/0x128)
[ 137.459503] [<c00733f8>] (lock_acquire+0x9c/0x128) from [<c0374130>] (_raw_spin_lock_irqsave+0x44/0x58)
[ 137.469360] [<c0374130>] (_raw_spin_lock_irqsave+0x44/0x58) from [<c02a1d50>] (skb_queue_tail+0x18/0x48)
[ 137.479339] [<c02a1d50>] (skb_queue_tail+0x18/0x48) from [<c0282238>] (h4_enqueue+0x2c/0x34)
[ 137.488189] [<c0282238>] (h4_enqueue+0x2c/0x34) from [<c02821cc>] (hci_uart_send_frame+0x34/0x68)
[ 137.497497] [<c02821cc>] (hci_uart_send_frame+0x34/0x68) from [<c032727c>] (hci_send_frame+0x50/0x88)
[ 137.507171] [<c032727c>] (hci_send_frame+0x50/0x88) from [<c03285f4>] (hci_cmd_work+0x74/0xd4)
[ 137.516204] [<c03285f4>] (hci_cmd_work+0x74/0xd4) from [<c004c914>] (process_one_work+0x1a0/0x4ec)
[ 137.525604] [<c004c914>] (process_one_work+0x1a0/0x4ec) from [<c004d6ac>] (worker_thread+0x154/0x344)
[ 137.535278] [<c004d6ac>] (worker_thread+0x154/0x344) from [<c00516b0>] (kthread+0x84/0x90)
[ 137.543975] [<c00516b0>] (kthread+0x84/0x90) from [<c0013b30>] (kernel_thread_exit+0x0/0x8)
[ 137.552734] Code: e59f4e5c e5941000 e3510000 0a000031 (e5971000)
[ 137.559234] ---[ end trace 1b75b31a2719ed1e ]---
Cc: stable <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/bluetooth/hci_ldisc.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index fd5adb4..98a8c05 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -299,11 +299,11 @@ static void hci_uart_tty_close(struct tty_struct *tty)
hci_uart_close(hdev);
if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
- hu->proto->close(hu);
if (hdev) {
hci_unregister_dev(hdev);
hci_free_dev(hdev);
}
+ hu->proto->close(hu);
}
kfree(hu);
--
1.7.8.4
On Wed, Mar 14, 2012 at 11:16:54AM -0700, Marcel Holtmann wrote:
> > > This is a revised series which also contains a minimal fix to the memory leak
> > > discovered by David Hermann upon which the first NULL-pointer-dereference fix
> > > also depends.
> > >
> > > These patches need to get to Linus ASAP as the problems are present in 3.3-rc6
> > > as well as earlier kernels and thus should be backported to the stable trees as
> > > well.
> >
> > Any chance to get these into 3.3? Otherwise, is it possible to rebase
> > bluetooth-next on top of these so that Greg can get them into 3.3.1 (and
> > the other stable trees) once bluetooth-next is merged?
> >
> > All three bugs can be used to crash any kernel with HCI-UART support and
> > can probably be used for exploits as they are extremely easy to trigger
> > reliably.
>
> only if you have access to the TTY device node in the first place. If
> you do not have access to that device node, you can not crash the
> kernel.
>
> Can you resend a clean set of patches for bluetooth-next and once we
> have that merged, we can talk on how to backport this to 3.3 and also
> -stable.
I'll respond to this mail with the two NULL-deref fixes against
bluetooth-next of today (44e612b3e6566f0b).
As I've mentioned before, a fix for the memory leak is already in
bluetooth-next and my first patch depends on it. Unfortunately, the
memory-leak fix in bluetooth-next is not a minimal fix but a more
invasive one:
797fe796c4335b3 ("Bluetooth: uart-ldisc: Fix memory leak and
remove destruct cb")
and it also depends on a second commit (from bluetooth-next):
010666a126fce7b ("Bluetooth: Make hci-destruct callback
optional")
Neither is marked for stable (and at least the latter probably shouldn't
be).
Please make sure that the memory leak fix also gets backported to
stable. A minimal (2-line) fix can be found here:
http://marc.info/?l=linux-bluetooth&m=133130797428708&w=2
Thanks,
Johan
Hi Johan,
> > This is a revised series which also contains a minimal fix to the memory leak
> > discovered by David Hermann upon which the first NULL-pointer-dereference fix
> > also depends.
> >
> > These patches need to get to Linus ASAP as the problems are present in 3.3-rc6
> > as well as earlier kernels and thus should be backported to the stable trees as
> > well.
>
> Any chance to get these into 3.3? Otherwise, is it possible to rebase
> bluetooth-next on top of these so that Greg can get them into 3.3.1 (and
> the other stable trees) once bluetooth-next is merged?
>
> All three bugs can be used to crash any kernel with HCI-UART support and
> can probably be used for exploits as they are extremely easy to trigger
> reliably.
only if you have access to the TTY device node in the first place. If
you do not have access to that device node, you can not crash the
kernel.
Can you resend a clean set of patches for bluetooth-next and once we
have that merged, we can talk on how to backport this to 3.3 and also
-stable.
Regards
Marcel
Hi Marcel and Gustavo,
On Fri, Mar 09, 2012 at 04:43:23PM +0100, Johan Hovold wrote:
> This is a revised series which also contains a minimal fix to the memory leak
> discovered by David Hermann upon which the first NULL-pointer-dereference fix
> also depends.
>
> These patches need to get to Linus ASAP as the problems are present in 3.3-rc6
> as well as earlier kernels and thus should be backported to the stable trees as
> well.
Any chance to get these into 3.3? Otherwise, is it possible to rebase
bluetooth-next on top of these so that Greg can get them into 3.3.1 (and
the other stable trees) once bluetooth-next is merged?
All three bugs can be used to crash any kernel with HCI-UART support and
can probably be used for exploits as they are extremely easy to trigger
reliably.
Thanks,
Johan
On Fri, Mar 09, 2012 at 04:43:25PM +0100, Johan Hovold wrote:
> Do not close protocol driver until device has been unregistered.
>
> This fixes a race between tty_close and hci_dev_open which can result in
> a NULL-pointer dereference.
>
> The line discipline closes the protocol driver while we may still have
> hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointer
> dereference when lock is acquired and hci_init_req called.
[...]
> Cc: stable <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
David (Herrmann), I forgot to add your Reviewed-by on this one. Feel
free to add it again if you want to.
Thanks,
Johan
> ---
> drivers/bluetooth/hci_ldisc.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 97c5faa..5119c4b 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -309,11 +309,11 @@ static void hci_uart_tty_close(struct tty_struct *tty)
> hci_uart_close(hdev);
>
> if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
> - hu->proto->close(hu);
> if (hdev) {
> hci_unregister_dev(hdev);
> hci_free_dev(hdev);
> }
> + hu->proto->close(hu);
> }
> kfree(hu);
> }
Do not close protocol driver until device has been unregistered.
This fixes a race between tty_close and hci_dev_open which can result in
a NULL-pointer dereference.
The line discipline closes the protocol driver while we may still have
hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointer
dereference when lock is acquired and hci_init_req called.
Bug is 100% reproducible using hciattach and a disconnected serial port:
0. # hciattach -n ttyO1 any noflow
1. hci_dev_open called from hci_power_on grabs req lock
2. hci_init_req executes but device fails to initialise (times out
eventually)
3. hci_dev_open is called from hci_sock_ioctl and sleeps on req lock
4. hci_uart_tty_close detaches protocol driver and cancels init req
5. hci_dev_open (1) releases req lock
6. hci_dev_open (3) grabs req lock, calls hci_init_req, which triggers oops
when request is prepared in hci_uart_send_frame
[ 137.201263] Unable to handle kernel NULL pointer dereference at virtual address 00000028
[ 137.209838] pgd = c0004000
[ 137.212677] [00000028] *pgd=00000000
[ 137.216430] Internal error: Oops: 17 [#1]
[ 137.220642] Modules linked in:
[ 137.223846] CPU: 0 Tainted: G W (3.3.0-rc6-dirty #406)
[ 137.230529] PC is at __lock_acquire+0x5c/0x1ab0
[ 137.235290] LR is at lock_acquire+0x9c/0x128
[ 137.239776] pc : [<c0071490>] lr : [<c00733f8>] psr: 20000093
[ 137.239776] sp : cf869dd8 ip : c0529554 fp : c051c730
[ 137.251800] r10: 00000000 r9 : cf8673c0 r8 : 00000080
[ 137.257293] r7 : 00000028 r6 : 00000002 r5 : 00000000 r4 : c053fd70
[ 137.264129] r3 : 00000000 r2 : 00000000 r1 : 00000000 r0 : 00000001
[ 137.270965] Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel
[ 137.278717] Control: 10c5387d Table: 8f0f4019 DAC: 00000015
[ 137.284729] Process kworker/u:1 (pid: 7, stack limit = 0xcf8682e8)
[ 137.291229] Stack: (0xcf869dd8 to 0xcf86a000)
[ 137.295776] 9dc0: c0529554 00000000
[ 137.304351] 9de0: cf8673c0 cf868000 d03ea1ef cf868000 000001ef 00000470 00000000 00000002
[ 137.312927] 9e00: cf8673c0 00000001 c051c730 c00716ec 0000000c 00000440 c0529554 00000001
[ 137.321533] 9e20: c051c730 cf868000 d03ea1f3 00000000 c053b978 00000000 00000028 cf868000
[ 137.330078] 9e40: 00000000 00000000 00000002 00000000 00000000 c00733f8 00000002 00000080
[ 137.338684] 9e60: 00000000 c02a1d50 00000000 00000001 60000013 c0969a1c 60000093 c053b96c
[ 137.347259] 9e80: 00000002 00000018 20000013 c02a1d50 cf0ac000 00000000 00000002 cf868000
[ 137.355834] 9ea0: 00000089 c0374130 00000002 00000000 c02a1d50 cf0ac000 0000000c cf0fc540
[ 137.364410] 9ec0: 00000018 c02a1d50 cf0fc540 00000000 cf0fc540 c0282238 c028220c cf178d80
[ 137.372985] 9ee0: 127525d8 c02821cc 9a1fa451 c032727c 9a1fa451 127525d8 cf0fc540 cf0ac4ec
[ 137.381561] 9f00: cf0ac000 cf0fc540 cf0ac584 c03285f4 c0328580 cf0ac4ec cf85c740 c05510cc
[ 137.390136] 9f20: ce825400 c004c914 00000002 00000000 c004c884 ce8254f5 cf869f48 00000000
[ 137.398712] 9f40: c0328580 ce825415 c0a7f914 c061af64 00000000 c048cf3c cf8673c0 cf85c740
[ 137.407287] 9f60: c05510cc c051a66c c05510ec c05510c4 cf85c750 cf868000 00000089 c004d6ac
[ 137.415863] 9f80: 00000000 c0073d14 00000001 cf853ed8 cf85c740 c004d558 00000013 00000000
[ 137.424438] 9fa0: 00000000 00000000 00000000 c00516b0 00000000 00000000 cf85c740 00000000
[ 137.433013] 9fc0: 00000001 dead4ead ffffffff ffffffff c0551674 00000000 00000000 c0450aa4
[ 137.441589] 9fe0: cf869fe0 cf869fe0 cf853ed8 c005162c c0013b30 c0013b30 00ffff00 00ffff00
[ 137.450164] [<c0071490>] (__lock_acquire+0x5c/0x1ab0) from [<c00733f8>] (lock_acquire+0x9c/0x128)
[ 137.459503] [<c00733f8>] (lock_acquire+0x9c/0x128) from [<c0374130>] (_raw_spin_lock_irqsave+0x44/0x58)
[ 137.469360] [<c0374130>] (_raw_spin_lock_irqsave+0x44/0x58) from [<c02a1d50>] (skb_queue_tail+0x18/0x48)
[ 137.479339] [<c02a1d50>] (skb_queue_tail+0x18/0x48) from [<c0282238>] (h4_enqueue+0x2c/0x34)
[ 137.488189] [<c0282238>] (h4_enqueue+0x2c/0x34) from [<c02821cc>] (hci_uart_send_frame+0x34/0x68)
[ 137.497497] [<c02821cc>] (hci_uart_send_frame+0x34/0x68) from [<c032727c>] (hci_send_frame+0x50/0x88)
[ 137.507171] [<c032727c>] (hci_send_frame+0x50/0x88) from [<c03285f4>] (hci_cmd_work+0x74/0xd4)
[ 137.516204] [<c03285f4>] (hci_cmd_work+0x74/0xd4) from [<c004c914>] (process_one_work+0x1a0/0x4ec)
[ 137.525604] [<c004c914>] (process_one_work+0x1a0/0x4ec) from [<c004d6ac>] (worker_thread+0x154/0x344)
[ 137.535278] [<c004d6ac>] (worker_thread+0x154/0x344) from [<c00516b0>] (kthread+0x84/0x90)
[ 137.543975] [<c00516b0>] (kthread+0x84/0x90) from [<c0013b30>] (kernel_thread_exit+0x0/0x8)
[ 137.552734] Code: e59f4e5c e5941000 e3510000 0a000031 (e5971000)
[ 137.559234] ---[ end trace 1b75b31a2719ed1e ]---
Cc: stable <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/bluetooth/hci_ldisc.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 97c5faa..5119c4b 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -309,11 +309,11 @@ static void hci_uart_tty_close(struct tty_struct *tty)
hci_uart_close(hdev);
if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
- hu->proto->close(hu);
if (hdev) {
hci_unregister_dev(hdev);
hci_free_dev(hdev);
}
+ hu->proto->close(hu);
}
kfree(hu);
}
--
1.7.8.4
Make sure hci_dev_open returns immediately if hci_dev_unregister has
been called.
This fixes a race between hci_dev_open and hci_dev_unregister which can
lead to a NULL-pointer dereference.
Bug is 100% reproducible using hciattach and a disconnected serial port:
0. # hciattach -n /dev/ttyO1 any noflow
1. hci_dev_open called from hci_power_on grabs req lock
2. hci_init_req executes but device fails to initialise (times out
eventually)
3. hci_dev_open is called from hci_sock_ioctl and sleeps on req lock
4. hci_uart_tty_close calls hci_dev_unregister and sleeps on req lock in
hci_dev_do_close
5. hci_dev_open (1) releases req lock
6. hci_dev_do_close grabs req lock and returns as device is not up
7. hci_dev_unregister sleeps in destroy_workqueue
8. hci_dev_open (3) grabs req lock, calls hci_init_req and eventually sleeps
9. hci_dev_unregister finishes, while hci_dev_open is still running...
[ 79.627136] INFO: trying to register non-static key.
[ 79.632354] the code is fine but needs lockdep annotation.
[ 79.638122] turning off the locking correctness validator.
[ 79.643920] [<c00188bc>] (unwind_backtrace+0x0/0xf8) from [<c00729c4>] (__lock_acquire+0x1590/0x1ab0)
[ 79.653594] [<c00729c4>] (__lock_acquire+0x1590/0x1ab0) from [<c00733f8>] (lock_acquire+0x9c/0x128)
[ 79.663085] [<c00733f8>] (lock_acquire+0x9c/0x128) from [<c0040a88>] (run_timer_softirq+0x150/0x3ac)
[ 79.672668] [<c0040a88>] (run_timer_softirq+0x150/0x3ac) from [<c003a3b8>] (__do_softirq+0xd4/0x22c)
[ 79.682281] [<c003a3b8>] (__do_softirq+0xd4/0x22c) from [<c003a924>] (irq_exit+0x8c/0x94)
[ 79.690856] [<c003a924>] (irq_exit+0x8c/0x94) from [<c0013a50>] (handle_IRQ+0x34/0x84)
[ 79.699157] [<c0013a50>] (handle_IRQ+0x34/0x84) from [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c)
[ 79.708648] [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c) from [<c037499c>] (__irq_usr+0x3c/0x60)
[ 79.718048] Exception stack(0xcf281fb0 to 0xcf281ff8)
[ 79.723358] 1fa0: 0001e6a0 be8dab00 0001e698 00036698
[ 79.731933] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000000 00000004 00000000
[ 79.740509] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 ffffffff
[ 79.747497] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[ 79.756011] pgd = cf3b4000
[ 79.758850] [00000000] *pgd=8f0c7831, *pte=00000000, *ppte=00000000
[ 79.765502] Internal error: Oops: 80000007 [#1]
[ 79.770294] Modules linked in:
[ 79.773529] CPU: 0 Tainted: G W (3.3.0-rc6-00002-gb5d5c87 #421)
[ 79.781066] PC is at 0x0
[ 79.783721] LR is at run_timer_softirq+0x16c/0x3ac
[ 79.788787] pc : [<00000000>] lr : [<c0040aa4>] psr: 60000113
[ 79.788787] sp : cf281ee0 ip : 00000000 fp : cf280000
[ 79.800903] r10: 00000004 r9 : 00000100 r8 : b6f234d0
[ 79.806427] r7 : c0519c28 r6 : cf093488 r5 : c0561a00 r4 : 00000000
[ 79.813323] r3 : 00000000 r2 : c054eee0 r1 : 00000001 r0 : 00000000
[ 79.820190] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 79.827728] Control: 10c5387d Table: 8f3b4019 DAC: 00000015
[ 79.833801] Process gpsd (pid: 1265, stack limit = 0xcf2802e8)
[ 79.839965] Stack: (0xcf281ee0 to 0xcf282000)
[ 79.844573] 1ee0: 00000002 00000000 c0040a24 00000000 00000002 cf281f08 00200200 00000000
[ 79.853210] 1f00: 00000000 cf281f18 cf281f08 00000000 00000000 00000000 cf281f18 cf281f18
[ 79.861816] 1f20: 00000000 00000001 c056184c 00000000 00000001 b6f234d0 c0561848 00000004
[ 79.870452] 1f40: cf280000 c003a3b8 c051e79c 00000001 00000000 00000100 3fa9e7b8 0000000a
[ 79.879089] 1f60: 00000025 cf280000 00000025 00000000 00000000 b6f234d0 00000000 00000004
[ 79.887756] 1f80: 00000000 c003a924 c053ad38 c0013a50 fa200000 cf281fb0 ffffffff c0008530
[ 79.896362] 1fa0: 0001e6a0 0000aab8 80000010 c037499c 0001e6a0 be8dab00 0001e698 00036698
[ 79.904998] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000000 00000004 00000000
[ 79.913665] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 ffffffff 00fbf700 04ffff00
[ 79.922302] [<c0040aa4>] (run_timer_softirq+0x16c/0x3ac) from [<c003a3b8>] (__do_softirq+0xd4/0x22c)
[ 79.931945] [<c003a3b8>] (__do_softirq+0xd4/0x22c) from [<c003a924>] (irq_exit+0x8c/0x94)
[ 79.940582] [<c003a924>] (irq_exit+0x8c/0x94) from [<c0013a50>] (handle_IRQ+0x34/0x84)
[ 79.948913] [<c0013a50>] (handle_IRQ+0x34/0x84) from [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c)
[ 79.958404] [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c) from [<c037499c>] (__irq_usr+0x3c/0x60)
[ 79.967773] Exception stack(0xcf281fb0 to 0xcf281ff8)
[ 79.973083] 1fa0: 0001e6a0 be8dab00 0001e698 00036698
[ 79.981658] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000000 00000004 00000000
[ 79.990234] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 ffffffff
[ 79.997161] Code: bad PC value
[ 80.000396] ---[ end trace 6f6739840475f9ee ]---
[ 80.005279] Kernel panic - not syncing: Fatal exception in interrupt
Cc: stable <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
include/net/bluetooth/hci.h | 2 ++
net/bluetooth/hci_core.c | 7 +++++++
2 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 00596e8..e8879b9 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -93,6 +93,8 @@ enum {
* states from the controller.
*/
enum {
+ HCI_UNREGISTER,
+
HCI_LE_SCAN,
};
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 5aeb624..9a01451 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -525,6 +525,11 @@ int hci_dev_open(__u16 dev)
hci_req_lock(hdev);
+ if (test_bit(HCI_UNREGISTER, &hdev->dev_flags)) {
+ ret = -ENODEV;
+ goto done;
+ }
+
if (hdev->rfkill && rfkill_blocked(hdev->rfkill)) {
ret = -ERFKILL;
goto done;
@@ -1577,6 +1582,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
+ set_bit(HCI_UNREGISTER, &hdev->dev_flags);
+
write_lock(&hci_dev_list_lock);
list_del(&hdev->list);
write_unlock(&hci_dev_list_lock);
--
1.7.8.4
The hci_uart is never freed when tty is closed if the protocol has not
been set.
This was discovered by David Hermann and fixed in bluetooth-next. Those
patches are more intrusive as they remove the mandatory destructor
completely.
This is the minimal fix, which leaves the destructor as an empty dummy
call for now.
Cc: David Herrmann <[email protected]>
Cc: stable <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/bluetooth/hci_ldisc.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 0711448..97c5faa 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -237,7 +237,6 @@ static void hci_uart_destruct(struct hci_dev *hdev)
return;
BT_DBG("%s", hdev->name);
- kfree(hdev->driver_data);
}
/* ------ LDISC part ------ */
@@ -316,6 +315,7 @@ static void hci_uart_tty_close(struct tty_struct *tty)
hci_free_dev(hdev);
}
}
+ kfree(hu);
}
}
--
1.7.8.4
On Fri, Mar 09, 2012 at 03:35:46PM +0100, David Herrmann wrote:
> On Fri, Mar 9, 2012 at 3:29 PM, Johan Hovold <[email protected]> wrote:
> > On Fri, Mar 09, 2012 at 02:44:30PM +0100, David Herrmann wrote:
> >> On Wed, Mar 7, 2012 at 5:01 PM, Johan Hovold <[email protected]> wrote:
> >> > Do not close protocol driver until device has been unregistered.
> >> >
> >> > This fixes a race between tty_close and hci_dev_open which can result in
> >> > a NULL-pointer dereference.
> >> >
> >> > The line discipline closes the protocol driver while we may still have
> >> > hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointer
> >> > dereference when lock is acquired and hci_init_req called.
> >
> > [...]
> >
> >> > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> >> > index 0711448..6946081 100644
> >> > --- a/drivers/bluetooth/hci_ldisc.c
> >> > +++ b/drivers/bluetooth/hci_ldisc.c
> >> > @@ -310,11 +310,11 @@ static void hci_uart_tty_close(struct tty_struct *tty)
> >> > ? ? ? ? ? ? ? ? ? ? ? ?hci_uart_close(hdev);
> >> >
> >> > ? ? ? ? ? ? ? ?if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
> >> > - ? ? ? ? ? ? ? ? ? ? ? hu->proto->close(hu);
> >> > ? ? ? ? ? ? ? ? ? ? ? ?if (hdev) {
> >> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?hci_unregister_dev(hdev);
> >> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?hci_free_dev(hdev);
> >> > ? ? ? ? ? ? ? ? ? ? ? ?}
> >> > + ? ? ? ? ? ? ? ? ? ? ? hu->proto->close(hu);
> >> > ? ? ? ? ? ? ? ?}
> >> > ? ? ? ?}
> >> > ?}
> >>
> >> I can confirm this. hci_uart_set_proto() opens the proto before it
> >> registers the hci device. Hence, we should also unregister the hci
> >> device before closing the proto. I also looked whether this introduces
> >> other race conditions but no proto-callback can be called here as they
> >> are all protected by the tty-layer which synchronizes all
> >> tty-callbacks. Therefore, I think this is the correct fix.
> >>
> >> We can apply this to stable even without the "destruct"-fixes from me
> >> as hu->proto->$cb$() doesn't care whether hdev is valid or not. I
> >> don't think the destruct-fixes are important enough to send them to
> >> stable.
> >
> > Unfortunately hu is is not valid once hci_unregister returns as it will
> > call the destruct callback. So my patch depends on changing this
> > behaviour first. (I could also store a pointer to the protocol before
> > calling unregister in my patch.)
>
> Right, I missed that, sorry.
>
> > Secondly, I must disagree with you regarding whether the memory leak you
> > found is critical enough to be added to the stable trees. We're leaking
> > kernel memory in a deterministic and easily triggered way which could be
> > exploited by a malicious user.
>
> Are you planning on sending a patch to stable-ML or should I do so? How about
> my proposal in the other mail? Could you include this fix when resending this?
This needs to go in through the bluetooth/networking trees (or their
maintainers at least) so that it gets in to 3.3, otherwise stable will
not pick it up for earlier trees.
I'll post a revised series which includes the minimal fix to the memory
leak so that all three patches can go to Linus and hopefully make it in
before 3.3 is out.
Sounds good?
Thanks,
Johan
On Fri, Mar 09, 2012 at 04:02:21PM +0100, David Herrmann wrote:
> On Fri, Mar 9, 2012 at 3:40 PM, Johan Hovold <[email protected]> wrote:
> > On Fri, Mar 09, 2012 at 02:52:00PM +0100, David Herrmann wrote:
> >> On Fri, Mar 9, 2012 at 2:04 PM, Johan Hovold <[email protected]> wrote:
> >> > On Thu, Mar 08, 2012 at 09:45:22AM -0800, Marcel Holtmann wrote:
> >> >> > > > Do not close protocol driver until device has been unregistered.
> >> >> > > >
> >> >> > > > This fixes a race between tty_close and hci_dev_open which can result in
> >> >> > > > a NULL-pointer dereference.
> >> >> > > >
> >> >> > > > The line discipline closes the protocol driver while we may still have
> >> >> > > > hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointer
> >> >> > > > dereference when lock is acquired and hci_init_req called.
> >> >> >
> >> >> > [...]
> >> >> >
> >> >> > > what kernel version is this against? Our changes in bluetooth-next fixed
> >> >> > > some of the destruct handling.
> >> >> >
> >> >> > This is against the latest rc as it needs to be fixed in 3.3, but I
> >> >> > missed a dependency to bluetooth-next as you point out below.
> >> >> >
> >> >> > > Also hci_unregister_dev should be calling the destruct handler and thus
> >> >> > > your change is now accessing hu but it got freed already.
> >> >> >
> >> >> > You're right, my patch depends on 010666a126fc ("Bluetooth: Make
> >> >> > hci-destruct callback optional") and 797fe796c4 ("Bluetooth: uart-ldisc:
> >> >> > Fix memory leak and remove destruct cb") from bluetooth-next.
> >> >> >
> >> >> > But since the latter one fixes a memory leak it should have been marked
> >> >> > for stable as well as pushed to Linus for 3.3, right?
> >> >>
> >> >> we need to look into this and propose patches for -stable. Is your
> >> >> problem still present with bluetooth-next or not?
> >> >
> >> > Yes, both races are present in bluetooth-next of today (b8622cbd58f34)
> >> > and only takes an additional manual step to trigger (as the core no
> >> > longer tries to open the device twice automatically).
> >> >
> >> > My two patches on top of either the two patches by David Herrmann
> >> > mentioned above or the following minimal fix of the same memory leak
> >> > would be sufficient to fix both races in 3.3:
> >> >
> >> > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> >> > index 0711448..97c5faa 100644
> >> > --- a/drivers/bluetooth/hci_ldisc.c
> >> > +++ b/drivers/bluetooth/hci_ldisc.c
> >> > @@ -237,7 +237,6 @@ static void hci_uart_destruct(struct hci_dev *hdev)
> >> > ? ? ? ? ? ? ? ?return;
> >> >
> >> > ? ? ? ?BT_DBG("%s", hdev->name);
> >> > - ? ? ? kfree(hdev->driver_data);
> >> > ?}
> >> >
> >> > ?/* ------ LDISC part ------ */
> >> > @@ -316,6 +315,7 @@ static void hci_uart_tty_close(struct tty_struct *tty)
> >> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?hci_free_dev(hdev);
> >> > ? ? ? ? ? ? ? ? ? ? ? ?}
> >> > ? ? ? ? ? ? ? ?}
> >> > + ? ? ? ? ? ? ? kfree(hu);
> >> > ? ? ? ?}
> >> > ?}
> >>
> >> The "destruct"-callback was broken in many ways but working around it
> >> without removing it seems wrong.
> >
> > The reason for not doing so would be to keep the fixes minimal and thus
> > more appropriate for the stable trees.
> >
> > Furthermore, according to you patch own description "Several drivers
> > already provide an empty callback" so I didn't consider it to be
> > a problem.
>
> It's just a proposal, feel free to keep your patch. But please include
> a comment in your commit-message that you explicitly avoid using the
> destruct-callback as it is, and always was, broken. Otherwise, it looks
> wrong seeing such a commit.
Agreed.
> Or simply link to the patches that remove the destruct callback in the
> -next tree.
Yes, I would definitely mention those patches.
> >> This memory-leak occurs only if a
> >> tty-device uses the uart-ldisc without a protocol bound to it.
> >> Therefore, I didn't consider it important enough for stable.
> >
> > See my answer to you previous mail regarding this.
> >
> >> However,
> >> if you want to fix this, leave the kfree() inside the destruct
> >> callback but add another kfree() into the hci_uart_close() in an
> >> "else"-clause like this:
> >>
> >> if (test_and_clear_bit(...)) {
> >> } else {
> >> + ? kfree(...);
> >> }
> >
> > You really don't want to free the hci_uart in it's own close method...
> >
> > The hci_uart is allocated in tty_open and should be freed in tty_close.
>
> Oops, I obviously meant hci_uart_tty_close(), sorry.
Ouch. I should have realised it was a typo, sorry.
Thanks,
Johan
On Fri, Mar 9, 2012 at 3:40 PM, Johan Hovold <[email protected]> wrote:
> On Fri, Mar 09, 2012 at 02:52:00PM +0100, David Herrmann wrote:
>> On Fri, Mar 9, 2012 at 2:04 PM, Johan Hovold <[email protected]> wrote:
>> > On Thu, Mar 08, 2012 at 09:45:22AM -0800, Marcel Holtmann wrote:
>> >> > > > Do not close protocol driver until device has been unregistered=
.
>> >> > > >
>> >> > > > This fixes a race between tty_close and hci_dev_open which can =
result in
>> >> > > > a NULL-pointer dereference.
>> >> > > >
>> >> > > > The line discipline closes the protocol driver while we may sti=
ll have
>> >> > > > hci_dev_open sleeping on the req_lock mutex resulting in a NULL=
-pointer
>> >> > > > dereference when lock is acquired and hci_init_req called.
>> >> >
>> >> > [...]
>> >> >
>> >> > > what kernel version is this against? Our changes in bluetooth-nex=
t fixed
>> >> > > some of the destruct handling.
>> >> >
>> >> > This is against the latest rc as it needs to be fixed in 3.3, but I
>> >> > missed a dependency to bluetooth-next as you point out below.
>> >> >
>> >> > > Also hci_unregister_dev should be calling the destruct handler an=
d thus
>> >> > > your change is now accessing hu but it got freed already.
>> >> >
>> >> > You're right, my patch depends on 010666a126fc ("Bluetooth: Make
>> >> > hci-destruct callback optional") and 797fe796c4 ("Bluetooth: uart-l=
disc:
>> >> > Fix memory leak and remove destruct cb") from bluetooth-next.
>> >> >
>> >> > But since the latter one fixes a memory leak it should have been ma=
rked
>> >> > for stable as well as pushed to Linus for 3.3, right?
>> >>
>> >> we need to look into this and propose patches for -stable. Is your
>> >> problem still present with bluetooth-next or not?
>> >
>> > Yes, both races are present in bluetooth-next of today (b8622cbd58f34)
>> > and only takes an additional manual step to trigger (as the core no
>> > longer tries to open the device twice automatically).
>> >
>> > My two patches on top of either the two patches by David Herrmann
>> > mentioned above or the following minimal fix of the same memory leak
>> > would be sufficient to fix both races in 3.3:
>> >
>> > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldi=
sc.c
>> > index 0711448..97c5faa 100644
>> > --- a/drivers/bluetooth/hci_ldisc.c
>> > +++ b/drivers/bluetooth/hci_ldisc.c
>> > @@ -237,7 +237,6 @@ static void hci_uart_destruct(struct hci_dev *hdev=
)
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return;
>> >
>> > =A0 =A0 =A0 =A0BT_DBG("%s", hdev->name);
>> > - =A0 =A0 =A0 kfree(hdev->driver_data);
>> > =A0}
>> >
>> > =A0/* ------ LDISC part ------ */
>> > @@ -316,6 +315,7 @@ static void hci_uart_tty_close(struct tty_struct *=
tty)
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hci_fre=
e_dev(hdev);
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree(hu);
>> > =A0 =A0 =A0 =A0}
>> > =A0}
>>
>> The "destruct"-callback was broken in many ways but working around it
>> without removing it seems wrong.
>
> The reason for not doing so would be to keep the fixes minimal and thus
> more appropriate for the stable trees.
>
> Furthermore, according to you patch own description "Several drivers
> already provide an empty callback" so I didn't consider it to be
> a problem.
It's just a proposal, feel free to keep your patch. But please include
a comment in your commit-message that you explicitly avoid using the
destruct-callback as it is, and always was, broken. Otherwise, it looks
wrong seeing such a commit.
Or simply link to the patches that remove the destruct callback in the
-next tree.
>> This memory-leak occurs only if a
>> tty-device uses the uart-ldisc without a protocol bound to it.
>> Therefore, I didn't consider it important enough for stable.
>
> See my answer to you previous mail regarding this.
>
>> However,
>> if you want to fix this, leave the kfree() inside the destruct
>> callback but add another kfree() into the hci_uart_close() in an
>> "else"-clause like this:
>>
>> if (test_and_clear_bit(...)) {
>> } else {
>> + =A0 kfree(...);
>> }
>
> You really don't want to free the hci_uart in it's own close method...
>
> The hci_uart is allocated in tty_open and should be freed in tty_close.
Oops, I obviously meant hci_uart_tty_close(), sorry.
>> This will still keep the bogus ref-counts inside hci_dev with the
>> destruct() callback but will also free the ldisc if no protocol is
>> set.
>
> Thanks,
> Johan
Hi David,
On Fri, Mar 09, 2012 at 03:04:11PM +0100, David Herrmann wrote:
> On Fri, Mar 9, 2012 at 1:53 PM, Johan Hovold <[email protected]> wrote:
> > Make sure hci_dev_open returns immediately if hci_dev_unregister has
> > been called.
> >
> > This fixes a race between hci_dev_open and hci_dev_unregister which can
> > lead to a NULL-pointer dereference.
> >
> > Bug is 100% reproducible using hciattach and a disconnected serial port:
> >
> > 0. # hciattach -n /dev/ttyO1 any noflow
> >
> > 1. hci_dev_open called from hci_power_on grabs req lock
> > 2. hci_init_req executes but device fails to initialise (times out
> > ? eventually)
> > 3. hci_dev_open is called from hci_sock_ioctl and sleeps on req lock
> > 4. hci_uart_tty_close calls hci_dev_unregister and sleeps on req lock in
> > ? hci_dev_do_close
> > 5. hci_dev_open (1) releases req lock
> > 6. hci_dev_do_close grabs req lock and returns as device is not up
> > 7. hci_dev_unregister sleeps in destroy_workqueue
> > 8. hci_dev_open (3) grabs req lock, calls hci_init_req and eventually sleeps
> > 9. hci_dev_unregister finishes, while hci_dev_open is still running...
[...]
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 00596e8..e8879b9 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -93,6 +93,8 @@ enum {
> > ?* states from the controller.
> > ?*/
> > ?enum {
> > + ? ? ? HCI_UNREGISTER,
> > +
> > ? ? ? ?HCI_LE_SCAN,
> > ?};
> >
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index d6448f0..22b6781 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -525,6 +525,11 @@ int hci_dev_open(__u16 dev)
> >
> > ? ? ? ?hci_req_lock(hdev);
> >
> > + ? ? ? if (test_bit(HCI_UNREGISTER, &hdev->dev_flags)) {
> > + ? ? ? ? ? ? ? ret = -ENODEV;
> > + ? ? ? ? ? ? ? goto done;
> > + ? ? ? }
> > +
>
> Isn't it enough to check for HCI_RUNNING here? We obviously have a
> race here as we take the device with hci_dev_get(), then sleep and
> then we do not check whether the device is still alive. However,
> drivers are required to reset HCI_RUNNING before calling
> hci_unregister_dev() (which is bogus anyway, but its the way we
> handled it in the past) therefore it should be enough for us to check
> for HCI_RUNNING.
I'm afraid this won't work as hci_dev_open is responsible for setting
HCI_RUNNING in the first place (set in hdev->open(hdev) called from
hci_dev_open).
Thanks,
Johan
On Fri, Mar 09, 2012 at 02:52:00PM +0100, David Herrmann wrote:
> On Fri, Mar 9, 2012 at 2:04 PM, Johan Hovold <[email protected]> wrote:
> > On Thu, Mar 08, 2012 at 09:45:22AM -0800, Marcel Holtmann wrote:
> >> > > > Do not close protocol driver until device has been unregistered.
> >> > > >
> >> > > > This fixes a race between tty_close and hci_dev_open which can result in
> >> > > > a NULL-pointer dereference.
> >> > > >
> >> > > > The line discipline closes the protocol driver while we may still have
> >> > > > hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointer
> >> > > > dereference when lock is acquired and hci_init_req called.
> >> >
> >> > [...]
> >> >
> >> > > what kernel version is this against? Our changes in bluetooth-next fixed
> >> > > some of the destruct handling.
> >> >
> >> > This is against the latest rc as it needs to be fixed in 3.3, but I
> >> > missed a dependency to bluetooth-next as you point out below.
> >> >
> >> > > Also hci_unregister_dev should be calling the destruct handler and thus
> >> > > your change is now accessing hu but it got freed already.
> >> >
> >> > You're right, my patch depends on 010666a126fc ("Bluetooth: Make
> >> > hci-destruct callback optional") and 797fe796c4 ("Bluetooth: uart-ldisc:
> >> > Fix memory leak and remove destruct cb") from bluetooth-next.
> >> >
> >> > But since the latter one fixes a memory leak it should have been marked
> >> > for stable as well as pushed to Linus for 3.3, right?
> >>
> >> we need to look into this and propose patches for -stable. Is your
> >> problem still present with bluetooth-next or not?
> >
> > Yes, both races are present in bluetooth-next of today (b8622cbd58f34)
> > and only takes an additional manual step to trigger (as the core no
> > longer tries to open the device twice automatically).
> >
> > My two patches on top of either the two patches by David Herrmann
> > mentioned above or the following minimal fix of the same memory leak
> > would be sufficient to fix both races in 3.3:
> >
> > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> > index 0711448..97c5faa 100644
> > --- a/drivers/bluetooth/hci_ldisc.c
> > +++ b/drivers/bluetooth/hci_ldisc.c
> > @@ -237,7 +237,6 @@ static void hci_uart_destruct(struct hci_dev *hdev)
> > ? ? ? ? ? ? ? ?return;
> >
> > ? ? ? ?BT_DBG("%s", hdev->name);
> > - ? ? ? kfree(hdev->driver_data);
> > ?}
> >
> > ?/* ------ LDISC part ------ */
> > @@ -316,6 +315,7 @@ static void hci_uart_tty_close(struct tty_struct *tty)
> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?hci_free_dev(hdev);
> > ? ? ? ? ? ? ? ? ? ? ? ?}
> > ? ? ? ? ? ? ? ?}
> > + ? ? ? ? ? ? ? kfree(hu);
> > ? ? ? ?}
> > ?}
>
> The "destruct"-callback was broken in many ways but working around it
> without removing it seems wrong.
The reason for not doing so would be to keep the fixes minimal and thus
more appropriate for the stable trees.
Furthermore, according to you patch own description "Several drivers
already provide an empty callback" so I didn't consider it to be
a problem.
> This memory-leak occurs only if a
> tty-device uses the uart-ldisc without a protocol bound to it.
> Therefore, I didn't consider it important enough for stable.
See my answer to you previous mail regarding this.
> However,
> if you want to fix this, leave the kfree() inside the destruct
> callback but add another kfree() into the hci_uart_close() in an
> "else"-clause like this:
>
> if (test_and_clear_bit(...)) {
> } else {
> + kfree(...);
> }
You really don't want to free the hci_uart in it's own close method...
The hci_uart is allocated in tty_open and should be freed in tty_close.
> This will still keep the bogus ref-counts inside hci_dev with the
> destruct() callback but will also free the ldisc if no protocol is
> set.
Thanks,
Johan
On Fri, Mar 9, 2012 at 3:29 PM, Johan Hovold <[email protected]> wrote:
> Hi David,
>
> On Fri, Mar 09, 2012 at 02:44:30PM +0100, David Herrmann wrote:
>> On Wed, Mar 7, 2012 at 5:01 PM, Johan Hovold <[email protected]> wrote:
>> > Do not close protocol driver until device has been unregistered.
>> >
>> > This fixes a race between tty_close and hci_dev_open which can result =
in
>> > a NULL-pointer dereference.
>> >
>> > The line discipline closes the protocol driver while we may still have
>> > hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointe=
r
>> > dereference when lock is acquired and hci_init_req called.
>
> [...]
>
>> > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldi=
sc.c
>> > index 0711448..6946081 100644
>> > --- a/drivers/bluetooth/hci_ldisc.c
>> > +++ b/drivers/bluetooth/hci_ldisc.c
>> > @@ -310,11 +310,11 @@ static void hci_uart_tty_close(struct tty_struct=
*tty)
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hci_uart_close(hdev);
>> >
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (test_and_clear_bit(HCI_UART_PROTO_S=
ET, &hu->flags)) {
>> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 hu->proto->close(hu);
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (hdev) {
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hci_unr=
egister_dev(hdev);
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hci_fre=
e_dev(hdev);
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 hu->proto->close(hu);
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
>> > =A0 =A0 =A0 =A0}
>> > =A0}
>>
>> I can confirm this. hci_uart_set_proto() opens the proto before it
>> registers the hci device. Hence, we should also unregister the hci
>> device before closing the proto. I also looked whether this introduces
>> other race conditions but no proto-callback can be called here as they
>> are all protected by the tty-layer which synchronizes all
>> tty-callbacks. Therefore, I think this is the correct fix.
>>
>> We can apply this to stable even without the "destruct"-fixes from me
>> as hu->proto->$cb$() doesn't care whether hdev is valid or not. I
>> don't think the destruct-fixes are important enough to send them to
>> stable.
>
> Unfortunately hu is is not valid once hci_unregister returns as it will
> call the destruct callback. So my patch depends on changing this
> behaviour first. (I could also store a pointer to the protocol before
> calling unregister in my patch.)
Right, I missed that, sorry.
> Secondly, I must disagree with you regarding whether the memory leak you
> found is critical enough to be added to the stable trees. We're leaking
> kernel memory in a deterministic and easily triggered way which could be
> exploited by a malicious user.
Are you planning on sending a patch to stable-ML or should I do so? How abo=
ut
my proposal in the other mail? Could you include this fix when resending th=
is?
>> Reviewed-by: David Herrmann <[email protected]>
>
> Thanks,
> Johan
Regards
David
Hi David,
On Fri, Mar 09, 2012 at 02:44:30PM +0100, David Herrmann wrote:
> On Wed, Mar 7, 2012 at 5:01 PM, Johan Hovold <[email protected]> wrote:
> > Do not close protocol driver until device has been unregistered.
> >
> > This fixes a race between tty_close and hci_dev_open which can result in
> > a NULL-pointer dereference.
> >
> > The line discipline closes the protocol driver while we may still have
> > hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointer
> > dereference when lock is acquired and hci_init_req called.
[...]
> > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> > index 0711448..6946081 100644
> > --- a/drivers/bluetooth/hci_ldisc.c
> > +++ b/drivers/bluetooth/hci_ldisc.c
> > @@ -310,11 +310,11 @@ static void hci_uart_tty_close(struct tty_struct *tty)
> > ? ? ? ? ? ? ? ? ? ? ? ?hci_uart_close(hdev);
> >
> > ? ? ? ? ? ? ? ?if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
> > - ? ? ? ? ? ? ? ? ? ? ? hu->proto->close(hu);
> > ? ? ? ? ? ? ? ? ? ? ? ?if (hdev) {
> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?hci_unregister_dev(hdev);
> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?hci_free_dev(hdev);
> > ? ? ? ? ? ? ? ? ? ? ? ?}
> > + ? ? ? ? ? ? ? ? ? ? ? hu->proto->close(hu);
> > ? ? ? ? ? ? ? ?}
> > ? ? ? ?}
> > ?}
>
> I can confirm this. hci_uart_set_proto() opens the proto before it
> registers the hci device. Hence, we should also unregister the hci
> device before closing the proto. I also looked whether this introduces
> other race conditions but no proto-callback can be called here as they
> are all protected by the tty-layer which synchronizes all
> tty-callbacks. Therefore, I think this is the correct fix.
>
> We can apply this to stable even without the "destruct"-fixes from me
> as hu->proto->$cb$() doesn't care whether hdev is valid or not. I
> don't think the destruct-fixes are important enough to send them to
> stable.
Unfortunately hu is is not valid once hci_unregister returns as it will
call the destruct callback. So my patch depends on changing this
behaviour first. (I could also store a pointer to the protocol before
calling unregister in my patch.)
Secondly, I must disagree with you regarding whether the memory leak you
found is critical enough to be added to the stable trees. We're leaking
kernel memory in a deterministic and easily triggered way which could be
exploited by a malicious user.
> Reviewed-by: David Herrmann <[email protected]>
Thanks,
Johan
Hi Johan
On Fri, Mar 9, 2012 at 1:53 PM, Johan Hovold <[email protected]> wrote:
> Make sure hci_dev_open returns immediately if hci_dev_unregister has
> been called.
>
> This fixes a race between hci_dev_open and hci_dev_unregister which can
> lead to a NULL-pointer dereference.
>
> Bug is 100% reproducible using hciattach and a disconnected serial port:
>
> 0. # hciattach -n /dev/ttyO1 any noflow
>
> 1. hci_dev_open called from hci_power_on grabs req lock
> 2. hci_init_req executes but device fails to initialise (times out
> =A0 eventually)
> 3. hci_dev_open is called from hci_sock_ioctl and sleeps on req lock
> 4. hci_uart_tty_close calls hci_dev_unregister and sleeps on req lock in
> =A0 hci_dev_do_close
> 5. hci_dev_open (1) releases req lock
> 6. hci_dev_do_close grabs req lock and returns as device is not up
> 7. hci_dev_unregister sleeps in destroy_workqueue
> 8. hci_dev_open (3) grabs req lock, calls hci_init_req and eventually sle=
eps
> 9. hci_dev_unregister finishes, while hci_dev_open is still running...
>
> [ =A0 79.627136] INFO: trying to register non-static key.
> [ =A0 79.632354] the code is fine but needs lockdep annotation.
> [ =A0 79.638122] turning off the locking correctness validator.
> [ =A0 79.643920] [<c00188bc>] (unwind_backtrace+0x0/0xf8) from [<c00729c4=
>] (__lock_acquire+0x1590/0x1ab0)
> [ =A0 79.653594] [<c00729c4>] (__lock_acquire+0x1590/0x1ab0) from [<c0073=
3f8>] (lock_acquire+0x9c/0x128)
> [ =A0 79.663085] [<c00733f8>] (lock_acquire+0x9c/0x128) from [<c0040a88>]=
(run_timer_softirq+0x150/0x3ac)
> [ =A0 79.672668] [<c0040a88>] (run_timer_softirq+0x150/0x3ac) from [<c003=
a3b8>] (__do_softirq+0xd4/0x22c)
> [ =A0 79.682281] [<c003a3b8>] (__do_softirq+0xd4/0x22c) from [<c003a924>]=
(irq_exit+0x8c/0x94)
> [ =A0 79.690856] [<c003a924>] (irq_exit+0x8c/0x94) from [<c0013a50>] (han=
dle_IRQ+0x34/0x84)
> [ =A0 79.699157] [<c0013a50>] (handle_IRQ+0x34/0x84) from [<c0008530>] (o=
map3_intc_handle_irq+0x48/0x4c)
> [ =A0 79.708648] [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c) from [<c0=
37499c>] (__irq_usr+0x3c/0x60)
> [ =A0 79.718048] Exception stack(0xcf281fb0 to 0xcf281ff8)
> [ =A0 79.723358] 1fa0: =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 0001e6a0 be8dab00 0001e698 00036698
> [ =A0 79.731933] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000=
000 00000004 00000000
> [ =A0 79.740509] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 fffff=
fff
> [ =A0 79.747497] Unable to handle kernel NULL pointer dereference at virt=
ual address 00000000
> [ =A0 79.756011] pgd =3D cf3b4000
> [ =A0 79.758850] [00000000] *pgd=3D8f0c7831, *pte=3D00000000, *ppte=3D000=
00000
> [ =A0 79.765502] Internal error: Oops: 80000007 [#1]
> [ =A0 79.770294] Modules linked in:
> [ =A0 79.773529] CPU: 0 =A0 =A0Tainted: G =A0 =A0 =A0 =A0W =A0 =A0 (3.3.0=
-rc6-00002-gb5d5c87 #421)
> [ =A0 79.781066] PC is at 0x0
> [ =A0 79.783721] LR is at run_timer_softirq+0x16c/0x3ac
> [ =A0 79.788787] pc : [<00000000>] =A0 =A0lr : [<c0040aa4>] =A0 =A0psr: 6=
0000113
> [ =A0 79.788787] sp : cf281ee0 =A0ip : 00000000 =A0fp : cf280000
> [ =A0 79.800903] r10: 00000004 =A0r9 : 00000100 =A0r8 : b6f234d0
> [ =A0 79.806427] r7 : c0519c28 =A0r6 : cf093488 =A0r5 : c0561a00 =A0r4 : =
00000000
> [ =A0 79.813323] r3 : 00000000 =A0r2 : c054eee0 =A0r1 : 00000001 =A0r0 : =
00000000
> [ =A0 79.820190] Flags: nZCv =A0IRQs on =A0FIQs on =A0Mode SVC_32 =A0ISA =
ARM =A0Segment user
> [ =A0 79.827728] Control: 10c5387d =A0Table: 8f3b4019 =A0DAC: 00000015
> [ =A0 79.833801] Process gpsd (pid: 1265, stack limit =3D 0xcf2802e8)
> [ =A0 79.839965] Stack: (0xcf281ee0 to 0xcf282000)
> [ =A0 79.844573] 1ee0: 00000002 00000000 c0040a24 00000000 00000002 cf281=
f08 00200200 00000000
> [ =A0 79.853210] 1f00: 00000000 cf281f18 cf281f08 00000000 00000000 00000=
000 cf281f18 cf281f18
> [ =A0 79.861816] 1f20: 00000000 00000001 c056184c 00000000 00000001 b6f23=
4d0 c0561848 00000004
> [ =A0 79.870452] 1f40: cf280000 c003a3b8 c051e79c 00000001 00000000 00000=
100 3fa9e7b8 0000000a
> [ =A0 79.879089] 1f60: 00000025 cf280000 00000025 00000000 00000000 b6f23=
4d0 00000000 00000004
> [ =A0 79.887756] 1f80: 00000000 c003a924 c053ad38 c0013a50 fa200000 cf281=
fb0 ffffffff c0008530
> [ =A0 79.896362] 1fa0: 0001e6a0 0000aab8 80000010 c037499c 0001e6a0 be8da=
b00 0001e698 00036698
> [ =A0 79.904998] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000=
000 00000004 00000000
> [ =A0 79.913665] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 fffff=
fff 00fbf700 04ffff00
> [ =A0 79.922302] [<c0040aa4>] (run_timer_softirq+0x16c/0x3ac) from [<c003=
a3b8>] (__do_softirq+0xd4/0x22c)
> [ =A0 79.931945] [<c003a3b8>] (__do_softirq+0xd4/0x22c) from [<c003a924>]=
(irq_exit+0x8c/0x94)
> [ =A0 79.940582] [<c003a924>] (irq_exit+0x8c/0x94) from [<c0013a50>] (han=
dle_IRQ+0x34/0x84)
> [ =A0 79.948913] [<c0013a50>] (handle_IRQ+0x34/0x84) from [<c0008530>] (o=
map3_intc_handle_irq+0x48/0x4c)
> [ =A0 79.958404] [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c) from [<c0=
37499c>] (__irq_usr+0x3c/0x60)
> [ =A0 79.967773] Exception stack(0xcf281fb0 to 0xcf281ff8)
> [ =A0 79.973083] 1fa0: =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 0001e6a0 be8dab00 0001e698 00036698
> [ =A0 79.981658] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000=
000 00000004 00000000
> [ =A0 79.990234] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 fffff=
fff
> [ =A0 79.997161] Code: bad PC value
> [ =A0 80.000396] ---[ end trace 6f6739840475f9ee ]---
> [ =A0 80.005279] Kernel panic - not syncing: Fatal exception in interrupt
>
> Cc: stable <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
>
> v2: use hdev->dev_flags for internal unregister flag
>
>
> =A0include/net/bluetooth/hci.h | =A0 =A02 ++
> =A0net/bluetooth/hci_core.c =A0 =A0| =A0 =A07 +++++++
> =A02 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 00596e8..e8879b9 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -93,6 +93,8 @@ enum {
> =A0* states from the controller.
> =A0*/
> =A0enum {
> + =A0 =A0 =A0 HCI_UNREGISTER,
> +
> =A0 =A0 =A0 =A0HCI_LE_SCAN,
> =A0};
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index d6448f0..22b6781 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -525,6 +525,11 @@ int hci_dev_open(__u16 dev)
>
> =A0 =A0 =A0 =A0hci_req_lock(hdev);
>
> + =A0 =A0 =A0 if (test_bit(HCI_UNREGISTER, &hdev->dev_flags)) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D -ENODEV;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto done;
> + =A0 =A0 =A0 }
> +
Isn't it enough to check for HCI_RUNNING here? We obviously have a
race here as we take the device with hci_dev_get(), then sleep and
then we do not check whether the device is still alive. However,
drivers are required to reset HCI_RUNNING before calling
hci_unregister_dev() (which is bogus anyway, but its the way we
handled it in the past) therefore it should be enough for us to check
for HCI_RUNNING.
Regards
David
> =A0 =A0 =A0 =A0if (hdev->rfkill && rfkill_blocked(hdev->rfkill)) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D -ERFKILL;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto done;
> @@ -1577,6 +1582,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
>
> =A0 =A0 =A0 =A0BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
>
> + =A0 =A0 =A0 set_bit(HCI_UNREGISTER, &hdev->dev_flags);
> +
> =A0 =A0 =A0 =A0write_lock(&hci_dev_list_lock);
> =A0 =A0 =A0 =A0list_del(&hdev->list);
> =A0 =A0 =A0 =A0write_unlock(&hci_dev_list_lock);
> --
> 1.7.8.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to [email protected]
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html
On Fri, Mar 9, 2012 at 2:04 PM, Johan Hovold <[email protected]> wrote:
> On Thu, Mar 08, 2012 at 09:45:22AM -0800, Marcel Holtmann wrote:
>> Hi Johan,
>>
>> > > > Do not close protocol driver until device has been unregistered.
>> > > >
>> > > > This fixes a race between tty_close and hci_dev_open which can res=
ult in
>> > > > a NULL-pointer dereference.
>> > > >
>> > > > The line discipline closes the protocol driver while we may still =
have
>> > > > hci_dev_open sleeping on the req_lock mutex resulting in a NULL-po=
inter
>> > > > dereference when lock is acquired and hci_init_req called.
>> >
>> > [...]
>> >
>> > > what kernel version is this against? Our changes in bluetooth-next f=
ixed
>> > > some of the destruct handling.
>> >
>> > This is against the latest rc as it needs to be fixed in 3.3, but I
>> > missed a dependency to bluetooth-next as you point out below.
>> >
>> > > Also hci_unregister_dev should be calling the destruct handler and t=
hus
>> > > your change is now accessing hu but it got freed already.
>> >
>> > You're right, my patch depends on 010666a126fc ("Bluetooth: Make
>> > hci-destruct callback optional") and 797fe796c4 ("Bluetooth: uart-ldis=
c:
>> > Fix memory leak and remove destruct cb") from bluetooth-next.
>> >
>> > But since the latter one fixes a memory leak it should have been marke=
d
>> > for stable as well as pushed to Linus for 3.3, right?
>>
>> we need to look into this and propose patches for -stable. Is your
>> problem still present with bluetooth-next or not?
>
> Yes, both races are present in bluetooth-next of today (b8622cbd58f34)
> and only takes an additional manual step to trigger (as the core no
> longer tries to open the device twice automatically).
>
> My two patches on top of either the two patches by David Herrmann
> mentioned above or the following minimal fix of the same memory leak
> would be sufficient to fix both races in 3.3:
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.=
c
> index 0711448..97c5faa 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -237,7 +237,6 @@ static void hci_uart_destruct(struct hci_dev *hdev)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return;
>
> =A0 =A0 =A0 =A0BT_DBG("%s", hdev->name);
> - =A0 =A0 =A0 kfree(hdev->driver_data);
> =A0}
>
> =A0/* ------ LDISC part ------ */
> @@ -316,6 +315,7 @@ static void hci_uart_tty_close(struct tty_struct *tty=
)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hci_free_d=
ev(hdev);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree(hu);
> =A0 =A0 =A0 =A0}
> =A0}
The "destruct"-callback was broken in many ways but working around it
without removing it seems wrong. This memory-leak occurs only if a
tty-device uses the uart-ldisc without a protocol bound to it.
Therefore, I didn't consider it important enough for stable. However,
if you want to fix this, leave the kfree() inside the destruct
callback but add another kfree() into the hci_uart_close() in an
"else"-clause like this:
if (test_and_clear_bit(...)) {
} else {
+ kfree(...);
}
This will still keep the bogus ref-counts inside hci_dev with the
destruct() callback but will also free the ldisc if no protocol is
set.
Regards
David
>
> Thanks,
> Johan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to [email protected]
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html
Hi Johan
On Wed, Mar 7, 2012 at 5:01 PM, Johan Hovold <[email protected]> wrote:
> Do not close protocol driver until device has been unregistered.
>
> This fixes a race between tty_close and hci_dev_open which can result in
> a NULL-pointer dereference.
>
> The line discipline closes the protocol driver while we may still have
> hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointer
> dereference when lock is acquired and hci_init_req called.
>
> Bug is 100% reproducible using hciattach and a disconnected serial port:
>
> 0. # hciattach -n ttyO1 any noflow
>
> 1. hci_dev_open called from hci_power_on grabs req lock
> 2. hci_init_req executes but device fails to initialise (times out
> =A0 eventually)
> 3. hci_dev_open is called from hci_sock_ioctl and sleeps on req lock
> 4. hci_uart_tty_close detaches protocol driver and cancels init req
> 5. hci_dev_open (1) releases req lock
> 6. hci_dev_open (3) grabs req lock, calls hci_init_req, which triggers oo=
ps
> =A0 when request is prepared in hci_uart_send_frame
>
> [ =A0137.201263] Unable to handle kernel NULL pointer dereference at virt=
ual address 00000028
> [ =A0137.209838] pgd =3D c0004000
> [ =A0137.212677] [00000028] *pgd=3D00000000
> [ =A0137.216430] Internal error: Oops: 17 [#1]
> [ =A0137.220642] Modules linked in:
> [ =A0137.223846] CPU: 0 =A0 =A0Tainted: G =A0 =A0 =A0 =A0W =A0 =A0 (3.3.0=
-rc6-dirty #406)
> [ =A0137.230529] PC is at __lock_acquire+0x5c/0x1ab0
> [ =A0137.235290] LR is at lock_acquire+0x9c/0x128
> [ =A0137.239776] pc : [<c0071490>] =A0 =A0lr : [<c00733f8>] =A0 =A0psr: 2=
0000093
> [ =A0137.239776] sp : cf869dd8 =A0ip : c0529554 =A0fp : c051c730
> [ =A0137.251800] r10: 00000000 =A0r9 : cf8673c0 =A0r8 : 00000080
> [ =A0137.257293] r7 : 00000028 =A0r6 : 00000002 =A0r5 : 00000000 =A0r4 : =
c053fd70
> [ =A0137.264129] r3 : 00000000 =A0r2 : 00000000 =A0r1 : 00000000 =A0r0 : =
00000001
> [ =A0137.270965] Flags: nzCv =A0IRQs off =A0FIQs on =A0Mode SVC_32 =A0ISA=
ARM =A0Segment kernel
> [ =A0137.278717] Control: 10c5387d =A0Table: 8f0f4019 =A0DAC: 00000015
> [ =A0137.284729] Process kworker/u:1 (pid: 7, stack limit =3D 0xcf8682e8)
> [ =A0137.291229] Stack: (0xcf869dd8 to 0xcf86a000)
> [ =A0137.295776] 9dc0: =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 c0529554 000000=
00
> [ =A0137.304351] 9de0: cf8673c0 cf868000 d03ea1ef cf868000 000001ef 00000=
470 00000000 00000002
> [ =A0137.312927] 9e00: cf8673c0 00000001 c051c730 c00716ec 0000000c 00000=
440 c0529554 00000001
> [ =A0137.321533] 9e20: c051c730 cf868000 d03ea1f3 00000000 c053b978 00000=
000 00000028 cf868000
> [ =A0137.330078] 9e40: 00000000 00000000 00000002 00000000 00000000 c0073=
3f8 00000002 00000080
> [ =A0137.338684] 9e60: 00000000 c02a1d50 00000000 00000001 60000013 c0969=
a1c 60000093 c053b96c
> [ =A0137.347259] 9e80: 00000002 00000018 20000013 c02a1d50 cf0ac000 00000=
000 00000002 cf868000
> [ =A0137.355834] 9ea0: 00000089 c0374130 00000002 00000000 c02a1d50 cf0ac=
000 0000000c cf0fc540
> [ =A0137.364410] 9ec0: 00000018 c02a1d50 cf0fc540 00000000 cf0fc540 c0282=
238 c028220c cf178d80
> [ =A0137.372985] 9ee0: 127525d8 c02821cc 9a1fa451 c032727c 9a1fa451 12752=
5d8 cf0fc540 cf0ac4ec
> [ =A0137.381561] 9f00: cf0ac000 cf0fc540 cf0ac584 c03285f4 c0328580 cf0ac=
4ec cf85c740 c05510cc
> [ =A0137.390136] 9f20: ce825400 c004c914 00000002 00000000 c004c884 ce825=
4f5 cf869f48 00000000
> [ =A0137.398712] 9f40: c0328580 ce825415 c0a7f914 c061af64 00000000 c048c=
f3c cf8673c0 cf85c740
> [ =A0137.407287] 9f60: c05510cc c051a66c c05510ec c05510c4 cf85c750 cf868=
000 00000089 c004d6ac
> [ =A0137.415863] 9f80: 00000000 c0073d14 00000001 cf853ed8 cf85c740 c004d=
558 00000013 00000000
> [ =A0137.424438] 9fa0: 00000000 00000000 00000000 c00516b0 00000000 00000=
000 cf85c740 00000000
> [ =A0137.433013] 9fc0: 00000001 dead4ead ffffffff ffffffff c0551674 00000=
000 00000000 c0450aa4
> [ =A0137.441589] 9fe0: cf869fe0 cf869fe0 cf853ed8 c005162c c0013b30 c0013=
b30 00ffff00 00ffff00
> [ =A0137.450164] [<c0071490>] (__lock_acquire+0x5c/0x1ab0) from [<c00733f=
8>] (lock_acquire+0x9c/0x128)
> [ =A0137.459503] [<c00733f8>] (lock_acquire+0x9c/0x128) from [<c0374130>]=
(_raw_spin_lock_irqsave+0x44/0x58)
> [ =A0137.469360] [<c0374130>] (_raw_spin_lock_irqsave+0x44/0x58) from [<c=
02a1d50>] (skb_queue_tail+0x18/0x48)
> [ =A0137.479339] [<c02a1d50>] (skb_queue_tail+0x18/0x48) from [<c0282238>=
] (h4_enqueue+0x2c/0x34)
> [ =A0137.488189] [<c0282238>] (h4_enqueue+0x2c/0x34) from [<c02821cc>] (h=
ci_uart_send_frame+0x34/0x68)
> [ =A0137.497497] [<c02821cc>] (hci_uart_send_frame+0x34/0x68) from [<c032=
727c>] (hci_send_frame+0x50/0x88)
> [ =A0137.507171] [<c032727c>] (hci_send_frame+0x50/0x88) from [<c03285f4>=
] (hci_cmd_work+0x74/0xd4)
> [ =A0137.516204] [<c03285f4>] (hci_cmd_work+0x74/0xd4) from [<c004c914>] =
(process_one_work+0x1a0/0x4ec)
> [ =A0137.525604] [<c004c914>] (process_one_work+0x1a0/0x4ec) from [<c004d=
6ac>] (worker_thread+0x154/0x344)
> [ =A0137.535278] [<c004d6ac>] (worker_thread+0x154/0x344) from [<c00516b0=
>] (kthread+0x84/0x90)
> [ =A0137.543975] [<c00516b0>] (kthread+0x84/0x90) from [<c0013b30>] (kern=
el_thread_exit+0x0/0x8)
> [ =A0137.552734] Code: e59f4e5c e5941000 e3510000 0a000031 (e5971000)
> [ =A0137.559234] ---[ end trace 1b75b31a2719ed1e ]---
>
> Cc: stable <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> =A0drivers/bluetooth/hci_ldisc.c | =A0 =A02 +-
> =A01 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.=
c
> index 0711448..6946081 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -310,11 +310,11 @@ static void hci_uart_tty_close(struct tty_struct *t=
ty)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hci_uart_close(hdev);
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (test_and_clear_bit(HCI_UART_PROTO_SET,=
&hu->flags)) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 hu->proto->close(hu);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (hdev) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hci_unregi=
ster_dev(hdev);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hci_free_d=
ev(hdev);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 hu->proto->close(hu);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0}
> =A0}
I can confirm this. hci_uart_set_proto() opens the proto before it
registers the hci device. Hence, we should also unregister the hci
device before closing the proto. I also looked whether this introduces
other race conditions but no proto-callback can be called here as they
are all protected by the tty-layer which synchronizes all
tty-callbacks. Therefore, I think this is the correct fix.
We can apply this to stable even without the "destruct"-fixes from me
as hu->proto->$cb$() doesn't care whether hdev is valid or not. I
don't think the destruct-fixes are important enough to send them to
stable.
Reviewed-by: David Herrmann <[email protected]>
Regards
David
> --
> 1.7.8.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to [email protected]
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html
On Thu, Mar 08, 2012 at 09:45:22AM -0800, Marcel Holtmann wrote:
> Hi Johan,
>
> > > > Do not close protocol driver until device has been unregistered.
> > > >
> > > > This fixes a race between tty_close and hci_dev_open which can result in
> > > > a NULL-pointer dereference.
> > > >
> > > > The line discipline closes the protocol driver while we may still have
> > > > hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointer
> > > > dereference when lock is acquired and hci_init_req called.
> >
> > [...]
> >
> > > what kernel version is this against? Our changes in bluetooth-next fixed
> > > some of the destruct handling.
> >
> > This is against the latest rc as it needs to be fixed in 3.3, but I
> > missed a dependency to bluetooth-next as you point out below.
> >
> > > Also hci_unregister_dev should be calling the destruct handler and thus
> > > your change is now accessing hu but it got freed already.
> >
> > You're right, my patch depends on 010666a126fc ("Bluetooth: Make
> > hci-destruct callback optional") and 797fe796c4 ("Bluetooth: uart-ldisc:
> > Fix memory leak and remove destruct cb") from bluetooth-next.
> >
> > But since the latter one fixes a memory leak it should have been marked
> > for stable as well as pushed to Linus for 3.3, right?
>
> we need to look into this and propose patches for -stable. Is your
> problem still present with bluetooth-next or not?
Yes, both races are present in bluetooth-next of today (b8622cbd58f34)
and only takes an additional manual step to trigger (as the core no
longer tries to open the device twice automatically).
My two patches on top of either the two patches by David Herrmann
mentioned above or the following minimal fix of the same memory leak
would be sufficient to fix both races in 3.3:
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 0711448..97c5faa 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -237,7 +237,6 @@ static void hci_uart_destruct(struct hci_dev *hdev)
return;
BT_DBG("%s", hdev->name);
- kfree(hdev->driver_data);
}
/* ------ LDISC part ------ */
@@ -316,6 +315,7 @@ static void hci_uart_tty_close(struct tty_struct *tty)
hci_free_dev(hdev);
}
}
+ kfree(hu);
}
}
Thanks,
Johan
Make sure hci_dev_open returns immediately if hci_dev_unregister has
been called.
This fixes a race between hci_dev_open and hci_dev_unregister which can
lead to a NULL-pointer dereference.
Bug is 100% reproducible using hciattach and a disconnected serial port:
0. # hciattach -n /dev/ttyO1 any noflow
1. hci_dev_open called from hci_power_on grabs req lock
2. hci_init_req executes but device fails to initialise (times out
eventually)
3. hci_dev_open is called from hci_sock_ioctl and sleeps on req lock
4. hci_uart_tty_close calls hci_dev_unregister and sleeps on req lock in
hci_dev_do_close
5. hci_dev_open (1) releases req lock
6. hci_dev_do_close grabs req lock and returns as device is not up
7. hci_dev_unregister sleeps in destroy_workqueue
8. hci_dev_open (3) grabs req lock, calls hci_init_req and eventually sleeps
9. hci_dev_unregister finishes, while hci_dev_open is still running...
[ 79.627136] INFO: trying to register non-static key.
[ 79.632354] the code is fine but needs lockdep annotation.
[ 79.638122] turning off the locking correctness validator.
[ 79.643920] [<c00188bc>] (unwind_backtrace+0x0/0xf8) from [<c00729c4>] (__lock_acquire+0x1590/0x1ab0)
[ 79.653594] [<c00729c4>] (__lock_acquire+0x1590/0x1ab0) from [<c00733f8>] (lock_acquire+0x9c/0x128)
[ 79.663085] [<c00733f8>] (lock_acquire+0x9c/0x128) from [<c0040a88>] (run_timer_softirq+0x150/0x3ac)
[ 79.672668] [<c0040a88>] (run_timer_softirq+0x150/0x3ac) from [<c003a3b8>] (__do_softirq+0xd4/0x22c)
[ 79.682281] [<c003a3b8>] (__do_softirq+0xd4/0x22c) from [<c003a924>] (irq_exit+0x8c/0x94)
[ 79.690856] [<c003a924>] (irq_exit+0x8c/0x94) from [<c0013a50>] (handle_IRQ+0x34/0x84)
[ 79.699157] [<c0013a50>] (handle_IRQ+0x34/0x84) from [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c)
[ 79.708648] [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c) from [<c037499c>] (__irq_usr+0x3c/0x60)
[ 79.718048] Exception stack(0xcf281fb0 to 0xcf281ff8)
[ 79.723358] 1fa0: 0001e6a0 be8dab00 0001e698 00036698
[ 79.731933] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000000 00000004 00000000
[ 79.740509] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 ffffffff
[ 79.747497] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[ 79.756011] pgd = cf3b4000
[ 79.758850] [00000000] *pgd=8f0c7831, *pte=00000000, *ppte=00000000
[ 79.765502] Internal error: Oops: 80000007 [#1]
[ 79.770294] Modules linked in:
[ 79.773529] CPU: 0 Tainted: G W (3.3.0-rc6-00002-gb5d5c87 #421)
[ 79.781066] PC is at 0x0
[ 79.783721] LR is at run_timer_softirq+0x16c/0x3ac
[ 79.788787] pc : [<00000000>] lr : [<c0040aa4>] psr: 60000113
[ 79.788787] sp : cf281ee0 ip : 00000000 fp : cf280000
[ 79.800903] r10: 00000004 r9 : 00000100 r8 : b6f234d0
[ 79.806427] r7 : c0519c28 r6 : cf093488 r5 : c0561a00 r4 : 00000000
[ 79.813323] r3 : 00000000 r2 : c054eee0 r1 : 00000001 r0 : 00000000
[ 79.820190] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 79.827728] Control: 10c5387d Table: 8f3b4019 DAC: 00000015
[ 79.833801] Process gpsd (pid: 1265, stack limit = 0xcf2802e8)
[ 79.839965] Stack: (0xcf281ee0 to 0xcf282000)
[ 79.844573] 1ee0: 00000002 00000000 c0040a24 00000000 00000002 cf281f08 00200200 00000000
[ 79.853210] 1f00: 00000000 cf281f18 cf281f08 00000000 00000000 00000000 cf281f18 cf281f18
[ 79.861816] 1f20: 00000000 00000001 c056184c 00000000 00000001 b6f234d0 c0561848 00000004
[ 79.870452] 1f40: cf280000 c003a3b8 c051e79c 00000001 00000000 00000100 3fa9e7b8 0000000a
[ 79.879089] 1f60: 00000025 cf280000 00000025 00000000 00000000 b6f234d0 00000000 00000004
[ 79.887756] 1f80: 00000000 c003a924 c053ad38 c0013a50 fa200000 cf281fb0 ffffffff c0008530
[ 79.896362] 1fa0: 0001e6a0 0000aab8 80000010 c037499c 0001e6a0 be8dab00 0001e698 00036698
[ 79.904998] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000000 00000004 00000000
[ 79.913665] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 ffffffff 00fbf700 04ffff00
[ 79.922302] [<c0040aa4>] (run_timer_softirq+0x16c/0x3ac) from [<c003a3b8>] (__do_softirq+0xd4/0x22c)
[ 79.931945] [<c003a3b8>] (__do_softirq+0xd4/0x22c) from [<c003a924>] (irq_exit+0x8c/0x94)
[ 79.940582] [<c003a924>] (irq_exit+0x8c/0x94) from [<c0013a50>] (handle_IRQ+0x34/0x84)
[ 79.948913] [<c0013a50>] (handle_IRQ+0x34/0x84) from [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c)
[ 79.958404] [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c) from [<c037499c>] (__irq_usr+0x3c/0x60)
[ 79.967773] Exception stack(0xcf281fb0 to 0xcf281ff8)
[ 79.973083] 1fa0: 0001e6a0 be8dab00 0001e698 00036698
[ 79.981658] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000000 00000004 00000000
[ 79.990234] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 ffffffff
[ 79.997161] Code: bad PC value
[ 80.000396] ---[ end trace 6f6739840475f9ee ]---
[ 80.005279] Kernel panic - not syncing: Fatal exception in interrupt
Cc: stable <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
v2: use hdev->dev_flags for internal unregister flag
include/net/bluetooth/hci.h | 2 ++
net/bluetooth/hci_core.c | 7 +++++++
2 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 00596e8..e8879b9 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -93,6 +93,8 @@ enum {
* states from the controller.
*/
enum {
+ HCI_UNREGISTER,
+
HCI_LE_SCAN,
};
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d6448f0..22b6781 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -525,6 +525,11 @@ int hci_dev_open(__u16 dev)
hci_req_lock(hdev);
+ if (test_bit(HCI_UNREGISTER, &hdev->dev_flags)) {
+ ret = -ENODEV;
+ goto done;
+ }
+
if (hdev->rfkill && rfkill_blocked(hdev->rfkill)) {
ret = -ERFKILL;
goto done;
@@ -1577,6 +1582,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
+ set_bit(HCI_UNREGISTER, &hdev->dev_flags);
+
write_lock(&hci_dev_list_lock);
list_del(&hdev->list);
write_unlock(&hci_dev_list_lock);
--
1.7.8.4
Hi Johan,
> > > Do not close protocol driver until device has been unregistered.
> > >
> > > This fixes a race between tty_close and hci_dev_open which can result in
> > > a NULL-pointer dereference.
> > >
> > > The line discipline closes the protocol driver while we may still have
> > > hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointer
> > > dereference when lock is acquired and hci_init_req called.
>
> [...]
>
> > what kernel version is this against? Our changes in bluetooth-next fixed
> > some of the destruct handling.
>
> This is against the latest rc as it needs to be fixed in 3.3, but I
> missed a dependency to bluetooth-next as you point out below.
>
> > Also hci_unregister_dev should be calling the destruct handler and thus
> > your change is now accessing hu but it got freed already.
>
> You're right, my patch depends on 010666a126fc ("Bluetooth: Make
> hci-destruct callback optional") and 797fe796c4 ("Bluetooth: uart-ldisc:
> Fix memory leak and remove destruct cb") from bluetooth-next.
>
> But since the latter one fixes a memory leak it should have been marked
> for stable as well as pushed to Linus for 3.3, right?
we need to look into this and propose patches for -stable. Is your
problem still present with bluetooth-next or not?
Regards
Marcel
Hi Johan,
> > > Make sure hci_dev_open returns immediately if hci_dev_unregister has
> > > been called.
> > >
> > > This fixes a race between hci_dev_open and hci_dev_unregister which can
> > > lead to a NULL-pointer dereference.
>
> [...]
>
> > what version of the kernel is this patch against? Since we cleaned up
> > the flags in bluetooth-next tree. Also in addition you can not just add
> > flags here.
>
> As this to be fixed in 3.3 it is against 3.3-rc6.
>
> > > /*
> > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > index 5aeb624..3937ce3 100644
> > > --- a/net/bluetooth/hci_core.c
> > > +++ b/net/bluetooth/hci_core.c
> > > @@ -525,6 +525,11 @@ int hci_dev_open(__u16 dev)
> > >
> > > hci_req_lock(hdev);
> > >
> > > + if (test_bit(HCI_UNREGISTER, &hdev->flags)) {
> > > + ret = -ENODEV;
> > > + goto done;
> > > + }
> > > +
> > > if (hdev->rfkill && rfkill_blocked(hdev->rfkill)) {
> > > ret = -ERFKILL;
> > > goto done;
> > > @@ -1577,6 +1582,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
> > >
> > > BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
> > >
> > > + set_bit(HCI_UNREGISTER, &hdev->flags);
> > > +
> > > write_lock(&hci_dev_list_lock);
> > > list_del(&hdev->list);
> > > write_unlock(&hci_dev_list_lock);
> >
> > Is this really enough to protect this race condition?
>
> 1. first hci_dev_open grabs req lock
> 2. second hci_dev_open sleeps on req lock
> 3. hci_dev_unregister sleep on req lock (in do_close)
> 4. first hci_dev_open times out and releases req lock
>
> Now either a) the second open grabs the lock or b) close does.
>
> a) The second open will time out eventually as well and setting a flag
> at unregister will only speed things up (at least when the first
> patch in my series is applied -- otherwise this leads to a
> NULL-pointer exception as well).
>
> b) If close grabs the lock while we have open sleeping on it things go
> really bad and this is the case this patch intends to fix.
>
> As far as I can see, a flag set at unregister (before acquiring the lock)
> will suffice to fix this race, but perhaps I'm missing something?
>
> Where should such an internal flag be added as hdev->flags can not be
> used? hdev->dev_flags?
please add them to hdev->dev_flags as internal flag.
Regards
Marcel
Hi Marcel,
On Wed, Mar 07, 2012 at 11:33:17AM -0800, Marcel Holtmann wrote:
> Hi Johan,
>
> > Do not close protocol driver until device has been unregistered.
> >
> > This fixes a race between tty_close and hci_dev_open which can result in
> > a NULL-pointer dereference.
> >
> > The line discipline closes the protocol driver while we may still have
> > hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointer
> > dereference when lock is acquired and hci_init_req called.
[...]
> what kernel version is this against? Our changes in bluetooth-next fixed
> some of the destruct handling.
This is against the latest rc as it needs to be fixed in 3.3, but I
missed a dependency to bluetooth-next as you point out below.
> Also hci_unregister_dev should be calling the destruct handler and thus
> your change is now accessing hu but it got freed already.
You're right, my patch depends on 010666a126fc ("Bluetooth: Make
hci-destruct callback optional") and 797fe796c4 ("Bluetooth: uart-ldisc:
Fix memory leak and remove destruct cb") from bluetooth-next.
But since the latter one fixes a memory leak it should have been marked
for stable as well as pushed to Linus for 3.3, right?
Thanks,
Johan
Hi Marcel,
On Wed, Mar 07, 2012 at 11:29:20AM -0800, Marcel Holtmann wrote:
> Hi Johan,
>
> > Make sure hci_dev_open returns immediately if hci_dev_unregister has
> > been called.
> >
> > This fixes a race between hci_dev_open and hci_dev_unregister which can
> > lead to a NULL-pointer dereference.
[...]
> what version of the kernel is this patch against? Since we cleaned up
> the flags in bluetooth-next tree. Also in addition you can not just add
> flags here.
As this to be fixed in 3.3 it is against 3.3-rc6.
> > /*
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 5aeb624..3937ce3 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -525,6 +525,11 @@ int hci_dev_open(__u16 dev)
> >
> > hci_req_lock(hdev);
> >
> > + if (test_bit(HCI_UNREGISTER, &hdev->flags)) {
> > + ret = -ENODEV;
> > + goto done;
> > + }
> > +
> > if (hdev->rfkill && rfkill_blocked(hdev->rfkill)) {
> > ret = -ERFKILL;
> > goto done;
> > @@ -1577,6 +1582,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
> >
> > BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
> >
> > + set_bit(HCI_UNREGISTER, &hdev->flags);
> > +
> > write_lock(&hci_dev_list_lock);
> > list_del(&hdev->list);
> > write_unlock(&hci_dev_list_lock);
>
> Is this really enough to protect this race condition?
1. first hci_dev_open grabs req lock
2. second hci_dev_open sleeps on req lock
3. hci_dev_unregister sleep on req lock (in do_close)
4. first hci_dev_open times out and releases req lock
Now either a) the second open grabs the lock or b) close does.
a) The second open will time out eventually as well and setting a flag
at unregister will only speed things up (at least when the first
patch in my series is applied -- otherwise this leads to a
NULL-pointer exception as well).
b) If close grabs the lock while we have open sleeping on it things go
really bad and this is the case this patch intends to fix.
As far as I can see, a flag set at unregister (before acquiring the lock)
will suffice to fix this race, but perhaps I'm missing something?
Where should such an internal flag be added as hdev->flags can not be
used? hdev->dev_flags?
Thanks,
Johan
Hi Johan,
> Do not close protocol driver until device has been unregistered.
>
> This fixes a race between tty_close and hci_dev_open which can result in
> a NULL-pointer dereference.
>
> The line discipline closes the protocol driver while we may still have
> hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointer
> dereference when lock is acquired and hci_init_req called.
>
> Bug is 100% reproducible using hciattach and a disconnected serial port:
>
> 0. # hciattach -n ttyO1 any noflow
>
> 1. hci_dev_open called from hci_power_on grabs req lock
> 2. hci_init_req executes but device fails to initialise (times out
> eventually)
> 3. hci_dev_open is called from hci_sock_ioctl and sleeps on req lock
> 4. hci_uart_tty_close detaches protocol driver and cancels init req
> 5. hci_dev_open (1) releases req lock
> 6. hci_dev_open (3) grabs req lock, calls hci_init_req, which triggers oops
> when request is prepared in hci_uart_send_frame
>
> [ 137.201263] Unable to handle kernel NULL pointer dereference at virtual address 00000028
> [ 137.209838] pgd = c0004000
> [ 137.212677] [00000028] *pgd=00000000
> [ 137.216430] Internal error: Oops: 17 [#1]
> [ 137.220642] Modules linked in:
> [ 137.223846] CPU: 0 Tainted: G W (3.3.0-rc6-dirty #406)
> [ 137.230529] PC is at __lock_acquire+0x5c/0x1ab0
> [ 137.235290] LR is at lock_acquire+0x9c/0x128
> [ 137.239776] pc : [<c0071490>] lr : [<c00733f8>] psr: 20000093
> [ 137.239776] sp : cf869dd8 ip : c0529554 fp : c051c730
> [ 137.251800] r10: 00000000 r9 : cf8673c0 r8 : 00000080
> [ 137.257293] r7 : 00000028 r6 : 00000002 r5 : 00000000 r4 : c053fd70
> [ 137.264129] r3 : 00000000 r2 : 00000000 r1 : 00000000 r0 : 00000001
> [ 137.270965] Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel
> [ 137.278717] Control: 10c5387d Table: 8f0f4019 DAC: 00000015
> [ 137.284729] Process kworker/u:1 (pid: 7, stack limit = 0xcf8682e8)
> [ 137.291229] Stack: (0xcf869dd8 to 0xcf86a000)
> [ 137.295776] 9dc0: c0529554 00000000
> [ 137.304351] 9de0: cf8673c0 cf868000 d03ea1ef cf868000 000001ef 00000470 00000000 00000002
> [ 137.312927] 9e00: cf8673c0 00000001 c051c730 c00716ec 0000000c 00000440 c0529554 00000001
> [ 137.321533] 9e20: c051c730 cf868000 d03ea1f3 00000000 c053b978 00000000 00000028 cf868000
> [ 137.330078] 9e40: 00000000 00000000 00000002 00000000 00000000 c00733f8 00000002 00000080
> [ 137.338684] 9e60: 00000000 c02a1d50 00000000 00000001 60000013 c0969a1c 60000093 c053b96c
> [ 137.347259] 9e80: 00000002 00000018 20000013 c02a1d50 cf0ac000 00000000 00000002 cf868000
> [ 137.355834] 9ea0: 00000089 c0374130 00000002 00000000 c02a1d50 cf0ac000 0000000c cf0fc540
> [ 137.364410] 9ec0: 00000018 c02a1d50 cf0fc540 00000000 cf0fc540 c0282238 c028220c cf178d80
> [ 137.372985] 9ee0: 127525d8 c02821cc 9a1fa451 c032727c 9a1fa451 127525d8 cf0fc540 cf0ac4ec
> [ 137.381561] 9f00: cf0ac000 cf0fc540 cf0ac584 c03285f4 c0328580 cf0ac4ec cf85c740 c05510cc
> [ 137.390136] 9f20: ce825400 c004c914 00000002 00000000 c004c884 ce8254f5 cf869f48 00000000
> [ 137.398712] 9f40: c0328580 ce825415 c0a7f914 c061af64 00000000 c048cf3c cf8673c0 cf85c740
> [ 137.407287] 9f60: c05510cc c051a66c c05510ec c05510c4 cf85c750 cf868000 00000089 c004d6ac
> [ 137.415863] 9f80: 00000000 c0073d14 00000001 cf853ed8 cf85c740 c004d558 00000013 00000000
> [ 137.424438] 9fa0: 00000000 00000000 00000000 c00516b0 00000000 00000000 cf85c740 00000000
> [ 137.433013] 9fc0: 00000001 dead4ead ffffffff ffffffff c0551674 00000000 00000000 c0450aa4
> [ 137.441589] 9fe0: cf869fe0 cf869fe0 cf853ed8 c005162c c0013b30 c0013b30 00ffff00 00ffff00
> [ 137.450164] [<c0071490>] (__lock_acquire+0x5c/0x1ab0) from [<c00733f8>] (lock_acquire+0x9c/0x128)
> [ 137.459503] [<c00733f8>] (lock_acquire+0x9c/0x128) from [<c0374130>] (_raw_spin_lock_irqsave+0x44/0x58)
> [ 137.469360] [<c0374130>] (_raw_spin_lock_irqsave+0x44/0x58) from [<c02a1d50>] (skb_queue_tail+0x18/0x48)
> [ 137.479339] [<c02a1d50>] (skb_queue_tail+0x18/0x48) from [<c0282238>] (h4_enqueue+0x2c/0x34)
> [ 137.488189] [<c0282238>] (h4_enqueue+0x2c/0x34) from [<c02821cc>] (hci_uart_send_frame+0x34/0x68)
> [ 137.497497] [<c02821cc>] (hci_uart_send_frame+0x34/0x68) from [<c032727c>] (hci_send_frame+0x50/0x88)
> [ 137.507171] [<c032727c>] (hci_send_frame+0x50/0x88) from [<c03285f4>] (hci_cmd_work+0x74/0xd4)
> [ 137.516204] [<c03285f4>] (hci_cmd_work+0x74/0xd4) from [<c004c914>] (process_one_work+0x1a0/0x4ec)
> [ 137.525604] [<c004c914>] (process_one_work+0x1a0/0x4ec) from [<c004d6ac>] (worker_thread+0x154/0x344)
> [ 137.535278] [<c004d6ac>] (worker_thread+0x154/0x344) from [<c00516b0>] (kthread+0x84/0x90)
> [ 137.543975] [<c00516b0>] (kthread+0x84/0x90) from [<c0013b30>] (kernel_thread_exit+0x0/0x8)
> [ 137.552734] Code: e59f4e5c e5941000 e3510000 0a000031 (e5971000)
> [ 137.559234] ---[ end trace 1b75b31a2719ed1e ]---
>
> Cc: stable <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/bluetooth/hci_ldisc.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 0711448..6946081 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -310,11 +310,11 @@ static void hci_uart_tty_close(struct tty_struct *tty)
> hci_uart_close(hdev);
>
> if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
> - hu->proto->close(hu);
> if (hdev) {
> hci_unregister_dev(hdev);
> hci_free_dev(hdev);
> }
> + hu->proto->close(hu);
> }
> }
> }
what kernel version is this against? Our changes in bluetooth-next fixed
some of the destruct handling.
Also hci_unregister_dev should be calling the destruct handler and thus
your change is now accessing hu but it got freed already.
Regards
Marcel
Hi Johan,
> Make sure hci_dev_open returns immediately if hci_dev_unregister has
> been called.
>
> This fixes a race between hci_dev_open and hci_dev_unregister which can
> lead to a NULL-pointer dereference.
>
> Bug is 100% reproducible using hciattach and a disconnected serial port:
>
> 0. # hciattach -n /dev/ttyO1 any noflow
>
> 1. hci_dev_open called from hci_power_on grabs req lock
> 2. hci_init_req executes but device fails to initialise (times out
> eventually)
> 3. hci_dev_open is called from hci_sock_ioctl and sleeps on req lock
> 4. hci_uart_tty_close calls hci_dev_unregister and sleeps on req lock in
> hci_dev_do_close
> 5. hci_dev_open (1) releases req lock
> 6. hci_dev_do_close grabs req lock and returns as device is not up
> 7. hci_dev_unregister sleeps in destroy_workqueue
> 8. hci_dev_open (3) grabs req lock, calls hci_init_req and eventually sleeps
> 9. hci_dev_unregister finishes, while hci_dev_open is still running...
>
> [ 79.627136] INFO: trying to register non-static key.
> [ 79.632354] the code is fine but needs lockdep annotation.
> [ 79.638122] turning off the locking correctness validator.
> [ 79.643920] [<c00188bc>] (unwind_backtrace+0x0/0xf8) from [<c00729c4>] (__lock_acquire+0x1590/0x1ab0)
> [ 79.653594] [<c00729c4>] (__lock_acquire+0x1590/0x1ab0) from [<c00733f8>] (lock_acquire+0x9c/0x128)
> [ 79.663085] [<c00733f8>] (lock_acquire+0x9c/0x128) from [<c0040a88>] (run_timer_softirq+0x150/0x3ac)
> [ 79.672668] [<c0040a88>] (run_timer_softirq+0x150/0x3ac) from [<c003a3b8>] (__do_softirq+0xd4/0x22c)
> [ 79.682281] [<c003a3b8>] (__do_softirq+0xd4/0x22c) from [<c003a924>] (irq_exit+0x8c/0x94)
> [ 79.690856] [<c003a924>] (irq_exit+0x8c/0x94) from [<c0013a50>] (handle_IRQ+0x34/0x84)
> [ 79.699157] [<c0013a50>] (handle_IRQ+0x34/0x84) from [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c)
> [ 79.708648] [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c) from [<c037499c>] (__irq_usr+0x3c/0x60)
> [ 79.718048] Exception stack(0xcf281fb0 to 0xcf281ff8)
> [ 79.723358] 1fa0: 0001e6a0 be8dab00 0001e698 00036698
> [ 79.731933] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000000 00000004 00000000
> [ 79.740509] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 ffffffff
> [ 79.747497] Unable to handle kernel NULL pointer dereference at virtual address 00000000
> [ 79.756011] pgd = cf3b4000
> [ 79.758850] [00000000] *pgd=8f0c7831, *pte=00000000, *ppte=00000000
> [ 79.765502] Internal error: Oops: 80000007 [#1]
> [ 79.770294] Modules linked in:
> [ 79.773529] CPU: 0 Tainted: G W (3.3.0-rc6-00002-gb5d5c87 #421)
> [ 79.781066] PC is at 0x0
> [ 79.783721] LR is at run_timer_softirq+0x16c/0x3ac
> [ 79.788787] pc : [<00000000>] lr : [<c0040aa4>] psr: 60000113
> [ 79.788787] sp : cf281ee0 ip : 00000000 fp : cf280000
> [ 79.800903] r10: 00000004 r9 : 00000100 r8 : b6f234d0
> [ 79.806427] r7 : c0519c28 r6 : cf093488 r5 : c0561a00 r4 : 00000000
> [ 79.813323] r3 : 00000000 r2 : c054eee0 r1 : 00000001 r0 : 00000000
> [ 79.820190] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
> [ 79.827728] Control: 10c5387d Table: 8f3b4019 DAC: 00000015
> [ 79.833801] Process gpsd (pid: 1265, stack limit = 0xcf2802e8)
> [ 79.839965] Stack: (0xcf281ee0 to 0xcf282000)
> [ 79.844573] 1ee0: 00000002 00000000 c0040a24 00000000 00000002 cf281f08 00200200 00000000
> [ 79.853210] 1f00: 00000000 cf281f18 cf281f08 00000000 00000000 00000000 cf281f18 cf281f18
> [ 79.861816] 1f20: 00000000 00000001 c056184c 00000000 00000001 b6f234d0 c0561848 00000004
> [ 79.870452] 1f40: cf280000 c003a3b8 c051e79c 00000001 00000000 00000100 3fa9e7b8 0000000a
> [ 79.879089] 1f60: 00000025 cf280000 00000025 00000000 00000000 b6f234d0 00000000 00000004
> [ 79.887756] 1f80: 00000000 c003a924 c053ad38 c0013a50 fa200000 cf281fb0 ffffffff c0008530
> [ 79.896362] 1fa0: 0001e6a0 0000aab8 80000010 c037499c 0001e6a0 be8dab00 0001e698 00036698
> [ 79.904998] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000000 00000004 00000000
> [ 79.913665] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 ffffffff 00fbf700 04ffff00
> [ 79.922302] [<c0040aa4>] (run_timer_softirq+0x16c/0x3ac) from [<c003a3b8>] (__do_softirq+0xd4/0x22c)
> [ 79.931945] [<c003a3b8>] (__do_softirq+0xd4/0x22c) from [<c003a924>] (irq_exit+0x8c/0x94)
> [ 79.940582] [<c003a924>] (irq_exit+0x8c/0x94) from [<c0013a50>] (handle_IRQ+0x34/0x84)
> [ 79.948913] [<c0013a50>] (handle_IRQ+0x34/0x84) from [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c)
> [ 79.958404] [<c0008530>] (omap3_intc_handle_irq+0x48/0x4c) from [<c037499c>] (__irq_usr+0x3c/0x60)
> [ 79.967773] Exception stack(0xcf281fb0 to 0xcf281ff8)
> [ 79.973083] 1fa0: 0001e6a0 be8dab00 0001e698 00036698
> [ 79.981658] 1fc0: 0002df98 0002df38 0000001f 00000000 b6f234d0 00000000 00000004 00000000
> [ 79.990234] 1fe0: 0001e6f8 be8d6aa0 be8dac50 0000aab8 80000010 ffffffff
> [ 79.997161] Code: bad PC value
> [ 80.000396] ---[ end trace 6f6739840475f9ee ]---
> [ 80.005279] Kernel panic - not syncing: Fatal exception in interrupt
>
> Cc: stable <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> include/net/bluetooth/hci.h | 1 +
> net/bluetooth/hci_core.c | 7 +++++++
> 2 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 00596e8..f626f44 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -86,6 +86,7 @@ enum {
> HCI_DEBUG_KEYS,
>
> HCI_RESET,
> + HCI_UNREGISTER,
> };
what version of the kernel is this patch against? Since we cleaned up
the flags in bluetooth-next tree. Also in addition you can not just add
flags here.
>
> /*
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 5aeb624..3937ce3 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -525,6 +525,11 @@ int hci_dev_open(__u16 dev)
>
> hci_req_lock(hdev);
>
> + if (test_bit(HCI_UNREGISTER, &hdev->flags)) {
> + ret = -ENODEV;
> + goto done;
> + }
> +
> if (hdev->rfkill && rfkill_blocked(hdev->rfkill)) {
> ret = -ERFKILL;
> goto done;
> @@ -1577,6 +1582,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
>
> BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
>
> + set_bit(HCI_UNREGISTER, &hdev->flags);
> +
> write_lock(&hci_dev_list_lock);
> list_del(&hdev->list);
> write_unlock(&hci_dev_list_lock);
Is this really enough to protect this race condition?
Regards
Marcel