[PATCH 1/2] Bluetooth: L2CAP: avoid maybe-return-locked and add locking annotations

[PATCH 1/2] Bluetooth: L2CAP: avoid maybe-return-locked and add locking annotations

[Pauli Virtanen]

Replace the maybe-return-locked behavior of
l2cap_get_chan_by_scid/dcid() by doing locking in the caller after NULL
check. This allows the static context analysis to handle this.  No
functional change intended.

Add context analysis annotations related to l2cap_chan_lock/unlock().

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

Notes:
    Possibly the l2cap_ops::alloc_skb callback should have generally
    __must_hold(&chan->lock) and not just in l2cap_sock.c
    
    This would imply l2cap_chan_send() should hold the lock, but trying to
    add the annotations reveals l2cap_chan_send is called without chan->lock
    from some places:
    
    net/bluetooth/smp.c:589:2: warning: calling function 'l2cap_chan_send' requires holding mutex 'conn->smp->lock' exclusively [-Wthread-safety-analysis]
      589 |         l2cap_chan_send(chan, &msg, 1 + len, NULL);
          |         ^
    1 warning generated.
    net/bluetooth/6lowpan.c:455:8: warning: calling function 'l2cap_chan_send' requires holding mutex 'chan->lock' exclusively [-Wthread-safety-analysis]
      455 |         err = l2cap_chan_send(chan, &msg, skb->len, NULL);
          |               ^
    
    Don't know now if the context in these places allows l2cap_chan_lock()

 include/net/bluetooth/l2cap.h |  2 ++
 net/bluetooth/l2cap_core.c    | 25 +++++++++++++++++--------
 net/bluetooth/l2cap_sock.c    |  1 +
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index ef6ce1c20a4f..53a68fc32f10 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -825,11 +825,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 538ae9aa3479..ba8e69dfbe11 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)
@@ -118,17 +118,15 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn,
 
 	c = __l2cap_get_chan_by_scid(conn, cid);
 	if (c) {
-		/* Only lock if chan reference is not 0 */
+		/* Only hold if chan reference is not 0 */
 		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)
@@ -137,10 +135,8 @@ static struct l2cap_chan *l2cap_get_chan_by_dcid(struct l2cap_conn *conn,
 
 	c = __l2cap_get_chan_by_dcid(conn, cid);
 	if (c) {
-		/* Only lock if chan reference is not 0 */
+		/* Only hold if chan reference is not 0 */
 		c = l2cap_chan_hold_unless_zero(c);
-		if (c)
-			l2cap_chan_lock(c);
 	}
 
 	return c;
@@ -4086,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;
@@ -4364,6 +4361,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,
@@ -4475,6 +4474,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);
@@ -4581,6 +4582,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);
@@ -4618,6 +4621,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);
@@ -5115,6 +5120,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");
@@ -6974,6 +6981,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
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 735167f73f31..4afc5b370b97 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1740,6 +1740,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 2/2] Bluetooth: enable context analysis for headers

[Pauli Virtanen]

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

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 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: [PATCH 2/2] Bluetooth: enable context analysis for headers

[Bart Van Assche]

On 7/12/26 5:42 AM, Pauli Virtanen wrote:
> Remove context analysis suppression for include/net/bluetooth/*, now
> that previous commits have resolved the warnings.
Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 1/2] Bluetooth: L2CAP: avoid maybe-return-locked and add locking annotations

[Bart Van Assche]

On 7/12/26 5:42 AM, Pauli Virtanen wrote:
> Replace the maybe-return-locked behavior of
> l2cap_get_chan_by_scid/dcid() by doing locking in the caller after NULL
> check. This allows the static context analysis to handle this.  No
> functional change intended.
> 
> Add context analysis annotations related to l2cap_chan_lock/unlock().
As one can see in the above description, this patch includes two
separate changes. Please split this patch into two patches: one patch
with the code refactoring and a second patch that adds the context
analysis annotations.

Thanks,

Bart.