* [PATCH v1] Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating
@ 2024-12-04 16:58 Luiz Augusto von Dentz
2024-12-04 16:58 ` [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context Luiz Augusto von Dentz
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Luiz Augusto von Dentz @ 2024-12-04 16:58 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
The usage of rcu_read_(un)lock while inside list_for_each_entry_rcu is
not safe since for the most part entries fetched this way shall be
treated as rcu_dereference:
Note that the value returned by rcu_dereference() is valid
only within the enclosing RCU read-side critical section [1]_.
For example, the following is **not** legal::
rcu_read_lock();
p = rcu_dereference(head.next);
rcu_read_unlock();
x = p->address; /* BUG!!! */
rcu_read_lock();
y = p->data; /* BUG!!! */
rcu_read_unlock();
Fixes: a0bfde167b50 ("Bluetooth: ISO: Add support for connecting multiple BISes")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
net/bluetooth/hci_event.c | 33 +++++++++++----------------------
1 file changed, 11 insertions(+), 22 deletions(-)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index aca121408369..0d59f2cfb9a4 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -6872,38 +6872,27 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
return;
hci_dev_lock(hdev);
- rcu_read_lock();
/* Connect all BISes that are bound to the BIG */
- list_for_each_entry_rcu(conn, &hdev->conn_hash.list, list) {
- if (bacmp(&conn->dst, BDADDR_ANY) ||
- conn->type != ISO_LINK ||
- conn->iso_qos.bcast.big != ev->handle)
+ while ((conn = hci_conn_hash_lookup_big_state(hdev, ev->handle,
+ BT_BOUND))) {
+ if (ev->status) {
+ hci_connect_cfm(conn, ev->status);
+ hci_conn_del(conn);
continue;
+ }
if (hci_conn_set_handle(conn,
__le16_to_cpu(ev->bis_handle[i++])))
continue;
- if (!ev->status) {
- conn->state = BT_CONNECTED;
- set_bit(HCI_CONN_BIG_CREATED, &conn->flags);
- rcu_read_unlock();
- hci_debugfs_create_conn(conn);
- hci_conn_add_sysfs(conn);
- hci_iso_setup_path(conn);
- rcu_read_lock();
- continue;
- }
-
- hci_connect_cfm(conn, ev->status);
- rcu_read_unlock();
- hci_conn_del(conn);
- rcu_read_lock();
+ conn->state = BT_CONNECTED;
+ set_bit(HCI_CONN_BIG_CREATED, &conn->flags);
+ hci_debugfs_create_conn(conn);
+ hci_conn_add_sysfs(conn);
+ hci_iso_setup_path(conn);
}
- rcu_read_unlock();
-
if (!ev->status && !i)
/* If no BISes have been connected for the BIG,
* terminate. This is in case all bound connections
--
2.47.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context 2024-12-04 16:58 [PATCH v1] Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating Luiz Augusto von Dentz @ 2024-12-04 16:58 ` Luiz Augusto von Dentz 2024-12-04 17:31 ` [v2] " bluez.test.bot ` (2 more replies) 2024-12-04 17:02 ` [PATCH v1] Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating Luiz Augusto von Dentz ` (2 subsequent siblings) 3 siblings, 3 replies; 11+ messages in thread From: Luiz Augusto von Dentz @ 2024-12-04 16:58 UTC (permalink / raw) To: linux-bluetooth From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> This reworks hci_cb_list to not use mutex hci_cb_list_lock to avoid bugs like the bellow: BUG: sleeping function called from invalid context at kernel/locking/mutex.c:585 in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 5070, name: kworker/u9:2 preempt_count: 0, expected: 0 RCU nest depth: 1, expected: 0 4 locks held by kworker/u9:2/5070: #0: ffff888015be3948 ((wq_completion)hci0#2){+.+.}-{0:0}, at: process_one_work kernel/workqueue.c:3229 [inline] #0: ffff888015be3948 ((wq_completion)hci0#2){+.+.}-{0:0}, at: process_scheduled_works+0x8e0/0x1770 kernel/workqueue.c:3335 #1: ffffc90003b6fd00 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, at: process_one_work kernel/workqueue.c:3230 [inline] #1: ffffc90003b6fd00 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, at: process_scheduled_works+0x91b/0x1770 kernel/workqueue.c:3335 #2: ffff8880665d0078 (&hdev->lock){+.+.}-{3:3}, at: hci_le_create_big_complete_evt+0xcf/0xae0 net/bluetooth/hci_event.c:6914 #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: rcu_lock_acquire include/linux/rcupdate.h:298 [inline] #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: rcu_read_lock include/linux/rcupdate.h:750 [inline] #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: hci_le_create_big_complete_evt+0xdb/0xae0 net/bluetooth/hci_event.c:6915 CPU: 0 PID: 5070 Comm: kworker/u9:2 Not tainted 6.8.0-syzkaller-08073-g480e035fc4c7 #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024 Workqueue: hci0 hci_rx_work Call Trace: <TASK> __dump_stack lib/dump_stack.c:88 [inline] dump_stack_lvl+0x241/0x360 lib/dump_stack.c:114 __might_resched+0x5d4/0x780 kernel/sched/core.c:10187 __mutex_lock_common kernel/locking/mutex.c:585 [inline] __mutex_lock+0xc1/0xd70 kernel/locking/mutex.c:752 hci_connect_cfm include/net/bluetooth/hci_core.h:2004 [inline] hci_le_create_big_complete_evt+0x3d9/0xae0 net/bluetooth/hci_event.c:6939 hci_event_func net/bluetooth/hci_event.c:7514 [inline] hci_event_packet+0xa53/0x1540 net/bluetooth/hci_event.c:7569 hci_rx_work+0x3e8/0xca0 net/bluetooth/hci_core.c:4171 process_one_work kernel/workqueue.c:3254 [inline] process_scheduled_works+0xa00/0x1770 kernel/workqueue.c:3335 worker_thread+0x86d/0xd70 kernel/workqueue.c:3416 kthread+0x2f0/0x390 kernel/kthread.c:388 ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:147 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:243 </TASK> Reported-by: syzbot+2fb0835e0c9cefc34614@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=2fb0835e0c9cefc34614 Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> --- include/net/bluetooth/hci_core.h | 108 ++++++++++++++++++++----------- net/bluetooth/hci_core.c | 9 +-- net/bluetooth/iso.c | 6 ++ net/bluetooth/l2cap_core.c | 12 ++-- net/bluetooth/rfcomm/core.c | 6 ++ net/bluetooth/sco.c | 12 ++-- 6 files changed, 97 insertions(+), 56 deletions(-) diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index ea798f07c5a2..ca22ead85dbe 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -804,7 +804,6 @@ struct hci_conn_params { extern struct list_head hci_dev_list; extern struct list_head hci_cb_list; extern rwlock_t hci_dev_list_lock; -extern struct mutex hci_cb_list_lock; #define hci_dev_set_flag(hdev, nr) set_bit((nr), (hdev)->dev_flags) #define hci_dev_clear_flag(hdev, nr) clear_bit((nr), (hdev)->dev_flags) @@ -2017,24 +2016,47 @@ struct hci_cb { char *name; + bool (*match) (struct hci_conn *conn); void (*connect_cfm) (struct hci_conn *conn, __u8 status); void (*disconn_cfm) (struct hci_conn *conn, __u8 status); void (*security_cfm) (struct hci_conn *conn, __u8 status, - __u8 encrypt); + __u8 encrypt); void (*key_change_cfm) (struct hci_conn *conn, __u8 status); void (*role_switch_cfm) (struct hci_conn *conn, __u8 status, __u8 role); }; +static inline void hci_cb_lookup(struct hci_conn *conn, struct list_head *list) +{ + struct hci_cb *cb, *cpy; + + rcu_read_lock(); + list_for_each_entry_rcu(cb, &hci_cb_list, list) { + if (cb->match && cb->match(conn)) { + cpy = kmalloc(sizeof(*cpy), GFP_ATOMIC); + if (!cpy) + break; + + *cpy = *cb; + INIT_LIST_HEAD(&cpy->list); + list_add_rcu(&cpy->list, list); + } + } + rcu_read_unlock(); +} + static inline void hci_connect_cfm(struct hci_conn *conn, __u8 status) { - struct hci_cb *cb; + struct list_head list; + struct hci_cb *cb, *tmp; - mutex_lock(&hci_cb_list_lock); - list_for_each_entry(cb, &hci_cb_list, list) { + INIT_LIST_HEAD(&list); + hci_cb_lookup(conn, &list); + + list_for_each_entry_safe(cb, tmp, &list, list) { if (cb->connect_cfm) cb->connect_cfm(conn, status); + kfree(cb); } - mutex_unlock(&hci_cb_list_lock); if (conn->connect_cfm_cb) conn->connect_cfm_cb(conn, status); @@ -2042,22 +2064,43 @@ static inline void hci_connect_cfm(struct hci_conn *conn, __u8 status) static inline void hci_disconn_cfm(struct hci_conn *conn, __u8 reason) { - struct hci_cb *cb; + struct list_head list; + struct hci_cb *cb, *tmp; - mutex_lock(&hci_cb_list_lock); - list_for_each_entry(cb, &hci_cb_list, list) { + INIT_LIST_HEAD(&list); + hci_cb_lookup(conn, &list); + + list_for_each_entry_safe(cb, tmp, &list, list) { if (cb->disconn_cfm) cb->disconn_cfm(conn, reason); + kfree(cb); } - mutex_unlock(&hci_cb_list_lock); if (conn->disconn_cfm_cb) conn->disconn_cfm_cb(conn, reason); } +static inline void hci_security_cfm(struct hci_conn *conn, __u8 status, + __u8 encrypt) +{ + struct list_head list; + struct hci_cb *cb, *tmp; + + INIT_LIST_HEAD(&list); + hci_cb_lookup(conn, &list); + + list_for_each_entry_safe(cb, tmp, &list, list) { + if (cb->security_cfm) + cb->security_cfm(conn, status, encrypt); + kfree(cb); + } + + if (conn->security_cfm_cb) + conn->security_cfm_cb(conn, status); +} + static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status) { - struct hci_cb *cb; __u8 encrypt; if (test_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags)) @@ -2065,20 +2108,11 @@ static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status) encrypt = test_bit(HCI_CONN_ENCRYPT, &conn->flags) ? 0x01 : 0x00; - mutex_lock(&hci_cb_list_lock); - list_for_each_entry(cb, &hci_cb_list, list) { - if (cb->security_cfm) - cb->security_cfm(conn, status, encrypt); - } - mutex_unlock(&hci_cb_list_lock); - - if (conn->security_cfm_cb) - conn->security_cfm_cb(conn, status); + hci_security_cfm(conn, status, encrypt); } static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) { - struct hci_cb *cb; __u8 encrypt; if (conn->state == BT_CONFIG) { @@ -2105,40 +2139,38 @@ static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) conn->sec_level = conn->pending_sec_level; } - mutex_lock(&hci_cb_list_lock); - list_for_each_entry(cb, &hci_cb_list, list) { - if (cb->security_cfm) - cb->security_cfm(conn, status, encrypt); - } - mutex_unlock(&hci_cb_list_lock); - - if (conn->security_cfm_cb) - conn->security_cfm_cb(conn, status); + hci_security_cfm(conn, status, encrypt); } static inline void hci_key_change_cfm(struct hci_conn *conn, __u8 status) { - struct hci_cb *cb; + struct list_head list; + struct hci_cb *cb, *tmp; - mutex_lock(&hci_cb_list_lock); - list_for_each_entry(cb, &hci_cb_list, list) { + INIT_LIST_HEAD(&list); + hci_cb_lookup(conn, &list); + + list_for_each_entry_safe(cb, tmp, &list, list) { if (cb->key_change_cfm) cb->key_change_cfm(conn, status); + kfree(cb); } - mutex_unlock(&hci_cb_list_lock); } static inline void hci_role_switch_cfm(struct hci_conn *conn, __u8 status, __u8 role) { - struct hci_cb *cb; + struct list_head list; + struct hci_cb *cb, *tmp; - mutex_lock(&hci_cb_list_lock); - list_for_each_entry(cb, &hci_cb_list, list) { + INIT_LIST_HEAD(&list); + hci_cb_lookup(conn, &list); + + list_for_each_entry_safe(cb, tmp, &list, list) { if (cb->role_switch_cfm) cb->role_switch_cfm(conn, status, role); + kfree(cb); } - mutex_unlock(&hci_cb_list_lock); } static inline bool hci_bdaddr_is_rpa(bdaddr_t *bdaddr, u8 addr_type) diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index f9e19f9cb5a3..25d180d225c1 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -2993,9 +2993,7 @@ int hci_register_cb(struct hci_cb *cb) { BT_DBG("%p name %s", cb, cb->name); - mutex_lock(&hci_cb_list_lock); - list_add_tail(&cb->list, &hci_cb_list); - mutex_unlock(&hci_cb_list_lock); + list_add_tail_rcu(&cb->list, &hci_cb_list); return 0; } @@ -3005,9 +3003,8 @@ int hci_unregister_cb(struct hci_cb *cb) { BT_DBG("%p name %s", cb, cb->name); - mutex_lock(&hci_cb_list_lock); - list_del(&cb->list); - mutex_unlock(&hci_cb_list_lock); + list_del_rcu(&cb->list); + synchronize_rcu(); return 0; } diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c index 8ed818254dc8..eccdaa16cba0 100644 --- a/net/bluetooth/iso.c +++ b/net/bluetooth/iso.c @@ -2154,6 +2154,11 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) return HCI_LM_ACCEPT; } +static bool iso_match(struct hci_conn *hcon) +{ + return hcon->type == ISO_LINK || hcon->type == LE_LINK; +} + static void iso_connect_cfm(struct hci_conn *hcon, __u8 status) { if (hcon->type != ISO_LINK) { @@ -2335,6 +2340,7 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags) static struct hci_cb iso_cb = { .name = "ISO", + .match = iso_match, .connect_cfm = iso_connect_cfm, .disconn_cfm = iso_disconn_cfm, }; diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c index 6544c1ed7143..27b4c4a2ba1f 100644 --- a/net/bluetooth/l2cap_core.c +++ b/net/bluetooth/l2cap_core.c @@ -7217,6 +7217,11 @@ static struct l2cap_chan *l2cap_global_fixed_chan(struct l2cap_chan *c, return NULL; } +static bool l2cap_match(struct hci_conn *hcon) +{ + return hcon->type == ACL_LINK || hcon->type == LE_LINK; +} + static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status) { struct hci_dev *hdev = hcon->hdev; @@ -7224,9 +7229,6 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status) struct l2cap_chan *pchan; u8 dst_type; - if (hcon->type != ACL_LINK && hcon->type != LE_LINK) - return; - BT_DBG("hcon %p bdaddr %pMR status %d", hcon, &hcon->dst, status); if (status) { @@ -7291,9 +7293,6 @@ int l2cap_disconn_ind(struct hci_conn *hcon) static void l2cap_disconn_cfm(struct hci_conn *hcon, u8 reason) { - if (hcon->type != ACL_LINK && hcon->type != LE_LINK) - return; - BT_DBG("hcon %p reason %d", hcon, reason); l2cap_conn_del(hcon, bt_to_errno(reason)); @@ -7572,6 +7571,7 @@ void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags) static struct hci_cb l2cap_cb = { .name = "L2CAP", + .match = l2cap_match, .connect_cfm = l2cap_connect_cfm, .disconn_cfm = l2cap_disconn_cfm, .security_cfm = l2cap_security_cfm, diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c index ad5177e3a69b..4c56ca5a216c 100644 --- a/net/bluetooth/rfcomm/core.c +++ b/net/bluetooth/rfcomm/core.c @@ -2134,6 +2134,11 @@ static int rfcomm_run(void *unused) return 0; } +static bool rfcomm_match(struct hci_conn *hcon) +{ + return hcon->type == ACL_LINK; +} + static void rfcomm_security_cfm(struct hci_conn *conn, u8 status, u8 encrypt) { struct rfcomm_session *s; @@ -2180,6 +2185,7 @@ static void rfcomm_security_cfm(struct hci_conn *conn, u8 status, u8 encrypt) static struct hci_cb rfcomm_cb = { .name = "RFCOMM", + .match = rfcomm_match, .security_cfm = rfcomm_security_cfm }; diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index 7eb8d3e04ec4..40c4957cfc0b 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -1397,11 +1397,13 @@ int sco_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) return lm; } +static bool sco_match(struct hci_conn *hcon) +{ + return hcon->type == SCO_LINK || hcon->type == ESCO_LINK; +} + static void sco_connect_cfm(struct hci_conn *hcon, __u8 status) { - if (hcon->type != SCO_LINK && hcon->type != ESCO_LINK) - return; - BT_DBG("hcon %p bdaddr %pMR status %u", hcon, &hcon->dst, status); if (!status) { @@ -1416,9 +1418,6 @@ static void sco_connect_cfm(struct hci_conn *hcon, __u8 status) static void sco_disconn_cfm(struct hci_conn *hcon, __u8 reason) { - if (hcon->type != SCO_LINK && hcon->type != ESCO_LINK) - return; - BT_DBG("hcon %p reason %d", hcon, reason); sco_conn_del(hcon, bt_to_errno(reason)); @@ -1444,6 +1443,7 @@ void sco_recv_scodata(struct hci_conn *hcon, struct sk_buff *skb) static struct hci_cb sco_cb = { .name = "SCO", + .match = sco_match, .connect_cfm = sco_connect_cfm, .disconn_cfm = sco_disconn_cfm, }; -- 2.47.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* RE: [v2] Bluetooth: hci_core: Fix sleeping function called from invalid context 2024-12-04 16:58 ` [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context Luiz Augusto von Dentz @ 2024-12-04 17:31 ` bluez.test.bot 2024-12-04 18:44 ` [PATCH v2] " Pauli Virtanen 2024-12-05 15:50 ` [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context patchwork-bot+bluetooth 2 siblings, 0 replies; 11+ messages in thread From: bluez.test.bot @ 2024-12-04 17:31 UTC (permalink / raw) To: linux-bluetooth, luiz.dentz [-- Attachment #1: Type: text/plain, Size: 2238 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=914655 ---Test result--- Test Summary: CheckPatch PENDING 0.41 seconds GitLint PENDING 0.26 seconds SubjectPrefix PASS 0.07 seconds BuildKernel PASS 25.20 seconds CheckAllWarning PASS 27.74 seconds CheckSparse WARNING 31.19 seconds BuildKernel32 PASS 24.93 seconds TestRunnerSetup PASS 446.35 seconds TestRunner_l2cap-tester PASS 20.67 seconds TestRunner_iso-tester FAIL 27.63 seconds TestRunner_bnep-tester PASS 4.83 seconds TestRunner_mgmt-tester PASS 121.29 seconds TestRunner_rfcomm-tester PASS 7.66 seconds TestRunner_sco-tester PASS 9.60 seconds TestRunner_ioctl-tester PASS 8.20 seconds TestRunner_mesh-tester PASS 6.06 seconds TestRunner_smp-tester PASS 7.02 seconds TestRunner_userchan-tester PASS 5.05 seconds IncrementalBuild PENDING 0.47 seconds Details ############################## Test: CheckPatch - PENDING Desc: Run checkpatch.pl script Output: ############################## Test: GitLint - PENDING Desc: Run gitlint Output: ############################## Test: CheckSparse - WARNING Desc: Run sparse tool with linux kernel Output: net/bluetooth/hci_core.c:60:1: warning: symbol 'hci_cb_list_lock' was not declared. Should it be static?net/bluetooth/sco.c: note: in included file:./include/net/bluetooth/hci_core.h:147:35: warning: array of flexible structures ############################## Test: TestRunner_iso-tester - FAIL Desc: Run iso-tester with test-runner Output: WARNING: possible circular locking dependency detected Total: 125, Passed: 121 (96.8%), Failed: 0, Not Run: 4 ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context 2024-12-04 16:58 ` [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context Luiz Augusto von Dentz 2024-12-04 17:31 ` [v2] " bluez.test.bot @ 2024-12-04 18:44 ` Pauli Virtanen 2024-12-04 18:59 ` Luiz Augusto von Dentz 2024-12-05 15:50 ` [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context patchwork-bot+bluetooth 2 siblings, 1 reply; 11+ messages in thread From: Pauli Virtanen @ 2024-12-04 18:44 UTC (permalink / raw) To: Luiz Augusto von Dentz, linux-bluetooth Hi Luiz, ke, 2024-12-04 kello 11:58 -0500, Luiz Augusto von Dentz kirjoitti: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > This reworks hci_cb_list to not use mutex hci_cb_list_lock to avoid bugs > like the bellow: This also in principle allows callbacks to be still running on another CPU when hci_unregister_cb() exits. Not sure if that is unwanted. > > BUG: sleeping function called from invalid context at kernel/locking/mutex.c:585 > in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 5070, name: kworker/u9:2 > preempt_count: 0, expected: 0 > RCU nest depth: 1, expected: 0 > 4 locks held by kworker/u9:2/5070: > #0: ffff888015be3948 ((wq_completion)hci0#2){+.+.}-{0:0}, at: process_one_work kernel/workqueue.c:3229 [inline] > #0: ffff888015be3948 ((wq_completion)hci0#2){+.+.}-{0:0}, at: process_scheduled_works+0x8e0/0x1770 kernel/workqueue.c:3335 > #1: ffffc90003b6fd00 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, at: process_one_work kernel/workqueue.c:3230 [inline] > #1: ffffc90003b6fd00 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, at: process_scheduled_works+0x91b/0x1770 kernel/workqueue.c:3335 > #2: ffff8880665d0078 (&hdev->lock){+.+.}-{3:3}, at: hci_le_create_big_complete_evt+0xcf/0xae0 net/bluetooth/hci_event.c:6914 > #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: rcu_lock_acquire include/linux/rcupdate.h:298 [inline] > #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: rcu_read_lock include/linux/rcupdate.h:750 [inline] > #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: hci_le_create_big_complete_evt+0xdb/0xae0 net/bluetooth/hci_event.c:6915 > CPU: 0 PID: 5070 Comm: kworker/u9:2 Not tainted 6.8.0-syzkaller-08073-g480e035fc4c7 #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024 > Workqueue: hci0 hci_rx_work > Call Trace: > <TASK> > __dump_stack lib/dump_stack.c:88 [inline] > dump_stack_lvl+0x241/0x360 lib/dump_stack.c:114 > __might_resched+0x5d4/0x780 kernel/sched/core.c:10187 > __mutex_lock_common kernel/locking/mutex.c:585 [inline] > __mutex_lock+0xc1/0xd70 kernel/locking/mutex.c:752 > hci_connect_cfm include/net/bluetooth/hci_core.h:2004 [inline] > hci_le_create_big_complete_evt+0x3d9/0xae0 net/bluetooth/hci_event.c:6939 > hci_event_func net/bluetooth/hci_event.c:7514 [inline] > hci_event_packet+0xa53/0x1540 net/bluetooth/hci_event.c:7569 > hci_rx_work+0x3e8/0xca0 net/bluetooth/hci_core.c:4171 > process_one_work kernel/workqueue.c:3254 [inline] > process_scheduled_works+0xa00/0x1770 kernel/workqueue.c:3335 > worker_thread+0x86d/0xd70 kernel/workqueue.c:3416 > kthread+0x2f0/0x390 kernel/kthread.c:388 > ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:147 > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:243 > </TASK> > > Reported-by: syzbot+2fb0835e0c9cefc34614@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=2fb0835e0c9cefc34614 > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > include/net/bluetooth/hci_core.h | 108 ++++++++++++++++++++----------- > net/bluetooth/hci_core.c | 9 +-- > net/bluetooth/iso.c | 6 ++ > net/bluetooth/l2cap_core.c | 12 ++-- > net/bluetooth/rfcomm/core.c | 6 ++ > net/bluetooth/sco.c | 12 ++-- > 6 files changed, 97 insertions(+), 56 deletions(-) > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index ea798f07c5a2..ca22ead85dbe 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -804,7 +804,6 @@ struct hci_conn_params { > extern struct list_head hci_dev_list; > extern struct list_head hci_cb_list; > extern rwlock_t hci_dev_list_lock; > -extern struct mutex hci_cb_list_lock; > > #define hci_dev_set_flag(hdev, nr) set_bit((nr), (hdev)->dev_flags) > #define hci_dev_clear_flag(hdev, nr) clear_bit((nr), (hdev)->dev_flags) > @@ -2017,24 +2016,47 @@ struct hci_cb { > > char *name; > > + bool (*match) (struct hci_conn *conn); > void (*connect_cfm) (struct hci_conn *conn, __u8 status); > void (*disconn_cfm) (struct hci_conn *conn, __u8 status); > void (*security_cfm) (struct hci_conn *conn, __u8 status, > - __u8 encrypt); > + __u8 encrypt); > void (*key_change_cfm) (struct hci_conn *conn, __u8 status); > void (*role_switch_cfm) (struct hci_conn *conn, __u8 status, __u8 role); > }; > > +static inline void hci_cb_lookup(struct hci_conn *conn, struct list_head *list) > +{ > + struct hci_cb *cb, *cpy; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(cb, &hci_cb_list, list) { > + if (cb->match && cb->match(conn)) { > + cpy = kmalloc(sizeof(*cpy), GFP_ATOMIC); > + if (!cpy) > + break; > + > + *cpy = *cb; > + INIT_LIST_HEAD(&cpy->list); > + list_add_rcu(&cpy->list, list); > + } > + } > + rcu_read_unlock(); > +} > + > static inline void hci_connect_cfm(struct hci_conn *conn, __u8 status) > { > - struct hci_cb *cb; > + struct list_head list; > + struct hci_cb *cb, *tmp; > > - mutex_lock(&hci_cb_list_lock); > - list_for_each_entry(cb, &hci_cb_list, list) { > + INIT_LIST_HEAD(&list); > + hci_cb_lookup(conn, &list); > + > + list_for_each_entry_safe(cb, tmp, &list, list) { > if (cb->connect_cfm) > cb->connect_cfm(conn, status); > + kfree(cb); > } > - mutex_unlock(&hci_cb_list_lock); > > if (conn->connect_cfm_cb) > conn->connect_cfm_cb(conn, status); > @@ -2042,22 +2064,43 @@ static inline void hci_connect_cfm(struct hci_conn *conn, __u8 status) > > static inline void hci_disconn_cfm(struct hci_conn *conn, __u8 reason) > { > - struct hci_cb *cb; > + struct list_head list; > + struct hci_cb *cb, *tmp; > > - mutex_lock(&hci_cb_list_lock); > - list_for_each_entry(cb, &hci_cb_list, list) { > + INIT_LIST_HEAD(&list); > + hci_cb_lookup(conn, &list); > + > + list_for_each_entry_safe(cb, tmp, &list, list) { > if (cb->disconn_cfm) > cb->disconn_cfm(conn, reason); > + kfree(cb); > } > - mutex_unlock(&hci_cb_list_lock); > > if (conn->disconn_cfm_cb) > conn->disconn_cfm_cb(conn, reason); > } > > +static inline void hci_security_cfm(struct hci_conn *conn, __u8 status, > + __u8 encrypt) > +{ > + struct list_head list; > + struct hci_cb *cb, *tmp; > + > + INIT_LIST_HEAD(&list); > + hci_cb_lookup(conn, &list); > + > + list_for_each_entry_safe(cb, tmp, &list, list) { > + if (cb->security_cfm) > + cb->security_cfm(conn, status, encrypt); > + kfree(cb); > + } > + > + if (conn->security_cfm_cb) > + conn->security_cfm_cb(conn, status); > +} > + > static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status) > { > - struct hci_cb *cb; > __u8 encrypt; > > if (test_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags)) > @@ -2065,20 +2108,11 @@ static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status) > > encrypt = test_bit(HCI_CONN_ENCRYPT, &conn->flags) ? 0x01 : 0x00; > > - mutex_lock(&hci_cb_list_lock); > - list_for_each_entry(cb, &hci_cb_list, list) { > - if (cb->security_cfm) > - cb->security_cfm(conn, status, encrypt); > - } > - mutex_unlock(&hci_cb_list_lock); > - > - if (conn->security_cfm_cb) > - conn->security_cfm_cb(conn, status); > + hci_security_cfm(conn, status, encrypt); > } > > static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) > { > - struct hci_cb *cb; > __u8 encrypt; > > if (conn->state == BT_CONFIG) { > @@ -2105,40 +2139,38 @@ static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) > conn->sec_level = conn->pending_sec_level; > } > > - mutex_lock(&hci_cb_list_lock); > - list_for_each_entry(cb, &hci_cb_list, list) { > - if (cb->security_cfm) > - cb->security_cfm(conn, status, encrypt); > - } > - mutex_unlock(&hci_cb_list_lock); > - > - if (conn->security_cfm_cb) > - conn->security_cfm_cb(conn, status); > + hci_security_cfm(conn, status, encrypt); > } > > static inline void hci_key_change_cfm(struct hci_conn *conn, __u8 status) > { > - struct hci_cb *cb; > + struct list_head list; > + struct hci_cb *cb, *tmp; > > - mutex_lock(&hci_cb_list_lock); > - list_for_each_entry(cb, &hci_cb_list, list) { > + INIT_LIST_HEAD(&list); > + hci_cb_lookup(conn, &list); > + > + list_for_each_entry_safe(cb, tmp, &list, list) { > if (cb->key_change_cfm) > cb->key_change_cfm(conn, status); > + kfree(cb); > } > - mutex_unlock(&hci_cb_list_lock); > } > > static inline void hci_role_switch_cfm(struct hci_conn *conn, __u8 status, > __u8 role) > { > - struct hci_cb *cb; > + struct list_head list; > + struct hci_cb *cb, *tmp; > > - mutex_lock(&hci_cb_list_lock); > - list_for_each_entry(cb, &hci_cb_list, list) { > + INIT_LIST_HEAD(&list); > + hci_cb_lookup(conn, &list); > + > + list_for_each_entry_safe(cb, tmp, &list, list) { > if (cb->role_switch_cfm) > cb->role_switch_cfm(conn, status, role); > + kfree(cb); > } > - mutex_unlock(&hci_cb_list_lock); > } > > static inline bool hci_bdaddr_is_rpa(bdaddr_t *bdaddr, u8 addr_type) > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index f9e19f9cb5a3..25d180d225c1 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -2993,9 +2993,7 @@ int hci_register_cb(struct hci_cb *cb) > { > BT_DBG("%p name %s", cb, cb->name); > > - mutex_lock(&hci_cb_list_lock); > - list_add_tail(&cb->list, &hci_cb_list); > - mutex_unlock(&hci_cb_list_lock); > + list_add_tail_rcu(&cb->list, &hci_cb_list); > > return 0; > } > @@ -3005,9 +3003,8 @@ int hci_unregister_cb(struct hci_cb *cb) > { > BT_DBG("%p name %s", cb, cb->name); > > - mutex_lock(&hci_cb_list_lock); > - list_del(&cb->list); > - mutex_unlock(&hci_cb_list_lock); > + list_del_rcu(&cb->list); > + synchronize_rcu(); > > return 0; > } > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c > index 8ed818254dc8..eccdaa16cba0 100644 > --- a/net/bluetooth/iso.c > +++ b/net/bluetooth/iso.c > @@ -2154,6 +2154,11 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) > return HCI_LM_ACCEPT; > } > > +static bool iso_match(struct hci_conn *hcon) > +{ > + return hcon->type == ISO_LINK || hcon->type == LE_LINK; > +} > + > static void iso_connect_cfm(struct hci_conn *hcon, __u8 status) > { > if (hcon->type != ISO_LINK) { > @@ -2335,6 +2340,7 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags) > > static struct hci_cb iso_cb = { > .name = "ISO", > + .match = iso_match, > .connect_cfm = iso_connect_cfm, > .disconn_cfm = iso_disconn_cfm, > }; > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > index 6544c1ed7143..27b4c4a2ba1f 100644 > --- a/net/bluetooth/l2cap_core.c > +++ b/net/bluetooth/l2cap_core.c > @@ -7217,6 +7217,11 @@ static struct l2cap_chan *l2cap_global_fixed_chan(struct l2cap_chan *c, > return NULL; > } > > +static bool l2cap_match(struct hci_conn *hcon) > +{ > + return hcon->type == ACL_LINK || hcon->type == LE_LINK; > +} > + > static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status) > { > struct hci_dev *hdev = hcon->hdev; > @@ -7224,9 +7229,6 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status) > struct l2cap_chan *pchan; > u8 dst_type; > > - if (hcon->type != ACL_LINK && hcon->type != LE_LINK) > - return; > - > BT_DBG("hcon %p bdaddr %pMR status %d", hcon, &hcon->dst, status); > > if (status) { > @@ -7291,9 +7293,6 @@ int l2cap_disconn_ind(struct hci_conn *hcon) > > static void l2cap_disconn_cfm(struct hci_conn *hcon, u8 reason) > { > - if (hcon->type != ACL_LINK && hcon->type != LE_LINK) > - return; > - > BT_DBG("hcon %p reason %d", hcon, reason); > > l2cap_conn_del(hcon, bt_to_errno(reason)); > @@ -7572,6 +7571,7 @@ void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags) > > static struct hci_cb l2cap_cb = { > .name = "L2CAP", > + .match = l2cap_match, > .connect_cfm = l2cap_connect_cfm, > .disconn_cfm = l2cap_disconn_cfm, > .security_cfm = l2cap_security_cfm, > diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c > index ad5177e3a69b..4c56ca5a216c 100644 > --- a/net/bluetooth/rfcomm/core.c > +++ b/net/bluetooth/rfcomm/core.c > @@ -2134,6 +2134,11 @@ static int rfcomm_run(void *unused) > return 0; > } > > +static bool rfcomm_match(struct hci_conn *hcon) > +{ > + return hcon->type == ACL_LINK; > +} > + > static void rfcomm_security_cfm(struct hci_conn *conn, u8 status, u8 encrypt) > { > struct rfcomm_session *s; > @@ -2180,6 +2185,7 @@ static void rfcomm_security_cfm(struct hci_conn *conn, u8 status, u8 encrypt) > > static struct hci_cb rfcomm_cb = { > .name = "RFCOMM", > + .match = rfcomm_match, > .security_cfm = rfcomm_security_cfm > }; > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > index 7eb8d3e04ec4..40c4957cfc0b 100644 > --- a/net/bluetooth/sco.c > +++ b/net/bluetooth/sco.c > @@ -1397,11 +1397,13 @@ int sco_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) > return lm; > } > > +static bool sco_match(struct hci_conn *hcon) > +{ > + return hcon->type == SCO_LINK || hcon->type == ESCO_LINK; > +} > + > static void sco_connect_cfm(struct hci_conn *hcon, __u8 status) > { > - if (hcon->type != SCO_LINK && hcon->type != ESCO_LINK) > - return; > - > BT_DBG("hcon %p bdaddr %pMR status %u", hcon, &hcon->dst, status); > > if (!status) { > @@ -1416,9 +1418,6 @@ static void sco_connect_cfm(struct hci_conn *hcon, __u8 status) > > static void sco_disconn_cfm(struct hci_conn *hcon, __u8 reason) > { > - if (hcon->type != SCO_LINK && hcon->type != ESCO_LINK) > - return; > - > BT_DBG("hcon %p reason %d", hcon, reason); > > sco_conn_del(hcon, bt_to_errno(reason)); > @@ -1444,6 +1443,7 @@ void sco_recv_scodata(struct hci_conn *hcon, struct sk_buff *skb) > > static struct hci_cb sco_cb = { > .name = "SCO", > + .match = sco_match, > .connect_cfm = sco_connect_cfm, > .disconn_cfm = sco_disconn_cfm, > }; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context 2024-12-04 18:44 ` [PATCH v2] " Pauli Virtanen @ 2024-12-04 18:59 ` Luiz Augusto von Dentz 2024-12-05 15:12 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 11+ messages in thread From: Luiz Augusto von Dentz @ 2024-12-04 18:59 UTC (permalink / raw) To: Pauli Virtanen; +Cc: linux-bluetooth Hi Pauli, On Wed, Dec 4, 2024 at 1:44 PM Pauli Virtanen <pav@iki.fi> wrote: > > Hi Luiz, > > ke, 2024-12-04 kello 11:58 -0500, Luiz Augusto von Dentz kirjoitti: > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > This reworks hci_cb_list to not use mutex hci_cb_list_lock to avoid bugs > > like the bellow: > > This also in principle allows callbacks to be still running on another > CPU when hci_unregister_cb() exits. Not sure if that is unwanted. Well hci_unregister_cb is only called if you unload the whole module e.g bt_exit, but you can't do that if the interface is not down afaik, so I don't think it is possible to hit the case where the copied entries would be invalid due to module being unloaded. > > > > BUG: sleeping function called from invalid context at kernel/locking/mutex.c:585 > > in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 5070, name: kworker/u9:2 > > preempt_count: 0, expected: 0 > > RCU nest depth: 1, expected: 0 > > 4 locks held by kworker/u9:2/5070: > > #0: ffff888015be3948 ((wq_completion)hci0#2){+.+.}-{0:0}, at: process_one_work kernel/workqueue.c:3229 [inline] > > #0: ffff888015be3948 ((wq_completion)hci0#2){+.+.}-{0:0}, at: process_scheduled_works+0x8e0/0x1770 kernel/workqueue.c:3335 > > #1: ffffc90003b6fd00 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, at: process_one_work kernel/workqueue.c:3230 [inline] > > #1: ffffc90003b6fd00 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, at: process_scheduled_works+0x91b/0x1770 kernel/workqueue.c:3335 > > #2: ffff8880665d0078 (&hdev->lock){+.+.}-{3:3}, at: hci_le_create_big_complete_evt+0xcf/0xae0 net/bluetooth/hci_event.c:6914 > > #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: rcu_lock_acquire include/linux/rcupdate.h:298 [inline] > > #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: rcu_read_lock include/linux/rcupdate.h:750 [inline] > > #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: hci_le_create_big_complete_evt+0xdb/0xae0 net/bluetooth/hci_event.c:6915 > > CPU: 0 PID: 5070 Comm: kworker/u9:2 Not tainted 6.8.0-syzkaller-08073-g480e035fc4c7 #0 > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024 > > Workqueue: hci0 hci_rx_work > > Call Trace: > > <TASK> > > __dump_stack lib/dump_stack.c:88 [inline] > > dump_stack_lvl+0x241/0x360 lib/dump_stack.c:114 > > __might_resched+0x5d4/0x780 kernel/sched/core.c:10187 > > __mutex_lock_common kernel/locking/mutex.c:585 [inline] > > __mutex_lock+0xc1/0xd70 kernel/locking/mutex.c:752 > > hci_connect_cfm include/net/bluetooth/hci_core.h:2004 [inline] > > hci_le_create_big_complete_evt+0x3d9/0xae0 net/bluetooth/hci_event.c:6939 > > hci_event_func net/bluetooth/hci_event.c:7514 [inline] > > hci_event_packet+0xa53/0x1540 net/bluetooth/hci_event.c:7569 > > hci_rx_work+0x3e8/0xca0 net/bluetooth/hci_core.c:4171 > > process_one_work kernel/workqueue.c:3254 [inline] > > process_scheduled_works+0xa00/0x1770 kernel/workqueue.c:3335 > > worker_thread+0x86d/0xd70 kernel/workqueue.c:3416 > > kthread+0x2f0/0x390 kernel/kthread.c:388 > > ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:147 > > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:243 > > </TASK> > > > > Reported-by: syzbot+2fb0835e0c9cefc34614@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?extid=2fb0835e0c9cefc34614 > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > --- > > include/net/bluetooth/hci_core.h | 108 ++++++++++++++++++++----------- > > net/bluetooth/hci_core.c | 9 +-- > > net/bluetooth/iso.c | 6 ++ > > net/bluetooth/l2cap_core.c | 12 ++-- > > net/bluetooth/rfcomm/core.c | 6 ++ > > net/bluetooth/sco.c | 12 ++-- > > 6 files changed, 97 insertions(+), 56 deletions(-) > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > index ea798f07c5a2..ca22ead85dbe 100644 > > --- a/include/net/bluetooth/hci_core.h > > +++ b/include/net/bluetooth/hci_core.h > > @@ -804,7 +804,6 @@ struct hci_conn_params { > > extern struct list_head hci_dev_list; > > extern struct list_head hci_cb_list; > > extern rwlock_t hci_dev_list_lock; > > -extern struct mutex hci_cb_list_lock; > > > > #define hci_dev_set_flag(hdev, nr) set_bit((nr), (hdev)->dev_flags) > > #define hci_dev_clear_flag(hdev, nr) clear_bit((nr), (hdev)->dev_flags) > > @@ -2017,24 +2016,47 @@ struct hci_cb { > > > > char *name; > > > > + bool (*match) (struct hci_conn *conn); > > void (*connect_cfm) (struct hci_conn *conn, __u8 status); > > void (*disconn_cfm) (struct hci_conn *conn, __u8 status); > > void (*security_cfm) (struct hci_conn *conn, __u8 status, > > - __u8 encrypt); > > + __u8 encrypt); > > void (*key_change_cfm) (struct hci_conn *conn, __u8 status); > > void (*role_switch_cfm) (struct hci_conn *conn, __u8 status, __u8 role); > > }; > > > > +static inline void hci_cb_lookup(struct hci_conn *conn, struct list_head *list) > > +{ > > + struct hci_cb *cb, *cpy; > > + > > + rcu_read_lock(); > > + list_for_each_entry_rcu(cb, &hci_cb_list, list) { > > + if (cb->match && cb->match(conn)) { > > + cpy = kmalloc(sizeof(*cpy), GFP_ATOMIC); > > + if (!cpy) > > + break; > > + > > + *cpy = *cb; > > + INIT_LIST_HEAD(&cpy->list); > > + list_add_rcu(&cpy->list, list); > > + } > > + } > > + rcu_read_unlock(); > > +} > > + > > static inline void hci_connect_cfm(struct hci_conn *conn, __u8 status) > > { > > - struct hci_cb *cb; > > + struct list_head list; > > + struct hci_cb *cb, *tmp; > > > > - mutex_lock(&hci_cb_list_lock); > > - list_for_each_entry(cb, &hci_cb_list, list) { > > + INIT_LIST_HEAD(&list); > > + hci_cb_lookup(conn, &list); > > + > > + list_for_each_entry_safe(cb, tmp, &list, list) { > > if (cb->connect_cfm) > > cb->connect_cfm(conn, status); > > + kfree(cb); > > } > > - mutex_unlock(&hci_cb_list_lock); > > > > if (conn->connect_cfm_cb) > > conn->connect_cfm_cb(conn, status); > > @@ -2042,22 +2064,43 @@ static inline void hci_connect_cfm(struct hci_conn *conn, __u8 status) > > > > static inline void hci_disconn_cfm(struct hci_conn *conn, __u8 reason) > > { > > - struct hci_cb *cb; > > + struct list_head list; > > + struct hci_cb *cb, *tmp; > > > > - mutex_lock(&hci_cb_list_lock); > > - list_for_each_entry(cb, &hci_cb_list, list) { > > + INIT_LIST_HEAD(&list); > > + hci_cb_lookup(conn, &list); > > + > > + list_for_each_entry_safe(cb, tmp, &list, list) { > > if (cb->disconn_cfm) > > cb->disconn_cfm(conn, reason); > > + kfree(cb); > > } > > - mutex_unlock(&hci_cb_list_lock); > > > > if (conn->disconn_cfm_cb) > > conn->disconn_cfm_cb(conn, reason); > > } > > > > +static inline void hci_security_cfm(struct hci_conn *conn, __u8 status, > > + __u8 encrypt) > > +{ > > + struct list_head list; > > + struct hci_cb *cb, *tmp; > > + > > + INIT_LIST_HEAD(&list); > > + hci_cb_lookup(conn, &list); > > + > > + list_for_each_entry_safe(cb, tmp, &list, list) { > > + if (cb->security_cfm) > > + cb->security_cfm(conn, status, encrypt); > > + kfree(cb); > > + } > > + > > + if (conn->security_cfm_cb) > > + conn->security_cfm_cb(conn, status); > > +} > > + > > static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status) > > { > > - struct hci_cb *cb; > > __u8 encrypt; > > > > if (test_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags)) > > @@ -2065,20 +2108,11 @@ static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status) > > > > encrypt = test_bit(HCI_CONN_ENCRYPT, &conn->flags) ? 0x01 : 0x00; > > > > - mutex_lock(&hci_cb_list_lock); > > - list_for_each_entry(cb, &hci_cb_list, list) { > > - if (cb->security_cfm) > > - cb->security_cfm(conn, status, encrypt); > > - } > > - mutex_unlock(&hci_cb_list_lock); > > - > > - if (conn->security_cfm_cb) > > - conn->security_cfm_cb(conn, status); > > + hci_security_cfm(conn, status, encrypt); > > } > > > > static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) > > { > > - struct hci_cb *cb; > > __u8 encrypt; > > > > if (conn->state == BT_CONFIG) { > > @@ -2105,40 +2139,38 @@ static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) > > conn->sec_level = conn->pending_sec_level; > > } > > > > - mutex_lock(&hci_cb_list_lock); > > - list_for_each_entry(cb, &hci_cb_list, list) { > > - if (cb->security_cfm) > > - cb->security_cfm(conn, status, encrypt); > > - } > > - mutex_unlock(&hci_cb_list_lock); > > - > > - if (conn->security_cfm_cb) > > - conn->security_cfm_cb(conn, status); > > + hci_security_cfm(conn, status, encrypt); > > } > > > > static inline void hci_key_change_cfm(struct hci_conn *conn, __u8 status) > > { > > - struct hci_cb *cb; > > + struct list_head list; > > + struct hci_cb *cb, *tmp; > > > > - mutex_lock(&hci_cb_list_lock); > > - list_for_each_entry(cb, &hci_cb_list, list) { > > + INIT_LIST_HEAD(&list); > > + hci_cb_lookup(conn, &list); > > + > > + list_for_each_entry_safe(cb, tmp, &list, list) { > > if (cb->key_change_cfm) > > cb->key_change_cfm(conn, status); > > + kfree(cb); > > } > > - mutex_unlock(&hci_cb_list_lock); > > } > > > > static inline void hci_role_switch_cfm(struct hci_conn *conn, __u8 status, > > __u8 role) > > { > > - struct hci_cb *cb; > > + struct list_head list; > > + struct hci_cb *cb, *tmp; > > > > - mutex_lock(&hci_cb_list_lock); > > - list_for_each_entry(cb, &hci_cb_list, list) { > > + INIT_LIST_HEAD(&list); > > + hci_cb_lookup(conn, &list); > > + > > + list_for_each_entry_safe(cb, tmp, &list, list) { > > if (cb->role_switch_cfm) > > cb->role_switch_cfm(conn, status, role); > > + kfree(cb); > > } > > - mutex_unlock(&hci_cb_list_lock); > > } > > > > static inline bool hci_bdaddr_is_rpa(bdaddr_t *bdaddr, u8 addr_type) > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > > index f9e19f9cb5a3..25d180d225c1 100644 > > --- a/net/bluetooth/hci_core.c > > +++ b/net/bluetooth/hci_core.c > > @@ -2993,9 +2993,7 @@ int hci_register_cb(struct hci_cb *cb) > > { > > BT_DBG("%p name %s", cb, cb->name); > > > > - mutex_lock(&hci_cb_list_lock); > > - list_add_tail(&cb->list, &hci_cb_list); > > - mutex_unlock(&hci_cb_list_lock); > > + list_add_tail_rcu(&cb->list, &hci_cb_list); > > > > return 0; > > } > > @@ -3005,9 +3003,8 @@ int hci_unregister_cb(struct hci_cb *cb) > > { > > BT_DBG("%p name %s", cb, cb->name); > > > > - mutex_lock(&hci_cb_list_lock); > > - list_del(&cb->list); > > - mutex_unlock(&hci_cb_list_lock); > > + list_del_rcu(&cb->list); > > + synchronize_rcu(); > > > > return 0; > > } > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c > > index 8ed818254dc8..eccdaa16cba0 100644 > > --- a/net/bluetooth/iso.c > > +++ b/net/bluetooth/iso.c > > @@ -2154,6 +2154,11 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) > > return HCI_LM_ACCEPT; > > } > > > > +static bool iso_match(struct hci_conn *hcon) > > +{ > > + return hcon->type == ISO_LINK || hcon->type == LE_LINK; > > +} > > + > > static void iso_connect_cfm(struct hci_conn *hcon, __u8 status) > > { > > if (hcon->type != ISO_LINK) { > > @@ -2335,6 +2340,7 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags) > > > > static struct hci_cb iso_cb = { > > .name = "ISO", > > + .match = iso_match, > > .connect_cfm = iso_connect_cfm, > > .disconn_cfm = iso_disconn_cfm, > > }; > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > > index 6544c1ed7143..27b4c4a2ba1f 100644 > > --- a/net/bluetooth/l2cap_core.c > > +++ b/net/bluetooth/l2cap_core.c > > @@ -7217,6 +7217,11 @@ static struct l2cap_chan *l2cap_global_fixed_chan(struct l2cap_chan *c, > > return NULL; > > } > > > > +static bool l2cap_match(struct hci_conn *hcon) > > +{ > > + return hcon->type == ACL_LINK || hcon->type == LE_LINK; > > +} > > + > > static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status) > > { > > struct hci_dev *hdev = hcon->hdev; > > @@ -7224,9 +7229,6 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status) > > struct l2cap_chan *pchan; > > u8 dst_type; > > > > - if (hcon->type != ACL_LINK && hcon->type != LE_LINK) > > - return; > > - > > BT_DBG("hcon %p bdaddr %pMR status %d", hcon, &hcon->dst, status); > > > > if (status) { > > @@ -7291,9 +7293,6 @@ int l2cap_disconn_ind(struct hci_conn *hcon) > > > > static void l2cap_disconn_cfm(struct hci_conn *hcon, u8 reason) > > { > > - if (hcon->type != ACL_LINK && hcon->type != LE_LINK) > > - return; > > - > > BT_DBG("hcon %p reason %d", hcon, reason); > > > > l2cap_conn_del(hcon, bt_to_errno(reason)); > > @@ -7572,6 +7571,7 @@ void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags) > > > > static struct hci_cb l2cap_cb = { > > .name = "L2CAP", > > + .match = l2cap_match, > > .connect_cfm = l2cap_connect_cfm, > > .disconn_cfm = l2cap_disconn_cfm, > > .security_cfm = l2cap_security_cfm, > > diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c > > index ad5177e3a69b..4c56ca5a216c 100644 > > --- a/net/bluetooth/rfcomm/core.c > > +++ b/net/bluetooth/rfcomm/core.c > > @@ -2134,6 +2134,11 @@ static int rfcomm_run(void *unused) > > return 0; > > } > > > > +static bool rfcomm_match(struct hci_conn *hcon) > > +{ > > + return hcon->type == ACL_LINK; > > +} > > + > > static void rfcomm_security_cfm(struct hci_conn *conn, u8 status, u8 encrypt) > > { > > struct rfcomm_session *s; > > @@ -2180,6 +2185,7 @@ static void rfcomm_security_cfm(struct hci_conn *conn, u8 status, u8 encrypt) > > > > static struct hci_cb rfcomm_cb = { > > .name = "RFCOMM", > > + .match = rfcomm_match, > > .security_cfm = rfcomm_security_cfm > > }; > > > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > index 7eb8d3e04ec4..40c4957cfc0b 100644 > > --- a/net/bluetooth/sco.c > > +++ b/net/bluetooth/sco.c > > @@ -1397,11 +1397,13 @@ int sco_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) > > return lm; > > } > > > > +static bool sco_match(struct hci_conn *hcon) > > +{ > > + return hcon->type == SCO_LINK || hcon->type == ESCO_LINK; > > +} > > + > > static void sco_connect_cfm(struct hci_conn *hcon, __u8 status) > > { > > - if (hcon->type != SCO_LINK && hcon->type != ESCO_LINK) > > - return; > > - > > BT_DBG("hcon %p bdaddr %pMR status %u", hcon, &hcon->dst, status); > > > > if (!status) { > > @@ -1416,9 +1418,6 @@ static void sco_connect_cfm(struct hci_conn *hcon, __u8 status) > > > > static void sco_disconn_cfm(struct hci_conn *hcon, __u8 reason) > > { > > - if (hcon->type != SCO_LINK && hcon->type != ESCO_LINK) > > - return; > > - > > BT_DBG("hcon %p reason %d", hcon, reason); > > > > sco_conn_del(hcon, bt_to_errno(reason)); > > @@ -1444,6 +1443,7 @@ void sco_recv_scodata(struct hci_conn *hcon, struct sk_buff *skb) > > > > static struct hci_cb sco_cb = { > > .name = "SCO", > > + .match = sco_match, > > .connect_cfm = sco_connect_cfm, > > .disconn_cfm = sco_disconn_cfm, > > }; > -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context 2024-12-04 18:59 ` Luiz Augusto von Dentz @ 2024-12-05 15:12 ` Luiz Augusto von Dentz 2024-12-05 15:35 ` [syzbot] [bluetooth?] BUG: sleeping function called from invalid context in hci_le_create_big_complete_evt syzbot 0 siblings, 1 reply; 11+ messages in thread From: Luiz Augusto von Dentz @ 2024-12-05 15:12 UTC (permalink / raw) To: syzbot+2fb0835e0c9cefc34614; +Cc: linux-bluetooth [-- Attachment #1: Type: text/plain, Size: 17833 bytes --] #syz test On Wed, Dec 4, 2024 at 1:59 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Pauli, > > On Wed, Dec 4, 2024 at 1:44 PM Pauli Virtanen <pav@iki.fi> wrote: > > > > Hi Luiz, > > > > ke, 2024-12-04 kello 11:58 -0500, Luiz Augusto von Dentz kirjoitti: > > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > > > This reworks hci_cb_list to not use mutex hci_cb_list_lock to avoid bugs > > > like the bellow: > > > > This also in principle allows callbacks to be still running on another > > CPU when hci_unregister_cb() exits. Not sure if that is unwanted. > > Well hci_unregister_cb is only called if you unload the whole module > e.g bt_exit, but you can't do that if the interface is not down afaik, > so I don't think it is possible to hit the case where the copied > entries would be invalid due to module being unloaded. > > > > > > > BUG: sleeping function called from invalid context at kernel/locking/mutex.c:585 > > > in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 5070, name: kworker/u9:2 > > > preempt_count: 0, expected: 0 > > > RCU nest depth: 1, expected: 0 > > > 4 locks held by kworker/u9:2/5070: > > > #0: ffff888015be3948 ((wq_completion)hci0#2){+.+.}-{0:0}, at: process_one_work kernel/workqueue.c:3229 [inline] > > > #0: ffff888015be3948 ((wq_completion)hci0#2){+.+.}-{0:0}, at: process_scheduled_works+0x8e0/0x1770 kernel/workqueue.c:3335 > > > #1: ffffc90003b6fd00 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, at: process_one_work kernel/workqueue.c:3230 [inline] > > > #1: ffffc90003b6fd00 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, at: process_scheduled_works+0x91b/0x1770 kernel/workqueue.c:3335 > > > #2: ffff8880665d0078 (&hdev->lock){+.+.}-{3:3}, at: hci_le_create_big_complete_evt+0xcf/0xae0 net/bluetooth/hci_event.c:6914 > > > #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: rcu_lock_acquire include/linux/rcupdate.h:298 [inline] > > > #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: rcu_read_lock include/linux/rcupdate.h:750 [inline] > > > #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: hci_le_create_big_complete_evt+0xdb/0xae0 net/bluetooth/hci_event.c:6915 > > > CPU: 0 PID: 5070 Comm: kworker/u9:2 Not tainted 6.8.0-syzkaller-08073-g480e035fc4c7 #0 > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024 > > > Workqueue: hci0 hci_rx_work > > > Call Trace: > > > <TASK> > > > __dump_stack lib/dump_stack.c:88 [inline] > > > dump_stack_lvl+0x241/0x360 lib/dump_stack.c:114 > > > __might_resched+0x5d4/0x780 kernel/sched/core.c:10187 > > > __mutex_lock_common kernel/locking/mutex.c:585 [inline] > > > __mutex_lock+0xc1/0xd70 kernel/locking/mutex.c:752 > > > hci_connect_cfm include/net/bluetooth/hci_core.h:2004 [inline] > > > hci_le_create_big_complete_evt+0x3d9/0xae0 net/bluetooth/hci_event.c:6939 > > > hci_event_func net/bluetooth/hci_event.c:7514 [inline] > > > hci_event_packet+0xa53/0x1540 net/bluetooth/hci_event.c:7569 > > > hci_rx_work+0x3e8/0xca0 net/bluetooth/hci_core.c:4171 > > > process_one_work kernel/workqueue.c:3254 [inline] > > > process_scheduled_works+0xa00/0x1770 kernel/workqueue.c:3335 > > > worker_thread+0x86d/0xd70 kernel/workqueue.c:3416 > > > kthread+0x2f0/0x390 kernel/kthread.c:388 > > > ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:147 > > > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:243 > > > </TASK> > > > > > > Reported-by: syzbot+2fb0835e0c9cefc34614@syzkaller.appspotmail.com > > > Closes: https://syzkaller.appspot.com/bug?extid=2fb0835e0c9cefc34614 > > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > --- > > > include/net/bluetooth/hci_core.h | 108 ++++++++++++++++++++----------- > > > net/bluetooth/hci_core.c | 9 +-- > > > net/bluetooth/iso.c | 6 ++ > > > net/bluetooth/l2cap_core.c | 12 ++-- > > > net/bluetooth/rfcomm/core.c | 6 ++ > > > net/bluetooth/sco.c | 12 ++-- > > > 6 files changed, 97 insertions(+), 56 deletions(-) > > > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > > index ea798f07c5a2..ca22ead85dbe 100644 > > > --- a/include/net/bluetooth/hci_core.h > > > +++ b/include/net/bluetooth/hci_core.h > > > @@ -804,7 +804,6 @@ struct hci_conn_params { > > > extern struct list_head hci_dev_list; > > > extern struct list_head hci_cb_list; > > > extern rwlock_t hci_dev_list_lock; > > > -extern struct mutex hci_cb_list_lock; > > > > > > #define hci_dev_set_flag(hdev, nr) set_bit((nr), (hdev)->dev_flags) > > > #define hci_dev_clear_flag(hdev, nr) clear_bit((nr), (hdev)->dev_flags) > > > @@ -2017,24 +2016,47 @@ struct hci_cb { > > > > > > char *name; > > > > > > + bool (*match) (struct hci_conn *conn); > > > void (*connect_cfm) (struct hci_conn *conn, __u8 status); > > > void (*disconn_cfm) (struct hci_conn *conn, __u8 status); > > > void (*security_cfm) (struct hci_conn *conn, __u8 status, > > > - __u8 encrypt); > > > + __u8 encrypt); > > > void (*key_change_cfm) (struct hci_conn *conn, __u8 status); > > > void (*role_switch_cfm) (struct hci_conn *conn, __u8 status, __u8 role); > > > }; > > > > > > +static inline void hci_cb_lookup(struct hci_conn *conn, struct list_head *list) > > > +{ > > > + struct hci_cb *cb, *cpy; > > > + > > > + rcu_read_lock(); > > > + list_for_each_entry_rcu(cb, &hci_cb_list, list) { > > > + if (cb->match && cb->match(conn)) { > > > + cpy = kmalloc(sizeof(*cpy), GFP_ATOMIC); > > > + if (!cpy) > > > + break; > > > + > > > + *cpy = *cb; > > > + INIT_LIST_HEAD(&cpy->list); > > > + list_add_rcu(&cpy->list, list); > > > + } > > > + } > > > + rcu_read_unlock(); > > > +} > > > + > > > static inline void hci_connect_cfm(struct hci_conn *conn, __u8 status) > > > { > > > - struct hci_cb *cb; > > > + struct list_head list; > > > + struct hci_cb *cb, *tmp; > > > > > > - mutex_lock(&hci_cb_list_lock); > > > - list_for_each_entry(cb, &hci_cb_list, list) { > > > + INIT_LIST_HEAD(&list); > > > + hci_cb_lookup(conn, &list); > > > + > > > + list_for_each_entry_safe(cb, tmp, &list, list) { > > > if (cb->connect_cfm) > > > cb->connect_cfm(conn, status); > > > + kfree(cb); > > > } > > > - mutex_unlock(&hci_cb_list_lock); > > > > > > if (conn->connect_cfm_cb) > > > conn->connect_cfm_cb(conn, status); > > > @@ -2042,22 +2064,43 @@ static inline void hci_connect_cfm(struct hci_conn *conn, __u8 status) > > > > > > static inline void hci_disconn_cfm(struct hci_conn *conn, __u8 reason) > > > { > > > - struct hci_cb *cb; > > > + struct list_head list; > > > + struct hci_cb *cb, *tmp; > > > > > > - mutex_lock(&hci_cb_list_lock); > > > - list_for_each_entry(cb, &hci_cb_list, list) { > > > + INIT_LIST_HEAD(&list); > > > + hci_cb_lookup(conn, &list); > > > + > > > + list_for_each_entry_safe(cb, tmp, &list, list) { > > > if (cb->disconn_cfm) > > > cb->disconn_cfm(conn, reason); > > > + kfree(cb); > > > } > > > - mutex_unlock(&hci_cb_list_lock); > > > > > > if (conn->disconn_cfm_cb) > > > conn->disconn_cfm_cb(conn, reason); > > > } > > > > > > +static inline void hci_security_cfm(struct hci_conn *conn, __u8 status, > > > + __u8 encrypt) > > > +{ > > > + struct list_head list; > > > + struct hci_cb *cb, *tmp; > > > + > > > + INIT_LIST_HEAD(&list); > > > + hci_cb_lookup(conn, &list); > > > + > > > + list_for_each_entry_safe(cb, tmp, &list, list) { > > > + if (cb->security_cfm) > > > + cb->security_cfm(conn, status, encrypt); > > > + kfree(cb); > > > + } > > > + > > > + if (conn->security_cfm_cb) > > > + conn->security_cfm_cb(conn, status); > > > +} > > > + > > > static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status) > > > { > > > - struct hci_cb *cb; > > > __u8 encrypt; > > > > > > if (test_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags)) > > > @@ -2065,20 +2108,11 @@ static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status) > > > > > > encrypt = test_bit(HCI_CONN_ENCRYPT, &conn->flags) ? 0x01 : 0x00; > > > > > > - mutex_lock(&hci_cb_list_lock); > > > - list_for_each_entry(cb, &hci_cb_list, list) { > > > - if (cb->security_cfm) > > > - cb->security_cfm(conn, status, encrypt); > > > - } > > > - mutex_unlock(&hci_cb_list_lock); > > > - > > > - if (conn->security_cfm_cb) > > > - conn->security_cfm_cb(conn, status); > > > + hci_security_cfm(conn, status, encrypt); > > > } > > > > > > static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) > > > { > > > - struct hci_cb *cb; > > > __u8 encrypt; > > > > > > if (conn->state == BT_CONFIG) { > > > @@ -2105,40 +2139,38 @@ static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) > > > conn->sec_level = conn->pending_sec_level; > > > } > > > > > > - mutex_lock(&hci_cb_list_lock); > > > - list_for_each_entry(cb, &hci_cb_list, list) { > > > - if (cb->security_cfm) > > > - cb->security_cfm(conn, status, encrypt); > > > - } > > > - mutex_unlock(&hci_cb_list_lock); > > > - > > > - if (conn->security_cfm_cb) > > > - conn->security_cfm_cb(conn, status); > > > + hci_security_cfm(conn, status, encrypt); > > > } > > > > > > static inline void hci_key_change_cfm(struct hci_conn *conn, __u8 status) > > > { > > > - struct hci_cb *cb; > > > + struct list_head list; > > > + struct hci_cb *cb, *tmp; > > > > > > - mutex_lock(&hci_cb_list_lock); > > > - list_for_each_entry(cb, &hci_cb_list, list) { > > > + INIT_LIST_HEAD(&list); > > > + hci_cb_lookup(conn, &list); > > > + > > > + list_for_each_entry_safe(cb, tmp, &list, list) { > > > if (cb->key_change_cfm) > > > cb->key_change_cfm(conn, status); > > > + kfree(cb); > > > } > > > - mutex_unlock(&hci_cb_list_lock); > > > } > > > > > > static inline void hci_role_switch_cfm(struct hci_conn *conn, __u8 status, > > > __u8 role) > > > { > > > - struct hci_cb *cb; > > > + struct list_head list; > > > + struct hci_cb *cb, *tmp; > > > > > > - mutex_lock(&hci_cb_list_lock); > > > - list_for_each_entry(cb, &hci_cb_list, list) { > > > + INIT_LIST_HEAD(&list); > > > + hci_cb_lookup(conn, &list); > > > + > > > + list_for_each_entry_safe(cb, tmp, &list, list) { > > > if (cb->role_switch_cfm) > > > cb->role_switch_cfm(conn, status, role); > > > + kfree(cb); > > > } > > > - mutex_unlock(&hci_cb_list_lock); > > > } > > > > > > static inline bool hci_bdaddr_is_rpa(bdaddr_t *bdaddr, u8 addr_type) > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > > > index f9e19f9cb5a3..25d180d225c1 100644 > > > --- a/net/bluetooth/hci_core.c > > > +++ b/net/bluetooth/hci_core.c > > > @@ -2993,9 +2993,7 @@ int hci_register_cb(struct hci_cb *cb) > > > { > > > BT_DBG("%p name %s", cb, cb->name); > > > > > > - mutex_lock(&hci_cb_list_lock); > > > - list_add_tail(&cb->list, &hci_cb_list); > > > - mutex_unlock(&hci_cb_list_lock); > > > + list_add_tail_rcu(&cb->list, &hci_cb_list); > > > > > > return 0; > > > } > > > @@ -3005,9 +3003,8 @@ int hci_unregister_cb(struct hci_cb *cb) > > > { > > > BT_DBG("%p name %s", cb, cb->name); > > > > > > - mutex_lock(&hci_cb_list_lock); > > > - list_del(&cb->list); > > > - mutex_unlock(&hci_cb_list_lock); > > > + list_del_rcu(&cb->list); > > > + synchronize_rcu(); > > > > > > return 0; > > > } > > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c > > > index 8ed818254dc8..eccdaa16cba0 100644 > > > --- a/net/bluetooth/iso.c > > > +++ b/net/bluetooth/iso.c > > > @@ -2154,6 +2154,11 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) > > > return HCI_LM_ACCEPT; > > > } > > > > > > +static bool iso_match(struct hci_conn *hcon) > > > +{ > > > + return hcon->type == ISO_LINK || hcon->type == LE_LINK; > > > +} > > > + > > > static void iso_connect_cfm(struct hci_conn *hcon, __u8 status) > > > { > > > if (hcon->type != ISO_LINK) { > > > @@ -2335,6 +2340,7 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags) > > > > > > static struct hci_cb iso_cb = { > > > .name = "ISO", > > > + .match = iso_match, > > > .connect_cfm = iso_connect_cfm, > > > .disconn_cfm = iso_disconn_cfm, > > > }; > > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > > > index 6544c1ed7143..27b4c4a2ba1f 100644 > > > --- a/net/bluetooth/l2cap_core.c > > > +++ b/net/bluetooth/l2cap_core.c > > > @@ -7217,6 +7217,11 @@ static struct l2cap_chan *l2cap_global_fixed_chan(struct l2cap_chan *c, > > > return NULL; > > > } > > > > > > +static bool l2cap_match(struct hci_conn *hcon) > > > +{ > > > + return hcon->type == ACL_LINK || hcon->type == LE_LINK; > > > +} > > > + > > > static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status) > > > { > > > struct hci_dev *hdev = hcon->hdev; > > > @@ -7224,9 +7229,6 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status) > > > struct l2cap_chan *pchan; > > > u8 dst_type; > > > > > > - if (hcon->type != ACL_LINK && hcon->type != LE_LINK) > > > - return; > > > - > > > BT_DBG("hcon %p bdaddr %pMR status %d", hcon, &hcon->dst, status); > > > > > > if (status) { > > > @@ -7291,9 +7293,6 @@ int l2cap_disconn_ind(struct hci_conn *hcon) > > > > > > static void l2cap_disconn_cfm(struct hci_conn *hcon, u8 reason) > > > { > > > - if (hcon->type != ACL_LINK && hcon->type != LE_LINK) > > > - return; > > > - > > > BT_DBG("hcon %p reason %d", hcon, reason); > > > > > > l2cap_conn_del(hcon, bt_to_errno(reason)); > > > @@ -7572,6 +7571,7 @@ void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags) > > > > > > static struct hci_cb l2cap_cb = { > > > .name = "L2CAP", > > > + .match = l2cap_match, > > > .connect_cfm = l2cap_connect_cfm, > > > .disconn_cfm = l2cap_disconn_cfm, > > > .security_cfm = l2cap_security_cfm, > > > diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c > > > index ad5177e3a69b..4c56ca5a216c 100644 > > > --- a/net/bluetooth/rfcomm/core.c > > > +++ b/net/bluetooth/rfcomm/core.c > > > @@ -2134,6 +2134,11 @@ static int rfcomm_run(void *unused) > > > return 0; > > > } > > > > > > +static bool rfcomm_match(struct hci_conn *hcon) > > > +{ > > > + return hcon->type == ACL_LINK; > > > +} > > > + > > > static void rfcomm_security_cfm(struct hci_conn *conn, u8 status, u8 encrypt) > > > { > > > struct rfcomm_session *s; > > > @@ -2180,6 +2185,7 @@ static void rfcomm_security_cfm(struct hci_conn *conn, u8 status, u8 encrypt) > > > > > > static struct hci_cb rfcomm_cb = { > > > .name = "RFCOMM", > > > + .match = rfcomm_match, > > > .security_cfm = rfcomm_security_cfm > > > }; > > > > > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > > index 7eb8d3e04ec4..40c4957cfc0b 100644 > > > --- a/net/bluetooth/sco.c > > > +++ b/net/bluetooth/sco.c > > > @@ -1397,11 +1397,13 @@ int sco_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) > > > return lm; > > > } > > > > > > +static bool sco_match(struct hci_conn *hcon) > > > +{ > > > + return hcon->type == SCO_LINK || hcon->type == ESCO_LINK; > > > +} > > > + > > > static void sco_connect_cfm(struct hci_conn *hcon, __u8 status) > > > { > > > - if (hcon->type != SCO_LINK && hcon->type != ESCO_LINK) > > > - return; > > > - > > > BT_DBG("hcon %p bdaddr %pMR status %u", hcon, &hcon->dst, status); > > > > > > if (!status) { > > > @@ -1416,9 +1418,6 @@ static void sco_connect_cfm(struct hci_conn *hcon, __u8 status) > > > > > > static void sco_disconn_cfm(struct hci_conn *hcon, __u8 reason) > > > { > > > - if (hcon->type != SCO_LINK && hcon->type != ESCO_LINK) > > > - return; > > > - > > > BT_DBG("hcon %p reason %d", hcon, reason); > > > > > > sco_conn_del(hcon, bt_to_errno(reason)); > > > @@ -1444,6 +1443,7 @@ void sco_recv_scodata(struct hci_conn *hcon, struct sk_buff *skb) > > > > > > static struct hci_cb sco_cb = { > > > .name = "SCO", > > > + .match = sco_match, > > > .connect_cfm = sco_connect_cfm, > > > .disconn_cfm = sco_disconn_cfm, > > > }; > > > > > -- > Luiz Augusto von Dentz -- Luiz Augusto von Dentz [-- Attachment #2: v2-0001-Bluetooth-hci_core-Fix-sleeping-function-called-f.patch --] [-- Type: text/x-patch, Size: 13195 bytes --] From d984fcb83178e73b99519d4e2ccbefbbcacb57b2 Mon Sep 17 00:00:00 2001 From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> Date: Tue, 3 Dec 2024 16:07:32 -0500 Subject: [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context This reworks hci_cb_list to not use mutex hci_cb_list_lock to avoid bugs like the bellow: BUG: sleeping function called from invalid context at kernel/locking/mutex.c:585 in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 5070, name: kworker/u9:2 preempt_count: 0, expected: 0 RCU nest depth: 1, expected: 0 4 locks held by kworker/u9:2/5070: #0: ffff888015be3948 ((wq_completion)hci0#2){+.+.}-{0:0}, at: process_one_work kernel/workqueue.c:3229 [inline] #0: ffff888015be3948 ((wq_completion)hci0#2){+.+.}-{0:0}, at: process_scheduled_works+0x8e0/0x1770 kernel/workqueue.c:3335 #1: ffffc90003b6fd00 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, at: process_one_work kernel/workqueue.c:3230 [inline] #1: ffffc90003b6fd00 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, at: process_scheduled_works+0x91b/0x1770 kernel/workqueue.c:3335 #2: ffff8880665d0078 (&hdev->lock){+.+.}-{3:3}, at: hci_le_create_big_complete_evt+0xcf/0xae0 net/bluetooth/hci_event.c:6914 #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: rcu_lock_acquire include/linux/rcupdate.h:298 [inline] #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: rcu_read_lock include/linux/rcupdate.h:750 [inline] #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: hci_le_create_big_complete_evt+0xdb/0xae0 net/bluetooth/hci_event.c:6915 CPU: 0 PID: 5070 Comm: kworker/u9:2 Not tainted 6.8.0-syzkaller-08073-g480e035fc4c7 #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024 Workqueue: hci0 hci_rx_work Call Trace: <TASK> __dump_stack lib/dump_stack.c:88 [inline] dump_stack_lvl+0x241/0x360 lib/dump_stack.c:114 __might_resched+0x5d4/0x780 kernel/sched/core.c:10187 __mutex_lock_common kernel/locking/mutex.c:585 [inline] __mutex_lock+0xc1/0xd70 kernel/locking/mutex.c:752 hci_connect_cfm include/net/bluetooth/hci_core.h:2004 [inline] hci_le_create_big_complete_evt+0x3d9/0xae0 net/bluetooth/hci_event.c:6939 hci_event_func net/bluetooth/hci_event.c:7514 [inline] hci_event_packet+0xa53/0x1540 net/bluetooth/hci_event.c:7569 hci_rx_work+0x3e8/0xca0 net/bluetooth/hci_core.c:4171 process_one_work kernel/workqueue.c:3254 [inline] process_scheduled_works+0xa00/0x1770 kernel/workqueue.c:3335 worker_thread+0x86d/0xd70 kernel/workqueue.c:3416 kthread+0x2f0/0x390 kernel/kthread.c:388 ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:147 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:243 </TASK> Reported-by: syzbot+2fb0835e0c9cefc34614@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=2fb0835e0c9cefc34614 Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> --- include/net/bluetooth/hci_core.h | 108 ++++++++++++++++++++----------- net/bluetooth/hci_core.c | 9 +-- net/bluetooth/iso.c | 6 ++ net/bluetooth/l2cap_core.c | 12 ++-- net/bluetooth/rfcomm/core.c | 6 ++ net/bluetooth/sco.c | 12 ++-- 6 files changed, 97 insertions(+), 56 deletions(-) diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index ea798f07c5a2..ca22ead85dbe 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -804,7 +804,6 @@ struct hci_conn_params { extern struct list_head hci_dev_list; extern struct list_head hci_cb_list; extern rwlock_t hci_dev_list_lock; -extern struct mutex hci_cb_list_lock; #define hci_dev_set_flag(hdev, nr) set_bit((nr), (hdev)->dev_flags) #define hci_dev_clear_flag(hdev, nr) clear_bit((nr), (hdev)->dev_flags) @@ -2017,24 +2016,47 @@ struct hci_cb { char *name; + bool (*match) (struct hci_conn *conn); void (*connect_cfm) (struct hci_conn *conn, __u8 status); void (*disconn_cfm) (struct hci_conn *conn, __u8 status); void (*security_cfm) (struct hci_conn *conn, __u8 status, - __u8 encrypt); + __u8 encrypt); void (*key_change_cfm) (struct hci_conn *conn, __u8 status); void (*role_switch_cfm) (struct hci_conn *conn, __u8 status, __u8 role); }; +static inline void hci_cb_lookup(struct hci_conn *conn, struct list_head *list) +{ + struct hci_cb *cb, *cpy; + + rcu_read_lock(); + list_for_each_entry_rcu(cb, &hci_cb_list, list) { + if (cb->match && cb->match(conn)) { + cpy = kmalloc(sizeof(*cpy), GFP_ATOMIC); + if (!cpy) + break; + + *cpy = *cb; + INIT_LIST_HEAD(&cpy->list); + list_add_rcu(&cpy->list, list); + } + } + rcu_read_unlock(); +} + static inline void hci_connect_cfm(struct hci_conn *conn, __u8 status) { - struct hci_cb *cb; + struct list_head list; + struct hci_cb *cb, *tmp; - mutex_lock(&hci_cb_list_lock); - list_for_each_entry(cb, &hci_cb_list, list) { + INIT_LIST_HEAD(&list); + hci_cb_lookup(conn, &list); + + list_for_each_entry_safe(cb, tmp, &list, list) { if (cb->connect_cfm) cb->connect_cfm(conn, status); + kfree(cb); } - mutex_unlock(&hci_cb_list_lock); if (conn->connect_cfm_cb) conn->connect_cfm_cb(conn, status); @@ -2042,22 +2064,43 @@ static inline void hci_connect_cfm(struct hci_conn *conn, __u8 status) static inline void hci_disconn_cfm(struct hci_conn *conn, __u8 reason) { - struct hci_cb *cb; + struct list_head list; + struct hci_cb *cb, *tmp; - mutex_lock(&hci_cb_list_lock); - list_for_each_entry(cb, &hci_cb_list, list) { + INIT_LIST_HEAD(&list); + hci_cb_lookup(conn, &list); + + list_for_each_entry_safe(cb, tmp, &list, list) { if (cb->disconn_cfm) cb->disconn_cfm(conn, reason); + kfree(cb); } - mutex_unlock(&hci_cb_list_lock); if (conn->disconn_cfm_cb) conn->disconn_cfm_cb(conn, reason); } +static inline void hci_security_cfm(struct hci_conn *conn, __u8 status, + __u8 encrypt) +{ + struct list_head list; + struct hci_cb *cb, *tmp; + + INIT_LIST_HEAD(&list); + hci_cb_lookup(conn, &list); + + list_for_each_entry_safe(cb, tmp, &list, list) { + if (cb->security_cfm) + cb->security_cfm(conn, status, encrypt); + kfree(cb); + } + + if (conn->security_cfm_cb) + conn->security_cfm_cb(conn, status); +} + static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status) { - struct hci_cb *cb; __u8 encrypt; if (test_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags)) @@ -2065,20 +2108,11 @@ static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status) encrypt = test_bit(HCI_CONN_ENCRYPT, &conn->flags) ? 0x01 : 0x00; - mutex_lock(&hci_cb_list_lock); - list_for_each_entry(cb, &hci_cb_list, list) { - if (cb->security_cfm) - cb->security_cfm(conn, status, encrypt); - } - mutex_unlock(&hci_cb_list_lock); - - if (conn->security_cfm_cb) - conn->security_cfm_cb(conn, status); + hci_security_cfm(conn, status, encrypt); } static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) { - struct hci_cb *cb; __u8 encrypt; if (conn->state == BT_CONFIG) { @@ -2105,40 +2139,38 @@ static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) conn->sec_level = conn->pending_sec_level; } - mutex_lock(&hci_cb_list_lock); - list_for_each_entry(cb, &hci_cb_list, list) { - if (cb->security_cfm) - cb->security_cfm(conn, status, encrypt); - } - mutex_unlock(&hci_cb_list_lock); - - if (conn->security_cfm_cb) - conn->security_cfm_cb(conn, status); + hci_security_cfm(conn, status, encrypt); } static inline void hci_key_change_cfm(struct hci_conn *conn, __u8 status) { - struct hci_cb *cb; + struct list_head list; + struct hci_cb *cb, *tmp; - mutex_lock(&hci_cb_list_lock); - list_for_each_entry(cb, &hci_cb_list, list) { + INIT_LIST_HEAD(&list); + hci_cb_lookup(conn, &list); + + list_for_each_entry_safe(cb, tmp, &list, list) { if (cb->key_change_cfm) cb->key_change_cfm(conn, status); + kfree(cb); } - mutex_unlock(&hci_cb_list_lock); } static inline void hci_role_switch_cfm(struct hci_conn *conn, __u8 status, __u8 role) { - struct hci_cb *cb; + struct list_head list; + struct hci_cb *cb, *tmp; - mutex_lock(&hci_cb_list_lock); - list_for_each_entry(cb, &hci_cb_list, list) { + INIT_LIST_HEAD(&list); + hci_cb_lookup(conn, &list); + + list_for_each_entry_safe(cb, tmp, &list, list) { if (cb->role_switch_cfm) cb->role_switch_cfm(conn, status, role); + kfree(cb); } - mutex_unlock(&hci_cb_list_lock); } static inline bool hci_bdaddr_is_rpa(bdaddr_t *bdaddr, u8 addr_type) diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index f9e19f9cb5a3..25d180d225c1 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -2993,9 +2993,7 @@ int hci_register_cb(struct hci_cb *cb) { BT_DBG("%p name %s", cb, cb->name); - mutex_lock(&hci_cb_list_lock); - list_add_tail(&cb->list, &hci_cb_list); - mutex_unlock(&hci_cb_list_lock); + list_add_tail_rcu(&cb->list, &hci_cb_list); return 0; } @@ -3005,9 +3003,8 @@ int hci_unregister_cb(struct hci_cb *cb) { BT_DBG("%p name %s", cb, cb->name); - mutex_lock(&hci_cb_list_lock); - list_del(&cb->list); - mutex_unlock(&hci_cb_list_lock); + list_del_rcu(&cb->list); + synchronize_rcu(); return 0; } diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c index 8ed818254dc8..eccdaa16cba0 100644 --- a/net/bluetooth/iso.c +++ b/net/bluetooth/iso.c @@ -2154,6 +2154,11 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) return HCI_LM_ACCEPT; } +static bool iso_match(struct hci_conn *hcon) +{ + return hcon->type == ISO_LINK || hcon->type == LE_LINK; +} + static void iso_connect_cfm(struct hci_conn *hcon, __u8 status) { if (hcon->type != ISO_LINK) { @@ -2335,6 +2340,7 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags) static struct hci_cb iso_cb = { .name = "ISO", + .match = iso_match, .connect_cfm = iso_connect_cfm, .disconn_cfm = iso_disconn_cfm, }; diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c index 6544c1ed7143..27b4c4a2ba1f 100644 --- a/net/bluetooth/l2cap_core.c +++ b/net/bluetooth/l2cap_core.c @@ -7217,6 +7217,11 @@ static struct l2cap_chan *l2cap_global_fixed_chan(struct l2cap_chan *c, return NULL; } +static bool l2cap_match(struct hci_conn *hcon) +{ + return hcon->type == ACL_LINK || hcon->type == LE_LINK; +} + static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status) { struct hci_dev *hdev = hcon->hdev; @@ -7224,9 +7229,6 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status) struct l2cap_chan *pchan; u8 dst_type; - if (hcon->type != ACL_LINK && hcon->type != LE_LINK) - return; - BT_DBG("hcon %p bdaddr %pMR status %d", hcon, &hcon->dst, status); if (status) { @@ -7291,9 +7293,6 @@ int l2cap_disconn_ind(struct hci_conn *hcon) static void l2cap_disconn_cfm(struct hci_conn *hcon, u8 reason) { - if (hcon->type != ACL_LINK && hcon->type != LE_LINK) - return; - BT_DBG("hcon %p reason %d", hcon, reason); l2cap_conn_del(hcon, bt_to_errno(reason)); @@ -7572,6 +7571,7 @@ void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags) static struct hci_cb l2cap_cb = { .name = "L2CAP", + .match = l2cap_match, .connect_cfm = l2cap_connect_cfm, .disconn_cfm = l2cap_disconn_cfm, .security_cfm = l2cap_security_cfm, diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c index ad5177e3a69b..4c56ca5a216c 100644 --- a/net/bluetooth/rfcomm/core.c +++ b/net/bluetooth/rfcomm/core.c @@ -2134,6 +2134,11 @@ static int rfcomm_run(void *unused) return 0; } +static bool rfcomm_match(struct hci_conn *hcon) +{ + return hcon->type == ACL_LINK; +} + static void rfcomm_security_cfm(struct hci_conn *conn, u8 status, u8 encrypt) { struct rfcomm_session *s; @@ -2180,6 +2185,7 @@ static void rfcomm_security_cfm(struct hci_conn *conn, u8 status, u8 encrypt) static struct hci_cb rfcomm_cb = { .name = "RFCOMM", + .match = rfcomm_match, .security_cfm = rfcomm_security_cfm }; diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index 7eb8d3e04ec4..40c4957cfc0b 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -1397,11 +1397,13 @@ int sco_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) return lm; } +static bool sco_match(struct hci_conn *hcon) +{ + return hcon->type == SCO_LINK || hcon->type == ESCO_LINK; +} + static void sco_connect_cfm(struct hci_conn *hcon, __u8 status) { - if (hcon->type != SCO_LINK && hcon->type != ESCO_LINK) - return; - BT_DBG("hcon %p bdaddr %pMR status %u", hcon, &hcon->dst, status); if (!status) { @@ -1416,9 +1418,6 @@ static void sco_connect_cfm(struct hci_conn *hcon, __u8 status) static void sco_disconn_cfm(struct hci_conn *hcon, __u8 reason) { - if (hcon->type != SCO_LINK && hcon->type != ESCO_LINK) - return; - BT_DBG("hcon %p reason %d", hcon, reason); sco_conn_del(hcon, bt_to_errno(reason)); @@ -1444,6 +1443,7 @@ void sco_recv_scodata(struct hci_conn *hcon, struct sk_buff *skb) static struct hci_cb sco_cb = { .name = "SCO", + .match = sco_match, .connect_cfm = sco_connect_cfm, .disconn_cfm = sco_disconn_cfm, }; -- 2.47.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [syzbot] [bluetooth?] BUG: sleeping function called from invalid context in hci_le_create_big_complete_evt 2024-12-05 15:12 ` Luiz Augusto von Dentz @ 2024-12-05 15:35 ` syzbot 0 siblings, 0 replies; 11+ messages in thread From: syzbot @ 2024-12-05 15:35 UTC (permalink / raw) To: linux-bluetooth, linux-kernel, luiz.dentz, syzkaller-bugs Hello, syzbot has tested the proposed patch and the reproducer did not trigger any issue: Reported-by: syzbot+2fb0835e0c9cefc34614@syzkaller.appspotmail.com Tested-by: syzbot+2fb0835e0c9cefc34614@syzkaller.appspotmail.com Tested on: commit: feffde68 Merge tag 'for-6.13-rc1-tag' of git://git.ker.. git tree: upstream console output: https://syzkaller.appspot.com/x/log.txt?x=144e1330580000 kernel config: https://syzkaller.appspot.com/x/.config?x=773e6b07384b3087 dashboard link: https://syzkaller.appspot.com/bug?extid=2fb0835e0c9cefc34614 compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40 patch: https://syzkaller.appspot.com/x/patch.diff?x=178490f8580000 Note: testing is done by a robot and is best-effort only. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context 2024-12-04 16:58 ` [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context Luiz Augusto von Dentz 2024-12-04 17:31 ` [v2] " bluez.test.bot 2024-12-04 18:44 ` [PATCH v2] " Pauli Virtanen @ 2024-12-05 15:50 ` patchwork-bot+bluetooth 2 siblings, 0 replies; 11+ messages in thread From: patchwork-bot+bluetooth @ 2024-12-05 15:50 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth Hello: This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: On Wed, 4 Dec 2024 11:58:49 -0500 you wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > This reworks hci_cb_list to not use mutex hci_cb_list_lock to avoid bugs > like the bellow: > > BUG: sleeping function called from invalid context at kernel/locking/mutex.c:585 > in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 5070, name: kworker/u9:2 > preempt_count: 0, expected: 0 > RCU nest depth: 1, expected: 0 > 4 locks held by kworker/u9:2/5070: > #0: ffff888015be3948 ((wq_completion)hci0#2){+.+.}-{0:0}, at: process_one_work kernel/workqueue.c:3229 [inline] > #0: ffff888015be3948 ((wq_completion)hci0#2){+.+.}-{0:0}, at: process_scheduled_works+0x8e0/0x1770 kernel/workqueue.c:3335 > #1: ffffc90003b6fd00 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, at: process_one_work kernel/workqueue.c:3230 [inline] > #1: ffffc90003b6fd00 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, at: process_scheduled_works+0x91b/0x1770 kernel/workqueue.c:3335 > #2: ffff8880665d0078 (&hdev->lock){+.+.}-{3:3}, at: hci_le_create_big_complete_evt+0xcf/0xae0 net/bluetooth/hci_event.c:6914 > #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: rcu_lock_acquire include/linux/rcupdate.h:298 [inline] > #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: rcu_read_lock include/linux/rcupdate.h:750 [inline] > #3: ffffffff8e132020 (rcu_read_lock){....}-{1:2}, at: hci_le_create_big_complete_evt+0xdb/0xae0 net/bluetooth/hci_event.c:6915 > CPU: 0 PID: 5070 Comm: kworker/u9:2 Not tainted 6.8.0-syzkaller-08073-g480e035fc4c7 #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024 > Workqueue: hci0 hci_rx_work > Call Trace: > <TASK> > __dump_stack lib/dump_stack.c:88 [inline] > dump_stack_lvl+0x241/0x360 lib/dump_stack.c:114 > __might_resched+0x5d4/0x780 kernel/sched/core.c:10187 > __mutex_lock_common kernel/locking/mutex.c:585 [inline] > __mutex_lock+0xc1/0xd70 kernel/locking/mutex.c:752 > hci_connect_cfm include/net/bluetooth/hci_core.h:2004 [inline] > hci_le_create_big_complete_evt+0x3d9/0xae0 net/bluetooth/hci_event.c:6939 > hci_event_func net/bluetooth/hci_event.c:7514 [inline] > hci_event_packet+0xa53/0x1540 net/bluetooth/hci_event.c:7569 > hci_rx_work+0x3e8/0xca0 net/bluetooth/hci_core.c:4171 > process_one_work kernel/workqueue.c:3254 [inline] > process_scheduled_works+0xa00/0x1770 kernel/workqueue.c:3335 > worker_thread+0x86d/0xd70 kernel/workqueue.c:3416 > kthread+0x2f0/0x390 kernel/kthread.c:388 > ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:147 > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:243 > </TASK> > > [...] Here is the summary with links: - [v2] Bluetooth: hci_core: Fix sleeping function called from invalid context https://git.kernel.org/bluetooth/bluetooth-next/c/20c9afdfb287 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating 2024-12-04 16:58 [PATCH v1] Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating Luiz Augusto von Dentz 2024-12-04 16:58 ` [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context Luiz Augusto von Dentz @ 2024-12-04 17:02 ` Luiz Augusto von Dentz 2024-12-04 17:31 ` [v1] " bluez.test.bot 2024-12-05 15:50 ` [PATCH v1] " patchwork-bot+bluetooth 3 siblings, 0 replies; 11+ messages in thread From: Luiz Augusto von Dentz @ 2024-12-04 17:02 UTC (permalink / raw) To: linux-bluetooth; +Cc: iulia-tanasescu On Wed, Dec 4, 2024 at 11:58 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > The usage of rcu_read_(un)lock while inside list_for_each_entry_rcu is > not safe since for the most part entries fetched this way shall be > treated as rcu_dereference: > > Note that the value returned by rcu_dereference() is valid > only within the enclosing RCU read-side critical section [1]_. > For example, the following is **not** legal:: > > rcu_read_lock(); > p = rcu_dereference(head.next); > rcu_read_unlock(); > x = p->address; /* BUG!!! */ > rcu_read_lock(); > y = p->data; /* BUG!!! */ > rcu_read_unlock(); > > Fixes: a0bfde167b50 ("Bluetooth: ISO: Add support for connecting multiple BISes") > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > net/bluetooth/hci_event.c | 33 +++++++++++---------------------- > 1 file changed, 11 insertions(+), 22 deletions(-) > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index aca121408369..0d59f2cfb9a4 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -6872,38 +6872,27 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data, > return; > > hci_dev_lock(hdev); > - rcu_read_lock(); > > /* Connect all BISes that are bound to the BIG */ > - list_for_each_entry_rcu(conn, &hdev->conn_hash.list, list) { > - if (bacmp(&conn->dst, BDADDR_ANY) || > - conn->type != ISO_LINK || > - conn->iso_qos.bcast.big != ev->handle) > + while ((conn = hci_conn_hash_lookup_big_state(hdev, ev->handle, > + BT_BOUND))) { > + if (ev->status) { > + hci_connect_cfm(conn, ev->status); > + hci_conn_del(conn); > continue; > + } > > if (hci_conn_set_handle(conn, > __le16_to_cpu(ev->bis_handle[i++]))) > continue; > > - if (!ev->status) { > - conn->state = BT_CONNECTED; > - set_bit(HCI_CONN_BIG_CREATED, &conn->flags); > - rcu_read_unlock(); > - hci_debugfs_create_conn(conn); > - hci_conn_add_sysfs(conn); > - hci_iso_setup_path(conn); > - rcu_read_lock(); > - continue; > - } > - > - hci_connect_cfm(conn, ev->status); > - rcu_read_unlock(); > - hci_conn_del(conn); > - rcu_read_lock(); > + conn->state = BT_CONNECTED; > + set_bit(HCI_CONN_BIG_CREATED, &conn->flags); > + hci_debugfs_create_conn(conn); > + hci_conn_add_sysfs(conn); > + hci_iso_setup_path(conn); > } > > - rcu_read_unlock(); > - > if (!ev->status && !i) > /* If no BISes have been connected for the BIG, > * terminate. This is in case all bound connections > -- > 2.47.1 > -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [v1] Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating 2024-12-04 16:58 [PATCH v1] Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating Luiz Augusto von Dentz 2024-12-04 16:58 ` [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context Luiz Augusto von Dentz 2024-12-04 17:02 ` [PATCH v1] Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating Luiz Augusto von Dentz @ 2024-12-04 17:31 ` bluez.test.bot 2024-12-05 15:50 ` [PATCH v1] " patchwork-bot+bluetooth 3 siblings, 0 replies; 11+ messages in thread From: bluez.test.bot @ 2024-12-04 17:31 UTC (permalink / raw) To: linux-bluetooth, luiz.dentz [-- Attachment #1: Type: text/plain, Size: 2371 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=914654 ---Test result--- Test Summary: CheckPatch PENDING 0.40 seconds GitLint PENDING 0.40 seconds SubjectPrefix PASS 0.06 seconds BuildKernel PASS 25.26 seconds CheckAllWarning PASS 27.87 seconds CheckSparse WARNING 31.48 seconds BuildKernel32 PASS 25.05 seconds TestRunnerSetup PASS 443.48 seconds TestRunner_l2cap-tester PASS 20.63 seconds TestRunner_iso-tester FAIL 30.80 seconds TestRunner_bnep-tester PASS 4.84 seconds TestRunner_mgmt-tester FAIL 122.16 seconds TestRunner_rfcomm-tester PASS 7.61 seconds TestRunner_sco-tester PASS 9.38 seconds TestRunner_ioctl-tester PASS 8.17 seconds TestRunner_mesh-tester PASS 6.04 seconds TestRunner_smp-tester PASS 7.09 seconds TestRunner_userchan-tester PASS 5.04 seconds IncrementalBuild PENDING 0.93 seconds Details ############################## Test: CheckPatch - PENDING Desc: Run checkpatch.pl script Output: ############################## Test: GitLint - PENDING Desc: Run gitlint Output: ############################## Test: CheckSparse - WARNING Desc: Run sparse tool with linux kernel Output: net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h): ############################## Test: TestRunner_iso-tester - FAIL Desc: Run iso-tester with test-runner Output: WARNING: possible circular locking dependency detected Total: 125, Passed: 121 (96.8%), Failed: 0, Not Run: 4 ############################## Test: TestRunner_mgmt-tester - FAIL Desc: Run mgmt-tester with test-runner Output: Total: 492, Passed: 487 (99.0%), Failed: 1, Not Run: 4 Failed Test Cases LL Privacy - Start Discovery 2 (Disable RL) Failed 0.194 seconds ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating 2024-12-04 16:58 [PATCH v1] Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating Luiz Augusto von Dentz ` (2 preceding siblings ...) 2024-12-04 17:31 ` [v1] " bluez.test.bot @ 2024-12-05 15:50 ` patchwork-bot+bluetooth 3 siblings, 0 replies; 11+ messages in thread From: patchwork-bot+bluetooth @ 2024-12-05 15:50 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth Hello: This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: On Wed, 4 Dec 2024 11:58:48 -0500 you wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > The usage of rcu_read_(un)lock while inside list_for_each_entry_rcu is > not safe since for the most part entries fetched this way shall be > treated as rcu_dereference: > > Note that the value returned by rcu_dereference() is valid > only within the enclosing RCU read-side critical section [1]_. > For example, the following is **not** legal:: > > [...] Here is the summary with links: - [v1] Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating https://git.kernel.org/bluetooth/bluetooth-next/c/042ca30229a0 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-12-05 15:50 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-12-04 16:58 [PATCH v1] Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating Luiz Augusto von Dentz 2024-12-04 16:58 ` [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context Luiz Augusto von Dentz 2024-12-04 17:31 ` [v2] " bluez.test.bot 2024-12-04 18:44 ` [PATCH v2] " Pauli Virtanen 2024-12-04 18:59 ` Luiz Augusto von Dentz 2024-12-05 15:12 ` Luiz Augusto von Dentz 2024-12-05 15:35 ` [syzbot] [bluetooth?] BUG: sleeping function called from invalid context in hci_le_create_big_complete_evt syzbot 2024-12-05 15:50 ` [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context patchwork-bot+bluetooth 2024-12-04 17:02 ` [PATCH v1] Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating Luiz Augusto von Dentz 2024-12-04 17:31 ` [v1] " bluez.test.bot 2024-12-05 15:50 ` [PATCH v1] " patchwork-bot+bluetooth
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.