[PATCH RESEND] Bluetooth: hci_core: add lockdep check to hci_conn lookups

[PATCH RESEND] Bluetooth: hci_core: add lockdep check to hci_conn lookups

[Pauli Virtanen]

Add lockdep check for RCU || hdev->lock in hci_conn_hash lookups that
return hci_conn pointer, as dereferencing that without locks can be
TOCTOU issue. It used to be several callsites did not hold appropriate
locks.

The check is equivalent to removing rcu_read_lock() and doing instead
list_for_each_entry_rcu(c, &h->list, list, lockdep_is_held(&hdev->lock))
Although there should not be any remaining callsites without locks,
don't remove the rcu_read_lock() for now, and just add the warning here.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    resend:
    - no changes

 include/net/bluetooth/hci_core.h | 44 ++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 4105c446ca98..c12cd6873f65 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1030,6 +1030,9 @@ static inline bool hci_conn_sc_enabled(struct hci_conn *conn)
 static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c)
 {
 	struct hci_conn_hash *h = &hdev->conn_hash;
+
+	lockdep_assert_held(&hdev->lock);
+
 	list_add_tail_rcu(&c->list, &h->list);
 	switch (c->type) {
 	case ACL_LINK:
@@ -1060,6 +1063,8 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c)
 {
 	struct hci_conn_hash *h = &hdev->conn_hash;
 
+	lockdep_assert_held(&hdev->lock);
+
 	list_del_rcu(&c->list);
 	synchronize_rcu();
 
@@ -1088,6 +1093,15 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c)
 	}
 }
 
+#ifdef CONFIG_PROVE_RCU
+#define HCI_CONN_HASH_LOCKDEP_CHECK(hdev)				\
+	RCU_LOCKDEP_WARN(!lockdep_is_held(&(hdev)->lock) &&		\
+			 !rcu_read_lock_held(),				\
+			 "suspicious hci_conn locking")
+#else
+#define HCI_CONN_HASH_LOCKDEP_CHECK(hdev) do { } while (0 && (hdev))
+#endif
+
 static inline unsigned int hci_conn_num(struct hci_dev *hdev, __u8 type)
 {
 	struct hci_conn_hash *h = &hdev->conn_hash;
@@ -1169,6 +1183,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_bis(struct hci_dev *hdev,
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn  *c;
 
+	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
+
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(c, &h->list, list) {
@@ -1191,6 +1207,8 @@ hci_conn_hash_lookup_create_pa_sync(struct hci_dev *hdev)
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn  *c;
 
+	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
+
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(c, &h->list, list) {
@@ -1217,6 +1235,8 @@ hci_conn_hash_lookup_per_adv_bis(struct hci_dev *hdev,
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn  *c;
 
+	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
+
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(c, &h->list, list) {
@@ -1241,6 +1261,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_handle(struct hci_dev *hdev,
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn  *c;
 
+	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
+
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(c, &h->list, list) {
@@ -1260,6 +1282,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_ba(struct hci_dev *hdev,
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn  *c;
 
+	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
+
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(c, &h->list, list) {
@@ -1281,6 +1305,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_role(struct hci_dev *hdev,
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn  *c;
 
+	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
+
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(c, &h->list, list) {
@@ -1302,6 +1328,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_le(struct hci_dev *hdev,
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn  *c;
 
+	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
+
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(c, &h->list, list) {
@@ -1328,6 +1356,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_cis(struct hci_dev *hdev,
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn  *c;
 
+	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
+
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(c, &h->list, list) {
@@ -1360,6 +1390,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_cig(struct hci_dev *hdev,
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn  *c;
 
+	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
+
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(c, &h->list, list) {
@@ -1383,6 +1415,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_big(struct hci_dev *hdev,
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn  *c;
 
+	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
+
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(c, &h->list, list) {
@@ -1407,6 +1441,8 @@ hci_conn_hash_lookup_big_sync_pend(struct hci_dev *hdev,
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn  *c;
 
+	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
+
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(c, &h->list, list) {
@@ -1431,6 +1467,8 @@ hci_conn_hash_lookup_big_state(struct hci_dev *hdev, __u8 handle, __u16 state,
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn  *c;
 
+	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
+
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(c, &h->list, list) {
@@ -1454,6 +1492,8 @@ hci_conn_hash_lookup_pa_sync_big_handle(struct hci_dev *hdev, __u8 big)
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn  *c;
 
+	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
+
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(c, &h->list, list) {
@@ -1477,6 +1517,8 @@ hci_conn_hash_lookup_pa_sync_handle(struct hci_dev *hdev, __u16 sync_handle)
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn  *c;
 
+	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
+
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(c, &h->list, list) {
@@ -1546,6 +1588,8 @@ static inline struct hci_conn *hci_lookup_le_connect(struct hci_dev *hdev)
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn  *c;
 
+	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
+
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(c, &h->list, list) {
-- 
2.55.0

RE: [RESEND] Bluetooth: hci_core: add lockdep check to hci_conn lookups

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2389 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=1146718

---Test result---

Test Summary:
CheckPatch                    PASS      0.77 seconds
VerifyFixes                   PASS      0.13 seconds
VerifySignedoff               PASS      0.13 seconds
GitLint                       PASS      0.32 seconds
SubjectPrefix                 PASS      0.27 seconds
BuildKernel                   PASS      27.43 seconds
CheckAllWarning               PASS      30.53 seconds
CheckSparse                   PASS      29.28 seconds
BuildKernel32                 PASS      26.86 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      508.42 seconds
TestRunner_l2cap-tester       PASS      66.05 seconds
TestRunner_iso-tester         PASS      83.58 seconds
TestRunner_bnep-tester        PASS      19.20 seconds
TestRunner_mgmt-tester        FAIL      225.44 seconds
TestRunner_rfcomm-tester      PASS      26.55 seconds
TestRunner_sco-tester         PASS      32.08 seconds
TestRunner_ioctl-tester       PASS      26.68 seconds
TestRunner_mesh-tester        FAIL      25.95 seconds
TestRunner_smp-tester         PASS      23.70 seconds
TestRunner_userchan-tester    PASS      20.34 seconds
TestRunner_6lowpan-tester     PASS      24.08 seconds
IncrementalBuild              PASS      25.72 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 501, Passed: 496 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.252 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    1.920 seconds
Mesh - Send cancel - 2                               Timed out    1.981 seconds


https://github.com/bluez/bluetooth-next/pull/593

---
Regards,
Linux Bluetooth

Re: [PATCH RESEND] Bluetooth: hci_core: add lockdep check to hci_conn lookups

[Pauli Virtanen]

su, 2026-08-16 kello 13:26 +0300, Pauli Virtanen kirjoitti:
> Add lockdep check for RCU || hdev->lock in hci_conn_hash lookups that
> return hci_conn pointer, as dereferencing that without locks can be
> TOCTOU issue. It used to be several callsites did not hold appropriate
> locks.

https://sashiko.dev/#/patchset/2be38d111362590f45776a0bc114f7890906ead9.1786875148.git.pav%40iki.fi

Sashiko review complains about HCI_CONN_HASH_LOCKDEP_CHECK() added to
wrong functions.

It applied the patch to bluetooth/master, instead of bluetooth-
next/master as intended.

Apparently the patch applies with fuzz also to bluetooth/master, but
some of the added HCI_CONN_HASH_LOCKDEP_CHECK land in wrong functions
there.

> The check is equivalent to removing rcu_read_lock() and doing instead
> list_for_each_entry_rcu(c, &h->list, list, lockdep_is_held(&hdev->lock))
> Although there should not be any remaining callsites without locks,
> don't remove the rcu_read_lock() for now, and just add the warning here.
> 
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
> 
> Notes:
>     resend:
>     - no changes
> 
>  include/net/bluetooth/hci_core.h | 44 ++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 4105c446ca98..c12cd6873f65 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1030,6 +1030,9 @@ static inline bool hci_conn_sc_enabled(struct hci_conn *conn)
>  static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c)
>  {
>  	struct hci_conn_hash *h = &hdev->conn_hash;
> +
> +	lockdep_assert_held(&hdev->lock);
> +
>  	list_add_tail_rcu(&c->list, &h->list);
>  	switch (c->type) {
>  	case ACL_LINK:
> @@ -1060,6 +1063,8 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c)
>  {
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  
> +	lockdep_assert_held(&hdev->lock);
> +
>  	list_del_rcu(&c->list);
>  	synchronize_rcu();
>  
> @@ -1088,6 +1093,15 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c)
>  	}
>  }
>  
> +#ifdef CONFIG_PROVE_RCU
> +#define HCI_CONN_HASH_LOCKDEP_CHECK(hdev)				\
> +	RCU_LOCKDEP_WARN(!lockdep_is_held(&(hdev)->lock) &&		\
> +			 !rcu_read_lock_held(),				\
> +			 "suspicious hci_conn locking")
> +#else
> +#define HCI_CONN_HASH_LOCKDEP_CHECK(hdev) do { } while (0 && (hdev))
> +#endif
> +
>  static inline unsigned int hci_conn_num(struct hci_dev *hdev, __u8 type)
>  {
>  	struct hci_conn_hash *h = &hdev->conn_hash;
> @@ -1169,6 +1183,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_bis(struct hci_dev *hdev,
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  	struct hci_conn  *c;
>  
> +	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(c, &h->list, list) {
> @@ -1191,6 +1207,8 @@ hci_conn_hash_lookup_create_pa_sync(struct hci_dev *hdev)
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  	struct hci_conn  *c;
>  
> +	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(c, &h->list, list) {
> @@ -1217,6 +1235,8 @@ hci_conn_hash_lookup_per_adv_bis(struct hci_dev *hdev,
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  	struct hci_conn  *c;
>  
> +	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(c, &h->list, list) {
> @@ -1241,6 +1261,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_handle(struct hci_dev *hdev,
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  	struct hci_conn  *c;
>  
> +	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(c, &h->list, list) {
> @@ -1260,6 +1282,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_ba(struct hci_dev *hdev,
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  	struct hci_conn  *c;
>  
> +	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(c, &h->list, list) {
> @@ -1281,6 +1305,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_role(struct hci_dev *hdev,
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  	struct hci_conn  *c;
>  
> +	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(c, &h->list, list) {
> @@ -1302,6 +1328,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_le(struct hci_dev *hdev,
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  	struct hci_conn  *c;
>  
> +	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(c, &h->list, list) {
> @@ -1328,6 +1356,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_cis(struct hci_dev *hdev,
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  	struct hci_conn  *c;
>  
> +	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(c, &h->list, list) {
> @@ -1360,6 +1390,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_cig(struct hci_dev *hdev,
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  	struct hci_conn  *c;
>  
> +	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(c, &h->list, list) {
> @@ -1383,6 +1415,8 @@ static inline struct hci_conn *hci_conn_hash_lookup_big(struct hci_dev *hdev,
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  	struct hci_conn  *c;
>  
> +	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(c, &h->list, list) {
> @@ -1407,6 +1441,8 @@ hci_conn_hash_lookup_big_sync_pend(struct hci_dev *hdev,
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  	struct hci_conn  *c;
>  
> +	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(c, &h->list, list) {
> @@ -1431,6 +1467,8 @@ hci_conn_hash_lookup_big_state(struct hci_dev *hdev, __u8 handle, __u16 state,
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  	struct hci_conn  *c;
>  
> +	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(c, &h->list, list) {
> @@ -1454,6 +1492,8 @@ hci_conn_hash_lookup_pa_sync_big_handle(struct hci_dev *hdev, __u8 big)
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  	struct hci_conn  *c;
>  
> +	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(c, &h->list, list) {
> @@ -1477,6 +1517,8 @@ hci_conn_hash_lookup_pa_sync_handle(struct hci_dev *hdev, __u16 sync_handle)
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  	struct hci_conn  *c;
>  
> +	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(c, &h->list, list) {
> @@ -1546,6 +1588,8 @@ static inline struct hci_conn *hci_lookup_le_connect(struct hci_dev *hdev)
>  	struct hci_conn_hash *h = &hdev->conn_hash;
>  	struct hci_conn  *c;
>  
> +	HCI_CONN_HASH_LOCKDEP_CHECK(hdev);
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(c, &h->list, list) {

-- 
Pauli Virtanen