[PATCH v3] Bluetooth: Add HCI_AUTO_CONN_DIRECT_REPORT_IND

[PATCH v3] Bluetooth: Add HCI_AUTO_CONN_DIRECT_REPORT_IND

[Alfonso Acosta]

HCI_AUTO_CONN_DIRECT_REPORT_IND (associated to a new autoconnect
action, 0x03) treats advertising reports like a combination of
HCI_AUTO_CONN_DIRECT and HCI_AUTO_CONN_REPORT:

* Autoconnects on ADV_DIRECT_IND reports.
* Notifies userland (MGMT_EV_DEVICE_FOUND) about ADV_IND reports.

This is useful to communicate with autoconnectable devices which
advertise meaningful ADV_IND reports.

HCI_AUTO_CONN_DIRECT_REPORT_IND requires being able to simultaneously
have a pending report and connection action. I merged pend_le_reports
and pend_le_conns into the new pend_le_actions in order to make that
possible while maintaining a unique list_head in hci_conn_parameters
(action). This results in simpler handling of pending actions.

Signed-off-by: Alfonso Acosta <fons@spotify.com>
---
 include/net/bluetooth/hci_core.h |  6 ++---
 net/bluetooth/hci_conn.c         |  2 +-
 net/bluetooth/hci_core.c         | 54 ++++++++++------------------------------
 net/bluetooth/hci_event.c        | 24 ++++++++++++------
 net/bluetooth/mgmt.c             | 31 ++++++++++++++++-------
 5 files changed, 56 insertions(+), 61 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 37ff1ae..59393ea 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -317,8 +317,7 @@ struct hci_dev {
 	struct list_head	remote_oob_data;
 	struct list_head	le_white_list;
 	struct list_head	le_conn_params;
-	struct list_head	pend_le_conns;
-	struct list_head	pend_le_reports;
+	struct list_head	pend_le_actions;
 
 	struct hci_dev_stats	stat;
 
@@ -461,6 +460,7 @@ struct hci_conn_params {
 		HCI_AUTO_CONN_DISABLED,
 		HCI_AUTO_CONN_REPORT,
 		HCI_AUTO_CONN_DIRECT,
+		HCI_AUTO_CONN_DIRECT_REPORT_IND,
 		HCI_AUTO_CONN_ALWAYS,
 		HCI_AUTO_CONN_LINK_LOSS,
 	} auto_connect;
@@ -881,7 +881,7 @@ void hci_conn_params_del(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type);
 void hci_conn_params_clear_all(struct hci_dev *hdev);
 void hci_conn_params_clear_disabled(struct hci_dev *hdev);
 
-struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
+struct hci_conn_params *hci_pend_le_action_lookup(struct hci_dev *hdev,
 						  bdaddr_t *addr,
 						  u8 addr_type);
 
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b9517bd..f65b705 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -596,7 +596,7 @@ 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,
+	params = hci_pend_le_action_lookup(&hdev->pend_le_actions, &conn->dst,
 					   conn->dst_type);
 	if (params && params->conn) {
 		hci_conn_drop(params->conn);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index cb05d7f..980ec2e 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3631,7 +3631,7 @@ static bool is_connected(struct hci_dev *hdev, bdaddr_t *addr, u8 type)
 }
 
 /* This function requires the caller holds hdev->lock */
-struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
+struct hci_conn_params *hci_pend_le_action_lookup(struct hci_dev *hdev,
 						  bdaddr_t *addr, u8 addr_type)
 {
 	struct hci_conn_params *param;
@@ -3640,7 +3640,7 @@ struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
 	if (!hci_is_identity_address(addr, addr_type))
 		return NULL;
 
-	list_for_each_entry(param, list, action) {
+	list_for_each_entry(param, &hdev->pend_le_actions, action) {
 		if (bacmp(&param->addr, addr) == 0 &&
 		    param->addr_type == addr_type)
 			return param;
@@ -3706,13 +3706,14 @@ int hci_conn_params_set(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
 		hci_update_background_scan(hdev);
 		break;
 	case HCI_AUTO_CONN_REPORT:
-		list_add(&params->action, &hdev->pend_le_reports);
+	case HCI_AUTO_CONN_DIRECT_REPORT_IND:
+		list_add(&params->action, &hdev->pend_le_actions);
 		hci_update_background_scan(hdev);
 		break;
 	case HCI_AUTO_CONN_DIRECT:
 	case HCI_AUTO_CONN_ALWAYS:
 		if (!is_connected(hdev, addr, addr_type)) {
-			list_add(&params->action, &hdev->pend_le_conns);
+			list_add(&params->action, &hdev->pend_le_actions);
 			hci_update_background_scan(hdev);
 		}
 		break;
@@ -4020,8 +4021,7 @@ struct hci_dev *hci_alloc_dev(void)
 	INIT_LIST_HEAD(&hdev->remote_oob_data);
 	INIT_LIST_HEAD(&hdev->le_white_list);
 	INIT_LIST_HEAD(&hdev->le_conn_params);
-	INIT_LIST_HEAD(&hdev->pend_le_conns);
-	INIT_LIST_HEAD(&hdev->pend_le_reports);
+	INIT_LIST_HEAD(&hdev->pend_le_actions);
 	INIT_LIST_HEAD(&hdev->conn_hash.list);
 
 	INIT_WORK(&hdev->rx_work, hci_rx_work);
@@ -5467,16 +5467,13 @@ static u8 update_white_list(struct hci_request *req)
 
 	/* Go through the current white list programmed into the
 	 * controller one by one and check if that address is still
-	 * in the list of pending connections or list of devices to
-	 * report. If not present in either list, then queue the
-	 * command to remove it from the controller.
+	 * in the list of pending actions list. If not present,
+	 * then queue the command to remove it from the controller.
 	 */
 	list_for_each_entry(b, &hdev->le_white_list, list) {
 		struct hci_cp_le_del_from_white_list cp;
 
-		if (hci_pend_le_action_lookup(&hdev->pend_le_conns,
-					      &b->bdaddr, b->bdaddr_type) ||
-		    hci_pend_le_action_lookup(&hdev->pend_le_reports,
+		if (hci_pend_le_action_lookup(hdev,
 					      &b->bdaddr, b->bdaddr_type)) {
 			white_list_entries++;
 			continue;
@@ -5490,7 +5487,7 @@ static u8 update_white_list(struct hci_request *req)
 	}
 
 	/* Since all no longer valid white list entries have been
-	 * removed, walk through the list of pending connections
+	 * removed, walk through the list of pending actions
 	 * and ensure that any new device gets programmed into
 	 * the controller.
 	 *
@@ -5499,31 +5496,7 @@ static u8 update_white_list(struct hci_request *req)
 	 * just abort and return filer policy value to not use the
 	 * white list.
 	 */
-	list_for_each_entry(params, &hdev->pend_le_conns, action) {
-		if (hci_bdaddr_list_lookup(&hdev->le_white_list,
-					   &params->addr, params->addr_type))
-			continue;
-
-		if (white_list_entries >= hdev->le_white_list_size) {
-			/* Select filter policy to accept all advertising */
-			return 0x00;
-		}
-
-		if (hci_find_irk_by_addr(hdev, &params->addr,
-					 params->addr_type)) {
-			/* White list can not be used with RPAs */
-			return 0x00;
-		}
-
-		white_list_entries++;
-		add_to_white_list(req, params);
-	}
-
-	/* After adding all new pending connections, walk through
-	 * the list of pending reports and also add these to the
-	 * white list if there is still space.
-	 */
-	list_for_each_entry(params, &hdev->pend_le_reports, action) {
+	list_for_each_entry(params, &hdev->pend_le_actions, action) {
 		if (hci_bdaddr_list_lookup(&hdev->le_white_list,
 					   &params->addr, params->addr_type))
 			continue;
@@ -5593,7 +5566,7 @@ static void update_background_scan_complete(struct hci_dev *hdev, u8 status)
 		       "status 0x%2.2x", status);
 }
 
-/* This function controls the background scanning based on hdev->pend_le_conns
+/* This function controls the background scanning based on hdev->pend_le_actions
  * list. If there are pending LE connection we start the background scanning,
  * otherwise we stop it.
  *
@@ -5623,8 +5596,7 @@ void hci_update_background_scan(struct hci_dev *hdev)
 
 	hci_req_init(&req, hdev);
 
-	if (list_empty(&hdev->pend_le_conns) &&
-	    list_empty(&hdev->pend_le_reports)) {
+	if (list_empty(&hdev->pend_le_actions)) {
 		/* If there is no pending LE connections or devices
 		 * to be scanned for, we should stop the background
 		 * scanning.
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 8b0a2a6..3d669da 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2265,9 +2265,10 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 			/* Fall through */
 
 		case HCI_AUTO_CONN_DIRECT:
+		case HCI_AUTO_CONN_DIRECT_REPORT_IND:
 		case HCI_AUTO_CONN_ALWAYS:
 			list_del_init(&params->action);
-			list_add(&params->action, &hdev->pend_le_conns);
+			list_add(&params->action, &hdev->pend_le_actions);
 			hci_update_background_scan(hdev);
 			break;
 
@@ -4292,13 +4293,13 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
 	/* If we're not connectable only connect devices that we have in
 	 * our pend_le_conns list.
 	 */
-	params = hci_pend_le_action_lookup(&hdev->pend_le_conns,
-					   addr, addr_type);
+	params = hci_pend_le_action_lookup(hdev, addr, addr_type);
 	if (!params)
 		return;
 
 	switch (params->auto_connect) {
 	case HCI_AUTO_CONN_DIRECT:
+	case HCI_AUTO_CONN_DIRECT_REPORT_IND:
 		/* Only devices advertising with ADV_DIRECT_IND are
 		 * triggering a connection attempt. This is allowing
 		 * incoming connections from slave devices.
@@ -4351,6 +4352,7 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
 	struct smp_irk *irk;
 	bool match;
 	u32 flags;
+	struct hci_conn_params *params;
 
 	/* Check if we need to convert to identity address */
 	irk = hci_get_irk(hdev, bdaddr, bdaddr_type);
@@ -4363,17 +4365,25 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
 	check_pending_le_conn(hdev, bdaddr, bdaddr_type, type);
 
 	/* Passive scanning shouldn't trigger any device found events,
-	 * except for devices marked as CONN_REPORT for which we do send
-	 * device found events.
+	 * except for devices marked as CONN_REPORT or CONN_DIRECT_REPORT_IND
+	 * for which we do send device found events.
 	 */
 	if (hdev->le_scan_type == LE_SCAN_PASSIVE) {
 		if (type == LE_ADV_DIRECT_IND)
 			return;
 
-		if (!hci_pend_le_action_lookup(&hdev->pend_le_reports,
-					       bdaddr, bdaddr_type))
+		params = hci_pend_le_action_lookup(hdev, bdaddr, bdaddr_type);
+		if (!params)
 			return;
 
+		switch (params->auto_connect) {
+		case HCI_AUTO_CONN_REPORT:
+		case HCI_AUTO_CONN_DIRECT_REPORT_IND:
+		  break;
+		default:
+		  return;
+		}
+
 		if (type == LE_ADV_NONCONN_IND || type == LE_ADV_SCAN_IND)
 			flags = MGMT_DEV_FOUND_NOT_CONNECTABLE;
 		else
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index efb71b0..956d893 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -5241,7 +5241,7 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
 				    MGMT_STATUS_INVALID_PARAMS,
 				    &cp->addr, sizeof(cp->addr));
 
-	if (cp->action != 0x00 && cp->action != 0x01 && cp->action != 0x02)
+	if (cp->action > 0x03)
 		return cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
 				    MGMT_STATUS_INVALID_PARAMS,
 				    &cp->addr, sizeof(cp->addr));
@@ -5272,7 +5272,9 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
 	else
 		addr_type = ADDR_LE_DEV_RANDOM;
 
-	if (cp->action == 0x02)
+	if (cp->action == 0x03)
+		auto_conn = HCI_AUTO_CONN_DIRECT_REPORT_IND;
+	else if (cp->action == 0x02)
 		auto_conn = HCI_AUTO_CONN_ALWAYS;
 	else if (cp->action == 0x01)
 		auto_conn = HCI_AUTO_CONN_DIRECT;
@@ -5838,10 +5840,9 @@ static void restart_le_actions(struct hci_dev *hdev)
 		switch (p->auto_connect) {
 		case HCI_AUTO_CONN_DIRECT:
 		case HCI_AUTO_CONN_ALWAYS:
-			list_add(&p->action, &hdev->pend_le_conns);
-			break;
+		case HCI_AUTO_CONN_DIRECT_REPORT_IND:
 		case HCI_AUTO_CONN_REPORT:
-			list_add(&p->action, &hdev->pend_le_reports);
+			list_add(&p->action, &hdev->pend_le_actions);
 			break;
 		default:
 			break;
@@ -6746,16 +6747,28 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 	char buf[512];
 	struct mgmt_ev_device_found *ev = (void *) buf;
 	size_t ev_size;
+	struct hci_conn_params *params;
 
 	/* Don't send events for a non-kernel initiated discovery. With
-	 * LE one exception is if we have pend_le_reports > 0 in which
-	 * case we're doing passive scanning and want these events.
+	 * LE one exception is if we have CONN_REPORT in pend_le_actions in
+	 * which case we're doing passive scanning and want these events.
 	 */
 	if (!hci_discovery_active(hdev)) {
 		if (link_type == ACL_LINK)
 			return;
-		if (link_type == LE_LINK && list_empty(&hdev->pend_le_reports))
-			return;
+		if (link_type == LE_LINK) {
+			params = hci_conn_params_lookup(hdev, bdaddr,
+							addr_type);
+			if (!params)
+				return;
+			switch (params->auto_connect) {
+			case HCI_AUTO_CONN_REPORT:
+			case HCI_AUTO_CONN_DIRECT_REPORT_IND:
+			  break;
+			default:
+			  return;
+			}
+		}
 	}
 
 	/* Make sure that the buffer is big enough. The 5 extra bytes
-- 
1.9.1

Re: [PATCH v3] Bluetooth: Add HCI_AUTO_CONN_DIRECT_REPORT_IND

[Marcel Holtmann]

Hi Alfonso,

> HCI_AUTO_CONN_DIRECT_REPORT_IND (associated to a new autoconnect
> action, 0x03) treats advertising reports like a combination of
> HCI_AUTO_CONN_DIRECT and HCI_AUTO_CONN_REPORT:
> 
> * Autoconnects on ADV_DIRECT_IND reports.
> * Notifies userland (MGMT_EV_DEVICE_FOUND) about ADV_IND reports.
> 
> This is useful to communicate with autoconnectable devices which
> advertise meaningful ADV_IND reports.
> 
> HCI_AUTO_CONN_DIRECT_REPORT_IND requires being able to simultaneously
> have a pending report and connection action. I merged pend_le_reports
> and pend_le_conns into the new pend_le_actions in order to make that
> possible while maintaining a unique list_head in hci_conn_parameters
> (action). This results in simpler handling of pending actions.

I am not really convinced that this is a good idea. In general either you really want Action 0x01 and only accept incoming connection or you want Action 0x02 and connect to the device no matter what.

Originally I had the idea that the advertising data that you receive from ADV_IND when Action 0x02 is in play gets also included in Device Connected event. My thinking there was that the Service Data might have changed and it would be useful to know ahead of time before starting any GATT procedures. However then it seems that was not really useful after all and I stopped considering it. Maybe this idea needs to be picked up again.

Regards

Marcel

Re: [PATCH v3] Bluetooth: Add HCI_AUTO_CONN_DIRECT_REPORT_IND

[Alfonso Acosta]

[-- Attachment #1: Type: text/plain, Size: 1605 bytes --]

Hi Marcel,


> I am not really convinced that this is a good idea. In general either you
> really want Action 0x01 and only accept incoming connection or you want
> Action 0x02 and connect to the device no matter what.
>

We are dealing with an HID device which sends meaningful information
in ADV_IND reports
and to which we want to connect when receiving ADV_DIRECT_IND reports.

Without kernel support for auto-connecting on ADV_IND while still
forwarding ADV_DIRECT_IND to userland, we are forced to keep active
scanning on so that the content of ADV_IND reports isn't lost.

We would of course like to avoid active scanning when possible, hence the
reason on the patch.

Originally I had the idea that the advertising data that you receive from
> ADV_IND when Action 0x02 is in play gets also included in Device Connected
> event.


In our particular case, based on the content of ADV_IND we may not want to
or simply can't connect to the device right away. In other words, the
Device Connected event may never arrive (if the connection fails) and if it
does, it's already too late to decide whether we want to connect or not.

One could argue that the device should be broadcasting ADV_NONCONN_IND
instead, but I don't think it makes it non-compliant.

Johan told me on IRC that, as opposed to introducing
HCI_AUTO_CONN_DIRECT_REPORT_IND, modifying HCI_AUTO_CONN_DIRECT to also
forward ADV_IND to userland in Device Found events could be considered. Are
you open to this idea?


-- 
Alfonso Acosta

Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com

[-- Attachment #2: Type: text/html, Size: 3250 bytes --]

Re: [PATCH v3] Bluetooth: Add HCI_AUTO_CONN_DIRECT_REPORT_IND

[Marcel Holtmann]

Hi Alfonso,

you can not send HTML emails here. The mailing list will just reject you.

> I am not really convinced that this is a good idea. In general either you really want Action 0x01 and only accept incoming connection or you want Action 0x02 and connect to the device no matter what.
> 
> We are dealing with an HID device which sends meaningful information in ADV_IND reports and to which we want to connect when receiving ADV_DIRECT_IND reports.
> 
> Without kernel support for auto-connecting on ADV_IND while still forwarding ADV_DIRECT_IND to userland, we are forced to keep active scanning on so that the content of ADV_IND reports isn't lost.
> 
> We would of course like to avoid active scanning when possible, hence the reason on the patch.

The kernel does support connecting on ADV_IND. When you use Action 0x02, then it auto-connects when seeing ADV_IND and ADV_DIRECT_IND.

Forwarding ADV_DIRECT_IND to userspace is not really useful since the only thing you can do with it is actually connect. In addition if you are using high duty cycle ADV_DIRECT_IND, then you need to connect rather quickly to that device. Even if you happen to have a Bluetooth 4.1 peripheral that uses low duty cycle ADV_DIRECT_IND it will still not carry any advertising data.

> Originally I had the idea that the advertising data that you receive from ADV_IND when Action 0x02 is in play gets also included in Device Connected event.
> 
> In our particular case, based on the content of ADV_IND we may not want to or simply can't connect to the device right away. In other words, the Device Connected event may never arrive (if the connection fails) and if it does, it's already too late to decide whether we want to connect or not.

If you have added a peripheral device address to the kernel, then it means you actually do want to connect to that device. The connection will normally always succeed in that case. The kernel auto-connection logic only uses low security level. The elevation into medium or high security levels is done by userspace. And if you do not like the connection, you can just reject it. However if you don't want the connection, then it should not be in the auto-connect list in the first place.

> One could argue that the device should be broadcasting ADV_NONCONN_IND instead, but I don't think it makes it non-compliant. 

If you advertising with ADV_NONCONN_IND, then your peripheral is not connectable at all. The specification calls these broadcaster devices.

> Johan told me on IRC that, as opposed to introducing HCI_AUTO_CONN_DIRECT_REPORT_IND, modifying HCI_AUTO_CONN_DIRECT to also forward ADV_IND to userland in Device Found events could be considered. Are you open to this idea?

The background with Add Device Action types is that they also need to map to BR/EDR as well. So Action 0x01 for BR/EDR defines the concept of incoming connections. It means it allows incoming connections. On LE that would map to ADV_DIRECT_IND since that is the only way for a slave to connect to a master. So that mapping is pretty straight forward.

This means that there is not equivalent of ADV_IND on BR/EDR side and this would make the Action 0x01 a little bit unbalanced.

I still think that what you really want is actually using Action 0x02 since you know the remote device address and you want to connect it. It does not matter if it happens via ADV_DIRECT_IND or ADV_IND in the end. However in case it happens via ADV_IND you want to get the advertising data and for that I am proposing to get it via Device Connected event.

Regards

Marcel

Re: [PATCH v3] Bluetooth: Add HCI_AUTO_CONN_DIRECT_REPORT_IND

[Alfonso Acosta]

Hi Marcel,

> you can not send HTML emails here. The mailing list will just reject you.

Sorry about that, I didn't even know that my email client was
inserting HTML tags.


>> We are dealing with an HID device which sends meaningful information in =
ADV_IND reports and to which we want to connect when receiving ADV_DIRECT_I=
ND reports.
>>
>> Without kernel support for auto-connecting on ADV_IND while still forwar=
ding ADV_DIRECT_IND to userland, we are forced to keep active scanning on s=
o that the content of ADV_IND reports isn't lost.
>>

I meant  "for auto-connecting on ADV_DIRECT_IND while still forwarding
ADV_IND to userland", sorry

> I still think that what you really want is actually using Action 0x02 sin=
ce you know the remote device address and you want to connect it. It does n=
ot matter if it happens via ADV_DIRECT_IND or ADV_IND in the end. However i=
n case it happens via ADV_IND you want to get the advertising data and for =
that I am proposing to get it via Device Connected event.

Unfortunately, that is not good enough for this device. Let me give
you some context.

This device doesn't have non-volatile memory, so, when its batteries
are replaced it loses the bonding information (i.e. LTK).

* When bonded, the device sends ADV_DIRECT_IND.
* When not bonded (fresh from factory or due to the batteries being
replaced) it sends ADV_IND containing a special message (let's call it
"power-on") reflecting that it's not bonded. When this happens we need
to regenerate the device's LTK and rebond.

Action 0x02 (even when forwarding the the ADV_IND  report content via
Device Connected event) is not good enough. In fact, none of the
current auto-connect actions work for this device:

* With 0x00, (HCI_AUTO_CONN_REPORT) the content of ADV_IND is
forwarded to userland but there is no auto-connection upon
ADV_DIRECT_IND
* With 0x01, (HCI_AUTO_CONN_DIRECT) there is auto-connection upon
ADV_DIRECT_IND but if the device's batteries are removed (power-on
ADV_IND) the device is not reconnected.
* With 0x02, (HCI_AUTO_CONN_ALWAYS) there is auto-connection upon
ADV_DIRECT_IND but replacing the device's batteries  (power-on
ADV_IND) causes an infinite connection loop: connect->encryption
on->disconnect(due to "encryption on" failing)->connect .... Getting
the report data inside the  Device Connected event won't help because
it will never get to that point.

The alternatives are

1. Keep using passive scanning and add/modify an action to connect
upon ADV_DIRECT_IND and forward ADV_IND to userland (to have the
opportunity or rebonding).
2. Constantly do active scanning, which, again, we would like to avoid.

Thanks,

--=20
Alfonso Acosta

Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com

Re: [PATCH v3] Bluetooth: Add HCI_AUTO_CONN_DIRECT_REPORT_IND

[Marcel Holtmann]

Hi Alfonso,

>>> We are dealing with an HID device which sends meaningful information in ADV_IND reports and to which we want to connect when receiving ADV_DIRECT_IND reports.
>>> 
>>> Without kernel support for auto-connecting on ADV_IND while still forwarding ADV_DIRECT_IND to userland, we are forced to keep active scanning on so that the content of ADV_IND reports isn't lost.
>>> 
> 
> I meant  "for auto-connecting on ADV_DIRECT_IND while still forwarding
> ADV_IND to userland", sorry
> 
>> I still think that what you really want is actually using Action 0x02 since you know the remote device address and you want to connect it. It does not matter if it happens via ADV_DIRECT_IND or ADV_IND in the end. However in case it happens via ADV_IND you want to get the advertising data and for that I am proposing to get it via Device Connected event.
> 
> Unfortunately, that is not good enough for this device. Let me give
> you some context.
> 
> This device doesn't have non-volatile memory, so, when its batteries
> are replaced it loses the bonding information (i.e. LTK).

if it can not store its LTK, then why doesn't it use a key hierarchy (as defined in the Bluetooth specification) so that it can restore its keys after reboot. If it remembers its Bluetooth address, then it could clearly remember a single master key.

But seriously, if you can remember your BD_ADDR, then you might want to remember your LTK as well. Just a hint here.

> * When bonded, the device sends ADV_DIRECT_IND.
> * When not bonded (fresh from factory or due to the batteries being
> replaced) it sends ADV_IND containing a special message (let's call it
> "power-on") reflecting that it's not bonded. When this happens we need
> to regenerate the device's LTK and rebond.
> 
> Action 0x02 (even when forwarding the the ADV_IND  report content via
> Device Connected event) is not good enough. In fact, none of the
> current auto-connect actions work for this device:
> 
> * With 0x00, (HCI_AUTO_CONN_REPORT) the content of ADV_IND is
> forwarded to userland but there is no auto-connection upon
> ADV_DIRECT_IND
> * With 0x01, (HCI_AUTO_CONN_DIRECT) there is auto-connection upon
> ADV_DIRECT_IND but if the device's batteries are removed (power-on
> ADV_IND) the device is not reconnected.
> * With 0x02, (HCI_AUTO_CONN_ALWAYS) there is auto-connection upon
> ADV_DIRECT_IND but replacing the device's batteries  (power-on
> ADV_IND) causes an infinite connection loop: connect->encryption
> on->disconnect(due to "encryption on" failing)->connect .... Getting
> the report data inside the  Device Connected event won't help because
> it will never get to that point.

So the encryption trigger is not done by the kernel. It is actually done by userspace when you have an existing LTK. The kernel will auto-conect the device with low security and then userspace will move it to medium security in case we have an LTK. It will also move it to medium security for all HID devices since that is mandatory.

It is not really the kernels fault that you have a loop here. The Action 0x02 would work perfectly fine. It is that userspace is too naive.

> The alternatives are
> 
> 1. Keep using passive scanning and add/modify an action to connect
> upon ADV_DIRECT_IND and forward ADV_IND to userland (to have the
> opportunity or rebonding).
> 2. Constantly do active scanning, which, again, we would like to avoid.

The active scanning will screw you over. It will increase your power consumption and also your latency. Using it for automatic connection trigger was a pretty bad idea from our side. It took us way too long to get kernel side auto-connection up and running.

Having the advertising data in Device Connected event will actually allow you to do exactly what you want. It would allow you to utilize Unpair Device (with Disconnect 0x00) and Pair Device to recreate the bonding. All without ever disconnecting the link.

The important piece of detail is that the security elevation from low to medium does not happen when the device is detected as initial powered on. So instead of security elevation, you do a re-bonding which will give you the encrypted link HID requires and also the new LTK.

So even if we allow Device Found with Action 0x01, then it still means that we would have to provide the same advertising data in Device Connected when using Action 0x02. Otherwise you have the gap that one Action provides all the information, the other one doesn't.

What could be done and I am not yet fully convinced that this is a good idea since it does not map to BR/EDR properly is the following:

0x00: ADV_DIRECT_IND is ignored, ADV_IND is reported via Device Found as connectable, ADV_NONCONN_IND and ADV_SCAN_IND are reported via Device Found as non-connectable (this is current behavior)

0x01: ADV_DIRECT_IND is auto-connected, ADV_IND is reported via Device Found as connectable, ADV_NONCONN_IND and ADV_SCAN_IND are reported via Device Found as non-conntable

0x02: ADV_DIRECT_IND and ADV_IND are auto-connected with advertising data in Device Connected event, ADV_NONCONN_IND and ADV_SCAN_IND are reported via Device Found as non-connectable

So before trying to redefine the Add Device command semantics, I would clearly go for adding the advertising data to Device Connected event and see if that gets you where you need to go.

Regards

Marcel

Re: [PATCH v3] Bluetooth: Add HCI_AUTO_CONN_DIRECT_REPORT_IND

[Alfonso Acosta]

>
> if it can not store its LTK, then why doesn't it use a key hierarchy (as =
defined in the Bluetooth specification) so that it can restore its keys aft=
er reboot. If it remembers its Bluetooth address, then it could clearly rem=
ember a single master key.
>
> But seriously, if you can remember your BD_ADDR, then you might want to r=
emember your LTK as well. Just a hint here.


Thanks for the hint, I wasn't familiar with "key hierarchies".

> So the encryption trigger is not done by the kernel. It is actually done =
by userspace when you have an existing LTK. The kernel will auto-conect the=
 device with low security and then userspace will move it to medium securit=
y in case we have an LTK. It will also move it to medium security for all H=
ID devices since that is mandatory.
>
[...]
> Having the advertising data in Device Connected event will actually allow=
 you to do exactly what you want. It would allow you to utilize Unpair Devi=
ce (with Disconnect 0x00) and Pair Device to recreate the bonding. All with=
out ever disconnecting the link.
>
> The important piece of detail is that the security elevation from low to =
medium does not happen when the device is detected as initial powered on. S=
o instead of security elevation, you do a re-bonding which will give you th=
e encrypted link HID requires and also the new LTK.

Oh, I see. I was wrongly assuming that the encryption elevation was
also done in the kernel without userspace involvement. Then, adding
the contents of the ADV_IND report to the "Device Connected event"
should indeed be good enough. I will try it out and send a patch if it
works.


> So before trying to redefine the Add Device command semantics, I would cl=
early go for adding the advertising data to Device Connected event and see =
if that gets you where you need to go.

Will do, thanks!



--=20
Alfonso Acosta

Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com

Re: [PATCH v3] Bluetooth: Add HCI_AUTO_CONN_DIRECT_REPORT_IND

[Marcel Holtmann]

Hi Alfonso,

>> if it can not store its LTK, then why doesn't it use a key hierarchy (as defined in the Bluetooth specification) so that it can restore its keys after reboot. If it remembers its Bluetooth address, then it could clearly remember a single master key.
>> 
>> But seriously, if you can remember your BD_ADDR, then you might want to remember your LTK as well. Just a hint here.
> 
> 
> Thanks for the hint, I wasn't familiar with "key hierarchies".

you should really only use them if your device has no persistent storage. If you have a choice, get a device that has persistent storage. Since generating a random LTK for each device (instead of deriving it from a key hierarchy) is of course more secure.

Regards

Marcel

Re: [PATCH v3] Bluetooth: Add HCI_AUTO_CONN_DIRECT_REPORT_IND

[Alfonso Acosta]

Hi Johan and Marcel,

As you guys suggested, including ADV_IND in the Device Connected event
made the trick (for which I just submitted a patch).

Everything goes well if I disconnect when rebonding (using Unpair
Device with Disconnect=3D0x01).

However, I am struggling to go one step further and rebond without
disconnecting (using Unpair Device with Disconnect=3D0x00) before the
security elevation happens. The sequence of "events" is exactly as
Johan described:

> 1. mgmt_ev_device_connected (connected_callback() in adapter.c)
> 2. Connection indication on ATT server socket (src/attrib-server.c)
> 3. device_attach_attrib() called (due to step 2)
> 4. bt_io_set(io, ..., BT_IO_SEC_MEDIUM, ...);

Now I can detect the need to rebond in (1), which gives me control
over the next steps. However, since rebonding is asynchronous, I don't
know a simple way to ensure it is done before the security elevation
happens. My ideas so far:

* Stall (3) through synchronization primitives until the rebonding is
done, which sounds horrible.
* Cancel (3) and somehow force an ATT reconnection (2) once the
rebonding is done, which in turn would cause another security
elevation, but I don't know how (I am probably making wrong
assumptions about how the ATT socket works).

Do you have any suggestions?

Thanks,

Fons

On Mon, Sep 29, 2014 at 4:49 PM, Alfonso Acosta <fons@spotify.com> wrote:
>>
>> if it can not store its LTK, then why doesn't it use a key hierarchy (as=
 defined in the Bluetooth specification) so that it can restore its keys af=
ter reboot. If it remembers its Bluetooth address, then it could clearly re=
member a single master key.
>>
>> But seriously, if you can remember your BD_ADDR, then you might want to =
remember your LTK as well. Just a hint here.
>
>
> Thanks for the hint, I wasn't familiar with "key hierarchies".
>
>> So the encryption trigger is not done by the kernel. It is actually done=
 by userspace when you have an existing LTK. The kernel will auto-conect th=
e device with low security and then userspace will move it to medium securi=
ty in case we have an LTK. It will also move it to medium security for all =
HID devices since that is mandatory.
>>
> [...]
>> Having the advertising data in Device Connected event will actually allo=
w you to do exactly what you want. It would allow you to utilize Unpair Dev=
ice (with Disconnect 0x00) and Pair Device to recreate the bonding. All wit=
hout ever disconnecting the link.
>>
>> The important piece of detail is that the security elevation from low to=
 medium does not happen when the device is detected as initial powered on. =
So instead of security elevation, you do a re-bonding which will give you t=
he encrypted link HID requires and also the new LTK.
>
> Oh, I see. I was wrongly assuming that the encryption elevation was
> also done in the kernel without userspace involvement. Then, adding
> the contents of the ADV_IND report to the "Device Connected event"
> should indeed be good enough. I will try it out and send a patch if it
> works.
>
>
>> So before trying to redefine the Add Device command semantics, I would c=
learly go for adding the advertising data to Device Connected event and see=
 if that gets you where you need to go.
>
> Will do, thanks!
>
>
>
> --
> Alfonso Acosta
>
> Embedded Systems Engineer at Spotify
> Birger Jarlsgatan 61, Stockholm, Sweden
> http://www.spotify.com



--=20
Alfonso Acosta

Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com

Re: [PATCH v3] Bluetooth: Add HCI_AUTO_CONN_DIRECT_REPORT_IND

[Marcel Holtmann]

Hi Alfonso,

> As you guys suggested, including ADV_IND in the Device Connected event
> made the trick (for which I just submitted a patch).
> 
> Everything goes well if I disconnect when rebonding (using Unpair
> Device with Disconnect=0x01).
> 
> However, I am struggling to go one step further and rebond without
> disconnecting (using Unpair Device with Disconnect=0x00) before the
> security elevation happens. The sequence of "events" is exactly as
> Johan described:
> 
>> 1. mgmt_ev_device_connected (connected_callback() in adapter.c)
>> 2. Connection indication on ATT server socket (src/attrib-server.c)
>> 3. device_attach_attrib() called (due to step 2)
>> 4. bt_io_set(io, ..., BT_IO_SEC_MEDIUM, ...);
> 
> Now I can detect the need to rebond in (1), which gives me control
> over the next steps. However, since rebonding is asynchronous, I don't
> know a simple way to ensure it is done before the security elevation
> happens. My ideas so far:
> 
> * Stall (3) through synchronization primitives until the rebonding is
> done, which sounds horrible.
> * Cancel (3) and somehow force an ATT reconnection (2) once the
> rebonding is done, which in turn would cause another security
> elevation, but I don't know how (I am probably making wrong
> assumptions about how the ATT socket works).

what sounds most simple to me is that we allow for a way to suspend and later resume the trigger for doing service discovery. Attaching the attribute channel is fine, but then essentially tell it to stay put.

This all becomes tricky and we might need to have some redesign and cleanups to do to make this work cleanly. We also long term want to switch to the newly designed gatt-client.c that we are working on. So something to keep in mind.

However one thing has to present and that is our GATT server. So even if we stall everything else and even if we drop the keys and unpair, the GATT server needs to be operational. So we need to attach the ATT protocol to the ATT socket that we are getting from the kernel on every single connection.

Regards

Marcel

Re: [PATCH v3] Bluetooth: Add HCI_AUTO_CONN_DIRECT_REPORT_IND

[Alfonso Acosta]

Hi Marcel,


> what sounds most simple to me is that we allow for a way to suspend and later resume the trigger for doing service discovery. Attaching the attribute channel is fine, but then essentially tell it to stay put.
>
> This all becomes tricky and we might need to have some redesign and cleanups to do to make this work cleanly. We also long term want to switch to the newly designed gatt-client.c that we are working on. So something to keep in mind.
>
> However one thing has to present and that is our GATT server. So even if we stall everything else and even if we drop the keys and unpair, the GATT server needs to be operational. So we need to attach the ATT protocol to the ATT socket that we are getting from the kernel on every single connection.

Thanks. Using your suggestion I am suspending the trigger for doing
service discovery until rebonding is completed. As expected, without
the connection re-establishment, rebonding is now considerably faster.

I, however, have to keep the ATT socket around until resuming the
service discovery, which is a bit dirty. But I guess I can live with
that until the new gatt-client is in.

Thanks again,

Fons

-- 
Alfonso Acosta

Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com

Re: [PATCH v3] Bluetooth: Add HCI_AUTO_CONN_DIRECT_REPORT_IND

[Johan Hedberg]

Hi Alfonso,

On Mon, Sep 29, 2014, Alfonso Acosta wrote:
> * With 0x02, (HCI_AUTO_CONN_ALWAYS) there is auto-connection upon
> ADV_DIRECT_IND but replacing the device's batteries  (power-on
> ADV_IND) causes an infinite connection loop: connect->encryption
> on->disconnect(due to "encryption on" failing)->connect .... Getting
> the report data inside the  Device Connected event won't help because
> it will never get to that point.

Are you sure that you've mapped out the order of events correctly for
this case? The way things should be going (based on going through the
code - I haven't verified this in practice) on the user space side is:

1. mgmt_ev_device_connected (connected_callback() in adapter.c)
2. Connection indication on ATT server socket (src/attrib-server.c)
3. device_attach_attrib() called (due to step 2)
4. bt_io_set(io, ..., BT_IO_SEC_MEDIUM, ...);

The last step above is what instructs the kernel to initiate encryption.
If the mgmt_ev_device_connected would contain hints telling us to call
mgmt_unpair + mgmt_pair instead of trying to elevate security your use
case should be solvable.

If the above order is not what's happening we might be having a race
condition between the mgmt and ATT sockets. This race *should* be
fixable by ensuring that the mgmt socket has higher priority in the
mainloop than the ATT socket.  We used to have it like that in the past,
but we might have lost it along the way because shared/mgmt.c that uses
shared/io.h.

As a quick hack to check if these sockets are indeed racing against each
other during the same mainloop iteration you could try changing
G_PRIORITY_DEFAULT to G_PRIORITY_HIGH in the io_set_read_handler()
function in src/shared/io-glib.c (the ATT socket shouldn't be using that
but shared/mgmt.c is).

Johan

Re: [PATCH v3] Bluetooth: Add HCI_AUTO_CONN_DIRECT_REPORT_IND

[Alfonso Acosta]

I was informally describing the HCI exchange between the device , not
the userspace calls. Sorry I didn't make that clear.

On Mon, Sep 29, 2014 at 4:35 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Alfonso,
>
> On Mon, Sep 29, 2014, Alfonso Acosta wrote:
>> * With 0x02, (HCI_AUTO_CONN_ALWAYS) there is auto-connection upon
>> ADV_DIRECT_IND but replacing the device's batteries  (power-on
>> ADV_IND) causes an infinite connection loop: connect->encryption
>> on->disconnect(due to "encryption on" failing)->connect .... Getting
>> the report data inside the  Device Connected event won't help because
>> it will never get to that point.
>
> Are you sure that you've mapped out the order of events correctly for
> this case? The way things should be going (based on going through the
> code - I haven't verified this in practice) on the user space side is:
>
> 1. mgmt_ev_device_connected (connected_callback() in adapter.c)
> 2. Connection indication on ATT server socket (src/attrib-server.c)
> 3. device_attach_attrib() called (due to step 2)
> 4. bt_io_set(io, ..., BT_IO_SEC_MEDIUM, ...);
>
> The last step above is what instructs the kernel to initiate encryption.
> If the mgmt_ev_device_connected would contain hints telling us to call
> mgmt_unpair + mgmt_pair instead of trying to elevate security your use
> case should be solvable.
>
> If the above order is not what's happening we might be having a race
> condition between the mgmt and ATT sockets. This race *should* be
> fixable by ensuring that the mgmt socket has higher priority in the
> mainloop than the ATT socket.  We used to have it like that in the past,
> but we might have lost it along the way because shared/mgmt.c that uses
> shared/io.h.
>
> As a quick hack to check if these sockets are indeed racing against each
> other during the same mainloop iteration you could try changing
> G_PRIORITY_DEFAULT to G_PRIORITY_HIGH in the io_set_read_handler()
> function in src/shared/io-glib.c (the ATT socket shouldn't be using that
> but shared/mgmt.c is).
>
> Johan



-- 
Alfonso Acosta

Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com