[PATCH 0/9] Bluetooth: More generic L2CAP fixed channel handling

[PATCH 0/9] Bluetooth: More generic L2CAP fixed channel handling

[johan.hedberg]

Hi,

Here's a set of patches to make fixed L2CAP channel handling more
generic. The main special-casing that's removed here is for ATT
channels. The A2MP stuff is left for later since its semantics would
change from creating the context only when there's data on the channel
to as soon as we have a baseband link (which I'm not sure is desirable).

The main motivation for all of this work is to pave the way to convert
SMP to fully use the l2cap_chan infrastructure.

Johan

----------------------------------------------------------------
Johan Hedberg (9):
      Bluetooth: Fix reference counting of global L2CAP channels
      Bluetooth: Fix __l2cap_no_conn_pending() usage with all channels
      Bluetooth: Resume BT_CONNECTED state after LE security elevation
      Bluetooth: Remove special handling of ATT in l2cap_security_cfm()
      Bluetooth: Refactor l2cap_connect_cfm
      Bluetooth: Move L2CAP fixed channel creation into l2cap_conn_cfm
      Bluetooth: Improve fixed channel lookup based on link type
      Bluetooth: Remove special ATT data channel handling
      Bluetooth: Move parts of fixed channel initialization to l2cap_add_scid

 include/net/bluetooth/l2cap.h |   1 +
 net/bluetooth/l2cap_core.c    | 244 ++++++++++++++++++--------------------
 net/bluetooth/l2cap_sock.c    |  15 +--

[PATCH 1/9] Bluetooth: Fix reference counting of global L2CAP channels

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

When looking up entries from the global L2CAP channel list there needs
to be a guarantee that other code doesn't go and remove the entry after
a channel has been returned by the lookup function. This patch makes
sure that the channel reference is incremented before the read lock is
released in the global channel lookup functions. The patch also adds the
corresponding l2cap_chan_put() calls once the channels pointers are
no-longer needed.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/l2cap_core.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 46547b920f88..a49e9646a6b9 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1440,6 +1440,7 @@ static struct l2cap_chan *l2cap_global_chan_by_scid(int state, u16 cid,
 			src_match = !bacmp(&c->src, src);
 			dst_match = !bacmp(&c->dst, dst);
 			if (src_match && dst_match) {
+				l2cap_chan_hold(c);
 				read_unlock(&chan_list_lock);
 				return c;
 			}
@@ -1453,6 +1454,9 @@ static struct l2cap_chan *l2cap_global_chan_by_scid(int state, u16 cid,
 		}
 	}
 
+	if (c1)
+		l2cap_chan_hold(c1);
+
 	read_unlock(&chan_list_lock);
 
 	return c1;
@@ -1475,13 +1479,13 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 
 	/* Client ATT sockets should override the server one */
 	if (__l2cap_get_chan_by_dcid(conn, L2CAP_CID_ATT))
-		return;
+		goto put;
 
 	dst_type = bdaddr_type(hcon, hcon->dst_type);
 
 	/* If device is blocked, do not create a channel for it */
 	if (hci_bdaddr_list_lookup(&hdev->blacklist, &hcon->dst, dst_type))
-		return;
+		goto put;
 
 	/* For LE slave connections, make sure the connection interval
 	 * is in the range of the minium and maximum interval that has
@@ -1517,6 +1521,8 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 
 clean:
 	l2cap_chan_unlock(pchan);
+put:
+	l2cap_chan_put(pchan);
 }
 
 static void l2cap_conn_ready(struct l2cap_conn *conn)
@@ -1794,6 +1800,7 @@ static struct l2cap_chan *l2cap_global_chan_by_psm(int state, __le16 psm,
 			src_match = !bacmp(&c->src, src);
 			dst_match = !bacmp(&c->dst, dst);
 			if (src_match && dst_match) {
+				l2cap_chan_hold(c);
 				read_unlock(&chan_list_lock);
 				return c;
 			}
@@ -1807,6 +1814,9 @@ static struct l2cap_chan *l2cap_global_chan_by_psm(int state, __le16 psm,
 		}
 	}
 
+	if (c1)
+		l2cap_chan_hold(c1);
+
 	read_unlock(&chan_list_lock);
 
 	return c1;
@@ -3884,6 +3894,7 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
 response:
 	l2cap_chan_unlock(pchan);
 	mutex_unlock(&conn->chan_lock);
+	l2cap_chan_put(pchan);
 
 sendresp:
 	rsp.scid   = cpu_to_le16(scid);
@@ -5497,6 +5508,7 @@ static int l2cap_le_connect_req(struct l2cap_conn *conn,
 response_unlock:
 	l2cap_chan_unlock(pchan);
 	mutex_unlock(&conn->chan_lock);
+	l2cap_chan_put(pchan);
 
 	if (result == L2CAP_CR_PEND)
 		return 0;
@@ -6844,8 +6856,10 @@ static void l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm,
 	struct hci_conn *hcon = conn->hcon;
 	struct l2cap_chan *chan;
 
-	if (hcon->type != ACL_LINK)
+	if (hcon->type != ACL_LINK) {
+		chan = NULL;
 		goto drop;
+	}
 
 	chan = l2cap_global_chan_by_psm(0, psm, &hcon->src, &hcon->dst,
 					ACL_LINK);
@@ -6865,10 +6879,13 @@ static void l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm,
 	bt_cb(skb)->psm = psm;
 
 	if (!chan->ops->recv(chan, skb))
-		return;
+		goto put;
 
 drop:
 	kfree_skb(skb);
+put:
+	if (chan)
+		l2cap_chan_put(chan);
 }
 
 static void l2cap_att_channel(struct l2cap_conn *conn,
@@ -6877,8 +6894,10 @@ static void l2cap_att_channel(struct l2cap_conn *conn,
 	struct hci_conn *hcon = conn->hcon;
 	struct l2cap_chan *chan;
 
-	if (hcon->type != LE_LINK)
+	if (hcon->type != LE_LINK) {
+		chan = NULL;
 		goto drop;
+	}
 
 	chan = l2cap_global_chan_by_scid(BT_CONNECTED, L2CAP_CID_ATT,
 					 &hcon->src, &hcon->dst);
@@ -6891,10 +6910,13 @@ static void l2cap_att_channel(struct l2cap_conn *conn,
 		goto drop;
 
 	if (!chan->ops->recv(chan, skb))
-		return;
+		goto put;
 
 drop:
 	kfree_skb(skb);
+put:
+	if (chan)
+		l2cap_chan_put(chan);
 }
 
 static void l2cap_recv_frame(struct l2cap_conn *conn, struct sk_buff *skb)
-- 
1.9.3

[PATCH 2/9] Bluetooth: Fix __l2cap_no_conn_pending() usage with all channels

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

The __l2cap_no_conn_pending() function would previously only return a
meaningful value for connection oriented channels and was therefore not
useful for anything else. As preparation of making the L2CAP code more
generic allow the function to be called for other channel types as well
by returning a meaningful value for them.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/l2cap_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index a49e9646a6b9..887f030beb5b 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1082,6 +1082,9 @@ static void l2cap_send_rr_or_rnr(struct l2cap_chan *chan, bool poll)
 
 static inline int __l2cap_no_conn_pending(struct l2cap_chan *chan)
 {
+	if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED)
+		return true;
+
 	return !test_bit(CONF_CONNECT_PEND, &chan->conf_state);
 }
 
-- 
1.9.3

[PATCH 3/9] Bluetooth: Resume BT_CONNECTED state after LE security elevation

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

The LE ATT socket uses a special trick where it temporarily sets
BT_CONFIG state for the duration of a security level elevation. In order
to not require special hacks for going back to BT_CONNECTED state in the
l2cap_core.c code the most reasonable place to resume the state is the
resume callback. This patch adds a new flag to track the pending
security level change and ensures that the state is set back to
BT_CONNECTED in the resume callback in case the flag is set.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/l2cap.h | 1 +
 net/bluetooth/l2cap_sock.c    | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 8df15ad0d43f..4a51e7596608 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -708,6 +708,7 @@ enum {
 	FLAG_EFS_ENABLE,
 	FLAG_DEFER_SETUP,
 	FLAG_LE_CONN_REQ_SENT,
+	FLAG_PENDING_SECURITY,
 };
 
 enum {
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 1884f72083c2..5a42f6a818c0 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -790,6 +790,7 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
 		if (chan->scid == L2CAP_CID_ATT) {
 			if (smp_conn_security(conn->hcon, sec.level))
 				break;
+			set_bit(FLAG_PENDING_SECURITY, &chan->flags);
 			sk->sk_state = BT_CONFIG;
 			chan->state = BT_CONFIG;
 
@@ -1359,6 +1360,11 @@ static void l2cap_sock_resume_cb(struct l2cap_chan *chan)
 {
 	struct sock *sk = chan->data;
 
+	if (test_and_clear_bit(FLAG_PENDING_SECURITY, &chan->flags)) {
+		sk->sk_state = BT_CONNECTED;
+		chan->state = BT_CONNECTED;
+	}
+
 	clear_bit(BT_SK_SUSPEND, &bt_sk(sk)->flags);
 	sk->sk_state_change(sk);
 }
-- 
1.9.3

[PATCH 4/9] Bluetooth: Remove special handling of ATT in l2cap_security_cfm()

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

With the update to sk->resume() and __l2cap_no_conn_pending() we
no-longer need to have special handling of ATT channels in the
l2cap_security_cfm() function. The chan->sec_level update when
encryption has been enabled is safe to do for any kind of channel, and
the loop takes later care of calling chan->ready() or chan->resume() if
necessary.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/l2cap_core.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 887f030beb5b..ff66d14cfd34 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -7343,15 +7343,8 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 			continue;
 		}
 
-		if (chan->scid == L2CAP_CID_ATT) {
-			if (!status && encrypt) {
-				chan->sec_level = hcon->sec_level;
-				l2cap_chan_ready(chan);
-			}
-
-			l2cap_chan_unlock(chan);
-			continue;
-		}
+		if (!status && encrypt)
+			chan->sec_level = hcon->sec_level;
 
 		if (!__l2cap_no_conn_pending(chan)) {
 			l2cap_chan_unlock(chan);
-- 
1.9.3

[PATCH 5/9] Bluetooth: Refactor l2cap_connect_cfm

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

This patch is a simple refactoring of l2cap_connect_cfm to allow easier
extension of the function.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/l2cap_core.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index ff66d14cfd34..304fa444efd3 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -7270,13 +7270,16 @@ void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
 
 	BT_DBG("hcon %p bdaddr %pMR status %d", hcon, &hcon->dst, status);
 
-	if (!status) {
-		conn = l2cap_conn_add(hcon);
-		if (conn)
-			l2cap_conn_ready(conn);
-	} else {
+	if (status) {
 		l2cap_conn_del(hcon, bt_to_errno(status));
+		return;
 	}
+
+	conn = l2cap_conn_add(hcon);
+	if (!conn)
+		return;
+
+	l2cap_conn_ready(conn);
 }
 
 int l2cap_disconn_ind(struct hci_conn *hcon)
-- 
1.9.3

[PATCH 6/9] Bluetooth: Move L2CAP fixed channel creation into l2cap_conn_cfm

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

In order to remove special handling of fixed L2CAP channels we need to
start creating them in a single place instead of having per-channel
exceptions. The most natural place is the l2cap_conn_cfm() function
which is called whenever there is a new baseband link.

The only really special case so far has been the ATT socket, so in order
not to break the code in between this patch removes the ATT special
handling at the same time as it adds the generic fixed channel handling
from l2cap_le_conn_ready() into the hci_conn_cfm() function. As a
related change the channel locking in l2cap_conn_ready() becomes simpler
and we can thereby move the smp_conn_security() call into the
l2cap_le_conn_ready() function.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/l2cap_core.c | 124 +++++++++++++++++++++++++++++----------------
 1 file changed, 80 insertions(+), 44 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 304fa444efd3..197b811e6206 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1469,26 +1469,14 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 {
 	struct hci_conn *hcon = conn->hcon;
 	struct hci_dev *hdev = hcon->hdev;
-	struct l2cap_chan *chan, *pchan;
-	u8 dst_type;
 
-	BT_DBG("");
-
-	/* Check if we have socket listening on cid */
-	pchan = l2cap_global_chan_by_scid(BT_LISTEN, L2CAP_CID_ATT,
-					  &hcon->src, &hcon->dst);
-	if (!pchan)
-		return;
-
-	/* Client ATT sockets should override the server one */
-	if (__l2cap_get_chan_by_dcid(conn, L2CAP_CID_ATT))
-		goto put;
-
-	dst_type = bdaddr_type(hcon, hcon->dst_type);
+	BT_DBG("%s conn %p", hdev->name, conn);
 
-	/* If device is blocked, do not create a channel for it */
-	if (hci_bdaddr_list_lookup(&hdev->blacklist, &hcon->dst, dst_type))
-		goto put;
+	/* For outgoing pairing which doesn't necessarily have an
+	 * associated socket (e.g. mgmt_pair_device).
+	 */
+	if (hcon->out)
+		smp_conn_security(hcon, hcon->pending_sec_level);
 
 	/* For LE slave connections, make sure the connection interval
 	 * is in the range of the minium and maximum interval that has
@@ -1508,24 +1496,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 		l2cap_send_cmd(conn, l2cap_get_ident(conn),
 			       L2CAP_CONN_PARAM_UPDATE_REQ, sizeof(req), &req);
 	}
-
-	l2cap_chan_lock(pchan);
-
-	chan = pchan->ops->new_connection(pchan);
-	if (!chan)
-		goto clean;
-
-	bacpy(&chan->src, &hcon->src);
-	bacpy(&chan->dst, &hcon->dst);
-	chan->src_type = bdaddr_type(hcon, hcon->src_type);
-	chan->dst_type = dst_type;
-
-	__l2cap_chan_add(conn, chan);
-
-clean:
-	l2cap_chan_unlock(pchan);
-put:
-	l2cap_chan_put(pchan);
 }
 
 static void l2cap_conn_ready(struct l2cap_conn *conn)
@@ -1535,17 +1505,11 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
 
 	BT_DBG("conn %p", conn);
 
-	/* For outgoing pairing which doesn't necessarily have an
-	 * associated socket (e.g. mgmt_pair_device).
-	 */
-	if (hcon->out && hcon->type == LE_LINK)
-		smp_conn_security(hcon, hcon->pending_sec_level);
-
-	mutex_lock(&conn->chan_lock);
-
 	if (hcon->type == LE_LINK)
 		l2cap_le_conn_ready(conn);
 
+	mutex_lock(&conn->chan_lock);
+
 	list_for_each_entry(chan, &conn->chan_l, list) {
 
 		l2cap_chan_lock(chan);
@@ -7264,9 +7228,44 @@ int l2cap_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr)
 	return exact ? lm1 : lm2;
 }
 
+/* Find the next fixed channel in BT_LISTEN state, continue iteration
+ * from an existing channel in the list or from the beginning of the
+ * global list (by passing NULL as first parameter).
+ */
+static struct l2cap_chan *l2cap_global_fixed_chan(struct l2cap_chan *c,
+						  bdaddr_t *src)
+{
+	read_lock(&chan_list_lock);
+
+	if (c)
+		c = list_next_entry(c, global_l);
+	else
+		c = list_entry(chan_list.next, typeof(*c), global_l);
+
+	list_for_each_entry_from(c, &chan_list, global_l) {
+		if (c->chan_type != L2CAP_CHAN_FIXED)
+			continue;
+		if (c->state != BT_LISTEN)
+			continue;
+		if (bacmp(&c->src, src) && bacmp(&c->src, BDADDR_ANY))
+			continue;
+
+		l2cap_chan_hold(c);
+		read_unlock(&chan_list_lock);
+		return c;
+	}
+
+	read_unlock(&chan_list_lock);
+
+	return NULL;
+}
+
 void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
 {
+	struct hci_dev *hdev = hcon->hdev;
 	struct l2cap_conn *conn;
+	struct l2cap_chan *pchan;
+	u8 dst_type;
 
 	BT_DBG("hcon %p bdaddr %pMR status %d", hcon, &hcon->dst, status);
 
@@ -7279,6 +7278,43 @@ void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
 	if (!conn)
 		return;
 
+	dst_type = bdaddr_type(hcon, hcon->dst_type);
+
+	/* If device is blocked, do not create channels for it */
+	if (hci_bdaddr_list_lookup(&hdev->blacklist, &hcon->dst, dst_type))
+		return;
+
+	/* Find fixed channels and notify them of the new connection. We
+	 * use multiple individual lookups, continuing each time where
+	 * we left off, because the list lock would prevent calling the
+	 * potentially sleeping l2cap_chan_lock() function.
+	 */
+	pchan = l2cap_global_fixed_chan(NULL, &hdev->bdaddr);
+	while (pchan) {
+		struct l2cap_chan *chan, *next;
+
+		/* Client fixed channels should override server ones */
+		if (__l2cap_get_chan_by_dcid(conn, pchan->scid))
+			goto next;
+
+		l2cap_chan_lock(pchan);
+		chan = pchan->ops->new_connection(pchan);
+		if (chan) {
+			bacpy(&chan->src, &hcon->src);
+			bacpy(&chan->dst, &hcon->dst);
+			chan->src_type = bdaddr_type(hcon, hcon->src_type);
+			chan->dst_type = dst_type;
+
+			__l2cap_chan_add(conn, chan);
+		}
+
+		l2cap_chan_unlock(pchan);
+next:
+		next = l2cap_global_fixed_chan(pchan, &hdev->bdaddr);
+		l2cap_chan_put(pchan);
+		pchan = next;
+	}
+
 	l2cap_conn_ready(conn);
 }
 
-- 
1.9.3

[PATCH 7/9] Bluetooth: Improve fixed channel lookup based on link type

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

When notifying global fixed channels of new connections it doesn't make
sense to consider channels meant for a different link type than the one
available. This patch adds an extra parameter to the
l2cap_global_fixed_chan() lookup function and ensures that only channels
matching the current hci_conn type are looked up.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/l2cap_core.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 197b811e6206..a1bd591253d5 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -7233,7 +7233,7 @@ int l2cap_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr)
  * global list (by passing NULL as first parameter).
  */
 static struct l2cap_chan *l2cap_global_fixed_chan(struct l2cap_chan *c,
-						  bdaddr_t *src)
+						  bdaddr_t *src, u8 link_type)
 {
 	read_lock(&chan_list_lock);
 
@@ -7249,6 +7249,10 @@ static struct l2cap_chan *l2cap_global_fixed_chan(struct l2cap_chan *c,
 			continue;
 		if (bacmp(&c->src, src) && bacmp(&c->src, BDADDR_ANY))
 			continue;
+		if (link_type == ACL_LINK && c->src_type != BDADDR_BREDR)
+			continue;
+		if (link_type == LE_LINK && c->src_type == BDADDR_BREDR)
+			continue;
 
 		l2cap_chan_hold(c);
 		read_unlock(&chan_list_lock);
@@ -7289,7 +7293,7 @@ void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
 	 * we left off, because the list lock would prevent calling the
 	 * potentially sleeping l2cap_chan_lock() function.
 	 */
-	pchan = l2cap_global_fixed_chan(NULL, &hdev->bdaddr);
+	pchan = l2cap_global_fixed_chan(NULL, &hdev->bdaddr, hcon->type);
 	while (pchan) {
 		struct l2cap_chan *chan, *next;
 
@@ -7310,7 +7314,8 @@ void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
 
 		l2cap_chan_unlock(pchan);
 next:
-		next = l2cap_global_fixed_chan(pchan, &hdev->bdaddr);
+		next = l2cap_global_fixed_chan(pchan, &hdev->bdaddr,
+					       hcon->type);
 		l2cap_chan_put(pchan);
 		pchan = next;
 	}
-- 
1.9.3

[PATCH 8/9] Bluetooth: Remove special ATT data channel handling

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

Now that we've got the fixed channel infrastructure cleaned up in a
generic way there's no longer a need to have a dedicated function for
handling data on the ATT channel. Instead the generic
l2cap_data_channel() handler will be able to do the exact same thing.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/l2cap_core.c | 80 ----------------------------------------------
 1 file changed, 80 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index a1bd591253d5..fbe8c37f1a9e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1420,51 +1420,6 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 	mutex_unlock(&conn->chan_lock);
 }
 
-/* Find socket with cid and source/destination bdaddr.
- * Returns closest match, locked.
- */
-static struct l2cap_chan *l2cap_global_chan_by_scid(int state, u16 cid,
-						    bdaddr_t *src,
-						    bdaddr_t *dst)
-{
-	struct l2cap_chan *c, *c1 = NULL;
-
-	read_lock(&chan_list_lock);
-
-	list_for_each_entry(c, &chan_list, global_l) {
-		if (state && c->state != state)
-			continue;
-
-		if (c->scid == cid) {
-			int src_match, dst_match;
-			int src_any, dst_any;
-
-			/* Exact match. */
-			src_match = !bacmp(&c->src, src);
-			dst_match = !bacmp(&c->dst, dst);
-			if (src_match && dst_match) {
-				l2cap_chan_hold(c);
-				read_unlock(&chan_list_lock);
-				return c;
-			}
-
-			/* Closest match */
-			src_any = !bacmp(&c->src, BDADDR_ANY);
-			dst_any = !bacmp(&c->dst, BDADDR_ANY);
-			if ((src_match && dst_any) || (src_any && dst_match) ||
-			    (src_any && dst_any))
-				c1 = c;
-		}
-	}
-
-	if (c1)
-		l2cap_chan_hold(c1);
-
-	read_unlock(&chan_list_lock);
-
-	return c1;
-}
-
 static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 {
 	struct hci_conn *hcon = conn->hcon;
@@ -6855,37 +6810,6 @@ put:
 		l2cap_chan_put(chan);
 }
 
-static void l2cap_att_channel(struct l2cap_conn *conn,
-			      struct sk_buff *skb)
-{
-	struct hci_conn *hcon = conn->hcon;
-	struct l2cap_chan *chan;
-
-	if (hcon->type != LE_LINK) {
-		chan = NULL;
-		goto drop;
-	}
-
-	chan = l2cap_global_chan_by_scid(BT_CONNECTED, L2CAP_CID_ATT,
-					 &hcon->src, &hcon->dst);
-	if (!chan)
-		goto drop;
-
-	BT_DBG("chan %p, len %d", chan, skb->len);
-
-	if (chan->imtu < skb->len)
-		goto drop;
-
-	if (!chan->ops->recv(chan, skb))
-		goto put;
-
-drop:
-	kfree_skb(skb);
-put:
-	if (chan)
-		l2cap_chan_put(chan);
-}
-
 static void l2cap_recv_frame(struct l2cap_conn *conn, struct sk_buff *skb)
 {
 	struct l2cap_hdr *lh = (void *) skb->data;
@@ -6931,10 +6855,6 @@ static void l2cap_recv_frame(struct l2cap_conn *conn, struct sk_buff *skb)
 		l2cap_conless_channel(conn, psm, skb);
 		break;
 
-	case L2CAP_CID_ATT:
-		l2cap_att_channel(conn, skb);
-		break;
-
 	case L2CAP_CID_LE_SIGNALING:
 		l2cap_le_sig_channel(conn, skb);
 		break;
-- 
1.9.3

[PATCH 9/9] Bluetooth: Move parts of fixed channel initialization to l2cap_add_scid

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

The l2cap_add_scid function is used for registering a fixed L2CAP
channel. Instead of having separate initialization of the channel type
and outgoing MTU in l2cap_sock.c it's more intuitive to do these things
in the l2cap_add_scid function itself (and thereby make the
functionality available to other users besides l2cap_sock.c).

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/l2cap_core.c | 4 ++++
 net/bluetooth/l2cap_sock.c | 9 ---------
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index fbe8c37f1a9e..4db668068804 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -210,6 +210,10 @@ int l2cap_add_scid(struct l2cap_chan *chan,  __u16 scid)
 {
 	write_lock(&chan_list_lock);
 
+	/* Override the defaults (which are for conn-oriented) */
+	chan->omtu = L2CAP_DEFAULT_MTU;
+	chan->chan_type = L2CAP_CHAN_FIXED;
+
 	chan->scid = scid;
 
 	write_unlock(&chan_list_lock);
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 5a42f6a818c0..ed06f88e6f10 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -99,15 +99,6 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
 	if (!bdaddr_type_is_valid(la.l2_bdaddr_type))
 		return -EINVAL;
 
-	if (la.l2_cid) {
-		/* When the socket gets created it defaults to
-		 * CHAN_CONN_ORIENTED, so we need to overwrite the
-		 * default here.
-		 */
-		chan->chan_type = L2CAP_CHAN_FIXED;
-		chan->omtu = L2CAP_DEFAULT_MTU;
-	}
-
 	if (bdaddr_type_is_le(la.l2_bdaddr_type)) {
 		/* We only allow ATT user space socket */
 		if (la.l2_cid &&
-- 
1.9.3

Re: [PATCH 1/9] Bluetooth: Fix reference counting of global L2CAP channels

[Marcel Holtmann]

Hi Johan,

> When looking up entries from the global L2CAP channel list there needs
> to be a guarantee that other code doesn't go and remove the entry after
> a channel has been returned by the lookup function. This patch makes
> sure that the channel reference is incremented before the read lock is
> released in the global channel lookup functions. The patch also adds the
> corresponding l2cap_chan_put() calls once the channels pointers are
> no-longer needed.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/l2cap_core.c | 34 ++++++++++++++++++++++++++++------
> 1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 46547b920f88..a49e9646a6b9 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1440,6 +1440,7 @@ static struct l2cap_chan *l2cap_global_chan_by_scid(int state, u16 cid,
> 			src_match = !bacmp(&c->src, src);
> 			dst_match = !bacmp(&c->dst, dst);
> 			if (src_match && dst_match) {
> +				l2cap_chan_hold(c);
> 				read_unlock(&chan_list_lock);
> 				return c;
> 			}
> @@ -1453,6 +1454,9 @@ static struct l2cap_chan *l2cap_global_chan_by_scid(int state, u16 cid,
> 		}
> 	}
> 
> +	if (c1)
> +		l2cap_chan_hold(c1);
> +
> 	read_unlock(&chan_list_lock);
> 
> 	return c1;
> @@ -1475,13 +1479,13 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
> 
> 	/* Client ATT sockets should override the server one */
> 	if (__l2cap_get_chan_by_dcid(conn, L2CAP_CID_ATT))
> -		return;
> +		goto put;
> 
> 	dst_type = bdaddr_type(hcon, hcon->dst_type);
> 
> 	/* If device is blocked, do not create a channel for it */
> 	if (hci_bdaddr_list_lookup(&hdev->blacklist, &hcon->dst, dst_type))
> -		return;
> +		goto put;
> 
> 	/* For LE slave connections, make sure the connection interval
> 	 * is in the range of the minium and maximum interval that has
> @@ -1517,6 +1521,8 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
> 
> clean:
> 	l2cap_chan_unlock(pchan);
> +put:
> +	l2cap_chan_put(pchan);
> }
> 
> static void l2cap_conn_ready(struct l2cap_conn *conn)
> @@ -1794,6 +1800,7 @@ static struct l2cap_chan *l2cap_global_chan_by_psm(int state, __le16 psm,
> 			src_match = !bacmp(&c->src, src);
> 			dst_match = !bacmp(&c->dst, dst);
> 			if (src_match && dst_match) {
> +				l2cap_chan_hold(c);
> 				read_unlock(&chan_list_lock);
> 				return c;
> 			}
> @@ -1807,6 +1814,9 @@ static struct l2cap_chan *l2cap_global_chan_by_psm(int state, __le16 psm,
> 		}
> 	}
> 
> +	if (c1)
> +		l2cap_chan_hold(c1);
> +
> 	read_unlock(&chan_list_lock);
> 
> 	return c1;
> @@ -3884,6 +3894,7 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
> response:
> 	l2cap_chan_unlock(pchan);
> 	mutex_unlock(&conn->chan_lock);
> +	l2cap_chan_put(pchan);
> 
> sendresp:
> 	rsp.scid   = cpu_to_le16(scid);
> @@ -5497,6 +5508,7 @@ static int l2cap_le_connect_req(struct l2cap_conn *conn,
> response_unlock:
> 	l2cap_chan_unlock(pchan);
> 	mutex_unlock(&conn->chan_lock);
> +	l2cap_chan_put(pchan);
> 
> 	if (result == L2CAP_CR_PEND)
> 		return 0;
> @@ -6844,8 +6856,10 @@ static void l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm,
> 	struct hci_conn *hcon = conn->hcon;
> 	struct l2cap_chan *chan;
> 
> -	if (hcon->type != ACL_LINK)
> +	if (hcon->type != ACL_LINK) {
> +		chan = NULL;

Why not just l2cap_chan_put(chan) here?

> 		goto drop;
> +	}
> 
> 	chan = l2cap_global_chan_by_psm(0, psm, &hcon->src, &hcon->dst,
> 					ACL_LINK);
> @@ -6865,10 +6879,13 @@ static void l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm,
> 	bt_cb(skb)->psm = psm;
> 
> 	if (!chan->ops->recv(chan, skb))
> -		return;

And here just l2cap_chan_put(chan) and return.

> +		goto put;
> 
> drop:
> 	kfree_skb(skb);
> +put:
> +	if (chan)
> +		l2cap_chan_put(chan);
> }

And not bother with the put label and the conditional here. In general I dislike the the combination of using put with a condition in a cleanup label. We might just need to organize the code a bit better.


> 
> static void l2cap_att_channel(struct l2cap_conn *conn,
> @@ -6877,8 +6894,10 @@ static void l2cap_att_channel(struct l2cap_conn *conn,
> 	struct hci_conn *hcon = conn->hcon;
> 	struct l2cap_chan *chan;
> 
> -	if (hcon->type != LE_LINK)
> +	if (hcon->type != LE_LINK) {
> +		chan = NULL;
> 		goto drop;
> +	}
> 
> 	chan = l2cap_global_chan_by_scid(BT_CONNECTED, L2CAP_CID_ATT,
> 					 &hcon->src, &hcon->dst);
> @@ -6891,10 +6910,13 @@ static void l2cap_att_channel(struct l2cap_conn *conn,
> 		goto drop;
> 
> 	if (!chan->ops->recv(chan, skb))
> -		return;
> +		goto put;
> 
> drop:
> 	kfree_skb(skb);
> +put:
> +	if (chan)
> +		l2cap_chan_put(chan);
> }

This here is similar to above.

Regards

Marcel

Re: [PATCH 0/9] Bluetooth: More generic L2CAP fixed channel handling

[Marcel Holtmann]

Hi Johan,

> Here's a set of patches to make fixed L2CAP channel handling more
> generic. The main special-casing that's removed here is for ATT
> channels. The A2MP stuff is left for later since its semantics would
> change from creating the context only when there's data on the channel
> to as soon as we have a baseband link (which I'm not sure is desirable).
> 
> The main motivation for all of this work is to pave the way to convert
> SMP to fully use the l2cap_chan infrastructure.
> 
> Johan
> 
> ----------------------------------------------------------------
> Johan Hedberg (9):
>      Bluetooth: Fix reference counting of global L2CAP channels
>      Bluetooth: Fix __l2cap_no_conn_pending() usage with all channels
>      Bluetooth: Resume BT_CONNECTED state after LE security elevation
>      Bluetooth: Remove special handling of ATT in l2cap_security_cfm()
>      Bluetooth: Refactor l2cap_connect_cfm
>      Bluetooth: Move L2CAP fixed channel creation into l2cap_conn_cfm
>      Bluetooth: Improve fixed channel lookup based on link type
>      Bluetooth: Remove special ATT data channel handling
>      Bluetooth: Move parts of fixed channel initialization to l2cap_add_scid
> 
> include/net/bluetooth/l2cap.h |   1 +
> net/bluetooth/l2cap_core.c    | 244 ++++++++++++++++++--------------------
> net/bluetooth/l2cap_sock.c    |  15 +--

all 9 patches have been applied to bluetooth-next tree.

Regards

Marcel