[PATCH 00/13] Bluetooth: L2CAP cleanups and fixes

[PATCH 00/13] Bluetooth: L2CAP cleanups and fixes

[Johan Hedberg]

Hi,

This set of patches contains some cleanups and fixes for future L2CAP
work. The L2CAP usage in coming core spec versions will no longer be as
constrined as it has so far with extensions to the LE signalling
channel etc. This means that we really do need separate code paths for
LE signalling instead of sharing BR/EDR related signalling code. The
patches to split this up keep the same functionality as before (for now)
but ensure that we have dedicated functions and structs for LE.

Johan

----------------------------------------------------------------
Johan Hedberg (13):
      Bluetooth: Handle LE L2CAP signalling in its own function
      Bluetooth: Create independent LE signalling defines and structs
      Bluetooth: Rename L2CAP_CID_LE_DATA to L2CAP_CID_ATT
      Bluetooth: Fix LE vs BR/EDR selection when connecting
      Bluetooth: Fix EBUSY condition test in l2cap_chan_connect
      Bluetooth: Fix hardcoding ATT CID in __l2cap_chan_add()
      Bluetooth: Add clarifying comment to l2cap_conn_ready()
      Bluetooth: Fix duplicate call to l2cap_chan_ready()
      Bluetooth: Remove useless sk variable in l2cap_le_conn_ready
      Bluetooth: Remove unnecessary L2CAP channel state check
      Bluetooth: Simplify hci_conn_hold/drop logic for L2CAP
      Bluetooth: Remove useless hci_conn disc_timeout setting
      Bluetooth: Fix multiple LE socket handling

 include/net/bluetooth/l2cap.h |   36 ++++++++--
 net/bluetooth/l2cap_core.c    |  145 ++++++++++++++++++++++++++---------------
 net/bluetooth/l2cap_sock.c    |    4 +-
 3 files changed, 126 insertions(+), 59 deletions(-)

[PATCH 01/13] Bluetooth: Handle LE L2CAP signalling in its own function

[Johan Hedberg]

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

The LE L2CAP signalling channel follows its own rules and will continue
to evolve independently from the BR/EDR signalling channel. Therefore,
it makes sense to have a clear split from BR/EDR by having a dedicated
function for handling LE signalling commands.

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index a76d1ac..1adda11 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5255,6 +5255,51 @@ static inline int l2cap_le_sig_cmd(struct l2cap_conn *conn,
 	}
 }
 
+static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
+					struct sk_buff *skb)
+{
+	u8 *data = skb->data;
+	int len = skb->len;
+	struct l2cap_cmd_hdr cmd;
+	int err;
+
+	l2cap_raw_recv(conn, skb);
+
+	while (len >= L2CAP_CMD_HDR_SIZE) {
+		u16 cmd_len;
+		memcpy(&cmd, data, L2CAP_CMD_HDR_SIZE);
+		data += L2CAP_CMD_HDR_SIZE;
+		len  -= L2CAP_CMD_HDR_SIZE;
+
+		cmd_len = le16_to_cpu(cmd.len);
+
+		BT_DBG("code 0x%2.2x len %d id 0x%2.2x", cmd.code, cmd_len,
+		       cmd.ident);
+
+		if (cmd_len > len || !cmd.ident) {
+			BT_DBG("corrupted command");
+			break;
+		}
+
+		err = l2cap_le_sig_cmd(conn, &cmd, data);
+		if (err) {
+			struct l2cap_cmd_rej_unk rej;
+
+			BT_ERR("Wrong link type (%d)", err);
+
+			/* FIXME: Map err to a valid reason */
+			rej.reason = __constant_cpu_to_le16(L2CAP_REJ_NOT_UNDERSTOOD);
+			l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
+				       sizeof(rej), &rej);
+		}
+
+		data += cmd_len;
+		len  -= cmd_len;
+	}
+
+	kfree_skb(skb);
+}
+
 static inline void l2cap_sig_channel(struct l2cap_conn *conn,
 				     struct sk_buff *skb)
 {
@@ -5281,11 +5326,7 @@ static inline void l2cap_sig_channel(struct l2cap_conn *conn,
 			break;
 		}
 
-		if (conn->hcon->type == LE_LINK)
-			err = l2cap_le_sig_cmd(conn, &cmd, data);
-		else
-			err = l2cap_bredr_sig_cmd(conn, &cmd, cmd_len, data);
-
+		err = l2cap_bredr_sig_cmd(conn, &cmd, cmd_len, data);
 		if (err) {
 			struct l2cap_cmd_rej_unk rej;
 
@@ -6358,6 +6399,8 @@ static void l2cap_recv_frame(struct l2cap_conn *conn, struct sk_buff *skb)
 
 	switch (cid) {
 	case L2CAP_CID_LE_SIGNALING:
+		l2cap_le_sig_channel(conn, skb);
+		break;
 	case L2CAP_CID_SIGNALING:
 		l2cap_sig_channel(conn, skb);
 		break;
-- 
1.7.10.4

[PATCH 02/13] Bluetooth: Create independent LE signalling defines and structs

[Johan Hedberg]

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

Since the LE signalling channel is independent from the BR/EDR
signalling channel and may experience changes incompatible with the
BR/EDR channel it makes sense from the start to have independent defines
and structs for it.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/l2cap.h |   34 ++++++++++++++++++++++++++++++----
 net/bluetooth/l2cap_core.c    |   24 ++++++++++++------------
 2 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index fb94cf1..2f52a28 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -110,8 +110,13 @@ struct l2cap_conninfo {
 #define L2CAP_MOVE_CHAN_RSP	0x0f
 #define L2CAP_MOVE_CHAN_CFM	0x10
 #define L2CAP_MOVE_CHAN_CFM_RSP	0x11
-#define L2CAP_CONN_PARAM_UPDATE_REQ	0x12
-#define L2CAP_CONN_PARAM_UPDATE_RSP	0x13
+
+/* L2CAP LE command codes */
+#define L2CAP_LE_COMMAND_REJ		0x01
+#define L2CAP_LE_DISCONN_REQ		0x06
+#define L2CAP_LE_DISCONN_RSP		0x07
+#define L2CAP_LE_CONN_PARAM_UPDATE_REQ	0x12
+#define L2CAP_LE_CONN_PARAM_UPDATE_RSP	0x13
 
 /* L2CAP extended feature mask */
 #define L2CAP_FEAT_FLOWCTL	0x00000001
@@ -406,14 +411,35 @@ struct l2cap_move_chan_cfm_rsp {
 #define L2CAP_IR_SUCCESS	0x0000
 #define L2CAP_IR_NOTSUPP	0x0001
 
-struct l2cap_conn_param_update_req {
+struct l2cap_le_cmd_hdr {
+	__u8       code;
+	__u8       ident;
+	__le16     len;
+} __packed;
+#define L2CAP_LE_CMD_HDR_SIZE	4
+
+struct l2cap_le_cmd_rej_unk {
+	__le16     reason;
+} __packed;
+
+struct l2cap_le_disconn_req {
+	__le16     dcid;
+	__le16     scid;
+} __packed;
+
+struct l2cap_le_disconn_rsp {
+	__le16     dcid;
+	__le16     scid;
+} __packed;
+
+struct l2cap_le_conn_param_update_req {
 	__le16      min;
 	__le16      max;
 	__le16      latency;
 	__le16      to_multiplier;
 } __packed;
 
-struct l2cap_conn_param_update_rsp {
+struct l2cap_le_conn_param_update_rsp {
 	__le16      result;
 } __packed;
 
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 1adda11..6bf5d19 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5113,13 +5113,13 @@ static inline int l2cap_check_conn_param(u16 min, u16 max, u16 latency,
 	return 0;
 }
 
-static inline int l2cap_conn_param_update_req(struct l2cap_conn *conn,
-					      struct l2cap_cmd_hdr *cmd,
-					      u8 *data)
+static inline int l2cap_le_conn_param_update_req(struct l2cap_conn *conn,
+						 struct l2cap_cmd_hdr *cmd,
+						 u8 *data)
 {
 	struct hci_conn *hcon = conn->hcon;
-	struct l2cap_conn_param_update_req *req;
-	struct l2cap_conn_param_update_rsp rsp;
+	struct l2cap_le_conn_param_update_req *req;
+	struct l2cap_le_conn_param_update_rsp rsp;
 	u16 min, max, latency, to_multiplier, cmd_len;
 	int err;
 
@@ -5127,10 +5127,10 @@ static inline int l2cap_conn_param_update_req(struct l2cap_conn *conn,
 		return -EINVAL;
 
 	cmd_len = __le16_to_cpu(cmd->len);
-	if (cmd_len != sizeof(struct l2cap_conn_param_update_req))
+	if (cmd_len != sizeof(struct l2cap_le_conn_param_update_req))
 		return -EPROTO;
 
-	req = (struct l2cap_conn_param_update_req *) data;
+	req = (struct l2cap_le_conn_param_update_req *) data;
 	min		= __le16_to_cpu(req->min);
 	max		= __le16_to_cpu(req->max);
 	latency		= __le16_to_cpu(req->latency);
@@ -5147,7 +5147,7 @@ static inline int l2cap_conn_param_update_req(struct l2cap_conn *conn,
 	else
 		rsp.result = __constant_cpu_to_le16(L2CAP_CONN_PARAM_ACCEPTED);
 
-	l2cap_send_cmd(conn, cmd->ident, L2CAP_CONN_PARAM_UPDATE_RSP,
+	l2cap_send_cmd(conn, cmd->ident, L2CAP_LE_CONN_PARAM_UPDATE_RSP,
 		       sizeof(rsp), &rsp);
 
 	if (!err)
@@ -5240,13 +5240,13 @@ static inline int l2cap_le_sig_cmd(struct l2cap_conn *conn,
 				   struct l2cap_cmd_hdr *cmd, u8 *data)
 {
 	switch (cmd->code) {
-	case L2CAP_COMMAND_REJ:
+	case L2CAP_LE_COMMAND_REJ:
 		return 0;
 
-	case L2CAP_CONN_PARAM_UPDATE_REQ:
-		return l2cap_conn_param_update_req(conn, cmd, data);
+	case L2CAP_LE_CONN_PARAM_UPDATE_REQ:
+		return l2cap_le_conn_param_update_req(conn, cmd, data);
 
-	case L2CAP_CONN_PARAM_UPDATE_RSP:
+	case L2CAP_LE_CONN_PARAM_UPDATE_RSP:
 		return 0;
 
 	default:
-- 
1.7.10.4

[PATCH 03/13] Bluetooth: Rename L2CAP_CID_LE_DATA to L2CAP_CID_ATT

[Johan Hedberg]

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

In future Core Specification versions the ATT CID will be just one of
many possible CIDs that can be used for data transfer. Therefore, it
makes sense to rename the define for the ATT CID to something less
ambigous.

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

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 2f52a28..c31ac21 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -247,7 +247,7 @@ struct l2cap_conn_rsp {
 #define L2CAP_CID_SIGNALING	0x0001
 #define L2CAP_CID_CONN_LESS	0x0002
 #define L2CAP_CID_A2MP		0x0003
-#define L2CAP_CID_LE_DATA	0x0004
+#define L2CAP_CID_ATT		0x0004
 #define L2CAP_CID_LE_SIGNALING	0x0005
 #define L2CAP_CID_SMP		0x0006
 #define L2CAP_CID_DYN_START	0x0040
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 6bf5d19..d5e4404 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -504,8 +504,8 @@ void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 		if (conn->hcon->type == LE_LINK) {
 			/* LE connection */
 			chan->omtu = L2CAP_DEFAULT_MTU;
-			chan->scid = L2CAP_CID_LE_DATA;
-			chan->dcid = L2CAP_CID_LE_DATA;
+			chan->scid = L2CAP_CID_ATT;
+			chan->dcid = L2CAP_CID_ATT;
 		} else {
 			/* Alloc CID for connection-oriented socket */
 			chan->scid = l2cap_alloc_cid(conn);
@@ -1344,7 +1344,7 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 	BT_DBG("");
 
 	/* Check if we have socket listening on cid */
-	pchan = l2cap_global_chan_by_scid(BT_LISTEN, L2CAP_CID_LE_DATA,
+	pchan = l2cap_global_chan_by_scid(BT_LISTEN, L2CAP_CID_ATT,
 					  conn->src, conn->dst);
 	if (!pchan)
 		return;
@@ -1792,7 +1792,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 
 	auth_type = l2cap_get_auth_type(chan);
 
-	if (chan->dcid == L2CAP_CID_LE_DATA)
+	if (chan->dcid == L2CAP_CID_ATT)
 		hcon = hci_connect(hdev, LE_LINK, dst, dst_type,
 				   chan->sec_level, auth_type);
 	else
@@ -6360,7 +6360,7 @@ static void l2cap_att_channel(struct l2cap_conn *conn,
 {
 	struct l2cap_chan *chan;
 
-	chan = l2cap_global_chan_by_scid(0, L2CAP_CID_LE_DATA,
+	chan = l2cap_global_chan_by_scid(0, L2CAP_CID_ATT,
 					 conn->src, conn->dst);
 	if (!chan)
 		goto drop;
@@ -6411,7 +6411,7 @@ static void l2cap_recv_frame(struct l2cap_conn *conn, struct sk_buff *skb)
 		l2cap_conless_channel(conn, psm, skb);
 		break;
 
-	case L2CAP_CID_LE_DATA:
+	case L2CAP_CID_ATT:
 		l2cap_att_channel(conn, skb);
 		break;
 
@@ -6537,7 +6537,7 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 			continue;
 		}
 
-		if (chan->scid == L2CAP_CID_LE_DATA) {
+		if (chan->scid == L2CAP_CID_ATT) {
 			if (!status && encrypt) {
 				chan->sec_level = hcon->sec_level;
 				l2cap_chan_ready(chan);
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 141e7b0..16b3e51 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -466,7 +466,7 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
 static bool l2cap_valid_mtu(struct l2cap_chan *chan, u16 mtu)
 {
 	switch (chan->scid) {
-	case L2CAP_CID_LE_DATA:
+	case L2CAP_CID_ATT:
 		if (mtu < L2CAP_LE_MIN_MTU)
 			return false;
 		break;
@@ -630,7 +630,7 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
 		conn = chan->conn;
 
 		/*change security for LE channels */
-		if (chan->scid == L2CAP_CID_LE_DATA) {
+		if (chan->scid == L2CAP_CID_ATT) {
 			if (!conn->hcon->out) {
 				err = -EINVAL;
 				break;
-- 
1.7.10.4

[PATCH 04/13] Bluetooth: Fix LE vs BR/EDR selection when connecting

[Johan Hedberg]

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

The choice between LE and BR/EDR should be made on the destination
address type instead of the destination CID. This is particularly
important when in the future more than one CID will be allowed for LE.

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index d5e4404..95d46cd 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1792,7 +1792,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 
 	auth_type = l2cap_get_auth_type(chan);
 
-	if (chan->dcid == L2CAP_CID_ATT)
+	if (bdaddr_type_is_le(dst_type))
 		hcon = hci_connect(hdev, LE_LINK, dst, dst_type,
 				   chan->sec_level, auth_type);
 	else
-- 
1.7.10.4

[PATCH 05/13] Bluetooth: Fix EBUSY condition test in l2cap_chan_connect

[Johan Hedberg]

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

The current test in l2cap_chan_connect is intended to protect against
multiple conflicting connect attempts. However, it assumes that there
will ever only be a single CID that is connected to, which is not true.
We do need to check for conflicts with connect attempts to the same
destination CID but this check is not in anyway specific to LE but can
be applied to BR/EDR as well.

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 95d46cd..fd8d3059 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1811,16 +1811,10 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 		goto done;
 	}
 
-	if (hcon->type == LE_LINK) {
-		err = 0;
-
-		if (!list_empty(&conn->chan_l)) {
-			err = -EBUSY;
-			hci_conn_drop(hcon);
-		}
-
-		if (err)
-			goto done;
+	if (cid && __l2cap_get_chan_by_dcid(conn, cid)) {
+		hci_conn_drop(hcon);
+		err = -EBUSY;
+		goto done;
 	}
 
 	/* Update source addr of the socket */
-- 
1.7.10.4

[PATCH 06/13] Bluetooth: Fix hardcoding ATT CID in __l2cap_chan_add()

[Johan Hedberg]

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

Since in the future more than the ATT CID may be permissible we should
not be hardcoding it for all LE connections in __l2cap_chan_add().
Instead, the source ATT CID should only be set if the destination is
also ATT, and in other cases we should just use the existing dynamic CID
allocation function.

Assigning scid based on dcid means that whenever __l2cap_chan_add() is
called that chan->dcid is properly initialized. l2cap_le_conn_ready()
wasn't initializing is properly so this is also taken care of in this
patch.

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index fd8d3059..dd7f9b3 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -504,8 +504,10 @@ void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 		if (conn->hcon->type == LE_LINK) {
 			/* LE connection */
 			chan->omtu = L2CAP_DEFAULT_MTU;
-			chan->scid = L2CAP_CID_ATT;
-			chan->dcid = L2CAP_CID_ATT;
+			if (chan->dcid == L2CAP_CID_ATT)
+				chan->scid = L2CAP_CID_ATT;
+			else
+				chan->scid = l2cap_alloc_cid(conn);
 		} else {
 			/* Alloc CID for connection-oriented socket */
 			chan->scid = l2cap_alloc_cid(conn);
@@ -1357,6 +1359,8 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 	if (!chan)
 		goto clean;
 
+	chan->dcid = L2CAP_CID_ATT;
+
 	sk = chan->sk;
 
 	hci_conn_hold(conn->hcon);
-- 
1.7.10.4

[PATCH 07/13] Bluetooth: Add clarifying comment to l2cap_conn_ready()

[Johan Hedberg]

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

There is an extra call to smp_conn_security() for outgoing LE
connections from l2cap_conn_ready() but the reason for this call is far
from clear. After a bit of commit history research and using git blame I
found out that this extra call is for socket-less pairing processes
added by commit 160dc6ac1. This patch adds a clarifying comment to the
code for this.

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 dd7f9b3..60d13bc 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1387,6 +1387,9 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
 	if (!hcon->out && hcon->type == LE_LINK)
 		l2cap_le_conn_ready(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);
 
-- 
1.7.10.4

[PATCH 08/13] Bluetooth: Fix duplicate call to l2cap_chan_ready()

[Johan Hedberg]

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

In l2cap_le_conn_ready() after doing l2cap_chann_add() the LE channel is
part of the list which is subsequently iterated in l2cap_conn_ready() in
this loop each channel will get l2cap_chan_ready() called which would
result in trying to set the channel two times into BT_CONNECTED state.
Instead it makes sense to just add the channel but not call chan_ready
in l2cap_le_conn_ready, which is what this patch does.

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 60d13bc..881be61 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1371,8 +1371,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 
 	l2cap_chan_add(conn, chan);
 
-	l2cap_chan_ready(chan);
-
 clean:
 	release_sock(parent);
 }
-- 
1.7.10.4

[PATCH 09/13] Bluetooth: Remove useless sk variable in l2cap_le_conn_ready

[Johan Hedberg]

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

The sk variable is of quite little use since it's only used to simplify
access in the two bt_sk() calls.

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 881be61..aa4bc70 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1340,7 +1340,7 @@ static struct l2cap_chan *l2cap_global_chan_by_scid(int state, u16 cid,
 
 static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 {
-	struct sock *parent, *sk;
+	struct sock *parent;
 	struct l2cap_chan *chan, *pchan;
 
 	BT_DBG("");
@@ -1361,13 +1361,11 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 
 	chan->dcid = L2CAP_CID_ATT;
 
-	sk = chan->sk;
-
 	hci_conn_hold(conn->hcon);
 	conn->hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
 
-	bacpy(&bt_sk(sk)->src, conn->src);
-	bacpy(&bt_sk(sk)->dst, conn->dst);
+	bacpy(&bt_sk(chan->sk)->src, conn->src);
+	bacpy(&bt_sk(chan->sk)->dst, conn->dst);
 
 	l2cap_chan_add(conn, chan);
 
-- 
1.7.10.4

[PATCH 10/13] Bluetooth: Remove unnecessary L2CAP channel state check

[Johan Hedberg]

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

In l2cap_att_channel() we're only interested in the BT_CONNECTED state
so this state can directly be passed to l2cap_global_chan_by_scid().
This way there's no need to do any additional state check later.

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index aa4bc70..e373b9d 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -6357,16 +6357,13 @@ static void l2cap_att_channel(struct l2cap_conn *conn,
 {
 	struct l2cap_chan *chan;
 
-	chan = l2cap_global_chan_by_scid(0, L2CAP_CID_ATT,
+	chan = l2cap_global_chan_by_scid(BT_CONNECTED, L2CAP_CID_ATT,
 					 conn->src, conn->dst);
 	if (!chan)
 		goto drop;
 
 	BT_DBG("chan %p, len %d", chan, skb->len);
 
-	if (chan->state != BT_BOUND && chan->state != BT_CONNECTED)
-		goto drop;
-
 	if (chan->imtu < skb->len)
 		goto drop;
 
-- 
1.7.10.4

[PATCH 11/13] Bluetooth: Simplify hci_conn_hold/drop logic for L2CAP

[Johan Hedberg]

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

The L2CAP code has been incrementing the hci_conn reference for each
l2cap_chan instance in the l2cap_conn list. Likewise, the reference is
dropped each time an l2cap_chan is removed from the list. The reference
counting policy with respect to removal has been clear and explicit in
the l2cap_chan_drop function, however for addition the function
calling 2cap_chan_add has always had to do a separate hci_conn_hold
call.

What made the counting even more hard to follow is that the
hci_connect() procedure increments the reference and the L2CAP layer
making this call took advantage of it to use it as its own reference.

This patch aims to clarify things by having the call to hci_conn_hold
inside __l2cap_chan_add, thereby removing the need to do it in the
functions calling __l2cap_chan_add. The reference count for hci_connect
is still kept as it's necessary for users such as mgmt_pair_device,
however for the L2CAP layer it means that an extra call to hci_conn_drop
must be performed once l2cap_chan_add has been done.

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index e373b9d..c78a510 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -545,6 +545,8 @@ void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 
 	l2cap_chan_hold(chan);
 
+	hci_conn_hold(conn->hcon);
+
 	list_add(&chan->list, &conn->chan_l);
 }
 
@@ -1361,7 +1363,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 
 	chan->dcid = L2CAP_CID_ATT;
 
-	hci_conn_hold(conn->hcon);
 	conn->hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
 
 	bacpy(&bt_sk(chan->sk)->src, conn->src);
@@ -1827,6 +1828,9 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 	l2cap_chan_add(conn, chan);
 	l2cap_chan_lock(chan);
 
+	/* l2cap_chan_add takes its own ref so we can drop this one */
+	hci_conn_drop(hcon);
+
 	l2cap_state_change(chan, BT_CONNECT);
 	__set_chan_timer(chan, sk->sk_sndtimeo);
 
@@ -3741,8 +3745,6 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
 
 	sk = chan->sk;
 
-	hci_conn_hold(conn->hcon);
-
 	bacpy(&bt_sk(sk)->src, conn->src);
 	bacpy(&bt_sk(sk)->dst, conn->dst);
 	chan->psm  = psm;
-- 
1.7.10.4

[PATCH 12/13] Bluetooth: Remove useless hci_conn disc_timeout setting

[Johan Hedberg]

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

There's no need to reset disc_timeout in l2cap_le_conn_ready since
HCI_DISCONN_TIMEOUT is the default when the hci_conn is created and
there should be no way for it to get changed between creation and
l2cap_le_conn_ready being called.

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index c78a510..d510550 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1363,8 +1363,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 
 	chan->dcid = L2CAP_CID_ATT;
 
-	conn->hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
-
 	bacpy(&bt_sk(chan->sk)->src, conn->src);
 	bacpy(&bt_sk(chan->sk)->dst, conn->dst);
 
-- 
1.7.10.4

[PATCH 13/13] Bluetooth: Fix multiple LE socket handling

[Johan Hedberg]

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

The LE ATT server socket needs to be superseded by any ATT client
sockets. Previously this was done by looking at the hcon->out variable
(indicating whether the connection is outgoing or incoming) which is a
too crude way of determining whether the server socket needs to be
picked or not (an outgoing connection doesn't necessarily mean that an
ATT client socket has triggered it).

This patch extends the ATT server socket lookup function
(l2cap_le_conn_ready) to be used for all LE connections (regardless of
the hcon->out value) and adds an internal check into the function for
the existence of any ATT client sockets (in which case the server socket
should be skipped). For this to work reliably all lookups must be done
while the l2cap_conn->chan_lock is held, meaning also that the call to
l2cap_chan_add needs to be changed to its lockless __l2cap_chan_add
counterpart.

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index d510550..05e6255 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1353,6 +1353,10 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 	if (!pchan)
 		return;
 
+	/* Client ATT sockets should override the server one */
+	if (__l2cap_get_chan_by_dcid(conn, L2CAP_CID_ATT))
+		return;
+
 	parent = pchan->sk;
 
 	lock_sock(parent);
@@ -1366,7 +1370,7 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 	bacpy(&bt_sk(chan->sk)->src, conn->src);
 	bacpy(&bt_sk(chan->sk)->dst, conn->dst);
 
-	l2cap_chan_add(conn, chan);
+	__l2cap_chan_add(conn, chan);
 
 clean:
 	release_sock(parent);
@@ -1379,9 +1383,6 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
 
 	BT_DBG("conn %p", conn);
 
-	if (!hcon->out && hcon->type == LE_LINK)
-		l2cap_le_conn_ready(conn);
-
 	/* For outgoing pairing which doesn't necessarily have an
 	 * associated socket (e.g. mgmt_pair_device).
 	 */
@@ -1390,6 +1391,9 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
 
 	mutex_lock(&conn->chan_lock);
 
+	if (hcon->type == LE_LINK)
+		l2cap_le_conn_ready(conn);
+
 	list_for_each_entry(chan, &conn->chan_l, list) {
 
 		l2cap_chan_lock(chan);
-- 
1.7.10.4

Re: [PATCH 00/13] Bluetooth: L2CAP cleanups and fixes

[Marcel Holtmann]

Hi Johan,

> This set of patches contains some cleanups and fixes for future L2CAP
> work. The L2CAP usage in coming core spec versions will no longer be as
> constrined as it has so far with extensions to the LE signalling
> channel etc. This means that we really do need separate code paths for
> LE signalling instead of sharing BR/EDR related signalling code. The
> patches to split this up keep the same functionality as before (for now)
> but ensure that we have dedicated functions and structs for LE.
> 
> Johan
> 
> ----------------------------------------------------------------
> Johan Hedberg (13):
>      Bluetooth: Handle LE L2CAP signalling in its own function
>      Bluetooth: Create independent LE signalling defines and structs
>      Bluetooth: Rename L2CAP_CID_LE_DATA to L2CAP_CID_ATT
>      Bluetooth: Fix LE vs BR/EDR selection when connecting
>      Bluetooth: Fix EBUSY condition test in l2cap_chan_connect
>      Bluetooth: Fix hardcoding ATT CID in __l2cap_chan_add()
>      Bluetooth: Add clarifying comment to l2cap_conn_ready()
>      Bluetooth: Fix duplicate call to l2cap_chan_ready()
>      Bluetooth: Remove useless sk variable in l2cap_le_conn_ready
>      Bluetooth: Remove unnecessary L2CAP channel state check
>      Bluetooth: Simplify hci_conn_hold/drop logic for L2CAP
>      Bluetooth: Remove useless hci_conn disc_timeout setting
>      Bluetooth: Fix multiple LE socket handling
> 
> include/net/bluetooth/l2cap.h |   36 ++++++++--
> net/bluetooth/l2cap_core.c    |  145 ++++++++++++++++++++++++++---------------
> net/bluetooth/l2cap_sock.c    |    4 +-
> 3 files changed, 126 insertions(+), 59 deletions(-)

the whole set looks fine to me.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH 04/13] Bluetooth: Fix LE vs BR/EDR selection when connecting

[Vinicius Costa Gomes]

Hi Johan,

On 19:35 Mon 29 Apr, Johan Hedberg wrote:
> From: Johan Hedberg <johan.hedberg@intel.com>
> 
> The choice between LE and BR/EDR should be made on the destination
> address type instead of the destination CID. This is particularly
> important when in the future more than one CID will be allowed for LE.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index d5e4404..95d46cd 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1792,7 +1792,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
>  
>  	auth_type = l2cap_get_auth_type(chan);
>  
> -	if (chan->dcid == L2CAP_CID_ATT)
> +	if (bdaddr_type_is_le(dst_type))

Wouldn't this break applications that rely on the CID for making a LE 
connection?

>  		hcon = hci_connect(hdev, LE_LINK, dst, dst_type,
>  				   chan->sec_level, auth_type);
>  	else
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers,
-- 
Vinicius

Re: [PATCH 04/13] Bluetooth: Fix LE vs BR/EDR selection when connecting

[Marcel Holtmann]

Hi Vinicius,

>> The choice between LE and BR/EDR should be made on the destination
>> address type instead of the destination CID. This is particularly
>> important when in the future more than one CID will be allowed for LE.
>> 
>> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
>> ---
>> net/bluetooth/l2cap_core.c |    2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index d5e4404..95d46cd 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -1792,7 +1792,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
>> 
>> 	auth_type = l2cap_get_auth_type(chan);
>> 
>> -	if (chan->dcid == L2CAP_CID_ATT)
>> +	if (bdaddr_type_is_le(dst_type))
> 
> Wouldn't this break applications that rely on the CID for making a LE 
> connection?

which application are these. They are all from the time where enable_le=0 was the default. We introduced the BD_ADDR address type before we enabled LE by default.

Regards

Marcel

Re: [PATCH 04/13] Bluetooth: Fix LE vs BR/EDR selection when connecting

[Johan Hedberg]

Hi Vinicius,

On Tue, Apr 30, 2013, Vinicius Costa Gomes wrote:
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -1792,7 +1792,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
> >  
> >  	auth_type = l2cap_get_auth_type(chan);
> >  
> > -	if (chan->dcid == L2CAP_CID_ATT)
> > +	if (bdaddr_type_is_le(dst_type))
> 
> Wouldn't this break applications that rely on the CID for making a LE 
> connection?

An application that wants to make an LE connection but doesn't provide
an LE address type in the l2_bdaddr_type member of sockaddr_l2 is
broken.

Johan

Re: [PATCH 04/13] Bluetooth: Fix LE vs BR/EDR selection when connecting

[Vinicius Costa Gomes]

Hi,

On 10:51 Tue 30 Apr, Marcel Holtmann wrote:
> Hi Vinicius,
> 
> >> The choice between LE and BR/EDR should be made on the destination
> >> address type instead of the destination CID. This is particularly
> >> important when in the future more than one CID will be allowed for LE.
> >> 
> >> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> >> ---
> >> net/bluetooth/l2cap_core.c |    2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> >> index d5e4404..95d46cd 100644
> >> --- a/net/bluetooth/l2cap_core.c
> >> +++ b/net/bluetooth/l2cap_core.c
> >> @@ -1792,7 +1792,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
> >> 
> >> 	auth_type = l2cap_get_auth_type(chan);
> >> 
> >> -	if (chan->dcid == L2CAP_CID_ATT)
> >> +	if (bdaddr_type_is_le(dst_type))
> > 
> > Wouldn't this break applications that rely on the CID for making a LE 
> > connection?
> 
> which application are these. They are all from the time where enable_le=0 was the default. We introduced the BD_ADDR address type before we enabled LE by default.

I was thinking "old" BlueZ releases, 4.99 would have this issue, 4.101 is
fine.

Fair enough. That argument about the address type being added before LE being
enabled by default convinced me.

> 
> Regards
> 
> Marcel
> 

Cheers,
-- 
Vinicius

Re: [PATCH 13/13] Bluetooth: Fix multiple LE socket handling

[Gustavo Padovan]

Hi Johan,

* Johan Hedberg <johan.hedberg@gmail.com> [2013-04-29 19:35:45 +0300]:

> From: Johan Hedberg <johan.hedberg@intel.com>
> 
> The LE ATT server socket needs to be superseded by any ATT client
> sockets. Previously this was done by looking at the hcon->out variable
> (indicating whether the connection is outgoing or incoming) which is a
> too crude way of determining whether the server socket needs to be
> picked or not (an outgoing connection doesn't necessarily mean that an
> ATT client socket has triggered it).
> 
> This patch extends the ATT server socket lookup function
> (l2cap_le_conn_ready) to be used for all LE connections (regardless of
> the hcon->out value) and adds an internal check into the function for
> the existence of any ATT client sockets (in which case the server socket
> should be skipped). For this to work reliably all lookups must be done
> while the l2cap_conn->chan_lock is held, meaning also that the call to
> l2cap_chan_add needs to be changed to its lockless __l2cap_chan_add
> counterpart.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)

All 13 patches have been applied to bluetooth-next. Thanks.

	Gustavo