Re: [PATCH] Bluetooth: hci_ldisc: Allow sleeping while proto locks are held.

[Dean Jenkins <Dean_Jenkins@mentor.com>]

Hi Ronald,

Sorry for my delay in replying to you.

On 18/10/17 03:00, Life is hard, and then you die wrote:
>
>> 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()
Yes, this looks bad. There is some protection in hci_uart_write_work() 
because hci_uart_dequeue(hu) checks HCI_UART_PROTO_READY but it will not 
be foolproof due to resources being removed by hci_uart_tty_close().
>
> 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);      // <---
I agree with this solution. I was going to suggest this but you beat me 
to it ;-)
>
>                  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).
>
I agree with your statement.

Regards,
Dean

-- 
Dean Jenkins
Embedded Software Engineer
Linux Transportation Solutions
Mentor Embedded Software Division
Mentor Graphics (UK) Ltd.