[PATCH RESEND v2 1/3] Bluetooth: L2CAP: avoid maybe-return-locked in l2cap_get_chan_by_scid/dcid

[PATCH RESEND v2 1/3] Bluetooth: L2CAP: avoid maybe-return-locked in l2cap_get_chan_by_scid/dcid

[Pauli Virtanen]

Replace the maybe-return-locked pattern in l2cap_get_chan_by_scid/dcid()
by doing locking in the caller after NULL check. This allows adding
context analysis annotations for the locking.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    resend:
    - no changes
    
    v2:
    - split code changes and adding annotations to separate patches
    - remove self-evident comment

 net/bluetooth/l2cap_core.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index ee459dd411f5..df41ef952500 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -109,7 +109,7 @@ static struct l2cap_chan *__l2cap_get_chan_by_scid(struct l2cap_conn *conn,
 }
 
 /* Find channel with given SCID.
- * Returns a reference locked channel.
+ * Returns a reference.
  */
 static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn,
 						 u16 cid)
@@ -117,18 +117,14 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn,
 	struct l2cap_chan *c;
 
 	c = __l2cap_get_chan_by_scid(conn, cid);
-	if (c) {
-		/* Only lock if chan reference is not 0 */
+	if (c)
 		c = l2cap_chan_hold_unless_zero(c);
-		if (c)
-			l2cap_chan_lock(c);
-	}
 
 	return c;
 }
 
 /* Find channel with given DCID.
- * Returns a reference locked channel.
+ * Returns a reference.
  */
 static struct l2cap_chan *l2cap_get_chan_by_dcid(struct l2cap_conn *conn,
 						 u16 cid)
@@ -136,12 +132,8 @@ static struct l2cap_chan *l2cap_get_chan_by_dcid(struct l2cap_conn *conn,
 	struct l2cap_chan *c;
 
 	c = __l2cap_get_chan_by_dcid(conn, cid);
-	if (c) {
-		/* Only lock if chan reference is not 0 */
+	if (c)
 		c = l2cap_chan_hold_unless_zero(c);
-		if (c)
-			l2cap_chan_lock(c);
-	}
 
 	return c;
 }
@@ -4368,6 +4360,8 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
 		return 0;
 	}
 
+	l2cap_chan_lock(chan);
+
 	if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2 &&
 	    chan->state != BT_CONNECTED) {
 		cmd_reject_invalid_cid(conn, cmd->ident, chan->scid,
@@ -4479,6 +4473,8 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn,
 	if (!chan)
 		return 0;
 
+	l2cap_chan_lock(chan);
+
 	switch (result) {
 	case L2CAP_CONF_SUCCESS:
 		l2cap_conf_rfc_get(chan, rsp->data, len);
@@ -4585,6 +4581,8 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn,
 		return 0;
 	}
 
+	l2cap_chan_lock(chan);
+
 	rsp.dcid = cpu_to_le16(chan->scid);
 	rsp.scid = cpu_to_le16(chan->dcid);
 	l2cap_send_cmd(conn, cmd->ident, L2CAP_DISCONN_RSP, sizeof(rsp), &rsp);
@@ -4622,6 +4620,8 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn,
 		return 0;
 	}
 
+	l2cap_chan_lock(chan);
+
 	if (chan->state != BT_DISCONN) {
 		l2cap_chan_unlock(chan);
 		l2cap_chan_put(chan);
@@ -5124,6 +5124,8 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
 	if (!chan)
 		return -EBADSLT;
 
+	l2cap_chan_lock(chan);
+
 	max_credits = LE_FLOWCTL_MAX_CREDITS - chan->tx_credits;
 	if (credits > max_credits) {
 		BT_ERR("LE credits overflow");
@@ -6983,6 +6985,8 @@ static void l2cap_data_channel(struct l2cap_conn *conn, u16 cid,
 		return;
 	}
 
+	l2cap_chan_lock(chan);
+
 	BT_DBG("chan %p, len %d", chan, skb->len);
 
 	/* If we receive data on a fixed channel before the info req/rsp
-- 
2.55.0

[PATCH RESEND v2 2/3] Bluetooth: L2CAP: add locking annotations for l2cap_chan_lock/unlock

[Pauli Virtanen]

Add minimal context analysis annotations to l2cap_chan_lock/unlock() and
callers required for no warnings.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    resend:
    - no changes
    
    v2:
    - split code changes and adding annotations to separate patches

 include/net/bluetooth/l2cap.h | 2 ++
 net/bluetooth/l2cap_core.c    | 1 +
 net/bluetooth/l2cap_sock.c    | 1 +
 3 files changed, 4 insertions(+)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 3d9a32094347..69d193fee351 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -830,11 +830,13 @@ struct l2cap_chan *l2cap_chan_hold_unless_zero(struct l2cap_chan *c);
 void l2cap_chan_put(struct l2cap_chan *c);
 
 static inline void l2cap_chan_lock(struct l2cap_chan *chan)
+	__acquires(&chan->lock)
 {
 	mutex_lock_nested(&chan->lock, atomic_read(&chan->nesting));
 }
 
 static inline void l2cap_chan_unlock(struct l2cap_chan *chan)
+	__releases(&chan->lock)
 {
 	mutex_unlock(&chan->lock);
 }
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index df41ef952500..358b11eabd4f 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4082,6 +4082,7 @@ static struct l2cap_chan *l2cap_new_connection(struct l2cap_conn *conn,
 
 static void l2cap_connect(struct l2cap_conn *conn, struct l2cap_cmd_hdr *cmd,
 			  u8 *data, u8 rsp_code)
+	__context_unsafe(/* conditional locking */)
 {
 	struct l2cap_conn_req *req = (struct l2cap_conn_req *) data;
 	struct l2cap_conn_rsp rsp;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 1194c37e466f..b553b6356af8 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1784,6 +1784,7 @@ static void l2cap_sock_state_change_cb(struct l2cap_chan *chan, int state,
 static struct sk_buff *l2cap_sock_alloc_skb_cb(struct l2cap_chan *chan,
 					       unsigned long hdr_len,
 					       unsigned long len, int nb)
+	__must_hold(&chan->lock)
 {
 	struct sock *sk = chan->data;
 	struct sk_buff *skb;
-- 
2.55.0

[PATCH RESEND v2 3/3] Bluetooth: enable context analysis for headers

[Pauli Virtanen]

Remove context analysis suppression for include/net/bluetooth/*, now
that previous commits have resolved the warnings.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    resend:
    - no changes
    
    v2:
    - no change

 scripts/context-analysis-suppression.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/context-analysis-suppression.txt b/scripts/context-analysis-suppression.txt
index 1c51b6153f08..d4476d9ed10a 100644
--- a/scripts/context-analysis-suppression.txt
+++ b/scripts/context-analysis-suppression.txt
@@ -32,3 +32,4 @@ src:*include/linux/seqlock*.h=emit
 src:*include/linux/spinlock*.h=emit
 src:*include/linux/srcu*.h=emit
 src:*include/linux/ww_mutex.h=emit
+src:*include/net/bluetooth/*=emit
-- 
2.55.0

RE: [RESEND,v2,1/3] Bluetooth: L2CAP: avoid maybe-return-locked in l2cap_get_chan_by_scid/dcid

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PASS      2.57 seconds
VerifyFixes                   PASS      0.16 seconds
VerifySignedoff               PASS      0.11 seconds
GitLint                       FAIL      0.96 seconds
SubjectPrefix                 PASS      0.28 seconds
BuildKernel                   PASS      27.52 seconds
CheckAllWarning               PASS      29.83 seconds
CheckSparse                   PASS      28.43 seconds
BuildKernel32                 PASS      26.06 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      499.40 seconds
TestRunner_l2cap-tester       PASS      63.93 seconds
TestRunner_iso-tester         PASS      83.06 seconds
TestRunner_bnep-tester        PASS      19.55 seconds
TestRunner_mgmt-tester        FAIL      222.28 seconds
TestRunner_rfcomm-tester      PASS      25.22 seconds
TestRunner_sco-tester         PASS      31.94 seconds
TestRunner_ioctl-tester       PASS      26.15 seconds
TestRunner_mesh-tester        FAIL      25.96 seconds
TestRunner_smp-tester         PASS      23.44 seconds
TestRunner_userchan-tester    PASS      20.01 seconds
TestRunner_6lowpan-tester     PASS      30.89 seconds
IncrementalBuild              PASS      35.40 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[RESEND,v2,1/3] Bluetooth: L2CAP: avoid maybe-return-locked in l2cap_get_chan_by_scid/dcid

1: T1 Title exceeds max length (90>80): "[RESEND,v2,1/3] Bluetooth: L2CAP: avoid maybe-return-locked in l2cap_get_chan_by_scid/dcid"
14: B2 Line has trailing whitespace: "    "
[RESEND,v2,2/3] Bluetooth: L2CAP: add locking annotations for l2cap_chan_lock/unlock

1: T1 Title exceeds max length (84>80): "[RESEND,v2,2/3] Bluetooth: L2CAP: add locking annotations for l2cap_chan_lock/unlock"
13: B2 Line has trailing whitespace: "    "
[RESEND,v2,3/3] Bluetooth: enable context analysis for headers

13: B2 Line has trailing whitespace: "    "
##############################
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.244 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    2.004 seconds
Mesh - Send cancel - 2                               Timed out    1.987 seconds


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

---
Regards,
Linux Bluetooth