Bluetooth: hci_ldisc: Allow sleeping while proto locks are held.

Commit dec2c92880cc5435381d50e3045ef018a762a917 ("Bluetooth: hci_ldisc:
Use rwlocking to avoid closing proto races") introduced locks in hci_ldisc that are held while calling the proto functions. These locks are rwlock's, and hence do not allow sleeping while they are held. However, the proto functions that hci_bcm registers use mutexes and hence need to be able to sleep.

We can't replace the mutex in hci_bcm, because there are other calls there that might sleep. Therefore this replaces the rwlock's in hci_ldisc with rw_semaphore's (which allow sleeping). This is a safer approach anyway as it reduces the restrictions on the proto callbacks. Also, because acquiring write-lock is very rare compared to acquiring the read-lock, the percpu variant of rw_semaphore is used.

Quoted 6 lines · On Wed, Oct 25, 2017 at 10:14:53PM -0700, =?UTF-8?q?Ronald=20Tschal=C3=A4r?=…

Commit dec2c92880cc5435381d50e3045ef018a762a917 ("Bluetooth: hci_ldisc:
Use rwlocking to avoid closing proto races") introduced locks in hci_ldisc that are held while calling the proto functions. These locks are rwlock's, and hence do not allow sleeping while they are held. However, the proto functions that hci_bcm registers use mutexes and hence need to be able to sleep.

[...]

Quoted 6 lines

We can't replace the mutex in hci_bcm, because there are other calls there that might sleep. Therefore this replaces the rwlock's in hci_ldisc with rw_semaphore's (which allow sleeping). This is a safer approach anyway as it reduces the restrictions on the proto callbacks. Also, because acquiring write-lock is very rare compared to acquiring the read-lock, the percpu variant of rw_semaphore is used.

The percpu_rw_semaphore is unusual (if fine I guess), it's only used by cgroups, uprobes and ext4 so far and I was unaware of its existence.

I don't have the hardware to test this but the rationale and patch itself LGTM, so:

Reviewed-by Lukas Wunner <[email protected]>

Commit dec2c92880cc5435381d50e3045ef018a762a917 ("Bluetooth: hci_ldisc:
Use rwlocking to avoid closing proto races") introduced locks in hci_ldisc that are held while calling the proto functions. These locks are rwlock's, and hence do not allow sleeping while they are held. However, the proto functions that hci_bcm registers use mutexes and hence need to be able to sleep.

In more detail: hci_uart_tty_receive() and hci_uart_dequeue() both acquire the rwlock, after which they call proto->recv() and proto->dequeue(), respectively. In the case of hci_bcm these point to bcm_recv() and bcm_dequeue(). The latter both acquire the bcm_device_lock, which is a mutex, so doing so results in a call to might_sleep(). But since we're holding a rwlock in hci_ldisc, that results in the following BUG (this for the dequeue case - a similar one for the receive case is omitted for brevity):

 BUG: sleeping function called from invalid context at kernel/locking/mutex.c
 in_atomic(): 1, irqs_disabled(): 0, pid: 7303, name: kworker/7:3
 INFO: lockdep is turned off.
 CPU: 7 PID: 7303 Comm: kworker/7:3 Tainted: G        W  OE   4.13.2+ #17
 Hardware name: Apple Inc. MacBookPro13,3/Mac-A5C67F76ED83108C, BIOS MBP133.8
 Workqueue: events hci_uart_write_work [hci_uart]
 Call Trace:
  dump_stack+0x8e/0xd6
  ___might_sleep+0x164/0x250
  __might_sleep+0x4a/0x80
  __mutex_lock+0x59/0xa00
  ? lock_acquire+0xa3/0x1f0
  ? lock_acquire+0xa3/0x1f0
  ? hci_uart_write_work+0xd3/0x160 [hci_uart]
  mutex_lock_nested+0x1b/0x20
  ? mutex_lock_nested+0x1b/0x20
  bcm_dequeue+0x21/0xc0 [hci_uart]
  hci_uart_write_work+0xe6/0x160 [hci_uart]
  process_one_work+0x253/0x6a0
  worker_thread+0x4d/0x3b0
  kthread+0x133/0x150

We can't replace the mutex in hci_bcm, because there are other calls there that might sleep. Therefore this replaces the rwlock's in hci_ldisc with rw_semaphore's (which allow sleeping). This is a safer approach anyway as it reduces the restrictions on the proto callbacks. Also, because acquiring write-lock is very rare compared to acquiring the read-lock, the percpu variant of rw_semaphore is used.

Lastly, because hci_uart_tx_wakeup() may be called from an IRQ context, we can't block (sleep) while trying acquire the read lock there, so we use the trylock variant.

Signed-off-by Ronald Tschalär <[email protected]>

Cc Lukas Wunner <[email protected]>

Cc Marcel Holtmann <[email protected]>

Cc Gustavo Padovan <[email protected]>

Cc Johan Hedberg <[email protected]>

Cc Dean Jenkins <[email protected]>

---
Changes in v2:
- Add back locking in hci_uart_tx_wakeup(). Removing the locking altogether there was wrong, as nicely pointed out by Dean Jenkins.

drivers/bluetooth/hci_ldisc.c | 38 ++++++++++++++++++++++---------------- drivers/bluetooth/hci_uart.h | 2 +-

2 files changed, 23 insertions(+), 17 deletions(-)