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@xxxxxxxxx> wrote:
> >
> > From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> >
> > 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@xxxxxxx/T/#t
> > Fixes: 4d94f0555827 ("Bluetooth: hci_core: Fix sleeping function called from invalid context")
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> > ---
> > 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
