Hi, On Thu, Sep 7, 2023 at 5:22 AM Ziyang Xuan <william.xuanziyang@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx > Closes: https://syzkaller.appspot.com/bug?extid=e7be5be00de0c3c2d782 > Signed-off-by: Ziyang Xuan <william.xuanziyang@xxxxxxxxxx> > --- > 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
