* [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: [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: [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: [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 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 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
* 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
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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).