diff mbox series

[v1] Bluetooth: hci_uart: fix race during initialization

Message ID a1db0c90-1803-e01c-3e23-d18e4343a4eb@salutedevices.com
State New
Headers show
Series [v1] Bluetooth: hci_uart: fix race during initialization | expand

Commit Message

Arseniy Krasnov Nov. 27, 2024, 12:38 p.m. UTC 
'hci_register_dev()' calls power up function, which is executed by
kworker - 'hci_power_on()'. This function does access to bluetooth chip
using callbacks from 'hci_ldisc.c', for example 'hci_uart_send_frame()'.
Now 'hci_uart_send_frame()' checks 'HCI_UART_PROTO_READY' bit set, and
if not - it fails. Problem is that 'HCI_UART_PROTO_READY' is set after
'hci_register_dev()', and there is tiny chance that 'hci_power_on()' will
be executed before setting this bit. In that case HCI init logic fails.

Patch adds one more bit in addition to 'HCI_UART_PROTO_READY' which
allows to perform power up logic without original bit set.

Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
---
 drivers/bluetooth/hci_ldisc.c | 19 ++++++++++++++-----
 drivers/bluetooth/hci_uart.h  |  1 +
 2 files changed, 15 insertions(+), 5 deletions(-)

Comments

Arseniy Krasnov Dec. 16, 2024, 11:58 a.m. UTC | #1 
Hi, sorry, pls ping

Thanks

On 27.11.2024 15:38, Arseniy Krasnov wrote:
> 'hci_register_dev()' calls power up function, which is executed by
> kworker - 'hci_power_on()'. This function does access to bluetooth chip
> using callbacks from 'hci_ldisc.c', for example 'hci_uart_send_frame()'.
> Now 'hci_uart_send_frame()' checks 'HCI_UART_PROTO_READY' bit set, and
> if not - it fails. Problem is that 'HCI_UART_PROTO_READY' is set after
> 'hci_register_dev()', and there is tiny chance that 'hci_power_on()' will
> be executed before setting this bit. In that case HCI init logic fails.
> 
> Patch adds one more bit in addition to 'HCI_UART_PROTO_READY' which
> allows to perform power up logic without original bit set.
> 
> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
> ---
>  drivers/bluetooth/hci_ldisc.c | 19 ++++++++++++++-----
>  drivers/bluetooth/hci_uart.h  |  1 +
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 30192bb083549..2e20f00649bd7 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -102,7 +102,8 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
>  	if (!skb) {
>  		percpu_down_read(&hu->proto_lock);
>  
> -		if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
> +		if (test_bit(HCI_UART_PROTO_READY, &hu->flags) ||
> +		    test_bit(HCI_UART_PROTO_INIT, &hu->flags))
>  			skb = hu->proto->dequeue(hu);
>  
>  		percpu_up_read(&hu->proto_lock);
> @@ -124,7 +125,8 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
>  	if (!percpu_down_read_trylock(&hu->proto_lock))
>  		return 0;
>  
> -	if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
> +	if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
> +	    !test_bit(HCI_UART_PROTO_INIT, &hu->flags))
>  		goto no_schedule;
>  
>  	set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
> @@ -278,7 +280,8 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
>  
>  	percpu_down_read(&hu->proto_lock);
>  
> -	if (!test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
> +	if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
> +	    !test_bit(HCI_UART_PROTO_INIT, &hu->flags)) {
>  		percpu_up_read(&hu->proto_lock);
>  		return -EUNATCH;
>  	}
> @@ -582,7 +585,8 @@ static void hci_uart_tty_wakeup(struct tty_struct *tty)
>  	if (tty != hu->tty)
>  		return;
>  
> -	if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
> +	if (test_bit(HCI_UART_PROTO_READY, &hu->flags) ||
> +	    test_bit(HCI_UART_PROTO_INIT, &hu->flags))
>  		hci_uart_tx_wakeup(hu);
>  }
>  
> @@ -608,7 +612,8 @@ static void hci_uart_tty_receive(struct tty_struct *tty, const u8 *data,
>  
>  	percpu_down_read(&hu->proto_lock);
>  
> -	if (!test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
> +	if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
> +	    !test_bit(HCI_UART_PROTO_INIT, &hu->flags)) {
>  		percpu_up_read(&hu->proto_lock);
>  		return;
>  	}
> @@ -704,12 +709,16 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id)
>  
>  	hu->proto = p;
>  
> +	set_bit(HCI_UART_PROTO_INIT, &hu->flags);
> +
>  	err = hci_uart_register_dev(hu);
>  	if (err) {
>  		return err;
>  	}
>  
>  	set_bit(HCI_UART_PROTO_READY, &hu->flags);
> +	clear_bit(HCI_UART_PROTO_INIT, &hu->flags);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index 00bf7ae82c5b7..39f39704be41f 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -89,6 +89,7 @@ struct hci_uart {
>  #define HCI_UART_REGISTERED		1
>  #define HCI_UART_PROTO_READY		2
>  #define HCI_UART_NO_SUSPEND_NOTIFIER	3
> +#define HCI_UART_PROTO_INIT		4
>  
>  /* TX states  */
>  #define HCI_UART_SENDING	1
Luiz Augusto von Dentz Dec. 16, 2024, 2:52 p.m. UTC | #2 
Hi Arseniy,

On Wed, Nov 27, 2024 at 7:38 AM Arseniy Krasnov
<avkrasnov@salutedevices.com> wrote:
>
> 'hci_register_dev()' calls power up function, which is executed by
> kworker - 'hci_power_on()'. This function does access to bluetooth chip
> using callbacks from 'hci_ldisc.c', for example 'hci_uart_send_frame()'.
> Now 'hci_uart_send_frame()' checks 'HCI_UART_PROTO_READY' bit set, and
> if not - it fails. Problem is that 'HCI_UART_PROTO_READY' is set after
> 'hci_register_dev()', and there is tiny chance that 'hci_power_on()' will
> be executed before setting this bit. In that case HCI init logic fails.
>
> Patch adds one more bit in addition to 'HCI_UART_PROTO_READY' which
> allows to perform power up logic without original bit set.
>
> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
> ---
>  drivers/bluetooth/hci_ldisc.c | 19 ++++++++++++++-----
>  drivers/bluetooth/hci_uart.h  |  1 +
>  2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 30192bb083549..2e20f00649bd7 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -102,7 +102,8 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
>         if (!skb) {
>                 percpu_down_read(&hu->proto_lock);
>
> -               if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
> +               if (test_bit(HCI_UART_PROTO_READY, &hu->flags) ||
> +                   test_bit(HCI_UART_PROTO_INIT, &hu->flags))
>                         skb = hu->proto->dequeue(hu);
>
>                 percpu_up_read(&hu->proto_lock);
> @@ -124,7 +125,8 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
>         if (!percpu_down_read_trylock(&hu->proto_lock))
>                 return 0;
>
> -       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
> +       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
> +           !test_bit(HCI_UART_PROTO_INIT, &hu->flags))
>                 goto no_schedule;
>
>         set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
> @@ -278,7 +280,8 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
>
>         percpu_down_read(&hu->proto_lock);
>
> -       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
> +       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
> +           !test_bit(HCI_UART_PROTO_INIT, &hu->flags)) {
>                 percpu_up_read(&hu->proto_lock);
>                 return -EUNATCH;
>         }
> @@ -582,7 +585,8 @@ static void hci_uart_tty_wakeup(struct tty_struct *tty)
>         if (tty != hu->tty)
>                 return;
>
> -       if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
> +       if (test_bit(HCI_UART_PROTO_READY, &hu->flags) ||
> +           test_bit(HCI_UART_PROTO_INIT, &hu->flags))
>                 hci_uart_tx_wakeup(hu);
>  }
>
> @@ -608,7 +612,8 @@ static void hci_uart_tty_receive(struct tty_struct *tty, const u8 *data,
>
>         percpu_down_read(&hu->proto_lock);
>
> -       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
> +       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
> +           !test_bit(HCI_UART_PROTO_INIT, &hu->flags)) {
>                 percpu_up_read(&hu->proto_lock);
>                 return;
>         }
> @@ -704,12 +709,16 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id)
>
>         hu->proto = p;
>
> +       set_bit(HCI_UART_PROTO_INIT, &hu->flags);
> +
>         err = hci_uart_register_dev(hu);
>         if (err) {
>                 return err;
>         }
>
>         set_bit(HCI_UART_PROTO_READY, &hu->flags);
> +       clear_bit(HCI_UART_PROTO_INIT, &hu->flags);

This should be quite obvious, why don't you just move the
HCI_UART_PROTO_READY above hci_uart_register_dev?

>         return 0;
>  }
>
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index 00bf7ae82c5b7..39f39704be41f 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -89,6 +89,7 @@ struct hci_uart {
>  #define HCI_UART_REGISTERED            1
>  #define HCI_UART_PROTO_READY           2
>  #define HCI_UART_NO_SUSPEND_NOTIFIER   3
> +#define HCI_UART_PROTO_INIT            4
>
>  /* TX states  */
>  #define HCI_UART_SENDING       1
> --
> 2.30.1
Arseniy Krasnov Dec. 16, 2024, 3:06 p.m. UTC | #3 
On 16.12.2024 17:52, Luiz Augusto von Dentz wrote:
> Hi Arseniy,
> 
> On Wed, Nov 27, 2024 at 7:38 AM Arseniy Krasnov
> <avkrasnov@salutedevices.com> wrote:
>>
>> 'hci_register_dev()' calls power up function, which is executed by
>> kworker - 'hci_power_on()'. This function does access to bluetooth chip
>> using callbacks from 'hci_ldisc.c', for example 'hci_uart_send_frame()'.
>> Now 'hci_uart_send_frame()' checks 'HCI_UART_PROTO_READY' bit set, and
>> if not - it fails. Problem is that 'HCI_UART_PROTO_READY' is set after
>> 'hci_register_dev()', and there is tiny chance that 'hci_power_on()' will
>> be executed before setting this bit. In that case HCI init logic fails.
>>
>> Patch adds one more bit in addition to 'HCI_UART_PROTO_READY' which
>> allows to perform power up logic without original bit set.
>>
>> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
>> ---
>>  drivers/bluetooth/hci_ldisc.c | 19 ++++++++++++++-----
>>  drivers/bluetooth/hci_uart.h  |  1 +
>>  2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> index 30192bb083549..2e20f00649bd7 100644
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -102,7 +102,8 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
>>         if (!skb) {
>>                 percpu_down_read(&hu->proto_lock);
>>
>> -               if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
>> +               if (test_bit(HCI_UART_PROTO_READY, &hu->flags) ||
>> +                   test_bit(HCI_UART_PROTO_INIT, &hu->flags))
>>                         skb = hu->proto->dequeue(hu);
>>
>>                 percpu_up_read(&hu->proto_lock);
>> @@ -124,7 +125,8 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
>>         if (!percpu_down_read_trylock(&hu->proto_lock))
>>                 return 0;
>>
>> -       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
>> +       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
>> +           !test_bit(HCI_UART_PROTO_INIT, &hu->flags))
>>                 goto no_schedule;
>>
>>         set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
>> @@ -278,7 +280,8 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
>>
>>         percpu_down_read(&hu->proto_lock);
>>
>> -       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
>> +       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
>> +           !test_bit(HCI_UART_PROTO_INIT, &hu->flags)) {
>>                 percpu_up_read(&hu->proto_lock);
>>                 return -EUNATCH;
>>         }
>> @@ -582,7 +585,8 @@ static void hci_uart_tty_wakeup(struct tty_struct *tty)
>>         if (tty != hu->tty)
>>                 return;
>>
>> -       if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
>> +       if (test_bit(HCI_UART_PROTO_READY, &hu->flags) ||
>> +           test_bit(HCI_UART_PROTO_INIT, &hu->flags))
>>                 hci_uart_tx_wakeup(hu);
>>  }
>>
>> @@ -608,7 +612,8 @@ static void hci_uart_tty_receive(struct tty_struct *tty, const u8 *data,
>>
>>         percpu_down_read(&hu->proto_lock);
>>
>> -       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
>> +       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
>> +           !test_bit(HCI_UART_PROTO_INIT, &hu->flags)) {
>>                 percpu_up_read(&hu->proto_lock);
>>                 return;
>>         }
>> @@ -704,12 +709,16 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id)
>>
>>         hu->proto = p;
>>
>> +       set_bit(HCI_UART_PROTO_INIT, &hu->flags);
>> +
>>         err = hci_uart_register_dev(hu);
>>         if (err) {
>>                 return err;
>>         }
>>
>>         set_bit(HCI_UART_PROTO_READY, &hu->flags);
>> +       clear_bit(HCI_UART_PROTO_INIT, &hu->flags);
> 
> This should be quite obvious, why don't you just move the
> HCI_UART_PROTO_READY above hci_uart_register_dev?
> 

Hi, this is simplest way, but I had doubts that this may be so obvious because
I don't have enough experience in bt logic.

I'll send v2 with it.

Thanks

>>         return 0;
>>  }
>>
>> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
>> index 00bf7ae82c5b7..39f39704be41f 100644
>> --- a/drivers/bluetooth/hci_uart.h
>> +++ b/drivers/bluetooth/hci_uart.h
>> @@ -89,6 +89,7 @@ struct hci_uart {
>>  #define HCI_UART_REGISTERED            1
>>  #define HCI_UART_PROTO_READY           2
>>  #define HCI_UART_NO_SUSPEND_NOTIFIER   3
>> +#define HCI_UART_PROTO_INIT            4
>>
>>  /* TX states  */
>>  #define HCI_UART_SENDING       1
>> --
>> 2.30.1
> 
> 
>
diff mbox series

Patch

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 30192bb083549..2e20f00649bd7 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -102,7 +102,8 @@  static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
 	if (!skb) {
 		percpu_down_read(&hu->proto_lock);
 
-		if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
+		if (test_bit(HCI_UART_PROTO_READY, &hu->flags) ||
+		    test_bit(HCI_UART_PROTO_INIT, &hu->flags))
 			skb = hu->proto->dequeue(hu);
 
 		percpu_up_read(&hu->proto_lock);
@@ -124,7 +125,8 @@  int hci_uart_tx_wakeup(struct hci_uart *hu)
 	if (!percpu_down_read_trylock(&hu->proto_lock))
 		return 0;
 
-	if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
+	if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
+	    !test_bit(HCI_UART_PROTO_INIT, &hu->flags))
 		goto no_schedule;
 
 	set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
@@ -278,7 +280,8 @@  static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 
 	percpu_down_read(&hu->proto_lock);
 
-	if (!test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
+	if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
+	    !test_bit(HCI_UART_PROTO_INIT, &hu->flags)) {
 		percpu_up_read(&hu->proto_lock);
 		return -EUNATCH;
 	}
@@ -582,7 +585,8 @@  static void hci_uart_tty_wakeup(struct tty_struct *tty)
 	if (tty != hu->tty)
 		return;
 
-	if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
+	if (test_bit(HCI_UART_PROTO_READY, &hu->flags) ||
+	    test_bit(HCI_UART_PROTO_INIT, &hu->flags))
 		hci_uart_tx_wakeup(hu);
 }
 
@@ -608,7 +612,8 @@  static void hci_uart_tty_receive(struct tty_struct *tty, const u8 *data,
 
 	percpu_down_read(&hu->proto_lock);
 
-	if (!test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
+	if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
+	    !test_bit(HCI_UART_PROTO_INIT, &hu->flags)) {
 		percpu_up_read(&hu->proto_lock);
 		return;
 	}
@@ -704,12 +709,16 @@  static int hci_uart_set_proto(struct hci_uart *hu, int id)
 
 	hu->proto = p;
 
+	set_bit(HCI_UART_PROTO_INIT, &hu->flags);
+
 	err = hci_uart_register_dev(hu);
 	if (err) {
 		return err;
 	}
 
 	set_bit(HCI_UART_PROTO_READY, &hu->flags);
+	clear_bit(HCI_UART_PROTO_INIT, &hu->flags);
+
 	return 0;
 }
 
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 00bf7ae82c5b7..39f39704be41f 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -89,6 +89,7 @@  struct hci_uart {
 #define HCI_UART_REGISTERED		1
 #define HCI_UART_PROTO_READY		2
 #define HCI_UART_NO_SUSPEND_NOTIFIER	3
+#define HCI_UART_PROTO_INIT		4
 
 /* TX states  */
 #define HCI_UART_SENDING	1