[PATCH 0/9] Bluetooth: Reference counting and other fixes

[PATCH 0/9] Bluetooth: Reference counting and other fixes

[johan.hedberg]

Hi,

Since we're not at the moment applying anything new to bluetooth-next
(due to the merge window being still open) I'm sending all my pending
patches in a single set to ease applying them in the correct order.

The two patches requiring some special consideration for 3.17 and stable
kernel trees are the following two:

   Bluetooth: Fix incorrect LE CoC PDU length restriction based on HCI MTU
   Bluetooth: Fix hci_conn reference counting for auto-connections

Of those two the second one is a "must have" for 3.17 whereas the first
one would be nice to get fixed for older kernels. All other patches in
this set are fine to wait for 3.18.

Johan

----------------------------------------------------------------
Johan Hedberg (9):
      Bluetooth: Fix incorrect LE CoC PDU length restriction based on HCI MTU
      Bluetooth: Remove unnecessary l2cap_chan_unlock before l2cap_chan_add
      Bluetooth: Fix hci_conn reference counting for fixed channels
      Bluetooth: Fix hci_conn reference counting for auto-connections
      Bluetooth: Set addr_type only when it's needed
      Bluetooth: Optimize connection parameter lookup for LE connections
      Bluetooth: Improve *_get() functions to return the object type
      Bluetooth: Fix using hci_conn_get() for hci_conn pointers
      Bluetooth: Refactor connection parameter freeing into its own function

 include/net/bluetooth/hci_core.h |  5 ++++-
 include/net/bluetooth/l2cap.h    |  3 ++-
 net/bluetooth/hci_conn.c         |  9 +++++++++
 net/bluetooth/hci_core.c         | 33 +++++++++++++++++++++++----------
 net/bluetooth/hci_event.c        | 31 +++++++++++++++++++++++--------
 net/bluetooth/hidp/core.c        | 10 ++++------
 net/bluetooth/l2cap_core.c       | 26 ++++++++++++++------------
 net/bluetooth/l2cap_sock.c       |  8 ++++++++
 net/bluetooth/mgmt.c             | 10 +++++++---
 9 files changed, 94 insertions(+), 41 deletions(-)

[PATCH 1/9] Bluetooth: Fix incorrect LE CoC PDU length restriction based on HCI MTU

[johan.hedberg]

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

The l2cap_create_le_flowctl_pdu() function that l2cap_segment_le_sdu()
calls is perfectly capable of doing packet fragmentation if given bigger
PDUs than the HCI buffers allow. Forcing the PDU length based on the HCI
MTU (conn->mtu) would therefore needlessly strict operation on hardware
with limited LE buffers (e.g. both Intel and Broadcom seem to have this
set to just 27 bytes).

This patch removes the restriction and makes it possible to send PDUs of
the full length that the remote MPS value allows.

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 4a90438d99df..52b56808d5d3 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2358,12 +2358,8 @@ static int l2cap_segment_le_sdu(struct l2cap_chan *chan,
 
 	BT_DBG("chan %p, msg %p, len %zu", chan, msg, len);
 
-	pdu_len = chan->conn->mtu - L2CAP_HDR_SIZE;
-
-	pdu_len = min_t(size_t, pdu_len, chan->remote_mps);
-
 	sdu_len = len;
-	pdu_len -= L2CAP_SDULEN_SIZE;
+	pdu_len = chan->remote_mps - L2CAP_SDULEN_SIZE;
 
 	while (len > 0) {
 		if (len <= pdu_len)
-- 
1.9.3

[PATCH 2/9] Bluetooth: Remove unnecessary l2cap_chan_unlock before l2cap_chan_add

[johan.hedberg]

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

The l2cap_chan_add() function doesn't require the channel to be
unlocked. It only requires the l2cap_conn to be unlocked. Therefore,
it's unnecessary to unlock a channel before calling l2cap_chan_add().
This patch removes such unnecessary unlocking from the
l2cap_chan_connect() function.

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 52b56808d5d3..777c41dbfdbe 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -7078,9 +7078,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 	bacpy(&chan->src, &hcon->src);
 	chan->src_type = bdaddr_type(hcon, hcon->src_type);
 
-	l2cap_chan_unlock(chan);
 	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);
-- 
1.9.3

[PATCH 3/9] Bluetooth: Fix hci_conn reference counting for fixed channels

[johan.hedberg]

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

Now that SMP has been converted to use fixed channels we've got a bit of
a problem with the hci_conn reference counting. So far the L2CAP code
has kept a reference for each L2CAP channel that was notified of the
connection. With SMP however this would mean that the connection is
never dropped even though there are no other users of it. Furthermore,
SMP already does its own hci_conn reference counting internally,
starting from a security or pairing request and ending with the key
distribution.

This patch makes L2CAP fixed channels default to the L2CAP core not
keeping a hci_conn reference for them. A new FLAG_HOLD_HCI_CONN flag is
added so that L2CAP users can declare an exception to this rule and hold
a reference even for their fixed channels. One such exception is the
L2CAP socket layer which does want a reference for each socket (e.g. an
ATT socket which uses a fixed channel).

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

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index cedda399f9c0..1558eccb19db 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -711,6 +711,7 @@ enum {
 	FLAG_DEFER_SETUP,
 	FLAG_LE_CONN_REQ_SENT,
 	FLAG_PENDING_SECURITY,
+	FLAG_HOLD_HCI_CONN,
 };
 
 enum {
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 777c41dbfdbe..a6559225bb50 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -546,7 +546,10 @@ void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 
 	l2cap_chan_hold(chan);
 
-	hci_conn_hold(conn->hcon);
+	/* Only keep a reference for fixed channels if they requested it */
+	if (chan->chan_type != L2CAP_CHAN_FIXED ||
+	    test_bit(FLAG_HOLD_HCI_CONN, &chan->flags))
+		hci_conn_hold(conn->hcon);
 
 	list_add(&chan->list, &conn->chan_l);
 }
@@ -577,7 +580,12 @@ void l2cap_chan_del(struct l2cap_chan *chan, int err)
 
 		chan->conn = NULL;
 
-		if (chan->scid != L2CAP_CID_A2MP)
+		/* Reference was only held for non-fixed channels or
+		 * fixed channels that explicitly requested it using the
+		 * FLAG_HOLD_HCI_CONN flag.
+		 */
+		if (chan->chan_type != L2CAP_CHAN_FIXED ||
+		    test_bit(FLAG_HOLD_HCI_CONN, &chan->flags))
 			hci_conn_drop(conn->hcon);
 
 		if (mgr && mgr->bredr_chan == chan)
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index ed06f88e6f10..75eff0056b42 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -146,6 +146,14 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
 	case L2CAP_CHAN_RAW:
 		chan->sec_level = BT_SECURITY_SDP;
 		break;
+	case L2CAP_CHAN_FIXED:
+		/* Fixed channels default to the L2CAP core not holding a
+		 * hci_conn reference for them. For fixed channels mapping to
+		 * L2CAP sockets we do want to hold a reference so set the
+		 * appropriate flag to request it.
+		 */
+		set_bit(FLAG_HOLD_HCI_CONN, &l2cap_pi(sk)->chan->flags);
+		break;
 	}
 
 	bacpy(&chan->src, &la.l2_bdaddr);
-- 
1.9.3

[PATCH 4/9] Bluetooth: Fix hci_conn reference counting for auto-connections

[johan.hedberg]

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

Recently the LE passive scanning and auto-connections feature was
introduced. It uses the hci_connect_le() API which returns a hci_conn
along with a reference count to that object. All previous users would
tie this returned reference to some existing object, such as an L2CAP
channel, and there'd be no leaked references this way. For
auto-connections however the reference was returned but not stored
anywhere, leaving established connections with one higher reference
count than they should have.

Instead of playing special tricks with hci_conn_hold/drop this patch
associates the returned reference from hci_connect_le() with the object
that in practice does own this reference, i.e. the hci_conn_params
struct that caused us to initiate a connection in the first place. Once
the connection is established or fails to establish this reference is
removed appropriately.

One extra thing needed is to call hci_pend_le_actions_clear() before
calling hci_conn_hash_flush() so that the reference is cleared before
the hci_conn objects are fully removed.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |  2 ++
 net/bluetooth/hci_conn.c         |  8 ++++++++
 net/bluetooth/hci_core.c         | 14 ++++++++++++--
 net/bluetooth/hci_event.c        | 17 +++++++++++++++--
 4 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 5f0b77b71b45..b0ded1333865 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -464,6 +464,8 @@ struct hci_conn_params {
 		HCI_AUTO_CONN_ALWAYS,
 		HCI_AUTO_CONN_LINK_LOSS,
 	} auto_connect;
+
+	struct hci_conn *conn;
 };
 
 extern struct list_head hci_dev_list;
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b50dabb3f86a..faff6247ac8f 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -589,6 +589,14 @@ EXPORT_SYMBOL(hci_get_route);
 void hci_le_conn_failed(struct hci_conn *conn, u8 status)
 {
 	struct hci_dev *hdev = conn->hdev;
+	struct hci_conn_params *params;
+
+	params = hci_pend_le_action_lookup(&hdev->pend_le_conns, &conn->dst,
+					   conn->dst_type);
+	if (params && params->conn) {
+		hci_conn_drop(params->conn);
+		params->conn = NULL;
+	}
 
 	conn->state = BT_CLOSED;
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index abeb5e47311e..9b7145959a49 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2538,8 +2538,13 @@ static void hci_pend_le_actions_clear(struct hci_dev *hdev)
 {
 	struct hci_conn_params *p;
 
-	list_for_each_entry(p, &hdev->le_conn_params, list)
+	list_for_each_entry(p, &hdev->le_conn_params, list) {
+		if (p->conn) {
+			hci_conn_drop(p->conn);
+			p->conn = NULL;
+		}
 		list_del_init(&p->action);
+	}
 
 	BT_DBG("All LE pending actions cleared");
 }
@@ -2580,8 +2585,8 @@ static int hci_dev_do_close(struct hci_dev *hdev)
 
 	hci_dev_lock(hdev);
 	hci_inquiry_cache_flush(hdev);
-	hci_conn_hash_flush(hdev);
 	hci_pend_le_actions_clear(hdev);
+	hci_conn_hash_flush(hdev);
 	hci_dev_unlock(hdev);
 
 	hci_notify(hdev, HCI_DEV_DOWN);
@@ -3729,6 +3734,9 @@ void hci_conn_params_del(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
 	if (!params)
 		return;
 
+	if (params->conn)
+		hci_conn_drop(params->conn);
+
 	list_del(&params->action);
 	list_del(&params->list);
 	kfree(params);
@@ -3759,6 +3767,8 @@ void hci_conn_params_clear_all(struct hci_dev *hdev)
 	struct hci_conn_params *params, *tmp;
 
 	list_for_each_entry_safe(params, tmp, &hdev->le_conn_params, list) {
+		if (params->conn)
+			hci_conn_drop(params->conn);
 		list_del(&params->action);
 		list_del(&params->list);
 		kfree(params);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index da7ab6b9bb69..3a99f30a3317 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4226,8 +4226,13 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	hci_proto_connect_cfm(conn, ev->status);
 
 	params = hci_conn_params_lookup(hdev, &conn->dst, conn->dst_type);
-	if (params)
+	if (params) {
 		list_del_init(&params->action);
+		if (params->conn) {
+			hci_conn_drop(params->conn);
+			params->conn = NULL;
+		}
+	}
 
 unlock:
 	hci_update_background_scan(hdev);
@@ -4309,8 +4314,16 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
 
 	conn = hci_connect_le(hdev, addr, addr_type, BT_SECURITY_LOW,
 			      HCI_LE_AUTOCONN_TIMEOUT, HCI_ROLE_MASTER);
-	if (!IS_ERR(conn))
+	if (!IS_ERR(conn)) {
+		/* Store the pointer since we don't really have any
+		 * other owner of the object besides the params that
+		 * triggered it. This way we can abort the connection if
+		 * the parameters get removed and keep the reference
+		 * count consistent once the connection is established.
+		 */
+		params->conn = conn;
 		return;
+	}
 
 	switch (PTR_ERR(conn)) {
 	case -EBUSY:
-- 
1.9.3

[PATCH 5/9] Bluetooth: Set addr_type only when it's needed

[johan.hedberg]

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

In the hci_le_conn_complete_evt() function there's no need to set the
addr_type value until it's actually needed, i.e. for the black list
lookup. This patch moves the code a bit further down in the function.

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

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 3a99f30a3317..e8f35a90add5 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4193,16 +4193,16 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		conn->dst_type = irk->addr_type;
 	}
 
-	if (conn->dst_type == ADDR_LE_DEV_PUBLIC)
-		addr_type = BDADDR_LE_PUBLIC;
-	else
-		addr_type = BDADDR_LE_RANDOM;
-
 	if (ev->status) {
 		hci_le_conn_failed(conn, ev->status);
 		goto unlock;
 	}
 
+	if (conn->dst_type == ADDR_LE_DEV_PUBLIC)
+		addr_type = BDADDR_LE_PUBLIC;
+	else
+		addr_type = BDADDR_LE_RANDOM;
+
 	/* Drop the connection if the device is blocked */
 	if (hci_bdaddr_list_lookup(&hdev->blacklist, &conn->dst, addr_type)) {
 		hci_conn_drop(conn);
-- 
1.9.3

[PATCH 6/9] Bluetooth: Optimize connection parameter lookup for LE connections

[johan.hedberg]

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

When we get an LE connection complete event there's really no reason to
look through the entire connection parameter list as the entry should be
present in the hdev->pend_le_conns list too. This patch changes the
lookup code to do a more restricted lookup only in the pend_le_conns
list.

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

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index e8f35a90add5..d2ee162ecddb 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4225,7 +4225,8 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	hci_proto_connect_cfm(conn, ev->status);
 
-	params = hci_conn_params_lookup(hdev, &conn->dst, conn->dst_type);
+	params = hci_pend_le_action_lookup(&hdev->pend_le_conns, &conn->dst,
+					   conn->dst_type);
 	if (params) {
 		list_del_init(&params->action);
 		if (params->conn) {
-- 
1.9.3

[PATCH 7/9] Bluetooth: Improve *_get() functions to return the object type

[johan.hedberg]

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

It's natural to have *_get() functions that increment the reference
count of an object to return the object type itself. This way it's
simple to make a copy of the object pointer and increase the reference
count in a single step. This patch updates two such get() functions,
namely hci_conn_get() and l2cap_conn_get(), and updates the users to
take advantage of the new API.

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index b0ded1333865..aa75eee8ad7f 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -756,9 +756,10 @@ void hci_le_conn_failed(struct hci_conn *conn, u8 status);
  * _get()/_drop() in it, but require the caller to have a valid ref (FIXME).
  */
 
-static inline void hci_conn_get(struct hci_conn *conn)
+static inline struct hci_conn *hci_conn_get(struct hci_conn *conn)
 {
 	get_device(&conn->dev);
+	return conn;
 }
 
 static inline void hci_conn_put(struct hci_conn *conn)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 1558eccb19db..8f1652ed3326 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -948,7 +948,7 @@ void l2cap_logical_cfm(struct l2cap_chan *chan, struct hci_chan *hchan,
 void __l2cap_physical_cfm(struct l2cap_chan *chan, int result);
 
 void l2cap_conn_shutdown(struct l2cap_conn *conn, int err);
-void l2cap_conn_get(struct l2cap_conn *conn);
+struct l2cap_conn *l2cap_conn_get(struct l2cap_conn *conn);
 void l2cap_conn_put(struct l2cap_conn *conn);
 
 int l2cap_register_user(struct l2cap_conn *conn, struct l2cap_user *user);
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 6c7ecf116e74..1b7d605706aa 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -915,7 +915,7 @@ static int hidp_session_new(struct hidp_session **out, const bdaddr_t *bdaddr,
 
 	/* connection management */
 	bacpy(&session->bdaddr, bdaddr);
-	session->conn = conn;
+	session->conn = l2cap_conn_get(conn);
 	session->user.probe = hidp_session_probe;
 	session->user.remove = hidp_session_remove;
 	session->ctrl_sock = ctrl_sock;
@@ -941,13 +941,13 @@ static int hidp_session_new(struct hidp_session **out, const bdaddr_t *bdaddr,
 	if (ret)
 		goto err_free;
 
-	l2cap_conn_get(session->conn);
 	get_file(session->intr_sock->file);
 	get_file(session->ctrl_sock->file);
 	*out = session;
 	return 0;
 
 err_free:
+	l2cap_conn_put(session->conn);
 	kfree(session);
 	return ret;
 }
@@ -1327,10 +1327,8 @@ int hidp_connection_add(struct hidp_connadd_req *req,
 
 	conn = NULL;
 	l2cap_chan_lock(chan);
-	if (chan->conn) {
-		l2cap_conn_get(chan->conn);
-		conn = chan->conn;
-	}
+	if (chan->conn)
+		conn = l2cap_conn_get(chan->conn);
 	l2cap_chan_unlock(chan);
 
 	if (!conn)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index a6559225bb50..cb36169ef300 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1695,9 +1695,10 @@ static void l2cap_conn_free(struct kref *ref)
 	kfree(conn);
 }
 
-void l2cap_conn_get(struct l2cap_conn *conn)
+struct l2cap_conn *l2cap_conn_get(struct l2cap_conn *conn)
 {
 	kref_get(&conn->ref);
+	return conn;
 }
 EXPORT_SYMBOL(l2cap_conn_get);
 
@@ -6908,8 +6909,7 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon)
 
 	kref_init(&conn->ref);
 	hcon->l2cap_data = conn;
-	conn->hcon = hcon;
-	hci_conn_get(conn->hcon);
+	conn->hcon = hci_conn_get(hcon);
 	conn->hchan = hchan;
 
 	BT_DBG("hcon %p conn %p hchan %p", hcon, conn, hchan);
-- 
1.9.3

[PATCH 8/9] Bluetooth: Fix using hci_conn_get() for hci_conn pointers

[johan.hedberg]

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

Wherever we keep hci_conn pointers around we should be using
hci_conn_get/put to ensure that they stay valid. This patch fixes
all places violating against the principle currently.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_conn.c  |  1 +
 net/bluetooth/hci_core.c  |  9 +++++++--
 net/bluetooth/hci_event.c |  3 ++-
 net/bluetooth/mgmt.c      | 10 +++++++---
 4 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index faff6247ac8f..4ecc9d5fce7a 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -595,6 +595,7 @@ void hci_le_conn_failed(struct hci_conn *conn, u8 status)
 					   conn->dst_type);
 	if (params && params->conn) {
 		hci_conn_drop(params->conn);
+		hci_conn_put(params->conn);
 		params->conn = NULL;
 	}
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 9b7145959a49..ed60d37ea646 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2541,6 +2541,7 @@ static void hci_pend_le_actions_clear(struct hci_dev *hdev)
 	list_for_each_entry(p, &hdev->le_conn_params, list) {
 		if (p->conn) {
 			hci_conn_drop(p->conn);
+			hci_conn_put(p->conn);
 			p->conn = NULL;
 		}
 		list_del_init(&p->action);
@@ -3734,8 +3735,10 @@ void hci_conn_params_del(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
 	if (!params)
 		return;
 
-	if (params->conn)
+	if (params->conn) {
 		hci_conn_drop(params->conn);
+		hci_conn_put(params->conn);
+	}
 
 	list_del(&params->action);
 	list_del(&params->list);
@@ -3767,8 +3770,10 @@ void hci_conn_params_clear_all(struct hci_dev *hdev)
 	struct hci_conn_params *params, *tmp;
 
 	list_for_each_entry_safe(params, tmp, &hdev->le_conn_params, list) {
-		if (params->conn)
+		if (params->conn) {
 			hci_conn_drop(params->conn);
+			hci_conn_put(params->conn);
+		}
 		list_del(&params->action);
 		list_del(&params->list);
 		kfree(params);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index d2ee162ecddb..e6a496ae0318 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4231,6 +4231,7 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		list_del_init(&params->action);
 		if (params->conn) {
 			hci_conn_drop(params->conn);
+			hci_conn_put(params->conn);
 			params->conn = NULL;
 		}
 	}
@@ -4322,7 +4323,7 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
 		 * the parameters get removed and keep the reference
 		 * count consistent once the connection is established.
 		 */
-		params->conn = conn;
+		params->conn = hci_conn_get(conn);
 		return;
 	}
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index c2457435a670..c2d571c10544 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3063,6 +3063,7 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
 	conn->disconn_cfm_cb = NULL;
 
 	hci_conn_drop(conn);
+	hci_conn_put(conn);
 
 	mgmt_pending_remove(cmd);
 }
@@ -4914,6 +4915,7 @@ static void get_conn_info_complete(struct pending_cmd *cmd, void *data)
 		     match->mgmt_status, &rp, sizeof(rp));
 
 	hci_conn_drop(conn);
+	hci_conn_put(conn);
 
 	mgmt_pending_remove(cmd);
 }
@@ -5070,7 +5072,7 @@ static int get_conn_info(struct sock *sk, struct hci_dev *hdev, void *data,
 		}
 
 		hci_conn_hold(conn);
-		cmd->user_data = conn;
+		cmd->user_data = hci_conn_get(conn);
 
 		conn->conn_info_timestamp = jiffies;
 	} else {
@@ -5134,8 +5136,10 @@ send_rsp:
 	cmd_complete(cmd->sk, cmd->index, cmd->opcode, mgmt_status(status),
 		     &rp, sizeof(rp));
 	mgmt_pending_remove(cmd);
-	if (conn)
+	if (conn) {
 		hci_conn_drop(conn);
+		hci_conn_put(conn);
+	}
 
 unlock:
 	hci_dev_unlock(hdev);
@@ -5198,7 +5202,7 @@ static int get_clock_info(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	if (conn) {
 		hci_conn_hold(conn);
-		cmd->user_data = conn;
+		cmd->user_data = hci_conn_get(conn);
 
 		hci_cp.handle = cpu_to_le16(conn->handle);
 		hci_cp.which = 0x01; /* Piconet clock */
-- 
1.9.3

[PATCH 9/9] Bluetooth: Refactor connection parameter freeing into its own function

[johan.hedberg]

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

The necessary steps for freeing connection paramaters have grown quite a
bit so we can simplify the code by factoring it out into its own
function.

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index ed60d37ea646..0d3782ad9a5b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3726,15 +3726,8 @@ int hci_conn_params_set(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
 	return 0;
 }
 
-/* This function requires the caller holds hdev->lock */
-void hci_conn_params_del(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
+static void hci_conn_params_free(struct hci_conn_params *params)
 {
-	struct hci_conn_params *params;
-
-	params = hci_conn_params_lookup(hdev, addr, addr_type);
-	if (!params)
-		return;
-
 	if (params->conn) {
 		hci_conn_drop(params->conn);
 		hci_conn_put(params->conn);
@@ -3743,6 +3736,18 @@ void hci_conn_params_del(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
 	list_del(&params->action);
 	list_del(&params->list);
 	kfree(params);
+}
+
+/* This function requires the caller holds hdev->lock */
+void hci_conn_params_del(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
+{
+	struct hci_conn_params *params;
+
+	params = hci_conn_params_lookup(hdev, addr, addr_type);
+	if (!params)
+		return;
+
+	hci_conn_params_free(params);
 
 	hci_update_background_scan(hdev);
 
@@ -3769,15 +3774,8 @@ void hci_conn_params_clear_all(struct hci_dev *hdev)
 {
 	struct hci_conn_params *params, *tmp;
 
-	list_for_each_entry_safe(params, tmp, &hdev->le_conn_params, list) {
-		if (params->conn) {
-			hci_conn_drop(params->conn);
-			hci_conn_put(params->conn);
-		}
-		list_del(&params->action);
-		list_del(&params->list);
-		kfree(params);
-	}
+	list_for_each_entry_safe(params, tmp, &hdev->le_conn_params, list)
+		hci_conn_params_free(params);
 
 	hci_update_background_scan(hdev);
 
-- 
1.9.3

[PATCH v2 3/9] Bluetooth: Fix hci_conn reference counting for fixed channels

[johan.hedberg]

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

Now that SMP has been converted to use fixed channels we've got a bit of
a problem with the hci_conn reference counting. So far the L2CAP code
has kept a reference for each L2CAP channel that was notified of the
connection. With SMP however this would mean that the connection is
never dropped even though there are no other users of it. Furthermore,
SMP already does its own hci_conn reference counting internally,
starting from a security or pairing request and ending with the key
distribution.

This patch makes L2CAP fixed channels default to the L2CAP core not
keeping a hci_conn reference for them. A new FLAG_HOLD_HCI_CONN flag is
added so that L2CAP users can declare an exception to this rule and hold
a reference even for their fixed channels. One such exception is the
L2CAP socket layer which does want a reference for each socket (e.g. an
ATT socket which uses a fixed channel).

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
v2: Simplify chan reference in set_bit() call

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

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index cedda399f9c0..1558eccb19db 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -711,6 +711,7 @@ enum {
 	FLAG_DEFER_SETUP,
 	FLAG_LE_CONN_REQ_SENT,
 	FLAG_PENDING_SECURITY,
+	FLAG_HOLD_HCI_CONN,
 };
 
 enum {
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 777c41dbfdbe..a6559225bb50 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -546,7 +546,10 @@ void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 
 	l2cap_chan_hold(chan);
 
-	hci_conn_hold(conn->hcon);
+	/* Only keep a reference for fixed channels if they requested it */
+	if (chan->chan_type != L2CAP_CHAN_FIXED ||
+	    test_bit(FLAG_HOLD_HCI_CONN, &chan->flags))
+		hci_conn_hold(conn->hcon);
 
 	list_add(&chan->list, &conn->chan_l);
 }
@@ -577,7 +580,12 @@ void l2cap_chan_del(struct l2cap_chan *chan, int err)
 
 		chan->conn = NULL;
 
-		if (chan->scid != L2CAP_CID_A2MP)
+		/* Reference was only held for non-fixed channels or
+		 * fixed channels that explicitly requested it using the
+		 * FLAG_HOLD_HCI_CONN flag.
+		 */
+		if (chan->chan_type != L2CAP_CHAN_FIXED ||
+		    test_bit(FLAG_HOLD_HCI_CONN, &chan->flags))
 			hci_conn_drop(conn->hcon);
 
 		if (mgr && mgr->bredr_chan == chan)
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index ed06f88e6f10..31f106e61ca2 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -146,6 +146,14 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
 	case L2CAP_CHAN_RAW:
 		chan->sec_level = BT_SECURITY_SDP;
 		break;
+	case L2CAP_CHAN_FIXED:
+		/* Fixed channels default to the L2CAP core not holding a
+		 * hci_conn reference for them. For fixed channels mapping to
+		 * L2CAP sockets we do want to hold a reference so set the
+		 * appropriate flag to request it.
+		 */
+		set_bit(FLAG_HOLD_HCI_CONN, &chan->flags);
+		break;
 	}
 
 	bacpy(&chan->src, &la.l2_bdaddr);
-- 
1.9.3

[PATCH v2 8/9] Bluetooth: Fix using hci_conn_get() for hci_conn pointers

[johan.hedberg]

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

Wherever we keep hci_conn pointers around we should be using
hci_conn_get/put to ensure that they stay valid. This patch fixes
all places violating against the principle currently.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
v2: Fix missing hci_conn_get() in pair_device()

 net/bluetooth/hci_conn.c  |  1 +
 net/bluetooth/hci_core.c  |  9 +++++++--
 net/bluetooth/hci_event.c |  3 ++-
 net/bluetooth/mgmt.c      | 12 ++++++++----
 4 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index faff6247ac8f..4ecc9d5fce7a 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -595,6 +595,7 @@ void hci_le_conn_failed(struct hci_conn *conn, u8 status)
 					   conn->dst_type);
 	if (params && params->conn) {
 		hci_conn_drop(params->conn);
+		hci_conn_put(params->conn);
 		params->conn = NULL;
 	}
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 9b7145959a49..ed60d37ea646 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2541,6 +2541,7 @@ static void hci_pend_le_actions_clear(struct hci_dev *hdev)
 	list_for_each_entry(p, &hdev->le_conn_params, list) {
 		if (p->conn) {
 			hci_conn_drop(p->conn);
+			hci_conn_put(p->conn);
 			p->conn = NULL;
 		}
 		list_del_init(&p->action);
@@ -3734,8 +3735,10 @@ void hci_conn_params_del(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
 	if (!params)
 		return;
 
-	if (params->conn)
+	if (params->conn) {
 		hci_conn_drop(params->conn);
+		hci_conn_put(params->conn);
+	}
 
 	list_del(&params->action);
 	list_del(&params->list);
@@ -3767,8 +3770,10 @@ void hci_conn_params_clear_all(struct hci_dev *hdev)
 	struct hci_conn_params *params, *tmp;
 
 	list_for_each_entry_safe(params, tmp, &hdev->le_conn_params, list) {
-		if (params->conn)
+		if (params->conn) {
 			hci_conn_drop(params->conn);
+			hci_conn_put(params->conn);
+		}
 		list_del(&params->action);
 		list_del(&params->list);
 		kfree(params);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index d2ee162ecddb..e6a496ae0318 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4231,6 +4231,7 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		list_del_init(&params->action);
 		if (params->conn) {
 			hci_conn_drop(params->conn);
+			hci_conn_put(params->conn);
 			params->conn = NULL;
 		}
 	}
@@ -4322,7 +4323,7 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
 		 * the parameters get removed and keep the reference
 		 * count consistent once the connection is established.
 		 */
-		params->conn = conn;
+		params->conn = hci_conn_get(conn);
 		return;
 	}
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index c2457435a670..d8c66663ade8 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3063,6 +3063,7 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
 	conn->disconn_cfm_cb = NULL;
 
 	hci_conn_drop(conn);
+	hci_conn_put(conn);
 
 	mgmt_pending_remove(cmd);
 }
@@ -3212,7 +3213,7 @@ static int pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
 	}
 
 	conn->io_capability = cp->io_cap;
-	cmd->user_data = conn;
+	cmd->user_data = hci_conn_get(conn);
 
 	if ((conn->state == BT_CONNECTED || conn->state == BT_CONFIG) &&
 	    hci_conn_security(conn, sec_level, auth_type, true))
@@ -4914,6 +4915,7 @@ static void get_conn_info_complete(struct pending_cmd *cmd, void *data)
 		     match->mgmt_status, &rp, sizeof(rp));
 
 	hci_conn_drop(conn);
+	hci_conn_put(conn);
 
 	mgmt_pending_remove(cmd);
 }
@@ -5070,7 +5072,7 @@ static int get_conn_info(struct sock *sk, struct hci_dev *hdev, void *data,
 		}
 
 		hci_conn_hold(conn);
-		cmd->user_data = conn;
+		cmd->user_data = hci_conn_get(conn);
 
 		conn->conn_info_timestamp = jiffies;
 	} else {
@@ -5134,8 +5136,10 @@ send_rsp:
 	cmd_complete(cmd->sk, cmd->index, cmd->opcode, mgmt_status(status),
 		     &rp, sizeof(rp));
 	mgmt_pending_remove(cmd);
-	if (conn)
+	if (conn) {
 		hci_conn_drop(conn);
+		hci_conn_put(conn);
+	}
 
 unlock:
 	hci_dev_unlock(hdev);
@@ -5198,7 +5202,7 @@ static int get_clock_info(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	if (conn) {
 		hci_conn_hold(conn);
-		cmd->user_data = conn;
+		cmd->user_data = hci_conn_get(conn);
 
 		hci_cp.handle = cpu_to_le16(conn->handle);
 		hci_cp.which = 0x01; /* Piconet clock */
-- 
1.9.3

Re: [PATCH 0/9] Bluetooth: Reference counting and other fixes

[Marcel Holtmann]

Hi Johan,

> Since we're not at the moment applying anything new to bluetooth-next
> (due to the merge window being still open) I'm sending all my pending
> patches in a single set to ease applying them in the correct order.
> 
> The two patches requiring some special consideration for 3.17 and stable
> kernel trees are the following two:
> 
>   Bluetooth: Fix incorrect LE CoC PDU length restriction based on HCI MTU
>   Bluetooth: Fix hci_conn reference counting for auto-connections
> 
> Of those two the second one is a "must have" for 3.17 whereas the first
> one would be nice to get fixed for older kernels. All other patches in
> this set are fine to wait for 3.18.
> 
> Johan
> 
> ----------------------------------------------------------------
> Johan Hedberg (9):
>      Bluetooth: Fix incorrect LE CoC PDU length restriction based on HCI MTU
>      Bluetooth: Remove unnecessary l2cap_chan_unlock before l2cap_chan_add
>      Bluetooth: Fix hci_conn reference counting for fixed channels
>      Bluetooth: Fix hci_conn reference counting for auto-connections
>      Bluetooth: Set addr_type only when it's needed
>      Bluetooth: Optimize connection parameter lookup for LE connections
>      Bluetooth: Improve *_get() functions to return the object type
>      Bluetooth: Fix using hci_conn_get() for hci_conn pointers
>      Bluetooth: Refactor connection parameter freeing into its own function
> 
> include/net/bluetooth/hci_core.h |  5 ++++-
> include/net/bluetooth/l2cap.h    |  3 ++-
> net/bluetooth/hci_conn.c         |  9 +++++++++
> net/bluetooth/hci_core.c         | 33 +++++++++++++++++++++++----------
> net/bluetooth/hci_event.c        | 31 +++++++++++++++++++++++--------
> net/bluetooth/hidp/core.c        | 10 ++++------
> net/bluetooth/l2cap_core.c       | 26 ++++++++++++++------------
> net/bluetooth/l2cap_sock.c       |  8 ++++++++
> net/bluetooth/mgmt.c             | 10 +++++++---
> 9 files changed, 94 insertions(+), 41 deletions(-)

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

Regards

Marcel