[RFC PATCH 0/5] Bluetooth: enable context analysis

[RFC PATCH 0/5] Bluetooth: enable context analysis

[Pauli Virtanen]

Set up compiler context analysis that generate compiler warnings on
problems that Clang -Wthread-safety can detect:

https://docs.kernel.org/dev-tools/context-analysis.html

Clang 22, and probably Clang 23 [1] will be required.  Sparse locking
analysis support was removed in commit
5b63d0ae94ccfd64dcbdb693d88eb3650eb3c64c, this is its successor.

This series enables the analysis and adds minimal annotations to silence
some false positives.

Also, one patch to fix what looks like a legitimate locking issue in
iso.c.

In future, it probably is a good idea to make more use of it and add
__must_hold, __guarded_by etc annotations.

Kernel test robot appears to be checking for these, but not sure in what
trees [2]

BlueZ testbot doesn't check these currently but it's possible to add
https://github.com/bluez/action-ci/pull/4

[1] https://lore.kernel.org/all/CANpmjNN4O=W70sAc5gaVkTAFdrGGOW+XBMyuehfz3_QMiT=uCw@mail.gmail.com/
[2] https://lore.kernel.org/all/202605060005.JYWpZXr2-lkp@intel.com/

Pauli Virtanen (5):
  Bluetooth: af_bluetooth: Add minimal context analysis annotations
  Bluetooth: hci_core: Add minimal context analysis annotations
  Bluetooth: ISO: lock same hdev as what is released
  Bluetooth: L2CAP: Add minimal context analysis annotations
  Bluetooth: enable context analysis

 drivers/bluetooth/Makefile   |  2 ++
 net/bluetooth/Makefile       |  2 ++
 net/bluetooth/af_bluetooth.c |  7 +++++--
 net/bluetooth/hci_core.c     |  3 +++
 net/bluetooth/iso.c          | 14 ++++++++------
 net/bluetooth/l2cap_sock.c   |  1 +
 6 files changed, 21 insertions(+), 8 deletions(-)

-- 
2.54.0

[RFC PATCH 1/5] Bluetooth: af_bluetooth: Add minimal context analysis annotations

[Pauli Virtanen]

Add minimal compiler context analysis annotations, required for
compilation to pass.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/af_bluetooth.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 9d68dd86023c..f42467159947 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -212,6 +212,7 @@ bool bt_sock_linked(struct bt_sock_list *l, struct sock *s)
 EXPORT_SYMBOL(bt_sock_linked);
 
 void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh)
+	__context_unsafe(/* conditional locking */)
 {
 	const struct cred *old_cred;
 	struct pid *old_pid;
@@ -819,7 +820,8 @@ EXPORT_SYMBOL(bt_sock_wait_ready);
 
 #ifdef CONFIG_PROC_FS
 static void *bt_seq_start(struct seq_file *seq, loff_t *pos)
-	__acquires(seq->private->l->lock)
+	__acquires_shared(&((struct bt_sock_list *)
+			    pde_data(file_inode(seq->file)))->lock)
 {
 	struct bt_sock_list *l = pde_data(file_inode(seq->file));
 
@@ -835,7 +837,8 @@ static void *bt_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 }
 
 static void bt_seq_stop(struct seq_file *seq, void *v)
-	__releases(seq->private->l->lock)
+	__releases_shared(&((struct bt_sock_list *)
+			    pde_data(file_inode(seq->file)))->lock)
 {
 	struct bt_sock_list *l = pde_data(file_inode(seq->file));
 
-- 
2.54.0

[RFC PATCH 2/5] Bluetooth: hci_core: Add minimal context analysis annotations

[Pauli Virtanen]

Add minimal compiler context analysis annotations, required for
compilation to pass.

compiler-context-analysis.h doesn't have tools to deal with the
conditional SRCU locking on return value used here, so just disable the
analysis in places.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/hci_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index c46c1236ebfa..fa87cb0bcf7b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -65,6 +65,7 @@ static DEFINE_IDA(hci_index_ida);
 /* Get HCI device by index.
  * Device is held on return. */
 static struct hci_dev *__hci_dev_get(int index, int *srcu_index)
+	__context_unsafe(/* conditional locking */)
 {
 	struct hci_dev *hdev = NULL, *d;
 
@@ -92,11 +93,13 @@ struct hci_dev *hci_dev_get(int index)
 }
 
 static struct hci_dev *hci_dev_get_srcu(int index, int *srcu_index)
+	__context_unsafe(/* conditional locking vs return */)
 {
 	return __hci_dev_get(index, srcu_index);
 }
 
 static void hci_dev_put_srcu(struct hci_dev *hdev, int srcu_index)
+	__context_unsafe(/* conditional locking vs return */)
 {
 	srcu_read_unlock(&hdev->srcu, srcu_index);
 	hci_dev_put(hdev);
-- 
2.54.0

[RFC PATCH 3/5] Bluetooth: ISO: lock same hdev as what is released

[Pauli Virtanen]

bis may be deleted concurrently after release_sock(sk).

To avoid this, lock the hdev obtained in iso_conn_get_hdev that we have
refcount for.  Also, obtain refcount on the hci_conn to avoid comparing
freed pointer in the race check below.

Addresses valid context analysis warning

net/bluetooth/iso.c:1095:2: warning: releasing mutex 'iso_conn_get_hdev(sk->conn).lock'
that was not held [-Wthread-safety-analysis]

Fixes: d3413703d5f8b ("Bluetooth: ISO: Add support to bind to trigger PAST")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/iso.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 7cb2864fe872..34991fc7e0a7 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1027,13 +1027,16 @@ static int iso_sock_rebind_bis(struct sock *sk, struct sockaddr_iso *sa,
 	return err;
 }
 
-static struct hci_dev *iso_conn_get_hdev(struct iso_conn *conn)
+static struct hci_dev *iso_conn_get_hdev(struct iso_conn *conn,
+					 struct hci_conn **hcon)
 {
 	struct hci_dev *hdev = NULL;
 
 	iso_conn_lock(conn);
-	if (conn->hcon)
+	if (conn->hcon) {
 		hdev = hci_dev_hold(conn->hcon->hdev);
+		*hcon = hci_conn_get(conn->hcon);
+	}
 	iso_conn_unlock(conn);
 
 	return hdev;
@@ -1065,18 +1068,16 @@ static int iso_sock_rebind_bc(struct sock *sk, struct sockaddr_iso *sa,
 	if (!bdaddr_type_is_le(sa->iso_bc->bc_bdaddr_type))
 		return -EINVAL;
 
-	hdev = iso_conn_get_hdev(iso_pi(sk)->conn);
+	hdev = iso_conn_get_hdev(iso_pi(sk)->conn, &bis);
 	if (!hdev)
 		return -EINVAL;
 
-	bis = iso_pi(sk)->conn->hcon;
-
 	/* Release the socket before lookups since that requires hci_dev_lock
 	 * which shall not be acquired while holding sock_lock for proper
 	 * ordering.
 	 */
 	release_sock(sk);
-	hci_dev_lock(bis->hdev);
+	hci_dev_lock(hdev);
 	lock_sock(sk);
 
 	if (!iso_pi(sk)->conn || iso_pi(sk)->conn->hcon != bis) {
@@ -1093,6 +1094,7 @@ static int iso_sock_rebind_bc(struct sock *sk, struct sockaddr_iso *sa,
 
 unlock:
 	hci_dev_unlock(hdev);
+	hci_conn_put(bis);
 	hci_dev_put(hdev);
 
 	return err;
-- 
2.54.0

[RFC PATCH 4/5] Bluetooth: L2CAP: Add minimal context analysis annotations

[Pauli Virtanen]

Context analysis has what looks like false positive vs. taking
conn->lock in l2cap_sock_shutdown().

chan->conn may be deleted concurrently and become NULL while retaking
chan->lock, but since chan shall not be reused, chan->conn cannot be
replaced by a different l2cap_conn.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/l2cap_sock.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index cf590a67d364..bbf883098b7f 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1336,6 +1336,7 @@ static int __l2cap_wait_ack(struct sock *sk, struct l2cap_chan *chan)
 }
 
 static int l2cap_sock_shutdown(struct socket *sock, int how)
+	__context_unsafe(/* complex chan->conn locking */)
 {
 	struct sock *sk = sock->sk;
 	struct l2cap_chan *chan;
-- 
2.54.0

[RFC PATCH 5/5] Bluetooth: enable context analysis

[Pauli Virtanen]

Enable compiler context analysis for Bluetooth subsystem and drivers.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 drivers/bluetooth/Makefile | 2 ++
 net/bluetooth/Makefile     | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index bafc26250b63..e6b1c1180d1d 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -50,3 +50,5 @@ hci_uart-$(CONFIG_BT_HCIUART_AG6XX)	+= hci_ag6xx.o
 hci_uart-$(CONFIG_BT_HCIUART_MRVL)	+= hci_mrvl.o
 hci_uart-$(CONFIG_BT_HCIUART_AML)	+= hci_aml.o
 hci_uart-objs				:= $(hci_uart-y)
+
+CONTEXT_ANALYSIS := y
diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile
index 41049b280887..ff466ea97436 100644
--- a/net/bluetooth/Makefile
+++ b/net/bluetooth/Makefile
@@ -25,3 +25,5 @@ bluetooth-$(CONFIG_BT_MSFTEXT) += msft.o
 bluetooth-$(CONFIG_BT_AOSPEXT) += aosp.o
 bluetooth-$(CONFIG_BT_DEBUGFS) += hci_debugfs.o
 bluetooth-$(CONFIG_BT_SELFTEST) += selftest.o
+
+CONTEXT_ANALYSIS := y
-- 
2.54.0

RE: Bluetooth: enable context analysis

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PASS      3.56 seconds
GitLint                       FAIL      1.79 seconds
SubjectPrefix                 PASS      0.59 seconds
BuildKernel                   PASS      26.34 seconds
CheckAllWarning               PASS      29.76 seconds
CheckSparse                   PASS      30.21 seconds
BuildKernel32                 PASS      28.37 seconds
TestRunnerSetup               PASS      552.61 seconds
TestRunner_l2cap-tester       PASS      379.41 seconds
TestRunner_iso-tester         PASS      591.09 seconds
TestRunner_bnep-tester        PASS      18.71 seconds
TestRunner_mgmt-tester        PASS      2023.25 seconds
TestRunner_rfcomm-tester      PASS      63.64 seconds
TestRunner_sco-tester         PASS      141.94 seconds
TestRunner_ioctl-tester       PASS      133.61 seconds
TestRunner_mesh-tester        PASS      60.34 seconds
TestRunner_smp-tester         PASS      18.18 seconds
TestRunner_userchan-tester    PASS      19.47 seconds
TestRunner_6lowpan-tester     PASS      51.14 seconds
IncrementalBuild              PASS      32.11 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[RFC,3/5] Bluetooth: ISO: lock same hdev as what is released

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
11: B1 Line exceeds max length (87>80): "net/bluetooth/iso.c:1095:2: warning: releasing mutex 'iso_conn_get_hdev(sk->conn).lock'"


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

---
Regards,
Linux Bluetooth