[PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting

[PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting

[Alfonso Acosta]

Systematically removing the LE connection parameters and autoconnect
action is inconvenient for rebonding without disconnecting from
userland (i.e. unpairing followed by repairing without
disconnecting). The parameters will be lost after unparing and
userland needs to take care of book-keeping them and re-adding them.

This patch allows userland to forget about parameter management when
rebonding without disconnecting. It defers clearing the connection
parameters when unparing without disconnecting, giving a chance of
keeping the parameters if a repairing happens before the connection is
closed.

Signed-off-by: Alfonso Acosta <fons@spotify.com>
---
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_conn.c         |  6 ++++++
 net/bluetooth/mgmt.c             | 16 +++++++++++++---
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 07ddeed62..b8685a7 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -555,6 +555,7 @@ enum {
 	HCI_CONN_STK_ENCRYPT,
 	HCI_CONN_AUTH_INITIATOR,
 	HCI_CONN_DROP,
+	HCI_CONN_PARAM_REMOVAL_PEND,
 };
 
 static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b9517bd..d106f29 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *work)
 		conn->state = BT_CLOSED;
 		break;
 	}
+
+	if (test_and_clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->flags))
+		hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
 }
 
 /* Enter sniff mode */
@@ -544,6 +547,9 @@ int hci_conn_del(struct hci_conn *conn)
 
 	hci_conn_del_sysfs(conn);
 
+	if (test_and_clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->flags))
+		hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
+
 	hci_dev_put(hdev);
 
 	hci_conn_put(conn);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3fd88b0..0f2c0e9 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2700,6 +2700,7 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
 	struct hci_cp_disconnect dc;
 	struct pending_cmd *cmd;
 	struct hci_conn *conn;
+	struct hci_conn *le_conn = NULL;
 	int err;
 
 	memset(&rp, 0, sizeof(rp));
@@ -2736,7 +2737,17 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
 
 		hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
 
-		hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
+		le_conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
+						  &cp->addr.bdaddr);
+
+		/* If the BLE connection is being used, defer clearing up
+		 * the connection parameters until closing to give a
+		 * chance of keeping them if a repairing happens.
+		 */
+		if (le_conn && !cp->disconnect)
+			set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &le_conn->flags);
+		else
+			hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
 
 		err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
 	}
@@ -2752,8 +2763,7 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
 			conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
 						       &cp->addr.bdaddr);
 		else
-			conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
-						       &cp->addr.bdaddr);
+			conn = le_conn;
 	} else {
 		conn = NULL;
 	}
-- 
1.9.1

Re: [PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting

[Alfonso Acosta]

Hi,

As an alternative to this patch, I am thinking that it may be worth
considering two other options:

1. Adding an additional repairing operation: MGMT_OP_REPAIR_DEVICE.
Making the repairing semantics explicit  would allow us to keep the connection
parameters even if we choose to disconnect when unpairing.  On the
other hand, one could argue that it clutters the API with an extra
operation which is the composite of two already existing operations.

2. Make MGMT_OP_PAIR_DEVICE behave like the repairing operation in (1)
if the device was already paired. If I recall properly, Johan suggested
this on IRC.

Comments?

Thanks,

Fons

On Thu, Oct 9, 2014 at 3:03 PM, Alfonso Acosta <fons@spotify.com> wrote:
> Systematically removing the LE connection parameters and autoconnect
> action is inconvenient for rebonding without disconnecting from
> userland (i.e. unpairing followed by repairing without
> disconnecting). The parameters will be lost after unparing and
> userland needs to take care of book-keeping them and re-adding them.
>
> This patch allows userland to forget about parameter management when
> rebonding without disconnecting. It defers clearing the connection
> parameters when unparing without disconnecting, giving a chance of
> keeping the parameters if a repairing happens before the connection is
> closed.
>
> Signed-off-by: Alfonso Acosta <fons@spotify.com>
> ---
>  include/net/bluetooth/hci_core.h |  1 +
>  net/bluetooth/hci_conn.c         |  6 ++++++
>  net/bluetooth/mgmt.c             | 16 +++++++++++++---
>  3 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 07ddeed62..b8685a7 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -555,6 +555,7 @@ enum {
>         HCI_CONN_STK_ENCRYPT,
>         HCI_CONN_AUTH_INITIATOR,
>         HCI_CONN_DROP,
> +       HCI_CONN_PARAM_REMOVAL_PEND,
>  };
>
>  static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index b9517bd..d106f29 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *work)
>                 conn->state = BT_CLOSED;
>                 break;
>         }
> +
> +       if (test_and_clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->flags))
> +               hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
>  }
>
>  /* Enter sniff mode */
> @@ -544,6 +547,9 @@ int hci_conn_del(struct hci_conn *conn)
>
>         hci_conn_del_sysfs(conn);
>
> +       if (test_and_clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->flags))
> +               hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
> +
>         hci_dev_put(hdev);
>
>         hci_conn_put(conn);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 3fd88b0..0f2c0e9 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2700,6 +2700,7 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>         struct hci_cp_disconnect dc;
>         struct pending_cmd *cmd;
>         struct hci_conn *conn;
> +       struct hci_conn *le_conn = NULL;
>         int err;
>
>         memset(&rp, 0, sizeof(rp));
> @@ -2736,7 +2737,17 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>
>                 hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
>
> -               hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
> +               le_conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> +                                                 &cp->addr.bdaddr);
> +
> +               /* If the BLE connection is being used, defer clearing up
> +                * the connection parameters until closing to give a
> +                * chance of keeping them if a repairing happens.
> +                */
> +               if (le_conn && !cp->disconnect)
> +                       set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &le_conn->flags);
> +               else
> +                       hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
>
>                 err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
>         }
> @@ -2752,8 +2763,7 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>                         conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
>                                                        &cp->addr.bdaddr);
>                 else
> -                       conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> -                                                      &cp->addr.bdaddr);
> +                       conn = le_conn;
>         } else {
>                 conn = NULL;
>         }
> --
> 1.9.1
>



-- 
Alfonso Acosta

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

Re: [PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting

[Johan Hedberg]

Hi Alfonso,

On Thu, Oct 09, 2014, Alfonso Acosta wrote:
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -555,6 +555,7 @@ enum {
>  	HCI_CONN_STK_ENCRYPT,
>  	HCI_CONN_AUTH_INITIATOR,
>  	HCI_CONN_DROP,
> +	HCI_CONN_PARAM_REMOVAL_PEND,
>  };
>  
>  static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index b9517bd..d106f29 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *work)
>  		conn->state = BT_CLOSED;
>  		break;
>  	}
> +
> +	if (test_and_clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->flags))
> +		hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
>  }
>  
>  /* Enter sniff mode */
> @@ -544,6 +547,9 @@ int hci_conn_del(struct hci_conn *conn)
>  
>  	hci_conn_del_sysfs(conn);
>  
> +	if (test_and_clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->flags))
> +		hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);

I suppose the above two test_and_clear_bit() calls should be operating
on HCI_CONN_PARAM_REMOVAL_PEND and not HCI_CONN_MODE_CHANGE_PEND?

Johan


>  	hci_dev_put(hdev);
>  
>  	hci_conn_put(conn);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 3fd88b0..0f2c0e9 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2700,6 +2700,7 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>  	struct hci_cp_disconnect dc;
>  	struct pending_cmd *cmd;
>  	struct hci_conn *conn;
> +	struct hci_conn *le_conn = NULL;

I don't think you need to initialize this here since it's
unconditionally set in the first LE addr type branch and only touched
again in the second LE branch. Actually do you even need this second
variable if you simply assign to conn when you look up the LE
connection?

> @@ -2752,8 +2763,7 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>  			conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
>  						       &cp->addr.bdaddr);
>  		else
> -			conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> -						       &cp->addr.bdaddr);
> +			conn = le_conn;

Here you could then simply remove the "else" branch with a comment that
the conn is already looked up for LE addresses earlier in the function.
Not sure if that makes the code harder to follow though...

Johan

Re: [PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting

[Alfonso Acosta]

Hi Johan,

>> +
>> +     if (test_and_clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->flags))
>> +             hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
>>  }
>>
>>  /* Enter sniff mode */
>> @@ -544,6 +547,9 @@ int hci_conn_del(struct hci_conn *conn)
>>
>>       hci_conn_del_sysfs(conn);
>>
>> +     if (test_and_clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->flags))
>> +             hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
> I suppose the above two test_and_clear_bit() calls should be operating
> on HCI_CONN_PARAM_REMOVAL_PEND and not HCI_CONN_MODE_CHANGE_PEND?

Darn. Yes, my bad.


>> @@ -2700,6 +2700,7 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>>       struct hci_cp_disconnect dc;
>>       struct pending_cmd *cmd;
>>       struct hci_conn *conn;
>> +     struct hci_conn *le_conn = NULL;
>
> I don't think you need to initialize this here since it's
> unconditionally set in the first LE addr type branch and only touched
> again in the second LE branch. Actually do you even need this second
> variable if you simply assign to conn when you look up the LE
> connection?

I did it to silence an unitialized-variable warning from gcc  (at least
the 4.8 version I am using is not good enough to conclude that the
variable is initialized).

>> @@ -2752,8 +2763,7 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>>                       conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
>>                                                      &cp->addr.bdaddr);
>>               else
>> -                     conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
>> -                                                    &cp->addr.bdaddr);
>> +                     conn = le_conn;
>
> Here you could then simply remove the "else" branch with a comment that
> the conn is already looked up for LE addresses earlier in the function.
> Not sure if that makes the code harder to follow though...

My intention was precisely to make it easier to follow by showing that
LE was being taken care of with an explicit assignment.

Buy yeah, I can simply remove le_conn altogether and add a comment instead.

Note that removing the else branch will also cause an
uninitialized-variable compiler warning on conn.

I could use uninitialized_var() instead of using NULL, I simply
decided against it after reading http://lwn.net/Articles/529954/

-- 
Alfonso Acosta

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

Re: [PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting

[Marcel Holtmann]

Hi Alfonso,

>>> +
>>> +     if (test_and_clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->flags))
>>> +             hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
>>> }
>>> 
>>> /* Enter sniff mode */
>>> @@ -544,6 +547,9 @@ int hci_conn_del(struct hci_conn *conn)
>>> 
>>>      hci_conn_del_sysfs(conn);
>>> 
>>> +     if (test_and_clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->flags))
>>> +             hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
>> I suppose the above two test_and_clear_bit() calls should be operating
>> on HCI_CONN_PARAM_REMOVAL_PEND and not HCI_CONN_MODE_CHANGE_PEND?
> 
> Darn. Yes, my bad.

don't we also need to clear the flag when the new pairing succeed?

If we destroy hci_conn anyway, there is pretty much no point in test_and_clear_bit and we could just use test_bit here.

Regards

Marcel

Re: [PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting

[Alfonso Acosta]

Hi Marcel,


> don't we also need to clear the flag when the new pairing succeed?

Yep, in fact that's the whole point of the flag .... which I missed.
Thanks for your patience :)

> If we destroy hci_conn anyway, there is pretty much no point in test_and_clear_bit and we could just use test_bit here.

Fair point, I will change them to test_bit.




-- 
Alfonso Acosta

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

Re: [PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting

[Alfonso Acosta]

Hi Jhoan and Marcel,

I sent v2 which I hope addresses all your remarks.

Thanks,

Fons

On Fri, Oct 10, 2014 at 10:14 AM, Alfonso Acosta <fons@spotify.com> wrote:
> Hi Marcel,
>
>
>> don't we also need to clear the flag when the new pairing succeed?
>
> Yep, in fact that's the whole point of the flag .... which I missed.
> Thanks for your patience :)
>
>> If we destroy hci_conn anyway, there is pretty much no point in test_and_clear_bit and we could just use test_bit here.
>
> Fair point, I will change them to test_bit.
>
>
>
>
> --
> Alfonso Acosta
>
> Embedded Systems Engineer at Spotify
> Birger Jarlsgatan 61, Stockholm, Sweden
> http://www.spotify.com



-- 
Alfonso Acosta

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

Re: [PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting

[Marcel Holtmann]

Hi Alfonso,

> As an alternative to this patch, I am thinking that it may be worth
> considering two other options:
> 
> 1. Adding an additional repairing operation: MGMT_OP_REPAIR_DEVICE.
> Making the repairing semantics explicit  would allow us to keep the connection
> parameters even if we choose to disconnect when unpairing.  On the
> other hand, one could argue that it clutters the API with an extra
> operation which is the composite of two already existing operations.
> 
> 2. Make MGMT_OP_PAIR_DEVICE behave like the repairing operation in (1)
> if the device was already paired. If I recall properly, Johan suggested
> this on IRC.

I am not in favor of making Pair Device just do re-pairing. I think that is a dangerous behavior since want bondings in the kernel only in two ways. Either they are loaded via Load Long Term Keys or via Pair Device. Doing something magic is something that I consider dangerous and can lead into some flaws. If we come with a clear error than we at least know that something went wrong.

Repairing command is possible, but I am not 100% convinced that it is a good idea. The normal workflow is that you pair and unpair a device. If you get stuck in a weird state, you unpair first which ensures that everything is cleaned out and then pair again. For the connection parameters we can just be smarter.

I bet there is still a lot of improvement for our connection parameter handling. For example we could just keep all connection parameters around in cache and just expire them after 1 day or so. That would improve general handling with devices with do not pair in the first. So I would focus on being smart when dropping connection parameters and not trying to bind it too much to mgmt API visible states.

Regards

Marcel

Re: [PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting

[Alfonso Acosta]

Hi Marcel,

> I am not in favor of making Pair Device just do re-pairing. I think that =
is a dangerous behavior since want bondings in the kernel only in two ways.=
 Either they are loaded via Load Long Term Keys or via Pair Device. Doing s=
omething magic is something that I consider dangerous and can lead into som=
e flaws. If we come with a clear error than we at least know that something=
 went wrong.
>
> Repairing command is possible, but I am not 100% convinced that it is a g=
ood idea. The normal workflow is that you pair and unpair a device. If you =
get stuck in a weird state, you unpair first which ensures that everything =
is cleaned out and then pair again. For the connection parameters we can ju=
st be smarter.

Fair enough. Does this mean that the current patch is still of
interest? I hope so :)

> I bet there is still a lot of improvement for our connection parameter ha=
ndling. For example we could just keep all connection parameters around in =
cache and just expire them after 1 day or so. That would improve general ha=
ndling with devices with do not pair in the first. So I would focus on bein=
g smart when dropping connection parameters and not trying to bind it too m=
uch to mgmt API visible states.

I actually have a patch lying around which keeps track of
conn_parameters within bluetoothd. I think that it would be a good
start (I would like to complete the submissions I sent this week
getting to it though).


--=20
Alfonso Acosta

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

Re: [PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting

[Marcel Holtmann]

Hi Alfonso,

>> I am not in favor of making Pair Device just do re-pairing. I think that is a dangerous behavior since want bondings in the kernel only in two ways. Either they are loaded via Load Long Term Keys or via Pair Device. Doing something magic is something that I consider dangerous and can lead into some flaws. If we come with a clear error than we at least know that something went wrong.
>> 
>> Repairing command is possible, but I am not 100% convinced that it is a good idea. The normal workflow is that you pair and unpair a device. If you get stuck in a weird state, you unpair first which ensures that everything is cleaned out and then pair again. For the connection parameters we can just be smarter.
> 
> Fair enough. Does this mean that the current patch is still of
> interest? I hope so :)

the kernel side for delaying the removal of the connection parameters is the right thing to do. We should have done in the first place actually.

>> I bet there is still a lot of improvement for our connection parameter handling. For example we could just keep all connection parameters around in cache and just expire them after 1 day or so. That would improve general handling with devices with do not pair in the first. So I would focus on being smart when dropping connection parameters and not trying to bind it too much to mgmt API visible states.
> 
> I actually have a patch lying around which keeps track of
> conn_parameters within bluetoothd. I think that it would be a good
> start (I would like to complete the submissions I sent this week
> getting to it though).

I am not in favor of that. bluetoothd is supposed to be stupid when it comes to these values. I mean stupid in a sense that when the kernel tells us to store them (conn params, IRK, LTK etc.), the daemon will store them. And on reboot just reload the whole thing back into the kernel. On unpairing/removing a device, the only other thing the daemon is suppose to be doing is clear all we know about that device. Meaning removing all stored keys and params. That is all it is suppose to be doing. Plain and simple.

This basic concept has gotten us pretty far and allowed us to put the kernel in control of handling the nasty details when it comes to pairing and connection handling.

But as I said before, we can be smarter inside the kernel. So the advertising data can actually contain proposed connection interval values from the peripheral. A peripheral can you tell the central its preferred values from the start. We are currently not parsing the advertising data and pre-filling the connection request with these values from the advertising report. That is something we could do. That will clearly improve our handling of peripherals that do not create a bond with us.

And for peripherals that do not keep a bond with us, we can actually just keep the connection parameters for x amount of time and then expire them like we expire the inquiry cache. That is exactly how the inquiry cache in BR/EDR works to speed up the connection creation. We will try to re-read the clock offset before terminating a connection so that when you do a re-connection right away, we use the right clock offset.

There are tons of improvements that we can do. And most of them can be done without changing any behavior in bluetoothd or changing the mgmt API.

Regards

Marcel