From: Deepak Das <Deepak_Das@xxxxxxxxxx>
There is a work queue hu->write_work that can be scheduled to handle
a transmission work item in BCSP scenarios that are described below.
In these scenarios, the payload of a BCSP frame can contain a HCI
message as follows:
a) Normal sending of a HCI message within a BCSP frame from upper
layers via hci_uart_send_frame().
b) BCSP retransmission due to a failure to get a BCSP ACK indication.
This resends an old HCI message triggered by the BCSP timer timeout event
handled by bcsp_timed_event().
c) Sending of a BCSP ACK frame in response to a received reliable BCSP
frame is handled in bcsp_complete_rx_pkt() which allows an empty payload
(no HCI message) to be sent.
A good reproduction scenario of the crash is for a bad UART driver to
corrupt or cause a total loss of incoming BCSP frames. This leads to a link
failure so the BCSP timer is periodically triggered in an attempt to
retransmit unacknowledged frames. Then upper layers request the HCI UART
to close immediately before the BCSP timer event is handled. This causes
a race condition between closing the HCI UART and retransmitting a BCSP
frame.
The current solution is using cancel_work_sync(&hu->write_work); within
hci_uart_tty_close() in an attempt to cleanse the work queue of the
transmission work item.
It should be noted that a transmission work item is scheduled into the work
queue by hci_uart_tx_wakeup() which can be running in an atomic context
such as a UART driver interrupt thread or in a process context. Therefore,
it is possible for hci_uart_tx_wakeup() to interrupt or run concurrently
with hci_uart_tty_close().
In addition, hu->proto->close(hu) calls bcsp_close() which frees the
various queues of messages and deletes the BCSP timer.
The current solution has a flaw because hci_uart_tty_close() fails to
inhibit the asynchronous b) and c) transmission events from adding a work
item to the work queue AFTER cancel_work_sync(&hu->write_work); has been
executed. A crash occurs after bcsp_close() has freed the message queues
which are subsequently accessed when the work queue work item runs
hci_uart_write_work(). Therefore, there is a race condition.
This flaw was masked as typically retransmissions should be rare and
Bluetooth has a low sub-system rate of being actively shutdown.
This fix introduces a new HCI_UART proto flag bit called
HCI_UART_UNREGISTERING to indicate the unregistering state of the HCI UART.
This flag is used to prevent the addition of a transmission work item after
hci_uart_tty_close() starts to run.
Note that a spinlock is necessary because it is not possible to check
the state of the atomic flag and to schedule the work queue in an atomic
sequence of operations in hci_uart_tx_wakeup(). This means that the flag
must be prevented from changing state after the flag is checked and before
the work queue is scheduled in hci_uart_tx_wakeup().
Signed-off-by: Deepak Das <Deepak_Das@xxxxxxxxxx>
Signed-off-by: Rajeev Kumar <rajeev_kumar@xxxxxxxxxx>
Signed-off-by: Dean_Jenkins@xxxxxxxxxx
Signed-off-by: Dean Jenkins <Dean_Jenkins@xxxxxxxxxx>
---
drivers/bluetooth/hci_ldisc.c | 16 +++++++++++++++-
drivers/bluetooth/hci_uart.h | 3 +++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index cf1883d..ddf4cda 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -125,15 +125,22 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
int hci_uart_tx_wakeup(struct hci_uart *hu)
{
+ unsigned long flags;
+
+ spin_lock_irqsave(&hu->schedule_lock, flags);
+ if (test_bit(HCI_UART_UNREGISTERING, &hu->flags))
+ goto no_schedule;
if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
- return 0;
+ goto no_schedule;
}
BT_DBG("");
schedule_work(&hu->write_work);
+no_schedule:
+ spin_unlock_irqrestore(&hu->schedule_lock, flags);
return 0;
}
@@ -464,6 +471,8 @@ static int hci_uart_tty_open(struct tty_struct *tty)
INIT_WORK(&hu->init_ready, hci_uart_init_work);
INIT_WORK(&hu->write_work, hci_uart_write_work);
+ spin_lock_init(&hu->schedule_lock);
+
/* Flush any pending characters in the driver */
tty_driver_flush_buffer(tty);
@@ -479,6 +488,7 @@ static void hci_uart_tty_close(struct tty_struct *tty)
{
struct hci_uart *hu = tty->disc_data;
struct hci_dev *hdev;
+ unsigned long flags;
BT_DBG("tty %p", tty);
@@ -488,6 +498,10 @@ static void hci_uart_tty_close(struct tty_struct *tty)
if (!hu)
return;
+ spin_lock_irqsave(&hu->schedule_lock, flags);
+ set_bit(HCI_UART_UNREGISTERING, &hu->flags);
+ spin_unlock_irqrestore(&hu->schedule_lock, flags);
+
hdev = hu->hdev;
if (hdev)
hci_uart_close(hdev);
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 839bad1..27a7708 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -88,6 +88,8 @@ struct hci_uart {
struct sk_buff *tx_skb;
unsigned long tx_state;
+ /* prevent scheduling work during close */
+ spinlock_t schedule_lock;
unsigned int init_speed;
unsigned int oper_speed;
};
@@ -96,6 +98,7 @@ struct hci_uart {
#define HCI_UART_PROTO_SET 0
#define HCI_UART_REGISTERED 1
#define HCI_UART_PROTO_READY 2
+#define HCI_UART_UNREGISTERING 3
/* TX states */
#define HCI_UART_SENDING 1
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
