[PATCH] Bluetooth: hci_ldisc: Fix another race when closing the tty.

[PATCH] Bluetooth: hci_ldisc: Fix another race when closing the tty.

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

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 <ronald@innovation.ch>
Cc: Dean Jenkins <Dean_Jenkins@mentor.com>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
---
 drivers/bluetooth/hci_ldisc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 31def781a562..c823914b3a80 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -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);
-- 
2.13.6

Re: [PATCH] Bluetooth: hci_ldisc: Fix another race when closing the tty.

[Lukas Wunner]

On Wed, Oct 25, 2017 at 10:15:19PM -0700, =?UTF-8?q?Ronald=20Tschal=C3=A4r?= wrote:
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -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);
> +

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

Reviewed-by: Lukas Wunner <lukas@wunner.de>

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

Re: [PATCH] Bluetooth: hci_ldisc: Fix another race when closing the tty.

[Marcel Holtmann]

Hi Ronald,

> 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 <ronald@innovation.ch>
> Cc: Dean Jenkins <Dean_Jenkins@mentor.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Johan Hedberg <johan.hedberg@gmail.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel