[PATCH 0/4] Bluetooth: L2CAP: part 2 of l2cap_conn::chan_l locking fixes

[PATCH 0/4] Bluetooth: L2CAP: part 2 of l2cap_conn::chan_l locking fixes

[Pauli Virtanen]

Rebase to fix conflict in remaining commits from series
https://lore.kernel.org/linux-bluetooth/cover.1788013041.git.pav@iki.fi/

Fixed minor conflict vs. moved mutex_lock + l2cap_chan_lock in diff
context
commit ddaccd985bb0 ("Bluetooth: L2CAP: fix out-of-bounds write in l2cap_ecred_connect")

No other changes than resolving that.

Pauli Virtanen (4):
  Bluetooth: L2CAP: take lock for l2cap_chan_del in
    l2cap_ecred_rsp_defer
  Bluetooth: L2CAP: annotate locking for l2cap_chan_del()
  Bluetooth: L2CAP: annotate locking for l2cap_ops callbacks
  Bluetooth: L2CAP: refuse __l2cap_chan_add if chan already has conn

 include/net/bluetooth/l2cap.h |  65 +++++++++++--------
 net/bluetooth/6lowpan.c       |   2 +
 net/bluetooth/l2cap_core.c    | 113 ++++++++++++++++++++++++++++++++--
 net/bluetooth/l2cap_sock.c    |   2 +
 4 files changed, 150 insertions(+), 32 deletions(-)

-- 
2.55.0

[PATCH 1/4] Bluetooth: L2CAP: take lock for l2cap_chan_del in l2cap_ecred_rsp_defer

[Pauli Virtanen]

l2cap_ecred_rsp_defer() calls l2cap_chan_del without holding chan->lock,
which ends up calling ops->teardown() with wrong lock context.

Fix by taking chan->lock in l2cap_ecred_rsp_defer().  AB-BA deadlocks
between sibling l2cap_chan are avoided here via requiring l2cap_conn::lock
to serialize all nested l2cap_chan locking on same nesting level.

In current code, there is no nested l2cap_chan locking on same nesting
level, so we can add this new requirement.

Also return early from  __l2cap_ecred_conn_rsp_defer() if chan did not
have FLAG_DEFER_SETUP, as then no RSP shall be sent for it, to make sure
SMP channels are excluded.

Also hold chan reference over l2cap_chan_del(), in case chan_l reference
was the last.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/net/bluetooth/l2cap.h |  4 +++
 net/bluetooth/l2cap_core.c    | 60 +++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 2315a3993c7b..c7e642abe404 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -758,6 +758,10 @@ enum {
  * otherwise considers all channels equal and will e.g. complain about a
  * connection oriented channel triggering SMP procedures or a listening
  * channel creating and locking a child channel.
+ *
+ * Lock nesting of channels at the same nesting level is allowed if the channels
+ * have the same l2cap_chan::conn and l2cap_chan::conn.lock is taken before the
+ * nested locks. l2cap_chan_try_sibling_lock() must be used.
  */
 enum {
 	L2CAP_NESTING_SMP,
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 9da689f0a50a..805623a48bae 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -848,6 +848,8 @@ static void __l2cap_chan_close(struct l2cap_chan *chan, int reason)
 
 	BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
 
+	lockdep_assert_held(&chan->lock);
+
 	switch (chan->state) {
 	case BT_LISTEN:
 		chan->ops->teardown(chan, 0);
@@ -3950,6 +3952,7 @@ static void l2cap_ecred_list_defer(struct l2cap_chan *chan, void *data)
 }
 
 struct l2cap_ecred_rsp_data {
+	struct l2cap_chan *locked_chan;
 	struct {
 		struct l2cap_ecred_conn_rsp_hdr rsp;
 		__le16 scid[L2CAP_ECRED_MAX_CID];
@@ -3957,11 +3960,42 @@ struct l2cap_ecred_rsp_data {
 	int count;
 };
 
+/* Lock @chan if it is not @locked_chan, and has same or lower nesting level.
+ *
+ * They must have the same chan->conn, and conn->lock must be held.
+ *
+ * Caller must ensure @chan has lock nesting level <= that of @locked_chan, as
+ * nested locking of l2cap_chan of different levels is allowed also without
+ * holding conn->lock.
+ *
+ * See l2cap.h for the global l2cap_chan locking rules.
+ */
+static bool l2cap_chan_try_sibling_lock(struct l2cap_chan *chan,
+					struct l2cap_chan *locked_chan)
+	__must_hold(&locked_chan->lock)
+	__must_hold(&locked_chan->conn->lock)
+	__cond_acquires(true, &chan->lock)
+{
+	if (chan == locked_chan)
+		return false;
+
+	if (WARN_ON_ONCE(locked_chan->conn != chan->conn))
+		return false;
+
+	if (WARN_ON_ONCE(atomic_read(&locked_chan->nesting)
+			 < atomic_read(&chan->nesting)))
+		return false;
+
+	mutex_lock_nest_lock(&chan->lock, &locked_chan->conn->lock);
+	return true;
+}
+
 static void l2cap_ecred_rsp_defer(struct l2cap_chan *chan, void *data)
 {
 	struct l2cap_ecred_rsp_data *rsp = data;
 	struct l2cap_ecred_conn_rsp *rsp_flex =
 		container_of(&rsp->pdu.rsp, struct l2cap_ecred_conn_rsp, hdr);
+	bool locked;
 
 	if (chan->mode != L2CAP_MODE_EXT_FLOWCTL)
 		return;
@@ -3973,6 +4007,22 @@ static void l2cap_ecred_rsp_defer(struct l2cap_chan *chan, void *data)
 	    !test_and_clear_bit(FLAG_DEFER_SETUP, &chan->flags))
 		return;
 
+	lockdep_assert_held(&rsp->locked_chan->lock);
+	lockdep_assert_held(&rsp->locked_chan->conn->lock);
+
+	l2cap_chan_hold(chan);
+
+	locked = l2cap_chan_try_sibling_lock(chan, rsp->locked_chan);
+
+	/* Cannot occur: PARENT channels do not appear in chan_l, and SMP
+	 * channels never have FLAG_DEFER_SETUP.
+	 */
+	if (context_unsafe(!locked && chan != rsp->locked_chan))
+		goto done;
+
+	lockdep_assert_held(&chan->lock);
+	lockdep_assert_held(&chan->conn->lock);
+
 	/* Reset ident so only one response is sent */
 	chan->ident = 0;
 
@@ -3985,6 +4035,12 @@ static void l2cap_ecred_rsp_defer(struct l2cap_chan *chan, void *data)
 		rsp_flex->dcid[rsp->count++] = cpu_to_le16(chan->scid);
 	else
 		l2cap_chan_del(chan, ECONNRESET);
+
+done:
+	if (locked)
+		l2cap_chan_unlock(chan);
+
+	l2cap_chan_put(chan);
 }
 
 void __l2cap_ecred_conn_rsp_defer(struct l2cap_chan *chan)
@@ -3996,11 +4052,15 @@ void __l2cap_ecred_conn_rsp_defer(struct l2cap_chan *chan)
 
 	if (!id)
 		return;
+	if (!test_bit(FLAG_DEFER_SETUP, &chan->flags))
+		return;
 
 	BT_DBG("chan %p id %d", chan, id);
 
 	memset(&data, 0, sizeof(data));
 
+	data.locked_chan = chan;
+
 	data.pdu.rsp.mtu     = cpu_to_le16(chan->imtu);
 	data.pdu.rsp.mps     = cpu_to_le16(chan->mps);
 	data.pdu.rsp.credits = cpu_to_le16(chan->rx_credits);
-- 
2.55.0

[PATCH 2/4] Bluetooth: L2CAP: annotate locking for l2cap_chan_del()

[Pauli Virtanen]

Add context analysis annotations for chan->lock and chan->conn->lock
involving l2cap_chan_del() usage.

Add necessary annotations and related lockdep_assert_held to callers.

Move struct l2cap_ops definition after struct l2cap_conn, so that the
callbacks can be annotated.

In l2cap_chan_close_unlocked() we consider chan->conn->lock as locked
even if chan->conn == NULL, to avoid needing to define separate
__l2cap_chan_close/del for this NULL case.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/net/bluetooth/l2cap.h | 56 +++++++++++++++++++----------------
 net/bluetooth/6lowpan.c       |  2 ++
 net/bluetooth/l2cap_core.c    | 49 +++++++++++++++++++++++++-----
 3 files changed, 74 insertions(+), 33 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index c7e642abe404..3e1e2b36d7b6 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -614,31 +614,6 @@ struct l2cap_chan {
 	struct mutex		lock;
 };
 
-struct l2cap_ops {
-	char			*name;
-
-	int			(*new_connection)(struct l2cap_chan *chan,
-						  struct l2cap_chan *new_chan);
-	int			(*recv) (struct l2cap_chan * chan,
-					 struct sk_buff *skb);
-	void			(*teardown) (struct l2cap_chan *chan, int err);
-	void			(*close) (struct l2cap_chan *chan);
-	void			(*state_change) (struct l2cap_chan *chan,
-						 int state, int err);
-	void			(*ready) (struct l2cap_chan *chan);
-	void			(*defer) (struct l2cap_chan *chan);
-	void			(*resume) (struct l2cap_chan *chan);
-	void			(*suspend) (struct l2cap_chan *chan);
-	void			(*set_shutdown) (struct l2cap_chan *chan);
-	long			(*get_sndtimeo) (struct l2cap_chan *chan);
-	struct pid		*(*get_peer_pid) (struct l2cap_chan *chan);
-	struct sk_buff		*(*alloc_skb) (struct l2cap_chan *chan,
-					       unsigned long hdr_len,
-					       unsigned long len, int nb);
-	int			(*filter) (struct l2cap_chan * chan,
-					   struct sk_buff *skb);
-};
-
 struct l2cap_conn {
 	struct hci_conn		*hcon;
 	struct hci_chan		*hchan;
@@ -674,6 +649,34 @@ struct l2cap_conn {
 	struct list_head	users;
 };
 
+struct l2cap_ops {
+	char			*name;
+
+	int			(*new_connection)(struct l2cap_chan *chan,
+						  struct l2cap_chan *new_chan);
+	int			(*recv) (struct l2cap_chan * chan,
+					 struct sk_buff *skb);
+	void			(*teardown) (struct l2cap_chan *chan, int err)
+					__must_hold(&chan->lock);
+	void			(*close) (struct l2cap_chan *chan);
+	void			(*state_change) (struct l2cap_chan *chan,
+						 int state, int err);
+	void			(*ready) (struct l2cap_chan *chan)
+					__must_hold(&chan->lock)
+					__must_hold(&chan->conn->lock);
+	void			(*defer) (struct l2cap_chan *chan);
+	void			(*resume) (struct l2cap_chan *chan);
+	void			(*suspend) (struct l2cap_chan *chan);
+	void			(*set_shutdown) (struct l2cap_chan *chan);
+	long			(*get_sndtimeo) (struct l2cap_chan *chan);
+	struct pid		*(*get_peer_pid) (struct l2cap_chan *chan);
+	struct sk_buff		*(*alloc_skb) (struct l2cap_chan *chan,
+					       unsigned long hdr_len,
+					       unsigned long len, int nb);
+	int			(*filter) (struct l2cap_chan * chan,
+					   struct sk_buff *skb);
+};
+
 struct l2cap_user {
 	struct list_head list;
 	int (*probe) (struct l2cap_conn *conn, struct l2cap_user *user);
@@ -983,7 +986,8 @@ void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 typedef void (*l2cap_chan_func_t)(struct l2cap_chan *chan, void *data);
 void l2cap_chan_list(struct l2cap_conn *conn, l2cap_chan_func_t func,
 		     void *data);
-void l2cap_chan_del(struct l2cap_chan *chan, int err);
+void l2cap_chan_del(struct l2cap_chan *chan, int err)
+	__must_hold(&chan->lock) __must_hold(&chan->conn->lock);
 void l2cap_send_conn_req(struct l2cap_chan *chan);
 
 struct l2cap_conn *l2cap_conn_get(struct l2cap_conn *conn);
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index ddcdd2aff91f..836add41f5d1 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -722,6 +722,8 @@ static int setup_netdev(struct l2cap_chan *chan, struct lowpan_btle_dev **dev)
 }
 
 static inline void chan_ready_cb(struct l2cap_chan *chan)
+	__must_hold(&chan->lock)
+	__must_hold(&chan->conn->lock)
 {
 	struct lowpan_btle_dev *dev;
 	bool new_netdev = false;
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 805623a48bae..219d92431be0 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -59,7 +59,8 @@ static void l2cap_tx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
 static void l2cap_retrans_timeout(struct work_struct *work);
 static void l2cap_monitor_timeout(struct work_struct *work);
 static void l2cap_ack_timeout(struct work_struct *work);
-static void __l2cap_chan_close(struct l2cap_chan *chan, int reason);
+static void __l2cap_chan_close(struct l2cap_chan *chan, int reason)
+	__must_hold(&chan->lock) __must_hold(&chan->conn->lock);
 
 static inline u8 bdaddr_type(u8 link_type, u8 bdaddr_type)
 {
@@ -681,6 +682,8 @@ void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 
 void l2cap_chan_del(struct l2cap_chan *chan, int err)
 {
+	lockdep_assert(!chan->conn || lockdep_is_held(&chan->conn->lock));
+
 	__clear_chan_timer(chan);
 
 	BT_DBG("chan %p, err %d, state %s", chan, err,
@@ -812,12 +815,11 @@ static void l2cap_chan_le_connect_reject(struct l2cap_chan *chan)
 }
 
 static void l2cap_chan_ecred_connect_reject(struct l2cap_chan *chan)
+	__must_hold(&chan->lock)
+	__must_hold(&chan->conn->lock)
 {
 	l2cap_state_change(chan, BT_DISCONN);
 
-	lockdep_assert_held(&chan->lock);
-	lockdep_assert_held(&chan->conn->lock);
-
 	__l2cap_ecred_conn_rsp_defer(chan);
 }
 
@@ -848,8 +850,6 @@ static void __l2cap_chan_close(struct l2cap_chan *chan, int reason)
 
 	BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
 
-	lockdep_assert_held(&chan->lock);
-
 	switch (chan->state) {
 	case BT_LISTEN:
 		chan->ops->teardown(chan, 0);
@@ -934,7 +934,10 @@ void l2cap_chan_close_unlocked(struct l2cap_chan *chan, int reason)
 	bool have_conn;
 
 	have_conn = l2cap_chan_lock_conn(chan);
-	__l2cap_chan_close(chan, reason);
+
+	/* Context analysis: consider chan->conn->lock held also if conn NULL */
+	context_unsafe(__l2cap_chan_close(chan, reason));
+
 	l2cap_chan_unlock_conn(chan, have_conn);
 }
 EXPORT_SYMBOL(l2cap_chan_close_unlocked);
@@ -1336,6 +1339,8 @@ void l2cap_send_conn_req(struct l2cap_chan *chan)
 }
 
 static void l2cap_chan_ready(struct l2cap_chan *chan)
+	__must_hold(&chan->lock)
+	__must_hold(&chan->conn->lock)
 {
 	/* The channel may have already been flagged as connected in
 	 * case of receiving data before the L2CAP info req/rsp
@@ -1471,6 +1476,7 @@ static void l2cap_ecred_connect(struct l2cap_chan *chan)
 }
 
 static void l2cap_le_start(struct l2cap_chan *chan)
+	__must_hold(&chan->lock)
 	__must_hold(&chan->conn->lock)
 {
 	struct l2cap_conn *conn = chan->conn;
@@ -1492,6 +1498,7 @@ static void l2cap_le_start(struct l2cap_chan *chan)
 }
 
 static void l2cap_start_connection(struct l2cap_chan *chan)
+	__must_hold(&chan->lock)
 	__must_hold(&chan->conn->lock)
 {
 	if (chan->conn->hcon->type == LE_LINK) {
@@ -1542,6 +1549,7 @@ static bool l2cap_check_enc_key_size(struct hci_conn *hcon,
 }
 
 static void l2cap_do_start(struct l2cap_chan *chan)
+	__must_hold(&chan->lock)
 	__must_hold(&chan->conn->lock)
 {
 	struct l2cap_conn *conn = chan->conn;
@@ -1893,6 +1901,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 		l2cap_chan_hold(chan);
 		l2cap_chan_lock(chan);
 
+		lockdep_assert_held(&chan->conn->lock);
+
 		l2cap_chan_del(chan, err);
 
 		chan->ops->close(chan);
@@ -4218,6 +4228,8 @@ static struct l2cap_chan *l2cap_new_connection(struct l2cap_conn *conn,
 
 	__l2cap_chan_add(conn, chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	if (pchan->ops->new_connection &&
 	    pchan->ops->new_connection(pchan, chan) < 0) {
 		l2cap_chan_del(chan, 0);
@@ -4419,6 +4431,8 @@ static int l2cap_connect_create_rsp(struct l2cap_conn *conn,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	switch (result) {
 	case L2CAP_CR_SUCCESS:
 		if (__l2cap_get_chan_by_dcid(conn, dcid)) {
@@ -4520,6 +4534,8 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2 &&
 	    chan->state != BT_CONNECTED) {
 		cmd_reject_invalid_cid(conn, cmd->ident, chan->scid,
@@ -4634,6 +4650,8 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	switch (result) {
 	case L2CAP_CONF_SUCCESS:
 		l2cap_conf_rfc_get(chan, rsp->data, len);
@@ -4743,6 +4761,8 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	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);
@@ -4783,6 +4803,8 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	if (chan->state != BT_DISCONN) {
 		l2cap_chan_unlock(chan);
 		l2cap_chan_put(chan);
@@ -4995,6 +5017,8 @@ static int l2cap_le_connect_rsp(struct l2cap_conn *conn,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	switch (result) {
 	case L2CAP_CR_LE_SUCCESS:
 		if (__l2cap_get_chan_by_dcid(conn, dcid)) {
@@ -5213,6 +5237,8 @@ static int l2cap_le_connect_req(struct l2cap_conn *conn,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	bacpy(&chan->src, &conn->hcon->src);
 	bacpy(&chan->dst, &conn->hcon->dst);
 	chan->src_type = bdaddr_src_type(conn->hcon);
@@ -5444,6 +5470,8 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
 
 		l2cap_chan_lock(chan);
 
+		lockdep_assert_held(&chan->conn->lock);
+
 		bacpy(&chan->src, &conn->hcon->src);
 		bacpy(&chan->dst, &conn->hcon->dst);
 		chan->src_type = bdaddr_src_type(conn->hcon);
@@ -5533,6 +5561,8 @@ static inline int l2cap_ecred_conn_rsp(struct l2cap_conn *conn,
 		l2cap_chan_hold(chan);
 		l2cap_chan_lock(chan);
 
+		lockdep_assert_held(&chan->conn->lock);
+
 		/* Check that there is a dcid for each pending channel */
 		if (cmd_len < sizeof(dcid)) {
 			l2cap_chan_del(chan, ECONNREFUSED);
@@ -5760,6 +5790,8 @@ static inline int l2cap_ecred_reconf_rsp(struct l2cap_conn *conn,
 			continue;
 		l2cap_chan_lock(chan);
 
+		lockdep_assert_held(&chan->conn->lock);
+
 		l2cap_chan_del(chan, ECONNRESET);
 
 		l2cap_chan_unlock(chan);
@@ -5789,6 +5821,7 @@ static inline int l2cap_le_command_rej(struct l2cap_conn *conn,
 		goto done;
 
 	l2cap_chan_lock(chan);
+	lockdep_assert_held(&chan->conn->lock);
 	l2cap_chan_del(chan, ECONNREFUSED);
 	l2cap_chan_unlock(chan);
 	l2cap_chan_put(chan);
@@ -7176,6 +7209,8 @@ static void l2cap_data_channel(struct l2cap_conn *conn, u16 cid,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	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 3/4] Bluetooth: L2CAP: annotate locking for l2cap_ops callbacks

[Pauli Virtanen]

Annotate current locking context for l2cap_ops callbacks.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/net/bluetooth/l2cap.h | 13 +++++++++----
 net/bluetooth/l2cap_core.c    |  1 +
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 3e1e2b36d7b6..efb9b7f422d1 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -653,21 +653,26 @@ struct l2cap_ops {
 	char			*name;
 
 	int			(*new_connection)(struct l2cap_chan *chan,
-						  struct l2cap_chan *new_chan);
+						  struct l2cap_chan *new_chan)
+					__must_hold(&chan->lock)
+					__must_hold(&new_chan->lock);
 	int			(*recv) (struct l2cap_chan * chan,
 					 struct sk_buff *skb);
 	void			(*teardown) (struct l2cap_chan *chan, int err)
 					__must_hold(&chan->lock);
-	void			(*close) (struct l2cap_chan *chan);
+	void			(*close) (struct l2cap_chan *chan)
+					__must_hold(&chan->lock);
 	void			(*state_change) (struct l2cap_chan *chan,
 						 int state, int err);
 	void			(*ready) (struct l2cap_chan *chan)
 					__must_hold(&chan->lock)
 					__must_hold(&chan->conn->lock);
 	void			(*defer) (struct l2cap_chan *chan);
-	void			(*resume) (struct l2cap_chan *chan);
+	void			(*resume) (struct l2cap_chan *chan)
+					__must_hold(&chan->lock);
 	void			(*suspend) (struct l2cap_chan *chan);
-	void			(*set_shutdown) (struct l2cap_chan *chan);
+	void			(*set_shutdown) (struct l2cap_chan *chan)
+					__must_hold(&chan->lock);
 	long			(*get_sndtimeo) (struct l2cap_chan *chan);
 	struct pid		*(*get_peer_pid) (struct l2cap_chan *chan);
 	struct sk_buff		*(*alloc_skb) (struct l2cap_chan *chan,
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 219d92431be0..b7d5fa6f6a83 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4214,6 +4214,7 @@ static inline int l2cap_command_rej(struct l2cap_conn *conn,
 static struct l2cap_chan *l2cap_new_connection(struct l2cap_conn *conn,
 					       struct l2cap_chan *pchan)
 	__must_hold(&conn->lock)
+	__must_hold(&pchan->lock)
 {
 	struct l2cap_chan *chan;
 
-- 
2.55.0

[PATCH 4/4] Bluetooth: L2CAP: refuse __l2cap_chan_add if chan already has conn

[Pauli Virtanen]

l2cap_chan may be linked to l2cap_conn at most once. This is assumed in
several places, eg l2cap_chan_del cleanup.

There is a TOCTOU race where the invariant is violated:

    [Task 1]                          [Task 2]
    l2cap_chan_connect                l2cap_sock_bind
      l2cap_chan_lock                   lock_sock
      l2cap_state_change                if (sk->sk_state != BT_OPEN)
        chan->state = BT_CONNECT
        l2cap_sock_state_change_cb      chan->state = BT_BOUND
                                        sk->sk_state = BT_BOUND
          lock_sock <------------------ release_sock
          sk->sk_state = BT_CONNECT

l2cap_sock_connect() does not check sk->sk_state, so since chan->state
is now BT_BOUND, subsequent connect() ends up with second
__l2cap_chan_add.

Explicitly document and check the invariant in __l2cap_chan_add with
WARN_ON_ONCE. The only callsite where it could be hit is
l2cap_chan_connect, so add pre-check there to avoid relying on
chan->state. chan->state read/write is not properly guarded currently so
there can be other TOCTOUC problems.

Add l2cap_lock_chan in l2cap_sock_bind() to guard chan->state write.

Fixes: b66774b48dd9 ("Bluetooth: L2CAP: Fix UAF in channel timeout by holding conn ref")
Assisted-by: deepseek-4-flash # finding the race condition
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/l2cap_core.c | 7 ++++++-
 net/bluetooth/l2cap_sock.c | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b7d5fa6f6a83..a22edd2baf5e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -623,6 +623,10 @@ void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 	BT_DBG("conn %p, psm 0x%2.2x, dcid 0x%4.4x", conn,
 	       __le16_to_cpu(chan->psm), chan->dcid);
 
+	/* Caller must ensure l2cap_chan is linked to l2cap_conn only once */
+	if (WARN_ON_ONCE(chan->conn || test_bit(FLAG_DEL, &chan->flags)))
+		return;
+
 	conn->disc_reason = HCI_ERROR_REMOTE_USER_TERM;
 
 	chan->conn = l2cap_conn_get(conn);
@@ -7623,7 +7627,8 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 		}
 	}
 
-	if (cid && __l2cap_get_chan_by_dcid(conn, cid)) {
+	if ((cid && __l2cap_get_chan_by_dcid(conn, cid)) || chan->conn ||
+	    test_bit(FLAG_DEL, &chan->flags)) {
 		hci_conn_drop(hcon);
 		err = -EBUSY;
 		goto chan_unlock;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index dee3025f0ec2..278adb05c4c9 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -109,6 +109,7 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr_unsized *addr, i
 			return -EINVAL;
 	}
 
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
 
 	if (sk->sk_state != BT_OPEN) {
@@ -174,6 +175,7 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr_unsized *addr, i
 
 done:
 	release_sock(sk);
+	l2cap_chan_unlock(chan);
 	return err;
 }
 
-- 
2.55.0

Re: [PATCH 0/4] Bluetooth: L2CAP: part 2 of l2cap_conn::chan_l locking fixes

[Pauli Virtanen]

ke, 2026-09-02 kello 00:04 +0300, Pauli Virtanen kirjoitti:
> Rebase to fix conflict in remaining commits from series
> https://lore.kernel.org/linux-bluetooth/cover.1788013041.git.pav@iki.fi/
> 
> Fixed minor conflict vs. moved mutex_lock + l2cap_chan_lock in diff
> context
> commit ddaccd985bb0 ("Bluetooth: L2CAP: fix out-of-bounds write in l2cap_ecred_connect")
> 
> No other changes than resolving that.
> 
> Pauli Virtanen (4):
>   Bluetooth: L2CAP: take lock for l2cap_chan_del in
>     l2cap_ecred_rsp_defer
>   Bluetooth: L2CAP: annotate locking for l2cap_chan_del()
>   Bluetooth: L2CAP: annotate locking for l2cap_ops callbacks

Sashiko report comments that some callbacks in functions in l2cap_ops
are not marked with __must_hold(chan->lock) annotation.

That comment is not correct, e.g. l2cap_chan_send is called from
6lowpan.c and smp.c without holding chan->lock, so the annotations
cannot be added, it'd need solving locking issues first.

>   Bluetooth: L2CAP: refuse __l2cap_chan_add if chan already has conn
> 
>  include/net/bluetooth/l2cap.h |  65 +++++++++++--------
>  net/bluetooth/6lowpan.c       |   2 +
>  net/bluetooth/l2cap_core.c    | 113 ++++++++++++++++++++++++++++++++--
>  net/bluetooth/l2cap_sock.c    |   2 +
>  4 files changed, 150 insertions(+), 32 deletions(-)

-- 
Pauli Virtanen

RE: Bluetooth: L2CAP: part 2 of l2cap_conn::chan_l locking fixes

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    FAIL      4.93 seconds
VerifyFixes                   PASS      0.14 seconds
VerifySignedoff               PASS      0.14 seconds
GitLint                       PASS      1.38 seconds
SubjectPrefix                 PASS      0.52 seconds
BuildKernel                   PASS      26.12 seconds
CheckAllWarning               PASS      28.68 seconds
CheckSparse                   PASS      27.83 seconds
BuildKernel32                 PASS      25.41 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      465.23 seconds
TestRunner_l2cap-tester       PASS      65.20 seconds
TestRunner_6lowpan-tester     PASS      23.15 seconds
IncrementalBuild              PASS      40.54 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[2/4] Bluetooth: L2CAP: annotate locking for l2cap_chan_del()
ERROR: "foo * bar" should be "foo *bar"
#154: FILE: include/net/bluetooth/l2cap.h:657:
+	int			(*recv) (struct l2cap_chan * chan,

WARNING: Unnecessary space before function pointer arguments
#154: FILE: include/net/bluetooth/l2cap.h:657:
+	int			(*recv) (struct l2cap_chan * chan,

WARNING: Unnecessary space before function pointer arguments
#156: FILE: include/net/bluetooth/l2cap.h:659:
+	void			(*teardown) (struct l2cap_chan *chan, int err)

WARNING: Unnecessary space before function pointer arguments
#158: FILE: include/net/bluetooth/l2cap.h:661:
+	void			(*close) (struct l2cap_chan *chan);

WARNING: Unnecessary space before function pointer arguments
#159: FILE: include/net/bluetooth/l2cap.h:662:
+	void			(*state_change) (struct l2cap_chan *chan,

WARNING: Unnecessary space before function pointer arguments
#161: FILE: include/net/bluetooth/l2cap.h:664:
+	void			(*ready) (struct l2cap_chan *chan)

WARNING: Unnecessary space before function pointer arguments
#164: FILE: include/net/bluetooth/l2cap.h:667:
+	void			(*defer) (struct l2cap_chan *chan);

WARNING: Unnecessary space before function pointer arguments
#165: FILE: include/net/bluetooth/l2cap.h:668:
+	void			(*resume) (struct l2cap_chan *chan);

WARNING: Unnecessary space before function pointer arguments
#166: FILE: include/net/bluetooth/l2cap.h:669:
+	void			(*suspend) (struct l2cap_chan *chan);

WARNING: Unnecessary space before function pointer arguments
#167: FILE: include/net/bluetooth/l2cap.h:670:
+	void			(*set_shutdown) (struct l2cap_chan *chan);

WARNING: Unnecessary space before function pointer arguments
#168: FILE: include/net/bluetooth/l2cap.h:671:
+	long			(*get_sndtimeo) (struct l2cap_chan *chan);

WARNING: Unnecessary space before function pointer arguments
#169: FILE: include/net/bluetooth/l2cap.h:672:
+	struct pid		*(*get_peer_pid) (struct l2cap_chan *chan);

WARNING: Unnecessary space before function pointer arguments
#170: FILE: include/net/bluetooth/l2cap.h:673:
+	struct sk_buff		*(*alloc_skb) (struct l2cap_chan *chan,

ERROR: "foo * bar" should be "foo *bar"
#173: FILE: include/net/bluetooth/l2cap.h:676:
+	int			(*filter) (struct l2cap_chan * chan,

WARNING: Unnecessary space before function pointer arguments
#173: FILE: include/net/bluetooth/l2cap.h:676:
+	int			(*filter) (struct l2cap_chan * chan,

total: 2 errors, 13 warnings, 0 checks, 272 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14782632.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[3/4] Bluetooth: L2CAP: annotate locking for l2cap_ops callbacks
WARNING: Unnecessary space before function pointer arguments
#116: FILE: include/net/bluetooth/l2cap.h:663:
+	void			(*close) (struct l2cap_chan *chan)

WARNING: Unnecessary space before function pointer arguments
#125: FILE: include/net/bluetooth/l2cap.h:671:
+	void			(*resume) (struct l2cap_chan *chan)

WARNING: Unnecessary space before function pointer arguments
#129: FILE: include/net/bluetooth/l2cap.h:674:
+	void			(*set_shutdown) (struct l2cap_chan *chan)

total: 0 errors, 3 warnings, 0 checks, 37 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14782634.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[4/4] Bluetooth: L2CAP: refuse __l2cap_chan_add if chan already has conn
WARNING: Assisted-by expects 'AGENT_NAME:MODEL_VERSION [TOOL1] [TOOL2]' format
#119: 
Assisted-by: deepseek-4-flash # finding the race condition

total: 0 errors, 1 warnings, 0 checks, 33 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14782635.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found


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

---
Regards,
Linux Bluetooth

Re: [PATCH 0/4] Bluetooth: L2CAP: part 2 of l2cap_conn::chan_l locking fixes

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Wed,  2 Sep 2026 00:04:32 +0300 you wrote:
> Rebase to fix conflict in remaining commits from series
> https://lore.kernel.org/linux-bluetooth/cover.1788013041.git.pav@iki.fi/
> 
> Fixed minor conflict vs. moved mutex_lock + l2cap_chan_lock in diff
> context
> commit ddaccd985bb0 ("Bluetooth: L2CAP: fix out-of-bounds write in l2cap_ecred_connect")
> 
> [...]

Here is the summary with links:
  - [1/4] Bluetooth: L2CAP: take lock for l2cap_chan_del in l2cap_ecred_rsp_defer
    https://git.kernel.org/bluetooth/bluetooth-next/c/c6d60c24cd7b
  - [2/4] Bluetooth: L2CAP: annotate locking for l2cap_chan_del()
    https://git.kernel.org/bluetooth/bluetooth-next/c/6873eb51dcdd
  - [3/4] Bluetooth: L2CAP: annotate locking for l2cap_ops callbacks
    https://git.kernel.org/bluetooth/bluetooth-next/c/4149ba2a806c
  - [4/4] Bluetooth: L2CAP: refuse __l2cap_chan_add if chan already has conn
    https://git.kernel.org/bluetooth/bluetooth-next/c/6696072ffe07

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html