* [PATCH] Bluetooth: When HCI work queue is drained, only queue chained work.
@ 2022-05-31 18:41 Schspa Shi
2022-05-31 18:58 ` Schspa Shi
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Schspa Shi @ 2022-05-31 18:41 UTC (permalink / raw)
To: marcel, johan.hedberg, luiz.dentz, davem, edumazet, kuba, pabeni
Cc: linux-bluetooth, netdev, linux-kernel, Schspa Shi,
syzbot+63bed493aebbf6872647
The HCI command, event, and data packet processing workqueue is drained
to avoid deadlock in commit
76727c02c1e1 ("Bluetooth: Call drain_workqueue() before resetting state").
There is another delayed work, which will queue command to this drained
workqueue. Which results in the following error report:
Bluetooth: hci2: command 0x040f tx timeout
WARNING: CPU: 1 PID: 18374 at kernel/workqueue.c:1438 __queue_work+0xdad/0x1140
Modules linked in:
CPU: 1 PID: 18374 Comm: kworker/1:9 Not tainted 5.18.0-rc6-next-20220516-syzkaller #0
Workqueue: events hci_cmd_timeout
RIP: 0010:__queue_work+0xdad/0x1140
RSP: 0000:ffffc90002cffc60 EFLAGS: 00010093
RAX: 0000000000000000 RBX: ffff8880b9d3ec00 RCX: 0000000000000000
RDX: ffff888024ba0000 RSI: ffffffff814e048d RDI: ffff8880b9d3ec08
RBP: 0000000000000008 R08: 0000000000000000 R09: 00000000b9d39700
R10: ffffffff814f73c6 R11: 0000000000000000 R12: ffff88807cce4c60
R13: 0000000000000000 R14: ffff8880796d8800 R15: ffff8880796d8800
FS: 0000000000000000(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000c0174b4000 CR3: 000000007cae9000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
? queue_work_on+0xcb/0x110
? lockdep_hardirqs_off+0x90/0xd0
queue_work_on+0xee/0x110
process_one_work+0x996/0x1610
? pwq_dec_nr_in_flight+0x2a0/0x2a0
? rwlock_bug.part.0+0x90/0x90
? _raw_spin_lock_irq+0x41/0x50
worker_thread+0x665/0x1080
? process_one_work+0x1610/0x1610
kthread+0x2e9/0x3a0
? kthread_complete_and_exit+0x40/0x40
ret_from_fork+0x1f/0x30
</TASK>
To fix this, we can add a new HCI_DRAIN_WQ flag, and don't queue the
timeout workqueue while command workqueue is draining.
Fixes: 76727c02c1e1 ("Bluetooth: Call drain_workqueue() before resetting state")
Reported-by: syzbot+63bed493aebbf6872647@syzkaller.appspotmail.com
Signed-off-by: Schspa Shi <schspa@gmail.com>
---
include/net/bluetooth/hci.h | 1 +
net/bluetooth/hci_core.c | 8 +++++++-
net/bluetooth/hci_event.c | 5 +++--
3 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index fe7935be7dc4..c2cba0a621d3 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -291,6 +291,7 @@ enum {
HCI_RAW,
HCI_RESET,
+ HCI_DRAIN_WQ,
};
/* HCI socket flags */
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 5abb2ca5b129..ef3bd543ce04 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -593,6 +593,10 @@ static int hci_dev_do_reset(struct hci_dev *hdev)
skb_queue_purge(&hdev->rx_q);
skb_queue_purge(&hdev->cmd_q);
+ hci_dev_lock(hdev);
+ set_bit(HCI_DRAIN_WQ, &hdev->flags);
+ hci_dev_unlock(hdev);
+ cancel_delayed_work(&hdev->cmd_timer);
/* Avoid potential lockdep warnings from the *_flush() calls by
* ensuring the workqueue is empty up front.
*/
@@ -601,6 +605,7 @@ static int hci_dev_do_reset(struct hci_dev *hdev)
hci_dev_lock(hdev);
hci_inquiry_cache_flush(hdev);
hci_conn_hash_flush(hdev);
+ clear_bit(HCI_DRAIN_WQ, &hdev->flags);
hci_dev_unlock(hdev);
if (hdev->flush)
@@ -3861,7 +3866,8 @@ static void hci_cmd_work(struct work_struct *work)
if (res < 0)
__hci_cmd_sync_cancel(hdev, -res);
- if (test_bit(HCI_RESET, &hdev->flags))
+ if (test_bit(HCI_RESET, &hdev->flags) ||
+ test_bit(HCI_DRAIN_WQ, &hdev->flags))
cancel_delayed_work(&hdev->cmd_timer);
else
schedule_delayed_work(&hdev->cmd_timer,
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index af17dfb20e01..700cd01df3a1 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3768,8 +3768,9 @@ static inline void handle_cmd_cnt_and_timer(struct hci_dev *hdev, u8 ncmd)
cancel_delayed_work(&hdev->ncmd_timer);
atomic_set(&hdev->cmd_cnt, 1);
} else {
- schedule_delayed_work(&hdev->ncmd_timer,
- HCI_NCMD_TIMEOUT);
+ if (!test_bit(HCI_DRAIN_WQ, &hdev->flags))
+ schedule_delayed_work(&hdev->ncmd_timer,
+ HCI_NCMD_TIMEOUT);
}
}
}
--
2.24.3 (Apple Git-128)
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] Bluetooth: When HCI work queue is drained, only queue chained work.
2022-05-31 18:41 [PATCH] Bluetooth: When HCI work queue is drained, only queue chained work Schspa Shi
@ 2022-05-31 18:58 ` Schspa Shi
2022-05-31 19:11 ` bluez.test.bot
2022-06-02 15:37 ` [PATCH] " Marcel Holtmann
2 siblings, 0 replies; 5+ messages in thread
From: Schspa Shi @ 2022-05-31 18:58 UTC (permalink / raw)
To: marcel, johan.hedberg, luiz.dentz, davem, Eric Dumazet, kuba, pabeni
Cc: linux-bluetooth, netdev, Linux Kernel Mailing List, Schspa Shi,
syzbot+63bed493aebbf6872647
Schspa Shi <schspa@gmail.com> writes:
> The HCI command, event, and data packet processing workqueue is drained
> to avoid deadlock in commit
> 76727c02c1e1 ("Bluetooth: Call drain_workqueue() before resetting state").
>
> There is another delayed work, which will queue command to this drained
> workqueue. Which results in the following error report:
>
> Bluetooth: hci2: command 0x040f tx timeout
> WARNING: CPU: 1 PID: 18374 at kernel/workqueue.c:1438 __queue_work+0xdad/0x1140
> Modules linked in:
> CPU: 1 PID: 18374 Comm: kworker/1:9 Not tainted 5.18.0-rc6-next-20220516-syzkaller #0
> Workqueue: events hci_cmd_timeout
> RIP: 0010:__queue_work+0xdad/0x1140
> RSP: 0000:ffffc90002cffc60 EFLAGS: 00010093
> RAX: 0000000000000000 RBX: ffff8880b9d3ec00 RCX: 0000000000000000
> RDX: ffff888024ba0000 RSI: ffffffff814e048d RDI: ffff8880b9d3ec08
> RBP: 0000000000000008 R08: 0000000000000000 R09: 00000000b9d39700
> R10: ffffffff814f73c6 R11: 0000000000000000 R12: ffff88807cce4c60
> R13: 0000000000000000 R14: ffff8880796d8800 R15: ffff8880796d8800
> FS: 0000000000000000(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000000c0174b4000 CR3: 000000007cae9000 CR4: 00000000003506e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> ? queue_work_on+0xcb/0x110
> ? lockdep_hardirqs_off+0x90/0xd0
> queue_work_on+0xee/0x110
> process_one_work+0x996/0x1610
> ? pwq_dec_nr_in_flight+0x2a0/0x2a0
> ? rwlock_bug.part.0+0x90/0x90
> ? _raw_spin_lock_irq+0x41/0x50
> worker_thread+0x665/0x1080
> ? process_one_work+0x1610/0x1610
> kthread+0x2e9/0x3a0
> ? kthread_complete_and_exit+0x40/0x40
> ret_from_fork+0x1f/0x30
> </TASK>
>
> To fix this, we can add a new HCI_DRAIN_WQ flag, and don't queue the
> timeout workqueue while command workqueue is draining.
>
> Fixes: 76727c02c1e1 ("Bluetooth: Call drain_workqueue() before resetting state")
> Reported-by: syzbot+63bed493aebbf6872647@syzkaller.appspotmail.com
> Signed-off-by: Schspa Shi <schspa@gmail.com>
> ---
> include/net/bluetooth/hci.h | 1 +
> net/bluetooth/hci_core.c | 8 +++++++-
> net/bluetooth/hci_event.c | 5 +++--
> 3 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index fe7935be7dc4..c2cba0a621d3 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -291,6 +291,7 @@ enum {
> HCI_RAW,
>
> HCI_RESET,
> + HCI_DRAIN_WQ,
> };
>
> /* HCI socket flags */
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 5abb2ca5b129..ef3bd543ce04 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -593,6 +593,10 @@ static int hci_dev_do_reset(struct hci_dev *hdev)
> skb_queue_purge(&hdev->rx_q);
> skb_queue_purge(&hdev->cmd_q);
>
> + hci_dev_lock(hdev);
> + set_bit(HCI_DRAIN_WQ, &hdev->flags);
> + hci_dev_unlock(hdev);
> + cancel_delayed_work(&hdev->cmd_timer);
Need to add cancel_delayed_work(&hdev->ncmd_timer);
I'll make a new patch after the rest of this patch has been reviewed.
> /* Avoid potential lockdep warnings from the *_flush() calls by
> * ensuring the workqueue is empty up front.
> */
> @@ -601,6 +605,7 @@ static int hci_dev_do_reset(struct hci_dev *hdev)
> hci_dev_lock(hdev);
> hci_inquiry_cache_flush(hdev);
> hci_conn_hash_flush(hdev);
> + clear_bit(HCI_DRAIN_WQ, &hdev->flags);
> hci_dev_unlock(hdev);
>
> if (hdev->flush)
> @@ -3861,7 +3866,8 @@ static void hci_cmd_work(struct work_struct *work)
> if (res < 0)
> __hci_cmd_sync_cancel(hdev, -res);
>
> - if (test_bit(HCI_RESET, &hdev->flags))
> + if (test_bit(HCI_RESET, &hdev->flags) ||
> + test_bit(HCI_DRAIN_WQ, &hdev->flags))
> cancel_delayed_work(&hdev->cmd_timer);
> else
> schedule_delayed_work(&hdev->cmd_timer,
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index af17dfb20e01..700cd01df3a1 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3768,8 +3768,9 @@ static inline void handle_cmd_cnt_and_timer(struct hci_dev *hdev, u8 ncmd)
> cancel_delayed_work(&hdev->ncmd_timer);
> atomic_set(&hdev->cmd_cnt, 1);
> } else {
> - schedule_delayed_work(&hdev->ncmd_timer,
> - HCI_NCMD_TIMEOUT);
> + if (!test_bit(HCI_DRAIN_WQ, &hdev->flags))
> + schedule_delayed_work(&hdev->ncmd_timer,
> + HCI_NCMD_TIMEOUT);
> }
> }
> }
---
BRs
Schspa Shi
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: Bluetooth: When HCI work queue is drained, only queue chained work.
2022-05-31 18:41 [PATCH] Bluetooth: When HCI work queue is drained, only queue chained work Schspa Shi
2022-05-31 18:58 ` Schspa Shi
@ 2022-05-31 19:11 ` bluez.test.bot
2022-06-02 15:37 ` [PATCH] " Marcel Holtmann
2 siblings, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2022-05-31 19:11 UTC (permalink / raw)
To: linux-bluetooth, schspa
[-- Attachment #1: Type: text/plain, Size: 1516 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=646337
---Test result---
Test Summary:
CheckPatch PASS 2.03 seconds
GitLint FAIL 1.10 seconds
SubjectPrefix PASS 0.83 seconds
BuildKernel PASS 32.53 seconds
BuildKernel32 PASS 27.94 seconds
Incremental Build with patchesPASS 39.13 seconds
TestRunner: Setup PASS 481.36 seconds
TestRunner: l2cap-tester PASS 17.23 seconds
TestRunner: bnep-tester PASS 6.08 seconds
TestRunner: mgmt-tester PASS 101.04 seconds
TestRunner: rfcomm-tester PASS 9.63 seconds
TestRunner: sco-tester PASS 9.45 seconds
TestRunner: smp-tester PASS 9.44 seconds
TestRunner: userchan-tester PASS 6.33 seconds
Details
##############################
Test: GitLint - FAIL - 1.10 seconds
Run gitlint with rule in .gitlint
Bluetooth: When HCI work queue is drained, only queue chained work.
1: T3 Title has trailing punctuation (.): "Bluetooth: When HCI work queue is drained, only queue chained work."
13: B1 Line exceeds max length (85>80): "CPU: 1 PID: 18374 Comm: kworker/1:9 Not tainted 5.18.0-rc6-next-20220516-syzkaller #0"
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Bluetooth: When HCI work queue is drained, only queue chained work.
2022-05-31 18:41 [PATCH] Bluetooth: When HCI work queue is drained, only queue chained work Schspa Shi
2022-05-31 18:58 ` Schspa Shi
2022-05-31 19:11 ` bluez.test.bot
@ 2022-06-02 15:37 ` Marcel Holtmann
2022-06-02 15:47 ` Schspa Shi
2 siblings, 1 reply; 5+ messages in thread
From: Marcel Holtmann @ 2022-06-02 15:37 UTC (permalink / raw)
To: Schspa Shi
Cc: Johan Hedberg, Luiz Augusto von Dentz, David S. Miller, edumazet,
kuba, pabeni, linux-bluetooth, netdev, linux-kernel,
syzbot+63bed493aebbf6872647
Hi Schspa,
> The HCI command, event, and data packet processing workqueue is drained
> to avoid deadlock in commit
> 76727c02c1e1 ("Bluetooth: Call drain_workqueue() before resetting state").
>
> There is another delayed work, which will queue command to this drained
> workqueue. Which results in the following error report:
>
> Bluetooth: hci2: command 0x040f tx timeout
> WARNING: CPU: 1 PID: 18374 at kernel/workqueue.c:1438 __queue_work+0xdad/0x1140
> Modules linked in:
> CPU: 1 PID: 18374 Comm: kworker/1:9 Not tainted 5.18.0-rc6-next-20220516-syzkaller #0
> Workqueue: events hci_cmd_timeout
> RIP: 0010:__queue_work+0xdad/0x1140
> RSP: 0000:ffffc90002cffc60 EFLAGS: 00010093
> RAX: 0000000000000000 RBX: ffff8880b9d3ec00 RCX: 0000000000000000
> RDX: ffff888024ba0000 RSI: ffffffff814e048d RDI: ffff8880b9d3ec08
> RBP: 0000000000000008 R08: 0000000000000000 R09: 00000000b9d39700
> R10: ffffffff814f73c6 R11: 0000000000000000 R12: ffff88807cce4c60
> R13: 0000000000000000 R14: ffff8880796d8800 R15: ffff8880796d8800
> FS: 0000000000000000(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000000c0174b4000 CR3: 000000007cae9000 CR4: 00000000003506e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> ? queue_work_on+0xcb/0x110
> ? lockdep_hardirqs_off+0x90/0xd0
> queue_work_on+0xee/0x110
> process_one_work+0x996/0x1610
> ? pwq_dec_nr_in_flight+0x2a0/0x2a0
> ? rwlock_bug.part.0+0x90/0x90
> ? _raw_spin_lock_irq+0x41/0x50
> worker_thread+0x665/0x1080
> ? process_one_work+0x1610/0x1610
> kthread+0x2e9/0x3a0
> ? kthread_complete_and_exit+0x40/0x40
> ret_from_fork+0x1f/0x30
> </TASK>
>
> To fix this, we can add a new HCI_DRAIN_WQ flag, and don't queue the
> timeout workqueue while command workqueue is draining.
>
> Fixes: 76727c02c1e1 ("Bluetooth: Call drain_workqueue() before resetting state")
> Reported-by: syzbot+63bed493aebbf6872647@syzkaller.appspotmail.com
> Signed-off-by: Schspa Shi <schspa@gmail.com>
> ---
> include/net/bluetooth/hci.h | 1 +
> net/bluetooth/hci_core.c | 8 +++++++-
> net/bluetooth/hci_event.c | 5 +++--
> 3 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index fe7935be7dc4..c2cba0a621d3 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -291,6 +291,7 @@ enum {
> HCI_RAW,
>
> HCI_RESET,
> + HCI_DRAIN_WQ,
> };
no addition to this enum please. This is ABI. Use the other one and hci_dev_{set,clear}_flag.
>
> /* HCI socket flags */
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 5abb2ca5b129..ef3bd543ce04 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -593,6 +593,10 @@ static int hci_dev_do_reset(struct hci_dev *hdev)
> skb_queue_purge(&hdev->rx_q);
> skb_queue_purge(&hdev->cmd_q);
>
> + hci_dev_lock(hdev);
> + set_bit(HCI_DRAIN_WQ, &hdev->flags);
> + hci_dev_unlock(hdev);
> + cancel_delayed_work(&hdev->cmd_timer);
Do you need to the locking here?
> /* Avoid potential lockdep warnings from the *_flush() calls by
> * ensuring the workqueue is empty up front.
> */
> @@ -601,6 +605,7 @@ static int hci_dev_do_reset(struct hci_dev *hdev)
> hci_dev_lock(hdev);
> hci_inquiry_cache_flush(hdev);
> hci_conn_hash_flush(hdev);
> + clear_bit(HCI_DRAIN_WQ, &hdev->flags);
> hci_dev_unlock(hdev);
>
> if (hdev->flush)
> @@ -3861,7 +3866,8 @@ static void hci_cmd_work(struct work_struct *work)
> if (res < 0)
> __hci_cmd_sync_cancel(hdev, -res);
>
> - if (test_bit(HCI_RESET, &hdev->flags))
> + if (test_bit(HCI_RESET, &hdev->flags) ||
> + test_bit(HCI_DRAIN_WQ, &hdev->flags))
> cancel_delayed_work(&hdev->cmd_timer);
> else
> schedule_delayed_work(&hdev->cmd_timer,
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index af17dfb20e01..700cd01df3a1 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3768,8 +3768,9 @@ static inline void handle_cmd_cnt_and_timer(struct hci_dev *hdev, u8 ncmd)
> cancel_delayed_work(&hdev->ncmd_timer);
> atomic_set(&hdev->cmd_cnt, 1);
> } else {
> - schedule_delayed_work(&hdev->ncmd_timer,
> - HCI_NCMD_TIMEOUT);
> + if (!test_bit(HCI_DRAIN_WQ, &hdev->flags))
> + schedule_delayed_work(&hdev->ncmd_timer,
> + HCI_NCMD_TIMEOUT);
> }
> }
> }
Regards
Marcel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Bluetooth: When HCI work queue is drained, only queue chained work.
2022-06-02 15:37 ` [PATCH] " Marcel Holtmann
@ 2022-06-02 15:47 ` Schspa Shi
0 siblings, 0 replies; 5+ messages in thread
From: Schspa Shi @ 2022-06-02 15:47 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Johan Hedberg, Luiz Augusto von Dentz, David S. Miller,
Eric Dumazet, kuba, pabeni, linux-bluetooth, netdev,
Linux Kernel Mailing List, syzbot+63bed493aebbf6872647
Marcel Holtmann <marcel@holtmann.org> writes:
> Hi Schspa,
>
>> The HCI command, event, and data packet processing workqueue is drained
>> to avoid deadlock in commit
>> 76727c02c1e1 ("Bluetooth: Call drain_workqueue() before resetting state").
>>
>> There is another delayed work, which will queue command to this drained
>> workqueue. Which results in the following error report:
>>
>> Bluetooth: hci2: command 0x040f tx timeout
>> WARNING: CPU: 1 PID: 18374 at kernel/workqueue.c:1438 __queue_work+0xdad/0x1140
>> Modules linked in:
>> CPU: 1 PID: 18374 Comm: kworker/1:9 Not tainted 5.18.0-rc6-next-20220516-syzkaller #0
>> Workqueue: events hci_cmd_timeout
>> RIP: 0010:__queue_work+0xdad/0x1140
>> RSP: 0000:ffffc90002cffc60 EFLAGS: 00010093
>> RAX: 0000000000000000 RBX: ffff8880b9d3ec00 RCX: 0000000000000000
>> RDX: ffff888024ba0000 RSI: ffffffff814e048d RDI: ffff8880b9d3ec08
>> RBP: 0000000000000008 R08: 0000000000000000 R09: 00000000b9d39700
>> R10: ffffffff814f73c6 R11: 0000000000000000 R12: ffff88807cce4c60
>> R13: 0000000000000000 R14: ffff8880796d8800 R15: ffff8880796d8800
>> FS: 0000000000000000(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 000000c0174b4000 CR3: 000000007cae9000 CR4: 00000000003506e0
>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> Call Trace:
>> <TASK>
>> ? queue_work_on+0xcb/0x110
>> ? lockdep_hardirqs_off+0x90/0xd0
>> queue_work_on+0xee/0x110
>> process_one_work+0x996/0x1610
>> ? pwq_dec_nr_in_flight+0x2a0/0x2a0
>> ? rwlock_bug.part.0+0x90/0x90
>> ? _raw_spin_lock_irq+0x41/0x50
>> worker_thread+0x665/0x1080
>> ? process_one_work+0x1610/0x1610
>> kthread+0x2e9/0x3a0
>> ? kthread_complete_and_exit+0x40/0x40
>> ret_from_fork+0x1f/0x30
>> </TASK>
>>
>> To fix this, we can add a new HCI_DRAIN_WQ flag, and don't queue the
>> timeout workqueue while command workqueue is draining.
>>
>> Fixes: 76727c02c1e1 ("Bluetooth: Call drain_workqueue() before resetting state")
>> Reported-by: syzbot+63bed493aebbf6872647@syzkaller.appspotmail.com
>> Signed-off-by: Schspa Shi <schspa@gmail.com>
>> ---
>> include/net/bluetooth/hci.h | 1 +
>> net/bluetooth/hci_core.c | 8 +++++++-
>> net/bluetooth/hci_event.c | 5 +++--
>> 3 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>> index fe7935be7dc4..c2cba0a621d3 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -291,6 +291,7 @@ enum {
>> HCI_RAW,
>>
>> HCI_RESET,
>> + HCI_DRAIN_WQ,
>> };
>
> no addition to this enum please. This is ABI. Use the other one and hci_dev_{set,clear}_flag.
>
>>
>> /* HCI socket flags */
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 5abb2ca5b129..ef3bd543ce04 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -593,6 +593,10 @@ static int hci_dev_do_reset(struct hci_dev *hdev)
>> skb_queue_purge(&hdev->rx_q);
>> skb_queue_purge(&hdev->cmd_q);
>>
>> + hci_dev_lock(hdev);
>> + set_bit(HCI_DRAIN_WQ, &hdev->flags);
>> + hci_dev_unlock(hdev);
>> + cancel_delayed_work(&hdev->cmd_timer);
>
> Do you need to the locking here?
>
No, it seems this lock can be removed.
I can send a new patch version to remove this.
>> /* Avoid potential lockdep warnings from the *_flush() calls by
>> * ensuring the workqueue is empty up front.
>> */
>> @@ -601,6 +605,7 @@ static int hci_dev_do_reset(struct hci_dev *hdev)
>> hci_dev_lock(hdev);
>> hci_inquiry_cache_flush(hdev);
>> hci_conn_hash_flush(hdev);
>> + clear_bit(HCI_DRAIN_WQ, &hdev->flags);
>> hci_dev_unlock(hdev);
>>
>> if (hdev->flush)
>> @@ -3861,7 +3866,8 @@ static void hci_cmd_work(struct work_struct *work)
>> if (res < 0)
>> __hci_cmd_sync_cancel(hdev, -res);
>>
>> - if (test_bit(HCI_RESET, &hdev->flags))
>> + if (test_bit(HCI_RESET, &hdev->flags) ||
>> + test_bit(HCI_DRAIN_WQ, &hdev->flags))
>> cancel_delayed_work(&hdev->cmd_timer);
>> else
>> schedule_delayed_work(&hdev->cmd_timer,
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index af17dfb20e01..700cd01df3a1 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -3768,8 +3768,9 @@ static inline void handle_cmd_cnt_and_timer(struct hci_dev *hdev, u8 ncmd)
>> cancel_delayed_work(&hdev->ncmd_timer);
>> atomic_set(&hdev->cmd_cnt, 1);
>> } else {
>> - schedule_delayed_work(&hdev->ncmd_timer,
>> - HCI_NCMD_TIMEOUT);
>> + if (!test_bit(HCI_DRAIN_WQ, &hdev->flags))
>> + schedule_delayed_work(&hdev->ncmd_timer,
>> + HCI_NCMD_TIMEOUT);
>> }
>> }
>> }
>
> Regards
>
> Marcel
--
BRs
Schspa Shi
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-02 15:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-31 18:41 [PATCH] Bluetooth: When HCI work queue is drained, only queue chained work Schspa Shi
2022-05-31 18:58 ` Schspa Shi
2022-05-31 19:11 ` bluez.test.bot
2022-06-02 15:37 ` [PATCH] " Marcel Holtmann
2022-06-02 15:47 ` Schspa Shi
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.