Re: [PATCH v2] Bluetooth: hci_core: Fix sleeping function called from invalid context

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Luiz,

ke, 2024-12-04 kello 11:58 -0500, Luiz Augusto von Dentz kirjoitti:
> From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=2fb0835e0c9cefc34614
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> ---
>  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,
>  };






[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux