| Message ID | 20250304153934.112156-1-luiz.dentz@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8da76b2ac810e911bc75480b15fa51282b5ff0a0 |
| Headers | show |
| Series | [v1] Revert "Bluetooth: hci_core: Fix sleeping function called from invalid context" | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | warning | CheckSparse WARNING net/bluetooth/sco.c: note: in included file:./include/net/bluetooth/hci_core.h:147:35: warning: array of flexible structures |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | success | TestRunner PASS |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | success | TestRunner PASS |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
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=940135 ---Test result--- Test Summary: CheckPatch PENDING 0.28 seconds GitLint PENDING 0.25 seconds SubjectPrefix PASS 0.52 seconds BuildKernel PASS 24.50 seconds CheckAllWarning PASS 26.67 seconds CheckSparse WARNING 30.45 seconds BuildKernel32 PASS 24.14 seconds TestRunnerSetup PASS 438.61 seconds TestRunner_l2cap-tester PASS 21.65 seconds TestRunner_iso-tester PASS 35.72 seconds TestRunner_bnep-tester PASS 4.99 seconds TestRunner_mgmt-tester PASS 123.93 seconds TestRunner_rfcomm-tester PASS 8.18 seconds TestRunner_sco-tester PASS 13.84 seconds TestRunner_ioctl-tester PASS 8.76 seconds TestRunner_mesh-tester PASS 6.16 seconds TestRunner_smp-tester PASS 7.53 seconds TestRunner_userchan-tester PASS 5.16 seconds IncrementalBuild PENDING 0.61 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/sco.c: note: in included file:./include/net/bluetooth/hci_core.h:147:35: warning: array of flexible structures ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth
Hi Pauli, Takashi, On Tue, Mar 4, 2025 at 10:39 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > This reverts commit 4d94f05558271654670d18c26c912da0c1c15549 which has > problems (see [1]) and is no longer needed since 581dd2dc168f > ("Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating") > has reworked the code where the original bug has been found. > > [1] Link: https://lore.kernel.org/linux-bluetooth/877c55ci1r.wl-tiwai@suse.de/T/#t > Fixes: 4d94f0555827 ("Bluetooth: hci_core: Fix sleeping function called from invalid context") > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > include/net/bluetooth/hci_core.h | 108 +++++++++++-------------------- > net/bluetooth/hci_core.c | 10 ++- > 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, 57 insertions(+), 97 deletions(-) > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index 2f320eeddfec..7966db4038cc 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -804,6 +804,7 @@ 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) > @@ -2014,47 +2015,24 @@ 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 list_head list; > - struct hci_cb *cb, *tmp; > + struct hci_cb *cb; > > - INIT_LIST_HEAD(&list); > - hci_cb_lookup(conn, &list); > - > - list_for_each_entry_safe(cb, tmp, &list, list) { > + mutex_lock(&hci_cb_list_lock); > + list_for_each_entry(cb, &hci_cb_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); > @@ -2062,43 +2040,22 @@ 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 list_head list; > - struct hci_cb *cb, *tmp; > + struct hci_cb *cb; > > - INIT_LIST_HEAD(&list); > - hci_cb_lookup(conn, &list); > - > - list_for_each_entry_safe(cb, tmp, &list, list) { > + mutex_lock(&hci_cb_list_lock); > + list_for_each_entry(cb, &hci_cb_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)) > @@ -2106,11 +2063,20 @@ static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status) > > encrypt = test_bit(HCI_CONN_ENCRYPT, &conn->flags) ? 0x01 : 0x00; > > - hci_security_cfm(conn, status, encrypt); > + 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); > } > > static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) > { > + struct hci_cb *cb; > __u8 encrypt; > > if (conn->state == BT_CONFIG) { > @@ -2137,38 +2103,40 @@ static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) > conn->sec_level = conn->pending_sec_level; > } > > - hci_security_cfm(conn, status, encrypt); > + 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); > } > > static inline void hci_key_change_cfm(struct hci_conn *conn, __u8 status) > { > - struct list_head list; > - struct hci_cb *cb, *tmp; > + struct hci_cb *cb; > > - INIT_LIST_HEAD(&list); > - hci_cb_lookup(conn, &list); > - > - list_for_each_entry_safe(cb, tmp, &list, list) { > + mutex_lock(&hci_cb_list_lock); > + list_for_each_entry(cb, &hci_cb_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 list_head list; > - struct hci_cb *cb, *tmp; > + struct hci_cb *cb; > > - INIT_LIST_HEAD(&list); > - hci_cb_lookup(conn, &list); > - > - list_for_each_entry_safe(cb, tmp, &list, list) { > + mutex_lock(&hci_cb_list_lock); > + list_for_each_entry(cb, &hci_cb_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 e7ec12437c8b..012fc107901a 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -57,6 +57,7 @@ DEFINE_RWLOCK(hci_dev_list_lock); > > /* HCI callback list */ > LIST_HEAD(hci_cb_list); > +DEFINE_MUTEX(hci_cb_list_lock); > > /* HCI ID Numbering */ > static DEFINE_IDA(hci_index_ida); > @@ -2972,7 +2973,9 @@ int hci_register_cb(struct hci_cb *cb) > { > BT_DBG("%p name %s", cb, cb->name); > > - list_add_tail_rcu(&cb->list, &hci_cb_list); > + mutex_lock(&hci_cb_list_lock); > + list_add_tail(&cb->list, &hci_cb_list); > + mutex_unlock(&hci_cb_list_lock); > > return 0; > } > @@ -2982,8 +2985,9 @@ int hci_unregister_cb(struct hci_cb *cb) > { > BT_DBG("%p name %s", cb, cb->name); > > - list_del_rcu(&cb->list); > - synchronize_rcu(); > + mutex_lock(&hci_cb_list_lock); > + list_del(&cb->list); > + mutex_unlock(&hci_cb_list_lock); > > return 0; > } > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c > index 44acddf58a0c..0cb52a3308ba 100644 > --- a/net/bluetooth/iso.c > +++ b/net/bluetooth/iso.c > @@ -2187,11 +2187,6 @@ 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) { > @@ -2373,7 +2368,6 @@ 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 d21267261a5e..7b4adab353cf 100644 > --- a/net/bluetooth/l2cap_core.c > +++ b/net/bluetooth/l2cap_core.c > @@ -7182,11 +7182,6 @@ 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; > @@ -7194,6 +7189,9 @@ 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) { > @@ -7258,6 +7256,9 @@ 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)); > @@ -7565,7 +7566,6 @@ 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 4c56ca5a216c..ad5177e3a69b 100644 > --- a/net/bluetooth/rfcomm/core.c > +++ b/net/bluetooth/rfcomm/core.c > @@ -2134,11 +2134,6 @@ 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; > @@ -2185,7 +2180,6 @@ 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 ed6846864ea9..5d1bc0d6aee0 100644 > --- a/net/bluetooth/sco.c > +++ b/net/bluetooth/sco.c > @@ -1407,13 +1407,11 @@ 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) { > @@ -1430,6 +1428,9 @@ 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)); > @@ -1455,7 +1456,6 @@ 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.48.1 > Are you guys ok if I push this?
On Wed, 05 Mar 2025 20:50:54 +0100, Luiz Augusto von Dentz wrote: > > Hi Pauli, Takashi, > > On Tue, Mar 4, 2025 at 10:39 AM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > This reverts commit 4d94f05558271654670d18c26c912da0c1c15549 which has > > problems (see [1]) and is no longer needed since 581dd2dc168f > > ("Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating") > > has reworked the code where the original bug has been found. > > > > [1] Link: https://lore.kernel.org/linux-bluetooth/877c55ci1r.wl-tiwai@suse.de/T/#t > > Fixes: 4d94f0555827 ("Bluetooth: hci_core: Fix sleeping function called from invalid context") > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > --- > > include/net/bluetooth/hci_core.h | 108 +++++++++++-------------------- > > net/bluetooth/hci_core.c | 10 ++- > > 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, 57 insertions(+), 97 deletions(-) > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > index 2f320eeddfec..7966db4038cc 100644 > > --- a/include/net/bluetooth/hci_core.h > > +++ b/include/net/bluetooth/hci_core.h > > @@ -804,6 +804,7 @@ 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) > > @@ -2014,47 +2015,24 @@ 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 list_head list; > > - struct hci_cb *cb, *tmp; > > + struct hci_cb *cb; > > > > - INIT_LIST_HEAD(&list); > > - hci_cb_lookup(conn, &list); > > - > > - list_for_each_entry_safe(cb, tmp, &list, list) { > > + mutex_lock(&hci_cb_list_lock); > > + list_for_each_entry(cb, &hci_cb_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); > > @@ -2062,43 +2040,22 @@ 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 list_head list; > > - struct hci_cb *cb, *tmp; > > + struct hci_cb *cb; > > > > - INIT_LIST_HEAD(&list); > > - hci_cb_lookup(conn, &list); > > - > > - list_for_each_entry_safe(cb, tmp, &list, list) { > > + mutex_lock(&hci_cb_list_lock); > > + list_for_each_entry(cb, &hci_cb_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)) > > @@ -2106,11 +2063,20 @@ static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status) > > > > encrypt = test_bit(HCI_CONN_ENCRYPT, &conn->flags) ? 0x01 : 0x00; > > > > - hci_security_cfm(conn, status, encrypt); > > + 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); > > } > > > > static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) > > { > > + struct hci_cb *cb; > > __u8 encrypt; > > > > if (conn->state == BT_CONFIG) { > > @@ -2137,38 +2103,40 @@ static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) > > conn->sec_level = conn->pending_sec_level; > > } > > > > - hci_security_cfm(conn, status, encrypt); > > + 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); > > } > > > > static inline void hci_key_change_cfm(struct hci_conn *conn, __u8 status) > > { > > - struct list_head list; > > - struct hci_cb *cb, *tmp; > > + struct hci_cb *cb; > > > > - INIT_LIST_HEAD(&list); > > - hci_cb_lookup(conn, &list); > > - > > - list_for_each_entry_safe(cb, tmp, &list, list) { > > + mutex_lock(&hci_cb_list_lock); > > + list_for_each_entry(cb, &hci_cb_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 list_head list; > > - struct hci_cb *cb, *tmp; > > + struct hci_cb *cb; > > > > - INIT_LIST_HEAD(&list); > > - hci_cb_lookup(conn, &list); > > - > > - list_for_each_entry_safe(cb, tmp, &list, list) { > > + mutex_lock(&hci_cb_list_lock); > > + list_for_each_entry(cb, &hci_cb_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 e7ec12437c8b..012fc107901a 100644 > > --- a/net/bluetooth/hci_core.c > > +++ b/net/bluetooth/hci_core.c > > @@ -57,6 +57,7 @@ DEFINE_RWLOCK(hci_dev_list_lock); > > > > /* HCI callback list */ > > LIST_HEAD(hci_cb_list); > > +DEFINE_MUTEX(hci_cb_list_lock); > > > > /* HCI ID Numbering */ > > static DEFINE_IDA(hci_index_ida); > > @@ -2972,7 +2973,9 @@ int hci_register_cb(struct hci_cb *cb) > > { > > BT_DBG("%p name %s", cb, cb->name); > > > > - list_add_tail_rcu(&cb->list, &hci_cb_list); > > + mutex_lock(&hci_cb_list_lock); > > + list_add_tail(&cb->list, &hci_cb_list); > > + mutex_unlock(&hci_cb_list_lock); > > > > return 0; > > } > > @@ -2982,8 +2985,9 @@ int hci_unregister_cb(struct hci_cb *cb) > > { > > BT_DBG("%p name %s", cb, cb->name); > > > > - list_del_rcu(&cb->list); > > - synchronize_rcu(); > > + mutex_lock(&hci_cb_list_lock); > > + list_del(&cb->list); > > + mutex_unlock(&hci_cb_list_lock); > > > > return 0; > > } > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c > > index 44acddf58a0c..0cb52a3308ba 100644 > > --- a/net/bluetooth/iso.c > > +++ b/net/bluetooth/iso.c > > @@ -2187,11 +2187,6 @@ 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) { > > @@ -2373,7 +2368,6 @@ 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 d21267261a5e..7b4adab353cf 100644 > > --- a/net/bluetooth/l2cap_core.c > > +++ b/net/bluetooth/l2cap_core.c > > @@ -7182,11 +7182,6 @@ 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; > > @@ -7194,6 +7189,9 @@ 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) { > > @@ -7258,6 +7256,9 @@ 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)); > > @@ -7565,7 +7566,6 @@ 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 4c56ca5a216c..ad5177e3a69b 100644 > > --- a/net/bluetooth/rfcomm/core.c > > +++ b/net/bluetooth/rfcomm/core.c > > @@ -2134,11 +2134,6 @@ 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; > > @@ -2185,7 +2180,6 @@ 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 ed6846864ea9..5d1bc0d6aee0 100644 > > --- a/net/bluetooth/sco.c > > +++ b/net/bluetooth/sco.c > > @@ -1407,13 +1407,11 @@ 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) { > > @@ -1430,6 +1428,9 @@ 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)); > > @@ -1455,7 +1456,6 @@ 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.48.1 > > > > Are you guys ok if I push this? Yes, thanks! Takashi
Hello: This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: On Tue, 4 Mar 2025 10:39:34 -0500 you wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > This reverts commit 4d94f05558271654670d18c26c912da0c1c15549 which has > problems (see [1]) and is no longer needed since 581dd2dc168f > ("Bluetooth: hci_event: Fix using rcu_read_(un)lock while iterating") > has reworked the code where the original bug has been found. > > [...] Here is the summary with links: - [v1] Revert "Bluetooth: hci_core: Fix sleeping function called from invalid context" https://git.kernel.org/bluetooth/bluetooth-next/c/8da76b2ac810 You are awesome, thank you!
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 2f320eeddfec..7966db4038cc 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -804,6 +804,7 @@ 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) @@ -2014,47 +2015,24 @@ 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 list_head list; - struct hci_cb *cb, *tmp; + struct hci_cb *cb; - INIT_LIST_HEAD(&list); - hci_cb_lookup(conn, &list); - - list_for_each_entry_safe(cb, tmp, &list, list) { + mutex_lock(&hci_cb_list_lock); + list_for_each_entry(cb, &hci_cb_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); @@ -2062,43 +2040,22 @@ 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 list_head list; - struct hci_cb *cb, *tmp; + struct hci_cb *cb; - INIT_LIST_HEAD(&list); - hci_cb_lookup(conn, &list); - - list_for_each_entry_safe(cb, tmp, &list, list) { + mutex_lock(&hci_cb_list_lock); + list_for_each_entry(cb, &hci_cb_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)) @@ -2106,11 +2063,20 @@ static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status) encrypt = test_bit(HCI_CONN_ENCRYPT, &conn->flags) ? 0x01 : 0x00; - hci_security_cfm(conn, status, encrypt); + 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); } static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) { + struct hci_cb *cb; __u8 encrypt; if (conn->state == BT_CONFIG) { @@ -2137,38 +2103,40 @@ static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status) conn->sec_level = conn->pending_sec_level; } - hci_security_cfm(conn, status, encrypt); + 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); } static inline void hci_key_change_cfm(struct hci_conn *conn, __u8 status) { - struct list_head list; - struct hci_cb *cb, *tmp; + struct hci_cb *cb; - INIT_LIST_HEAD(&list); - hci_cb_lookup(conn, &list); - - list_for_each_entry_safe(cb, tmp, &list, list) { + mutex_lock(&hci_cb_list_lock); + list_for_each_entry(cb, &hci_cb_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 list_head list; - struct hci_cb *cb, *tmp; + struct hci_cb *cb; - INIT_LIST_HEAD(&list); - hci_cb_lookup(conn, &list); - - list_for_each_entry_safe(cb, tmp, &list, list) { + mutex_lock(&hci_cb_list_lock); + list_for_each_entry(cb, &hci_cb_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 e7ec12437c8b..012fc107901a 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -57,6 +57,7 @@ DEFINE_RWLOCK(hci_dev_list_lock); /* HCI callback list */ LIST_HEAD(hci_cb_list); +DEFINE_MUTEX(hci_cb_list_lock); /* HCI ID Numbering */ static DEFINE_IDA(hci_index_ida); @@ -2972,7 +2973,9 @@ int hci_register_cb(struct hci_cb *cb) { BT_DBG("%p name %s", cb, cb->name); - list_add_tail_rcu(&cb->list, &hci_cb_list); + mutex_lock(&hci_cb_list_lock); + list_add_tail(&cb->list, &hci_cb_list); + mutex_unlock(&hci_cb_list_lock); return 0; } @@ -2982,8 +2985,9 @@ int hci_unregister_cb(struct hci_cb *cb) { BT_DBG("%p name %s", cb, cb->name); - list_del_rcu(&cb->list); - synchronize_rcu(); + mutex_lock(&hci_cb_list_lock); + list_del(&cb->list); + mutex_unlock(&hci_cb_list_lock); return 0; } diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c index 44acddf58a0c..0cb52a3308ba 100644 --- a/net/bluetooth/iso.c +++ b/net/bluetooth/iso.c @@ -2187,11 +2187,6 @@ 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) { @@ -2373,7 +2368,6 @@ 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 d21267261a5e..7b4adab353cf 100644 --- a/net/bluetooth/l2cap_core.c +++ b/net/bluetooth/l2cap_core.c @@ -7182,11 +7182,6 @@ 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; @@ -7194,6 +7189,9 @@ 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) { @@ -7258,6 +7256,9 @@ 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)); @@ -7565,7 +7566,6 @@ 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 4c56ca5a216c..ad5177e3a69b 100644 --- a/net/bluetooth/rfcomm/core.c +++ b/net/bluetooth/rfcomm/core.c @@ -2134,11 +2134,6 @@ 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; @@ -2185,7 +2180,6 @@ 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 ed6846864ea9..5d1bc0d6aee0 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -1407,13 +1407,11 @@ 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) { @@ -1430,6 +1428,9 @@ 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)); @@ -1455,7 +1456,6 @@ 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, };