Hi Dean,
apologies for the slow reply, and thank you for the detailed response.
On Mon, Oct 16, 2017 at 06:08:37PM +0100, Dean Jenkins wrote:
>
> On 15/10/17 11:40, =?UTF-8?q?Ronald=20Tschal=C3=A4r?= wrote:
> >Lastly, the read-lock in the hci_uart_tx_wakeup() callback now needed
> >to be removed because that function is called from an IRQ context. But
> >since it doesn't actually call any proto functions, instead just
> >queueing the work, and the other operations are atomic bit operations,
> >this is ok.
[snip]
> There is at least 1 race condition to consider. The atomic variables do not
> help because the codeblock as a whole is not atomic. Hence locking is needed
> as follows.
>
> The issue is that hci_uart_tty_close() runs asynchronously to
> hci_uart_tx_wakeup() which is shown below (assuming a SMP system).
>
> int hci_uart_tx_wakeup(struct hci_uart *hu)
> {
> read_lock(&hu->proto_lock);
>
> In parallel, hci_uart_tty_close() can be running here:
>
> if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
> goto no_schedule;
>
> In parallel, hci_uart_tty_close() can be running here:
>
> if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
>
> In parallel, hci_uart_tty_close() can be running here:
>
> set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
> goto no_schedule;
> }
>
> BT_DBG("");
>
> In parallel, hci_uart_tty_close() can be running here:
>
> schedule_work(&hu->write_work);
>
> In parallel, hci_uart_tty_close() can be running here:
>
> no_schedule:
> read_unlock(&hu->proto_lock);
>
> return 0;
> }
>
> hci_uart_tty_close() progressively removes the resources needed by the
> scheduled work queue hu->write_work which runs hci_uart_write_work(). Also
> there is a delay between the scheduling and hci_uart_write_work actually
> running. This means hci_uart_write_work() can race with
> hci_uart_tty_close(), sometimes causing hci_uart_tty_close() to crash, for
> example due to the write buffer no longer being there.
>
> static void hci_uart_write_work(struct work_struct *work)
> {
> <snipped>
> set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> len = tty->ops->write(tty, skb->data, skb->len); <== can crash
> inside the write function pointer as the write buffer is no longer valid
> hdev->stat.byte_tx += len; <== can crash here as hdev now invalid
> <snipped>
> }
>
> Therefore, a robust solution is to lock out hci_uart_tty_close() when
> hci_uart_tx_wakeup() runs, that is the reason why read_lock(&hu->proto_lock)
> is used in hci_uart_write_work(). The atomic flag HCI_UART_PROTO_READY is
> prevented from being cleared whilst hci_uart_tx_wakeup() runs.
Got it, thanks. So to summarize, the core issue is that in
hci_uart_tx_wakeup() these two need to be atomic
if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
goto no_schedule;
schedule_work(&hu->write_work);
so that new work won't be scheduled after close.
I think then that this can fixed by using the trylock variants here:
int hci_uart_tx_wakeup(struct hci_uart *hu)
{
- read_lock(&hu->proto_lock);
+ /* This may be called in an IRQ context, so we can't sleep. Therefore
+ * we only try to acquire the lock, and if that fails we assume the
+ * tty is being closed because that is the only time the write lock is
+ * acquired. If, however, at some point in the future the write lock
+ * is also acquired in other situations, then this must be revisited.
+ */
+ if (!percpu_down_read_trylock(&hu->proto_lock))
+ return 0;
if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
goto no_schedule;
@@ -145,8 +143,6 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
schedule_work(&hu->write_work);
no_schedule:
- read_unlock(&hu->proto_lock);
+ percpu_up_read(&hu->proto_lock);
return 0;
}
> In fact, hci_uart_tty_close() is really a bit of a mess because it
> progressively removes resources. It is is based on old code which has been
> patched over the many years. Therefore it has kept code which is probably
> obsolete or duplicated. Ideally, hci_uart_tty_close() should be rewritten.
>
> For example, there is a call
> cancel_work_sync(&hu->write_work);
> in hci_uart_tty_close() which at first glance seems to be helpful but it is
> flawed because hci_uart_tx_wakeup() can schedule a new work item AFTER
> cancel_work_sync(&hu->write_work) runs. Therefore, locking is needed to
> prevent hci_uart_tx_wakeup() from being scheduled whilst
> HCI_UART_PROTO_READY is being cleared in hci_uart_tty_close().
Actually, I think there's still problem: in hci_uart_tty_close()
cancel_work_sync() is called before the HCI_UART_PROTO_READY flag is
cleared, opening the following race:
P1 P2
cancel_work_sync()
hci_uart_tx_wakeup()
clear_bit(HCI_UART_PROTO_READY)
clean up
hci_uart_write_work()
So AFAICT cancel_work_sync() needs to be done after clearing the flag:
if (test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
write_lock_irqsave(&hu->proto_lock, flags);
clear_bit(HCI_UART_PROTO_READY, &hu->flags);
write_unlock_irqrestore(&hu->proto_lock, flags);
cancel_work_sync(&hu->write_work); // <---
if (hdev) {
(if HCI_UART_PROTO_READY is already clear, then no work can be pending,
so no need to cancel work in that case).
Cheers,
Ronald
--
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
