* [PATCH v1] Bluetooth: hci_uart: fix race during initialization
@ 2024-11-27 12:38 Arseniy Krasnov
2024-11-27 13:21 ` [v1] " bluez.test.bot
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Arseniy Krasnov @ 2024-11-27 12:38 UTC (permalink / raw)
To: Marcel Holtmann, Luiz Augusto von Dentz
Cc: linux-bluetooth, linux-kernel, oxffffaa, ddrokosov
'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
--
2.30.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* RE: [v1] Bluetooth: hci_uart: fix race during initialization
2024-11-27 12:38 [PATCH v1] Bluetooth: hci_uart: fix race during initialization Arseniy Krasnov
@ 2024-11-27 13:21 ` bluez.test.bot
2024-12-16 11:58 ` [PATCH v1] " Arseniy Krasnov
2024-12-16 14:52 ` Luiz Augusto von Dentz
2 siblings, 0 replies; 6+ messages in thread
From: bluez.test.bot @ 2024-11-27 13:21 UTC (permalink / raw)
To: linux-bluetooth, avkrasnov
[-- Attachment #1: Type: text/plain, Size: 2308 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=912808
---Test result---
Test Summary:
CheckPatch PENDING 0.28 seconds
GitLint PENDING 0.25 seconds
SubjectPrefix PASS 0.09 seconds
BuildKernel PASS 24.87 seconds
CheckAllWarning PASS 27.04 seconds
CheckSparse PASS 30.44 seconds
BuildKernel32 PASS 24.55 seconds
TestRunnerSetup PASS 435.17 seconds
TestRunner_l2cap-tester PASS 20.15 seconds
TestRunner_iso-tester FAIL 28.03 seconds
TestRunner_bnep-tester PASS 4.81 seconds
TestRunner_mgmt-tester PASS 122.28 seconds
TestRunner_rfcomm-tester PASS 7.51 seconds
TestRunner_sco-tester PASS 9.32 seconds
TestRunner_ioctl-tester PASS 8.01 seconds
TestRunner_mesh-tester FAIL 8.07 seconds
TestRunner_smp-tester PASS 7.44 seconds
TestRunner_userchan-tester PASS 5.00 seconds
IncrementalBuild PENDING 0.46 seconds
Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:
##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
WARNING: possible circular locking dependency detected
Total: 124, Passed: 120 (96.8%), Failed: 0, Not Run: 4
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
BUG: KASAN: slab-use-after-free in run_timer_softirq+0x76c/0x7d0
WARNING: CPU: 0 PID: 33 at kernel/workqueue.c:2257 __queue_work+0x687/0xb40
Total: 10, Passed: 9 (90.0%), Failed: 1, Not Run: 0
Failed Test Cases
Mesh - Send cancel - 1 Failed 0.103 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] Bluetooth: hci_uart: fix race during initialization
2024-11-27 12:38 [PATCH v1] Bluetooth: hci_uart: fix race during initialization Arseniy Krasnov
2024-11-27 13:21 ` [v1] " bluez.test.bot
@ 2024-12-16 11:58 ` Arseniy Krasnov
2024-12-16 14:52 ` Luiz Augusto von Dentz
2 siblings, 0 replies; 6+ messages in thread
From: Arseniy Krasnov @ 2024-12-16 11:58 UTC (permalink / raw)
To: Marcel Holtmann, Luiz Augusto von Dentz
Cc: linux-bluetooth, linux-kernel, oxffffaa, ddrokosov
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v1] Bluetooth: hci_uart: fix race during initialization
2024-11-27 12:38 [PATCH v1] Bluetooth: hci_uart: fix race during initialization Arseniy Krasnov
2024-11-27 13:21 ` [v1] " bluez.test.bot
2024-12-16 11:58 ` [PATCH v1] " Arseniy Krasnov
@ 2024-12-16 14:52 ` Luiz Augusto von Dentz
2024-12-16 15:06 ` Arseniy Krasnov
2 siblings, 1 reply; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2024-12-16 14:52 UTC (permalink / raw)
To: Arseniy Krasnov
Cc: Marcel Holtmann, linux-bluetooth, linux-kernel, oxffffaa,
ddrokosov
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
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v1] Bluetooth: hci_uart: fix race during initialization
2024-12-16 14:52 ` Luiz Augusto von Dentz
@ 2024-12-16 15:06 ` Arseniy Krasnov
0 siblings, 0 replies; 6+ messages in thread
From: Arseniy Krasnov @ 2024-12-16 15:06 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Marcel Holtmann, linux-bluetooth, linux-kernel, oxffffaa,
ddrokosov
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
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v1] Bluetooth: hci_uart: fix race during initialization
@ 2025-02-12 9:52 Arseniy Krasnov
2025-02-12 16:02 ` [v1] " bluez.test.bot
0 siblings, 1 reply; 6+ messages in thread
From: Arseniy Krasnov @ 2025-02-12 9:52 UTC (permalink / raw)
To: Marcel Holtmann, Luiz Augusto von Dentz
Cc: oxffffaa, linux-kernel, linux-bluetooth, kernel
Do not set 'HCI_UART_PROTO_READY' before call 'hci_uart_register_dev()'.
Possible race is when someone calls 'hci_tty_uart_close()' after this bit
is set, but 'hci_uart_register_dev()' wasn't done. This leads to access
to uninitialized fields. To fix it let's set this bit after device was
registered (as before patch c411c62cc133) and to fix previous problem let's
add one more bit in addition to 'HCI_UART_PROTO_READY' which allows to
perform power up without original bit set (pls see commit c411c62cc133).
Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
Reported-by: syzbot+683f8cb11b94b1824c77@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-bluetooth/d159c57f-8490-4c26-79da-6ad3612c4a14@salutedevices.com/
Fixes: c411c62cc133 ("Bluetooth: hci_uart: fix race during initialization")
---
drivers/bluetooth/hci_ldisc.c | 20 ++++++++++++++------
drivers/bluetooth/hci_uart.h | 1 +
2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 2651e2e33f2a1..2f322f890b81f 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;
}
@@ -585,7 +588,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);
}
@@ -611,7 +615,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;
}
@@ -707,13 +712,16 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id)
hu->proto = p;
- set_bit(HCI_UART_PROTO_READY, &hu->flags);
+ 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 fbf3079b92a53..5ea5dd80e297c 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -90,6 +90,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
^ permalink raw reply related [flat|nested] 6+ messages in thread* RE: [v1] Bluetooth: hci_uart: fix race during initialization
2025-02-12 9:52 Arseniy Krasnov
@ 2025-02-12 16:02 ` bluez.test.bot
0 siblings, 0 replies; 6+ messages in thread
From: bluez.test.bot @ 2025-02-12 16:02 UTC (permalink / raw)
To: linux-bluetooth, avkrasnov
[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=933152
---Test result---
Test Summary:
CheckPatch PENDING 0.29 seconds
GitLint PENDING 0.18 seconds
SubjectPrefix PASS 0.12 seconds
BuildKernel PASS 25.44 seconds
CheckAllWarning PASS 27.60 seconds
CheckSparse PASS 31.87 seconds
BuildKernel32 PASS 24.96 seconds
TestRunnerSetup PASS 442.34 seconds
TestRunner_l2cap-tester PASS 21.03 seconds
TestRunner_iso-tester PASS 32.77 seconds
TestRunner_bnep-tester PASS 4.97 seconds
TestRunner_mgmt-tester FAIL 122.86 seconds
TestRunner_rfcomm-tester PASS 7.75 seconds
TestRunner_sco-tester PASS 9.59 seconds
TestRunner_ioctl-tester PASS 8.25 seconds
TestRunner_mesh-tester PASS 6.04 seconds
TestRunner_smp-tester PASS 7.17 seconds
TestRunner_userchan-tester PASS 5.24 seconds
IncrementalBuild PENDING 0.68 seconds
Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:
##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 485 (99.0%), Failed: 1, Not Run: 4
Failed Test Cases
LL Privacy - Set Flags 3 (2 Devices to RL) Failed 0.178 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-02-12 16:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-27 12:38 [PATCH v1] Bluetooth: hci_uart: fix race during initialization Arseniy Krasnov
2024-11-27 13:21 ` [v1] " bluez.test.bot
2024-12-16 11:58 ` [PATCH v1] " Arseniy Krasnov
2024-12-16 14:52 ` Luiz Augusto von Dentz
2024-12-16 15:06 ` Arseniy Krasnov
-- strict thread matches above, loose matches on Subject: below --
2025-02-12 9:52 Arseniy Krasnov
2025-02-12 16:02 ` [v1] " bluez.test.bot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.