Skip to contentSkip to conversation

Bluetooth: hci_ldisc: Fix another race when closing the tty.

PATCH In review =?UTF-8?q?Ronald=20Tschal=C3=A4r?= 3 participants 3 messages Oct 26, 2017 – Oct 30, 2017 Bluetooth drivers

lore
Share & tools
lore

=?UTF-8?q?Ronald=20Tschal=C3=A4r?=

author
#0 lore raw

PATCH 1/1 Bluetooth: hci_ldisc: Fix another race when closing the tty.

The following race condition still existed:

P1 P2
cancel_work_sync()
hci_uart_tx_wakeup() hci_uart_write_work() hci_uart_dequeue()
clear_bit(HCI_UART_PROTO_READY) hci_unregister_dev(hdev)
hci_free_dev(hdev)
hu->proto->close(hu)
kfree(hu)
access to hdev and hu

Cancelling the work after clearing the HCI_UART_PROTO_READY bit avoids this as any hci_uart_tx_wakeup() issued after the flag is cleared will detect that and not schedule further work.

Signed-off-by Ronald Tschalär <[email protected]>
Cc Dean Jenkins <[email protected]>
Cc Lukas Wunner <[email protected]>
Cc Marcel Holtmann <[email protected]>
Cc Gustavo Padovan <[email protected]>
Cc Johan Hedberg <[email protected]>
---
 drivers/bluetooth/hci_ldisc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/bluetooth/hci_ldisc.c +2 -2
index 31def781a562..c823914b3a80 100644
@@ -523,13 +523,13 @@ static void hci_uart_tty_close(struct tty_struct *tty)
 	if (hdev)
 		hci_uart_close(hdev);
 
-	cancel_work_sync(&hu->write_work);
-
 	if (test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
 		percpu_down_write(&hu->proto_lock);
 		clear_bit(HCI_UART_PROTO_READY, &hu->flags);
 		percpu_up_write(&hu->proto_lock);
 
+		cancel_work_sync(&hu->write_work);
+
 		if (hdev) {
 			if (test_bit(HCI_UART_REGISTERED, &hu->flags))
 				hci_unregister_dev(hdev);
Signature · 1 line
2.13.6
Reviewed-by on v1 1/1
#1 lore raw
Quoted 5 lines · On Wed, Oct 25, 2017 at 10:15:19PM -0700, =?UTF-8?q?Ronald=20Tschal=C3=A4r?=…
drivers/bluetooth/hci_ldisc.c +0 -0
@@ -523,13 +523,13 @@ static void hci_uart_tty_close(struct tty_struct *tty)
 	if (hdev)
 		hci_uart_close(hdev);

drivers/bluetooth/hci_ldisc.c static void hci_uart_tty_close(struct tty_struct *tty) · line 532

Quoted 10 lines
drivers/bluetooth/hci_ldisc.c +2 -2
 
-	cancel_work_sync(&hu->write_work);
-
 	if (test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
 		percpu_down_write(&hu->proto_lock);
 		clear_bit(HCI_UART_PROTO_READY, &hu->flags);
 		percpu_up_write(&hu->proto_lock);
 
+		cancel_work_sync(&hu->write_work);
+

Now the work is only cancelled if HCI_UART_PROTO_READY is set, but that seems fine, so

Reviewed-by Lukas Wunner <[email protected]>

It should be noted that this patch only applies cleanly if Ronald's other patch is applied before.

maintainer
#2 lore raw
In reply to =?UTF-8?q?Ronald=20Tschal=C3=A4r?=

Hi Ronald,

Quoted 24 lines

The following race condition still existed:

P1 P2
cancel_work_sync()
hci_uart_tx_wakeup() hci_uart_write_work() hci_uart_dequeue()
clear_bit(HCI_UART_PROTO_READY) hci_unregister_dev(hdev)
hci_free_dev(hdev)
hu->proto->close(hu)
kfree(hu)
access to hdev and hu

Cancelling the work after clearing the HCI_UART_PROTO_READY bit avoids this as any hci_uart_tx_wakeup() issued after the flag is cleared will detect that and not schedule further work.

Signed-off-by Ronald Tschalär <[email protected]>
Cc Dean Jenkins <[email protected]>
Cc Lukas Wunner <[email protected]>
Cc Marcel Holtmann <[email protected]>
Cc Gustavo Padovan <[email protected]>
Cc Johan Hedberg <[email protected]>
Quoted 3 lines

---
drivers/bluetooth/hci_ldisc.c | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel