[PATCH 0/8] Bluetooth: SMP & disconnection fixes

[PATCH 0/8] Bluetooth: SMP & disconnection fixes

[johan.hedberg]

Hi,

The previous patches for SMP disconnection (that already got applied) do
indeed fix the hci_conn_hold/drop ballance and do ensure that we
disconnect the link when the SMP code wants it. Unfortunately the
patches introduce a regression to a third use case which is the one
where we want to bring the link down when there are no more active users
for it.

It turns out that relying on connection users to diligently call
hci_conn_drop() isn't a reliable way to ensure that the connection goes
away. Firstly, we can't have hci_chan own such a reference (since it's
"always there") and secondly there might not be any users at all
(besides SMP) in which case there will be no-one to call hci_conn_drop.

A simpler solution is to just do a direct disonnect from smp.c with the
help of the hci_disconnect() function. This has the side effect of not
sending any SMP Failure PDU first, but the cases needing a disconnection
do not really need that PDU to be sent (i.e. SMP timeout or garbage
data).

The last three patches in this set unify hci_disconnect() usage to also
cover the Disconnect Device mgmt command and move clock offset reading
into the function to ensure all users have this feature at their
disposal.

Johan

----------------------------------------------------------------
Johan Hedberg (8):
      Bluetooth: Remove hci_conn_hold/drop from hci_chan
      Bluetooth: Set discon_timeout to 0 in l2cap_conn_del
      Bluetooth: Use hci_disconnect for immediate disconnection from SMP
      Bluetooth: Remove unused l2cap_conn_shutdown API
      Bluetooth: Fix SMP error and response to be mutually exclusive
      Bluetooth: Update hci_disconnect() to return an error value
      Bluetooth: Use hci_disconnect() for mgmt_disconnect_device()
      Bluetooth: Move clock offset reading into hci_disconnect()

 include/net/bluetooth/hci_core.h |  2 +-
 include/net/bluetooth/l2cap.h    |  4 ----
 net/bluetooth/hci_conn.c         | 41 ++++++++++++++----------------------
 net/bluetooth/l2cap_core.c       | 28 +++---------------------
 net/bluetooth/mgmt.c             |  6 +-----
 net/bluetooth/smp.c              | 17 ++++++---------
 6 files changed, 28 insertions(+), 70 deletions(-)

[PATCH 1/8] Bluetooth: Remove hci_conn_hold/drop from hci_chan

[johan.hedberg]

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

We can't have hci_chan contribute to the "active" reference counting of
the hci_conn since otherwise the connection would never get dropped when
there are no more users (since hci_chan would be counted as a user).
This patch removes hold() when creating the hci_chan and drop() when
destroying it.

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

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index aaa7e388d026..5157a0990732 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1301,7 +1301,6 @@ struct hci_chan *hci_chan_create(struct hci_conn *conn)
 		return NULL;
 
 	chan->conn = hci_conn_get(conn);
-	hci_conn_hold(conn);
 	skb_queue_head_init(&chan->data_q);
 	chan->state = BT_CONNECTED;
 
@@ -1321,11 +1320,9 @@ void hci_chan_del(struct hci_chan *chan)
 
 	synchronize_rcu();
 
-	/* Force the connection to be immediately dropped */
-	conn->disc_timeout = 0;
+	/* Prevent new hci_chan's to be created for this hci_conn */
 	set_bit(HCI_CONN_DROP, &conn->flags);
 
-	hci_conn_drop(conn);
 	hci_conn_put(conn);
 
 	skb_queue_purge(&chan->data_q);
-- 
1.9.3

[PATCH 2/8] Bluetooth: Set discon_timeout to 0 in l2cap_conn_del

[johan.hedberg]

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

When the l2cap_conn_del() function is used we do not want to wait around
"in case something happens" before disconnecting. This patch sets the
disconnection timeout to 0 so that the disconnection routines get
immediately scheduled.

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 cb36169ef300..2d550afe4322 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1640,6 +1640,9 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 
 	l2cap_unregister_all_users(conn);
 
+	/* Force the connection to be immediately dropped */
+	hcon->disc_timeout = 0;
+
 	mutex_lock(&conn->chan_lock);
 
 	/* Kill channels */
-- 
1.9.3

[PATCH 3/8] Bluetooth: Use hci_disconnect for immediate disconnection from SMP

[johan.hedberg]

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

Relying on the l2cap_conn_del procedure (triggered through the
l2cap_conn_shutdown API) to get the connection disconnected is not
reliable as it depends on all users releasing (through hci_conn_drop)
and that there's at least one user (so hci_conn_drop is called at least
one time).

A much simpler and more reliable solution is to call hci_disconnect()
directly from the SMP code when we want to disconnect. One side-effect
this has is that it prevents any SMP Failure PDU from being sent before
the disconnection, however neither one of the scenarios where
l2cap_conn_shutdown was used really requires this.

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

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 07ca4ce0943b..496584921fdc 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -829,7 +829,7 @@ static void smp_timeout(struct work_struct *work)
 
 	BT_DBG("conn %p", conn);
 
-	l2cap_conn_shutdown(conn, ETIMEDOUT);
+	hci_disconnect(conn->hcon, HCI_ERROR_REMOTE_USER_TERM);
 }
 
 static struct smp_chan *smp_chan_create(struct l2cap_conn *conn)
@@ -1569,7 +1569,7 @@ static int smp_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
 		if (smp)
 			cancel_delayed_work_sync(&smp->security_timer);
 
-		l2cap_conn_shutdown(chan->conn, -err);
+		hci_disconnect(chan->conn->hcon, HCI_ERROR_AUTH_FAILURE);
 	}
 
 	return err;
-- 
1.9.3

[PATCH 4/8] Bluetooth: Remove unused l2cap_conn_shutdown API

[johan.hedberg]

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

Now that there are no more users of the l2cap_conn_shutdown API (since
smp.c switched to using hci_disconnect) we can simply remove it along
with all of it's l2cap_conn variables.

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

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 8f1652ed3326..be25eddea615 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -625,9 +625,6 @@ struct l2cap_conn {
 
 	struct delayed_work	info_timer;
 
-	int			disconn_err;
-	struct work_struct	disconn_work;
-
 	struct sk_buff		*rx_skb;
 	__u32			rx_len;
 	__u8			tx_ident;
@@ -947,7 +944,6 @@ void l2cap_logical_cfm(struct l2cap_chan *chan, struct hci_chan *hchan,
 		       u8 status);
 void __l2cap_physical_cfm(struct l2cap_chan *chan, int result);
 
-void l2cap_conn_shutdown(struct l2cap_conn *conn, int err);
 struct l2cap_conn *l2cap_conn_get(struct l2cap_conn *conn);
 void l2cap_conn_put(struct l2cap_conn *conn);
 
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 2d550afe4322..2d9a2b58d2c8 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1635,9 +1635,6 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 	if (work_pending(&conn->pending_rx_work))
 		cancel_work_sync(&conn->pending_rx_work);
 
-	if (work_pending(&conn->disconn_work))
-		cancel_work_sync(&conn->disconn_work);
-
 	l2cap_unregister_all_users(conn);
 
 	/* Force the connection to be immediately dropped */
@@ -1670,26 +1667,6 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 	l2cap_conn_put(conn);
 }
 
-static void disconn_work(struct work_struct *work)
-{
-	struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
-					       disconn_work);
-
-	BT_DBG("conn %p", conn);
-
-	l2cap_conn_del(conn->hcon, conn->disconn_err);
-}
-
-void l2cap_conn_shutdown(struct l2cap_conn *conn, int err)
-{
-	struct hci_dev *hdev = conn->hcon->hdev;
-
-	BT_DBG("conn %p err %d", conn, err);
-
-	conn->disconn_err = err;
-	queue_work(hdev->workqueue, &conn->disconn_work);
-}
-
 static void l2cap_conn_free(struct kref *ref)
 {
 	struct l2cap_conn *conn = container_of(ref, struct l2cap_conn, ref);
@@ -6943,8 +6920,6 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon)
 
 	INIT_DELAYED_WORK(&conn->info_timer, l2cap_info_timeout);
 
-	INIT_WORK(&conn->disconn_work, disconn_work);
-
 	skb_queue_head_init(&conn->pending_rx);
 	INIT_WORK(&conn->pending_rx_work, process_pending_rx);
 
-- 
1.9.3

[PATCH 5/8] Bluetooth: Fix SMP error and response to be mutually exclusive

[johan.hedberg]

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

Returning failure from the SMP data parsing function will cause an
immediate disconnect, making any attempts to send a response PDU futile.
This patch updates the function to always either send a response or
return an error, but never both at the same time:

* In the case that HCI_LE_ENABLED is not set we want to send a Pairing Not
  Supported response but it is not required to force a disconnection, so
  do not set the error return in this case.

* If we get garbage SMP data we can just fail with the handler function
  instead of also trying to send an SMP Failure PDU.

* There's no reason to force a disconnection if we receive an unknown SMP
  command. Instead simply send a proper Command Not Supported SMP
  response.

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

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 496584921fdc..16c181181775 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -1430,7 +1430,6 @@ static int smp_sig_channel(struct l2cap_chan *chan, struct sk_buff *skb)
 		return -EILSEQ;
 
 	if (!test_bit(HCI_LE_ENABLED, &hcon->hdev->dev_flags)) {
-		err = -EOPNOTSUPP;
 		reason = SMP_PAIRING_NOTSUPP;
 		goto done;
 	}
@@ -1447,7 +1446,6 @@ static int smp_sig_channel(struct l2cap_chan *chan, struct sk_buff *skb)
 	if (code != SMP_CMD_PAIRING_REQ && code != SMP_CMD_SECURITY_REQ &&
 	    !test_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags)) {
 		BT_ERR("Unexpected SMP command 0x%02x. Disconnecting.", code);
-		reason = SMP_CMD_NOTSUPP;
 		err = -EOPNOTSUPP;
 		goto done;
 	}
@@ -1459,7 +1457,6 @@ static int smp_sig_channel(struct l2cap_chan *chan, struct sk_buff *skb)
 
 	case SMP_CMD_PAIRING_FAIL:
 		smp_failure(conn, 0);
-		reason = 0;
 		err = -EPERM;
 		break;
 
@@ -1501,17 +1498,17 @@ static int smp_sig_channel(struct l2cap_chan *chan, struct sk_buff *skb)
 
 	default:
 		BT_DBG("Unknown command code 0x%2.2x", code);
-
 		reason = SMP_CMD_NOTSUPP;
-		err = -EOPNOTSUPP;
 		goto done;
 	}
 
 done:
-	if (reason)
-		smp_failure(conn, reason);
-	if (!err)
+	if (!err) {
+		if (reason)
+			smp_failure(conn, reason);
 		kfree_skb(skb);
+	}
+
 	return err;
 }
 
-- 
1.9.3

[PATCH 6/8] Bluetooth: Update hci_disconnect() to return an error value

[johan.hedberg]

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

We'll soon use hci_disconnect() from places that are interested to know
whether the hci_send_cmd() really succeeded or not. This patch updates
hci_disconnect() to pass on any error returned from hci_send_cmd().

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index dbe73642c54c..2b6e04d37593 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -703,7 +703,7 @@ static inline struct hci_conn *hci_conn_hash_lookup_state(struct hci_dev *hdev,
 	return NULL;
 }
 
-void hci_disconnect(struct hci_conn *conn, __u8 reason);
+int hci_disconnect(struct hci_conn *conn, __u8 reason);
 bool hci_setup_sync(struct hci_conn *conn, __u16 handle);
 void hci_sco_setup(struct hci_conn *conn, __u8 status);
 
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 5157a0990732..dd2df20b0f7d 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -122,7 +122,7 @@ static void hci_reject_sco(struct hci_conn *conn)
 	hci_send_cmd(conn->hdev, HCI_OP_REJECT_SYNC_CONN_REQ, sizeof(cp), &cp);
 }
 
-void hci_disconnect(struct hci_conn *conn, __u8 reason)
+int hci_disconnect(struct hci_conn *conn, __u8 reason)
 {
 	struct hci_cp_disconnect cp;
 
@@ -132,7 +132,7 @@ void hci_disconnect(struct hci_conn *conn, __u8 reason)
 
 	cp.handle = cpu_to_le16(conn->handle);
 	cp.reason = reason;
-	hci_send_cmd(conn->hdev, HCI_OP_DISCONNECT, sizeof(cp), &cp);
+	return hci_send_cmd(conn->hdev, HCI_OP_DISCONNECT, sizeof(cp), &cp);
 }
 
 static void hci_amp_disconn(struct hci_conn *conn)
-- 
1.9.3

[PATCH 7/8] Bluetooth: Use hci_disconnect() for mgmt_disconnect_device()

[johan.hedberg]

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

There's no reason to custom build the HCI_Disconnect command in the
Disconnect Device mgmt command handler. This patch updates the code to
use hci_disconnect() instead.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index d8c66663ade8..ab9521ae3c63 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2788,7 +2788,6 @@ static int disconnect(struct sock *sk, struct hci_dev *hdev, void *data,
 {
 	struct mgmt_cp_disconnect *cp = data;
 	struct mgmt_rp_disconnect rp;
-	struct hci_cp_disconnect dc;
 	struct pending_cmd *cmd;
 	struct hci_conn *conn;
 	int err;
@@ -2836,10 +2835,7 @@ static int disconnect(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto failed;
 	}
 
-	dc.handle = cpu_to_le16(conn->handle);
-	dc.reason = HCI_ERROR_REMOTE_USER_TERM;
-
-	err = hci_send_cmd(hdev, HCI_OP_DISCONNECT, sizeof(dc), &dc);
+	err = hci_disconnect(conn, HCI_ERROR_REMOTE_USER_TERM);
 	if (err < 0)
 		mgmt_pending_remove(cmd);
 
-- 
1.9.3

[PATCH 8/8] Bluetooth: Move clock offset reading into hci_disconnect()

[johan.hedberg]

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

To give all hci_disconnect() users the advantage of getting the clock
offset read automatically this patch moves the necessary code from
hci_ocnn_timeout() into hci_disconnect(). This way we pretty much always
update the clock offset when disconnecting.

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

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index dd2df20b0f7d..e3d7ae9e2edd 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -128,6 +128,19 @@ int hci_disconnect(struct hci_conn *conn, __u8 reason)
 
 	BT_DBG("hcon %p", conn);
 
+	/* When we are master of an established connection and it enters
+	 * the disconnect timeout, then go ahead and try to read the
+	 * current clock offset.  Processing of the result is done
+	 * within the event handling and hci_clock_offset_evt function.
+	 */
+	if (conn->type == ACL_LINK && conn->role == HCI_ROLE_MASTER) {
+		struct hci_dev *hdev = conn->hdev;
+		struct hci_cp_read_clock_offset cp;
+
+		cp.handle = cpu_to_le16(conn->handle);
+		hci_send_cmd(hdev, HCI_OP_READ_CLOCK_OFFSET, sizeof(cp), &cp);
+	}
+
 	conn->state = BT_DISCONN;
 
 	cp.handle = cpu_to_le16(conn->handle);
@@ -325,25 +338,6 @@ static void hci_conn_timeout(struct work_struct *work)
 			hci_amp_disconn(conn);
 		} else {
 			__u8 reason = hci_proto_disconn_ind(conn);
-
-			/* When we are master of an established connection
-			 * and it enters the disconnect timeout, then go
-			 * ahead and try to read the current clock offset.
-			 *
-			 * Processing of the result is done within the
-			 * event handling and hci_clock_offset_evt function.
-			 */
-			if (conn->type == ACL_LINK &&
-			    conn->role == HCI_ROLE_MASTER) {
-				struct hci_dev *hdev = conn->hdev;
-				struct hci_cp_read_clock_offset cp;
-
-				cp.handle = cpu_to_le16(conn->handle);
-
-				hci_send_cmd(hdev, HCI_OP_READ_CLOCK_OFFSET,
-					     sizeof(cp), &cp);
-			}
-
 			hci_disconnect(conn, reason);
 		}
 		break;
-- 
1.9.3

Re: [PATCH 0/8] Bluetooth: SMP & disconnection fixes

[Marcel Holtmann]

Hi Johan,

> The previous patches for SMP disconnection (that already got applied) do
> indeed fix the hci_conn_hold/drop ballance and do ensure that we
> disconnect the link when the SMP code wants it. Unfortunately the
> patches introduce a regression to a third use case which is the one
> where we want to bring the link down when there are no more active users
> for it.
> 
> It turns out that relying on connection users to diligently call
> hci_conn_drop() isn't a reliable way to ensure that the connection goes
> away. Firstly, we can't have hci_chan own such a reference (since it's
> "always there") and secondly there might not be any users at all
> (besides SMP) in which case there will be no-one to call hci_conn_drop.
> 
> A simpler solution is to just do a direct disonnect from smp.c with the
> help of the hci_disconnect() function. This has the side effect of not
> sending any SMP Failure PDU first, but the cases needing a disconnection
> do not really need that PDU to be sent (i.e. SMP timeout or garbage
> data).
> 
> The last three patches in this set unify hci_disconnect() usage to also
> cover the Disconnect Device mgmt command and move clock offset reading
> into the function to ensure all users have this feature at their
> disposal.
> 
> Johan
> 
> ----------------------------------------------------------------
> Johan Hedberg (8):
>      Bluetooth: Remove hci_conn_hold/drop from hci_chan
>      Bluetooth: Set discon_timeout to 0 in l2cap_conn_del
>      Bluetooth: Use hci_disconnect for immediate disconnection from SMP
>      Bluetooth: Remove unused l2cap_conn_shutdown API
>      Bluetooth: Fix SMP error and response to be mutually exclusive
>      Bluetooth: Update hci_disconnect() to return an error value
>      Bluetooth: Use hci_disconnect() for mgmt_disconnect_device()
>      Bluetooth: Move clock offset reading into hci_disconnect()
> 
> include/net/bluetooth/hci_core.h |  2 +-
> include/net/bluetooth/l2cap.h    |  4 ----
> net/bluetooth/hci_conn.c         | 41 ++++++++++++++----------------------
> net/bluetooth/l2cap_core.c       | 28 +++---------------------
> net/bluetooth/mgmt.c             |  6 +-----
> net/bluetooth/smp.c              | 17 ++++++---------
> 6 files changed, 28 insertions(+), 70 deletions(-)

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

Regards

Marcel