Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1466172 > unrolled thread

[PATCH 0/4] Bluetooth: hci_uart: various fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] Bluetooth: hci_uart: various fixes Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-08-19 09:40 +0200
    [PATCH 1/4] Bluetooth: hci_ldisc: fix a race in the hdev closing path Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-08-19 09:40 +0200
    [PATCH 2/4] Bluetooth: hci_h5: fix a race in the closing path Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-08-19 09:40 +0200
    [PATCH 3/4] Bluetooth: hci_ldisc: don't release resources in hci_uart_init_work() Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-08-19 09:40 +0200

#1466172 — [PATCH 0/4] Bluetooth: hci_uart: various fixes

Hi,

We recently faced some problems when using an BT uart chip interfaced
through the H5 proto (rtk_h5). Here are the logs of the 2 different
issues we had when closing the line discipline (actually, restoring
the previous one) [1][2]. I know the kernel is Tainted in those logs,
but after some investigations I found a few potential issues that might
explain what we're seeing.

Patches 1 and 2 are fixing 2 potential 'use after free' bugs: in some
(unlikely) cases the timer and work we try to cancel in the closing
path can be re-scheduled in our back, and since we're releasing the
memory region assigned to those elements at the end of the closing
procedure we can end-up with those invalid pointer exception when the
work or timer handler is called.

Note that this problem is pretty hard to reproduce, so I'm not sure
my patches are fixing all the racy paths.

Patches 3 and 4 are fixing potential issues that I didn't directly
face but may be worth fixing. Path 3 is fixing a potential double
free issue (proto->close() called twice if the hdev registration
failed). Patch 4 is making sure we don't loose some TX events.

Let me know what you think.

Thanks,

Boris

[1]http://code.bulix.org/8qtjly-105082
[2]http://code.bulix.org/qzur9n-105083


Boris Brezillon (4):
  Bluetooth: hci_ldisc: fix a race in the hdev closing path
  Bluetooth: hci_h5: fix a race in the closing path
  Bluetooth: hci_ldisc: don't release resources in hci_uart_init_work()
  Bluetooth: hci_ldisc: make sure we don't loose HCI_UART_TX_WAKEUP
    events

 drivers/bluetooth/hci_h5.c    |  7 ++++++-
 drivers/bluetooth/hci_ldisc.c | 30 ++++++++++++++++++++++++++----
 drivers/bluetooth/hci_uart.h  |  1 +
 3 files changed, 33 insertions(+), 5 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1466175 — [PATCH 1/4] Bluetooth: hci_ldisc: fix a race in the hdev closing path

hci_uart_tty_close() is cancelling any pending write work, but some
hci_uart_proto implementations might re-schedule this work after its
cancellation (by calling hci_uart_tx_wakeup()).

Make sure the write work is not re-scheduled in our back while we're
closing the device.

We also cancel any pending init work and prevent the active one (if
any) from registering the hdev if the line discipline is being closed.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/bluetooth/hci_ldisc.c | 15 ++++++++++++++-
 drivers/bluetooth/hci_uart.h  |  1 +
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index dda97398c59a..de7f7f1f995c 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -130,7 +130,9 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
 
 	BT_DBG("");
 
-	schedule_work(&hu->write_work);
+	/* Don't schedule the work if the device is being closed. */
+	if (!test_bit(HCI_UART_CLOSING, &hu->flags))
+		schedule_work(&hu->write_work);
 
 	return 0;
 }
@@ -180,6 +182,11 @@ static void hci_uart_init_work(struct work_struct *work)
 	if (!test_and_clear_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
 		return;
 
+	if (test_bit(HCI_UART_CLOSING, &hu->flags)) {
+		BT_DBG("HCI device is being closed, don't register it.");
+		return;
+	}
+
 	err = hci_register_dev(hu->hdev);
 	if (err < 0) {
 		BT_ERR("Can't register HCI device");
@@ -490,7 +497,13 @@ static void hci_uart_tty_close(struct tty_struct *tty)
 	if (hdev)
 		hci_uart_close(hdev);
 
+	/*
+	 * Set the closing bit to make sure nobody re-schedules the write work
+	 * in our back.
+	 */
+	set_bit(HCI_UART_CLOSING, &hu->flags);
 	cancel_work_sync(&hu->write_work);
+	cancel_work_sync(&hu->init_ready);
 
 	if (test_and_clear_bit(HCI_UART_PROTO_READY, &hu->flags)) {
 		if (hdev) {
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 839bad1d8152..c092ad6607e1 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -96,6 +96,7 @@ struct hci_uart {
 #define HCI_UART_PROTO_SET	0
 #define HCI_UART_REGISTERED	1
 #define HCI_UART_PROTO_READY	2
+#define HCI_UART_CLOSING	3
 
 /* TX states  */
 #define HCI_UART_SENDING	1
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1466184 — [PATCH 2/4] Bluetooth: hci_h5: fix a race in the closing path

The H5 timer should not be rescheduled while we are closing the device,
otherwise it's defeating the del_timer_sync() call done in h5_close().

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/bluetooth/hci_h5.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index 0879d64b1caf..d9720c59cffa 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -140,7 +140,12 @@ static void h5_timed_event(unsigned long arg)
 	}
 
 	if (h5->state != H5_ACTIVE) {
-		mod_timer(&h5->timer, jiffies + H5_SYNC_TIMEOUT);
+		/*
+		 * Do not re-schedule the timer if the device is being closed.
+		 */
+		if (!test_bit(HCI_UART_CLOSING, &hu->flags))
+			mod_timer(&h5->timer, jiffies + H5_SYNC_TIMEOUT);
+
 		goto wakeup;
 	}
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1466186 — [PATCH 3/4] Bluetooth: hci_ldisc: don't release resources in hci_uart_init_work()

This is done in hci_uart_tty_close() and can lead to double cleanup if
proto->close() is not tracking the closed/open state.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/bluetooth/hci_ldisc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index de7f7f1f995c..27f73294edcb 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -190,9 +190,7 @@ static void hci_uart_init_work(struct work_struct *work)
 	err = hci_register_dev(hu->hdev);
 	if (err < 0) {
 		BT_ERR("Can't register HCI device");
-		hci_free_dev(hu->hdev);
-		hu->hdev = NULL;
-		hu->proto->close(hu);
+		return;
 	}
 
 	set_bit(HCI_UART_REGISTERED, &hu->flags);
-- 
2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web