2017-10-26 05:15:19

by Life is hard, and then you die

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

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 <[email protected]>
Cc: Dean Jenkins <[email protected]>
Cc: Lukas Wunner <[email protected]>
Cc: Marcel Holtmann <[email protected]>
Cc: Gustavo Padovan <[email protected]>
Cc: Johan Hedberg <[email protected]>
---
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

2017-10-30 14:49:16

by Marcel Holtmann

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

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 <[email protected]>
> Cc: Dean Jenkins <[email protected]>
> Cc: Lukas Wunner <[email protected]>
> Cc: Marcel Holtmann <[email protected]>
> Cc: Gustavo Padovan <[email protected]>
> Cc: Johan Hedberg <[email protected]>
> ---
> drivers/bluetooth/hci_ldisc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

2017-10-26 06:52:02

by Lukas Wunner

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

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 <[email protected]>

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