* [PATCH] Bluetooth: Fix sleeping function called from invalid context in hci_cmd_sync_submit()
@ 2023-09-07 12:22 Ziyang Xuan
2023-09-07 19:05 ` bluez.test.bot
2023-09-07 19:07 ` [PATCH] " Luiz Augusto von Dentz
0 siblings, 2 replies; 3+ messages in thread
From: Ziyang Xuan @ 2023-09-07 12:22 UTC (permalink / raw)
To: marcel, johan.hedberg, luiz.dentz, linux-bluetooth; +Cc: jiangzp
Syzbot reports a sleeping function called from invalid context problem.
BUG: sleeping function called from invalid context at kernel/locking/mutex.c:580
in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 4440, name: kworker/u5:1
preempt_count: 0, expected: 0
RCU nest depth: 1, expected: 0
...
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0x125/0x1b0 lib/dump_stack.c:106
__might_resched+0x3c3/0x5e0 kernel/sched/core.c:10187
__mutex_lock_common kernel/locking/mutex.c:580 [inline]
__mutex_lock+0xee/0x1340 kernel/locking/mutex.c:747
hci_cmd_sync_submit+0x3f/0x340 net/bluetooth/hci_sync.c:699
hci_cmd_sync_queue+0x79/0xa0 net/bluetooth/hci_sync.c:739
hci_abort_conn+0x15b/0x330 net/bluetooth/hci_conn.c:2928
hci_disconnect+0xc4/0x220 net/bluetooth/hci_conn.c:258
hci_link_tx_to net/bluetooth/hci_core.c:3421 [inline]
__check_timeout net/bluetooth/hci_core.c:3567 [inline]
__check_timeout+0x331/0x4e0 net/bluetooth/hci_core.c:3547
hci_sched_le net/bluetooth/hci_core.c:3750 [inline]
hci_tx_work+0x818/0x1d30 net/bluetooth/hci_core.c:3828
process_one_work+0xaa2/0x16f0 kernel/workqueue.c:2600
worker_thread+0x687/0x1110 kernel/workqueue.c:2751
kthread+0x33a/0x430 kernel/kthread.c:389
ret_from_fork+0x2c/0x70 arch/x86/kernel/process.c:145
ret_from_fork_asm+0x11/0x20 arch/x86/entry/entry_64.S:304
Use rcu_read_lock() to protect hci_conn_hash->list in hci_link_tx_to().
rcu_read_lock() will disable preempt when CONFIG_PREEMPT_RCU is enabled.
When it needs to abort connection, it will submit a command by
hci_cmd_sync_submit() which has some sleeping functions like mutex_lock()
and kmalloc(, GFP_KERNEL). That triggers the bug.
Convert cmd_sync_work_lock and unregister_lock to spin_lock type and
replace GFP_KERNEL with GFP_ATOMIC to fix the bug.
Fixes: 1857c19941c8 ("Bluetooth: hci_sync: add lock to protect HCI_UNREGISTER")
Fixes: 6a98e3836fa2 ("Bluetooth: Add helper for serialized HCI command execution")
Reported-by: syzbot+e7be5be00de0c3c2d782@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e7be5be00de0c3c2d782
Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
---
include/net/bluetooth/hci_core.h | 4 ++--
net/bluetooth/hci_core.c | 4 ++--
net/bluetooth/hci_sync.c | 28 +++++++++++++++-------------
3 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e6359f7346f1..7a046a80a1ac 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -514,8 +514,8 @@ struct hci_dev {
struct work_struct error_reset;
struct work_struct cmd_sync_work;
struct list_head cmd_sync_work_list;
- struct mutex cmd_sync_work_lock;
- struct mutex unregister_lock;
+ spinlock_t cmd_sync_work_lock;
+ spinlock_t unregister_lock;
struct work_struct cmd_sync_cancel_work;
struct work_struct reenable_adv_work;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index a5992f1b3c9b..c94bfde68228 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2716,9 +2716,9 @@ void hci_unregister_dev(struct hci_dev *hdev)
{
BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
- mutex_lock(&hdev->unregister_lock);
+ spin_lock(&hdev->unregister_lock);
hci_dev_set_flag(hdev, HCI_UNREGISTER);
- mutex_unlock(&hdev->unregister_lock);
+ spin_unlock(&hdev->unregister_lock);
write_lock(&hci_dev_list_lock);
list_del(&hdev->list);
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 9b93653c6197..4356ee29cd5f 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -286,13 +286,13 @@ static void hci_cmd_sync_work(struct work_struct *work)
while (1) {
struct hci_cmd_sync_work_entry *entry;
- mutex_lock(&hdev->cmd_sync_work_lock);
+ spin_lock(&hdev->cmd_sync_work_lock);
entry = list_first_entry_or_null(&hdev->cmd_sync_work_list,
struct hci_cmd_sync_work_entry,
list);
if (entry)
list_del(&entry->list);
- mutex_unlock(&hdev->cmd_sync_work_lock);
+ spin_unlock(&hdev->cmd_sync_work_lock);
if (!entry)
break;
@@ -629,8 +629,8 @@ void hci_cmd_sync_init(struct hci_dev *hdev)
{
INIT_WORK(&hdev->cmd_sync_work, hci_cmd_sync_work);
INIT_LIST_HEAD(&hdev->cmd_sync_work_list);
- mutex_init(&hdev->cmd_sync_work_lock);
- mutex_init(&hdev->unregister_lock);
+ spin_lock_init(&hdev->cmd_sync_work_lock);
+ spin_lock_init(&hdev->unregister_lock);
INIT_WORK(&hdev->cmd_sync_cancel_work, hci_cmd_sync_cancel_work);
INIT_WORK(&hdev->reenable_adv_work, reenable_adv);
@@ -646,15 +646,17 @@ void hci_cmd_sync_clear(struct hci_dev *hdev)
cancel_work_sync(&hdev->cmd_sync_work);
cancel_work_sync(&hdev->reenable_adv_work);
- mutex_lock(&hdev->cmd_sync_work_lock);
+ spin_lock(&hdev->cmd_sync_work_lock);
list_for_each_entry_safe(entry, tmp, &hdev->cmd_sync_work_list, list) {
+ list_del(&entry->list);
+ spin_unlock(&hdev->cmd_sync_work_lock);
+
if (entry->destroy)
entry->destroy(hdev, entry->data, -ECANCELED);
-
- list_del(&entry->list);
kfree(entry);
+ spin_lock(&hdev->cmd_sync_work_lock);
}
- mutex_unlock(&hdev->cmd_sync_work_lock);
+ spin_unlock(&hdev->cmd_sync_work_lock);
}
void __hci_cmd_sync_cancel(struct hci_dev *hdev, int err)
@@ -696,13 +698,13 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
struct hci_cmd_sync_work_entry *entry;
int err = 0;
- mutex_lock(&hdev->unregister_lock);
+ spin_lock(&hdev->unregister_lock);
if (hci_dev_test_flag(hdev, HCI_UNREGISTER)) {
err = -ENODEV;
goto unlock;
}
- entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+ entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
if (!entry) {
err = -ENOMEM;
goto unlock;
@@ -711,14 +713,14 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
entry->data = data;
entry->destroy = destroy;
- mutex_lock(&hdev->cmd_sync_work_lock);
+ spin_lock(&hdev->cmd_sync_work_lock);
list_add_tail(&entry->list, &hdev->cmd_sync_work_list);
- mutex_unlock(&hdev->cmd_sync_work_lock);
+ spin_unlock(&hdev->cmd_sync_work_lock);
queue_work(hdev->req_workqueue, &hdev->cmd_sync_work);
unlock:
- mutex_unlock(&hdev->unregister_lock);
+ spin_unlock(&hdev->unregister_lock);
return err;
}
EXPORT_SYMBOL(hci_cmd_sync_submit);
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* RE: Bluetooth: Fix sleeping function called from invalid context in hci_cmd_sync_submit()
2023-09-07 12:22 [PATCH] Bluetooth: Fix sleeping function called from invalid context in hci_cmd_sync_submit() Ziyang Xuan
@ 2023-09-07 19:05 ` bluez.test.bot
2023-09-07 19:07 ` [PATCH] " Luiz Augusto von Dentz
1 sibling, 0 replies; 3+ messages in thread
From: bluez.test.bot @ 2023-09-07 19:05 UTC (permalink / raw)
To: linux-bluetooth, william.xuanziyang
[-- Attachment #1: Type: text/plain, Size: 2088 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=782320
---Test result---
Test Summary:
CheckPatch PASS 1.26 seconds
GitLint FAIL 0.69 seconds
SubjectPrefix PASS 0.13 seconds
BuildKernel PASS 40.08 seconds
CheckAllWarning PASS 42.91 seconds
CheckSparse PASS 49.03 seconds
CheckSmatch PASS 132.91 seconds
BuildKernel32 PASS 38.10 seconds
TestRunnerSetup PASS 572.79 seconds
TestRunner_l2cap-tester PASS 31.53 seconds
TestRunner_iso-tester PASS 73.02 seconds
TestRunner_bnep-tester PASS 12.11 seconds
TestRunner_mgmt-tester PASS 247.10 seconds
TestRunner_rfcomm-tester PASS 20.03 seconds
TestRunner_sco-tester PASS 22.58 seconds
TestRunner_ioctl-tester PASS 21.66 seconds
TestRunner_mesh-tester PASS 16.07 seconds
TestRunner_smp-tester PASS 16.94 seconds
TestRunner_userchan-tester PASS 14.02 seconds
IncrementalBuild PASS 34.24 seconds
Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
Bluetooth: Fix sleeping function called from invalid context in hci_cmd_sync_submit()
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (85>80): "Bluetooth: Fix sleeping function called from invalid context in hci_cmd_sync_submit()"
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Bluetooth: Fix sleeping function called from invalid context in hci_cmd_sync_submit()
2023-09-07 12:22 [PATCH] Bluetooth: Fix sleeping function called from invalid context in hci_cmd_sync_submit() Ziyang Xuan
2023-09-07 19:05 ` bluez.test.bot
@ 2023-09-07 19:07 ` Luiz Augusto von Dentz
1 sibling, 0 replies; 3+ messages in thread
From: Luiz Augusto von Dentz @ 2023-09-07 19:07 UTC (permalink / raw)
To: Ziyang Xuan; +Cc: marcel, johan.hedberg, linux-bluetooth, jiangzp
Hi,
On Thu, Sep 7, 2023 at 5:22 AM Ziyang Xuan
<william.xuanziyang@huawei.com> wrote:
>
> Syzbot reports a sleeping function called from invalid context problem.
>
> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:580
> in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 4440, name: kworker/u5:1
> preempt_count: 0, expected: 0
> RCU nest depth: 1, expected: 0
> ...
> Call Trace:
> <TASK>
> __dump_stack lib/dump_stack.c:88 [inline]
> dump_stack_lvl+0x125/0x1b0 lib/dump_stack.c:106
> __might_resched+0x3c3/0x5e0 kernel/sched/core.c:10187
> __mutex_lock_common kernel/locking/mutex.c:580 [inline]
> __mutex_lock+0xee/0x1340 kernel/locking/mutex.c:747
> hci_cmd_sync_submit+0x3f/0x340 net/bluetooth/hci_sync.c:699
> hci_cmd_sync_queue+0x79/0xa0 net/bluetooth/hci_sync.c:739
> hci_abort_conn+0x15b/0x330 net/bluetooth/hci_conn.c:2928
> hci_disconnect+0xc4/0x220 net/bluetooth/hci_conn.c:258
> hci_link_tx_to net/bluetooth/hci_core.c:3421 [inline]
> __check_timeout net/bluetooth/hci_core.c:3567 [inline]
> __check_timeout+0x331/0x4e0 net/bluetooth/hci_core.c:3547
> hci_sched_le net/bluetooth/hci_core.c:3750 [inline]
> hci_tx_work+0x818/0x1d30 net/bluetooth/hci_core.c:3828
> process_one_work+0xaa2/0x16f0 kernel/workqueue.c:2600
> worker_thread+0x687/0x1110 kernel/workqueue.c:2751
> kthread+0x33a/0x430 kernel/kthread.c:389
> ret_from_fork+0x2c/0x70 arch/x86/kernel/process.c:145
> ret_from_fork_asm+0x11/0x20 arch/x86/entry/entry_64.S:304
>
> Use rcu_read_lock() to protect hci_conn_hash->list in hci_link_tx_to().
> rcu_read_lock() will disable preempt when CONFIG_PREEMPT_RCU is enabled.
> When it needs to abort connection, it will submit a command by
> hci_cmd_sync_submit() which has some sleeping functions like mutex_lock()
> and kmalloc(, GFP_KERNEL). That triggers the bug.
>
> Convert cmd_sync_work_lock and unregister_lock to spin_lock type and
> replace GFP_KERNEL with GFP_ATOMIC to fix the bug.
Seems like this was already fixed by:
https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=d606d5f4024de697167584ad5136a3e56d7debcd
> Fixes: 1857c19941c8 ("Bluetooth: hci_sync: add lock to protect HCI_UNREGISTER")
> Fixes: 6a98e3836fa2 ("Bluetooth: Add helper for serialized HCI command execution")
> Reported-by: syzbot+e7be5be00de0c3c2d782@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e7be5be00de0c3c2d782
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> ---
> include/net/bluetooth/hci_core.h | 4 ++--
> net/bluetooth/hci_core.c | 4 ++--
> net/bluetooth/hci_sync.c | 28 +++++++++++++++-------------
> 3 files changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index e6359f7346f1..7a046a80a1ac 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -514,8 +514,8 @@ struct hci_dev {
> struct work_struct error_reset;
> struct work_struct cmd_sync_work;
> struct list_head cmd_sync_work_list;
> - struct mutex cmd_sync_work_lock;
> - struct mutex unregister_lock;
> + spinlock_t cmd_sync_work_lock;
> + spinlock_t unregister_lock;
> struct work_struct cmd_sync_cancel_work;
> struct work_struct reenable_adv_work;
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index a5992f1b3c9b..c94bfde68228 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2716,9 +2716,9 @@ void hci_unregister_dev(struct hci_dev *hdev)
> {
> BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
>
> - mutex_lock(&hdev->unregister_lock);
> + spin_lock(&hdev->unregister_lock);
> hci_dev_set_flag(hdev, HCI_UNREGISTER);
> - mutex_unlock(&hdev->unregister_lock);
> + spin_unlock(&hdev->unregister_lock);
>
> write_lock(&hci_dev_list_lock);
> list_del(&hdev->list);
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 9b93653c6197..4356ee29cd5f 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -286,13 +286,13 @@ static void hci_cmd_sync_work(struct work_struct *work)
> while (1) {
> struct hci_cmd_sync_work_entry *entry;
>
> - mutex_lock(&hdev->cmd_sync_work_lock);
> + spin_lock(&hdev->cmd_sync_work_lock);
> entry = list_first_entry_or_null(&hdev->cmd_sync_work_list,
> struct hci_cmd_sync_work_entry,
> list);
> if (entry)
> list_del(&entry->list);
> - mutex_unlock(&hdev->cmd_sync_work_lock);
> + spin_unlock(&hdev->cmd_sync_work_lock);
>
> if (!entry)
> break;
> @@ -629,8 +629,8 @@ void hci_cmd_sync_init(struct hci_dev *hdev)
> {
> INIT_WORK(&hdev->cmd_sync_work, hci_cmd_sync_work);
> INIT_LIST_HEAD(&hdev->cmd_sync_work_list);
> - mutex_init(&hdev->cmd_sync_work_lock);
> - mutex_init(&hdev->unregister_lock);
> + spin_lock_init(&hdev->cmd_sync_work_lock);
> + spin_lock_init(&hdev->unregister_lock);
>
> INIT_WORK(&hdev->cmd_sync_cancel_work, hci_cmd_sync_cancel_work);
> INIT_WORK(&hdev->reenable_adv_work, reenable_adv);
> @@ -646,15 +646,17 @@ void hci_cmd_sync_clear(struct hci_dev *hdev)
> cancel_work_sync(&hdev->cmd_sync_work);
> cancel_work_sync(&hdev->reenable_adv_work);
>
> - mutex_lock(&hdev->cmd_sync_work_lock);
> + spin_lock(&hdev->cmd_sync_work_lock);
> list_for_each_entry_safe(entry, tmp, &hdev->cmd_sync_work_list, list) {
> + list_del(&entry->list);
> + spin_unlock(&hdev->cmd_sync_work_lock);
> +
> if (entry->destroy)
> entry->destroy(hdev, entry->data, -ECANCELED);
> -
> - list_del(&entry->list);
> kfree(entry);
> + spin_lock(&hdev->cmd_sync_work_lock);
> }
> - mutex_unlock(&hdev->cmd_sync_work_lock);
> + spin_unlock(&hdev->cmd_sync_work_lock);
> }
>
> void __hci_cmd_sync_cancel(struct hci_dev *hdev, int err)
> @@ -696,13 +698,13 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> struct hci_cmd_sync_work_entry *entry;
> int err = 0;
>
> - mutex_lock(&hdev->unregister_lock);
> + spin_lock(&hdev->unregister_lock);
> if (hci_dev_test_flag(hdev, HCI_UNREGISTER)) {
> err = -ENODEV;
> goto unlock;
> }
>
> - entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> + entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
> if (!entry) {
> err = -ENOMEM;
> goto unlock;
> @@ -711,14 +713,14 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> entry->data = data;
> entry->destroy = destroy;
>
> - mutex_lock(&hdev->cmd_sync_work_lock);
> + spin_lock(&hdev->cmd_sync_work_lock);
> list_add_tail(&entry->list, &hdev->cmd_sync_work_list);
> - mutex_unlock(&hdev->cmd_sync_work_lock);
> + spin_unlock(&hdev->cmd_sync_work_lock);
>
> queue_work(hdev->req_workqueue, &hdev->cmd_sync_work);
>
> unlock:
> - mutex_unlock(&hdev->unregister_lock);
> + spin_unlock(&hdev->unregister_lock);
> return err;
> }
> EXPORT_SYMBOL(hci_cmd_sync_submit);
> --
> 2.25.1
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-09-07 19:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-07 12:22 [PATCH] Bluetooth: Fix sleeping function called from invalid context in hci_cmd_sync_submit() Ziyang Xuan
2023-09-07 19:05 ` bluez.test.bot
2023-09-07 19:07 ` [PATCH] " Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).