Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] Bluetooth: Fix updating connecton state in `hci_encrypt_cfm`
@ 2020-07-15  8:59 Patrick Steinhardt
  2020-07-15 15:40 ` Luiz Augusto von Dentz
  2020-07-15 17:43 ` [PATCH v2] Bluetooth: Fix update of connection " Patrick Steinhardt
  0 siblings, 2 replies; 6+ messages in thread
From: Patrick Steinhardt @ 2020-07-15  8:59 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz


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

Starting with the upgrade to v5.8-rc3, I've noticed I wasn't able to
connect to my Bluetooth headset properly anymore. While connecting to
the device would eventually succeed, bluetoothd seemed to be confused
about the current connection state where the state was flapping hence
and forth. Bisecting this issue led to commit 3ca44c16b0dc ("Bluetooth:
Consolidate encryption handling in hci_encrypt_cfm"), which refactored
`hci_encrypt_cfm` to also handle updating the connection state.

The commit in question changed the code to call `hci_connect_cfm` inside
`hci_encrypt_cfm` and updating the connection state. But the conversion
didn't keep old behaviour of when the connection state is updated, which
now causes us to not properly update it anymore.

Fix the issue by adding another parameter to the function that allows
callers to specify whether the connection state should be updated, which
allows us to restore previous behaviour.

Fixes: 3ca44c16b0dc ("Bluetooth: Consolidate encryption handling in hci_encrypt_cfm")
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 include/net/bluetooth/hci_core.h | 4 ++--
 net/bluetooth/hci_event.c        | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index cdd4f1db8670..9abcc4a89abc 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1381,13 +1381,13 @@ static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status)
 		conn->security_cfm_cb(conn, status);
 }
 
-static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status)
+static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status, __u8 update_state)
 {
 	struct hci_cb *cb;
 	__u8 encrypt;
 
 	if (conn->state == BT_CONFIG) {
-		if (status)
+		if (update_state)
 			conn->state = BT_CONNECTED;
 
 		hci_connect_cfm(conn, status);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index cfeaee347db3..483d35eda2f1 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2931,7 +2931,7 @@ static void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 				     &cp);
 		} else {
 			clear_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags);
-			hci_encrypt_cfm(conn, ev->status);
+			hci_encrypt_cfm(conn, ev->status, 0);
 		}
 	}
 
@@ -3016,7 +3016,7 @@ static void read_enc_key_size_complete(struct hci_dev *hdev, u8 status,
 		conn->enc_key_size = rp->key_size;
 	}
 
-	hci_encrypt_cfm(conn, 0);
+	hci_encrypt_cfm(conn, 0, 1);
 
 unlock:
 	hci_dev_unlock(hdev);
@@ -3134,7 +3134,7 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	}
 
 notify:
-	hci_encrypt_cfm(conn, ev->status);
+	hci_encrypt_cfm(conn, ev->status, !ev->status);
 
 unlock:
 	hci_dev_unlock(hdev);
-- 
2.27.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Bluetooth: Fix updating connecton state in `hci_encrypt_cfm`
  2020-07-15  8:59 [PATCH] Bluetooth: Fix updating connecton state in `hci_encrypt_cfm` Patrick Steinhardt
@ 2020-07-15 15:40 ` Luiz Augusto von Dentz
  2020-07-15 17:26   ` Patrick Steinhardt
  2020-07-15 17:43 ` [PATCH v2] Bluetooth: Fix update of connection " Patrick Steinhardt
  1 sibling, 1 reply; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2020-07-15 15:40 UTC (permalink / raw)
  To: Patrick Steinhardt
  Cc: linux-bluetooth, Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz

Hi Patrick,

On Wed, Jul 15, 2020 at 7:50 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> Starting with the upgrade to v5.8-rc3, I've noticed I wasn't able to
> connect to my Bluetooth headset properly anymore. While connecting to
> the device would eventually succeed, bluetoothd seemed to be confused
> about the current connection state where the state was flapping hence
> and forth. Bisecting this issue led to commit 3ca44c16b0dc ("Bluetooth:
> Consolidate encryption handling in hci_encrypt_cfm"), which refactored
> `hci_encrypt_cfm` to also handle updating the connection state.
>
> The commit in question changed the code to call `hci_connect_cfm` inside
> `hci_encrypt_cfm` and updating the connection state. But the conversion
> didn't keep old behaviour of when the connection state is updated, which
> now causes us to not properly update it anymore.
>
> Fix the issue by adding another parameter to the function that allows
> callers to specify whether the connection state should be updated, which
> allows us to restore previous behaviour.
>
> Fixes: 3ca44c16b0dc ("Bluetooth: Consolidate encryption handling in hci_encrypt_cfm")
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  include/net/bluetooth/hci_core.h | 4 ++--
>  net/bluetooth/hci_event.c        | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index cdd4f1db8670..9abcc4a89abc 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1381,13 +1381,13 @@ static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status)
>                 conn->security_cfm_cb(conn, status);
>  }
>
> -static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status)
> +static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status, __u8 update_state)
>  {
>         struct hci_cb *cb;
>         __u8 encrypt;
>
>         if (conn->state == BT_CONFIG) {
> -               if (status)
> +               if (update_state)

The intent was actually to have if (!status) as it means the
encryption has succeeded the state can be considered connected, so I
wonder if we really need to introduce another parameter. Anyway as it
is broken we need to come up with a fix rather quickly.

>                         conn->state = BT_CONNECTED;
>
>                 hci_connect_cfm(conn, status);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index cfeaee347db3..483d35eda2f1 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2931,7 +2931,7 @@ static void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
>                                      &cp);
>                 } else {
>                         clear_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags);
> -                       hci_encrypt_cfm(conn, ev->status);
> +                       hci_encrypt_cfm(conn, ev->status, 0);
>                 }
>         }
>
> @@ -3016,7 +3016,7 @@ static void read_enc_key_size_complete(struct hci_dev *hdev, u8 status,
>                 conn->enc_key_size = rp->key_size;
>         }
>
> -       hci_encrypt_cfm(conn, 0);
> +       hci_encrypt_cfm(conn, 0, 1);
>
>  unlock:
>         hci_dev_unlock(hdev);
> @@ -3134,7 +3134,7 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
>         }
>
>  notify:
> -       hci_encrypt_cfm(conn, ev->status);
> +       hci_encrypt_cfm(conn, ev->status, !ev->status);
>
>  unlock:
>         hci_dev_unlock(hdev);
> --
> 2.27.0
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Bluetooth: Fix updating connecton state in `hci_encrypt_cfm`
  2020-07-15 15:40 ` Luiz Augusto von Dentz
@ 2020-07-15 17:26   ` Patrick Steinhardt
  0 siblings, 0 replies; 6+ messages in thread
From: Patrick Steinhardt @ 2020-07-15 17:26 UTC (permalink / raw)
  To: Luiz Augusto von Dentz
  Cc: linux-bluetooth, Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz


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

On Wed, Jul 15, 2020 at 08:40:05AM -0700, Luiz Augusto von Dentz wrote:
> Hi Patrick,
> 
> On Wed, Jul 15, 2020 at 7:50 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > Starting with the upgrade to v5.8-rc3, I've noticed I wasn't able to
> > connect to my Bluetooth headset properly anymore. While connecting to
> > the device would eventually succeed, bluetoothd seemed to be confused
> > about the current connection state where the state was flapping hence
> > and forth. Bisecting this issue led to commit 3ca44c16b0dc ("Bluetooth:
> > Consolidate encryption handling in hci_encrypt_cfm"), which refactored
> > `hci_encrypt_cfm` to also handle updating the connection state.
> >
> > The commit in question changed the code to call `hci_connect_cfm` inside
> > `hci_encrypt_cfm` and updating the connection state. But the conversion
> > didn't keep old behaviour of when the connection state is updated, which
> > now causes us to not properly update it anymore.
> >
> > Fix the issue by adding another parameter to the function that allows
> > callers to specify whether the connection state should be updated, which
> > allows us to restore previous behaviour.
> >
> > Fixes: 3ca44c16b0dc ("Bluetooth: Consolidate encryption handling in hci_encrypt_cfm")
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  include/net/bluetooth/hci_core.h | 4 ++--
> >  net/bluetooth/hci_event.c        | 6 +++---
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index cdd4f1db8670..9abcc4a89abc 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -1381,13 +1381,13 @@ static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status)
> >                 conn->security_cfm_cb(conn, status);
> >  }
> >
> > -static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status)
> > +static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status, __u8 update_state)
> >  {
> >         struct hci_cb *cb;
> >         __u8 encrypt;
> >
> >         if (conn->state == BT_CONFIG) {
> > -               if (status)
> > +               if (update_state)
> 
> The intent was actually to have if (!status) as it means the
> encryption has succeeded the state can be considered connected, so I
> wonder if we really need to introduce another parameter. Anyway as it
> is broken we need to come up with a fix rather quickly.

Yeah, I kind of figured that to be the case but wanted to go with the
"safe" fix of restoring old behaviour first. I'll test whether `if
(!status)` fixes the problem I'm seeing and will send a v2 in case it
does.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] Bluetooth: Fix update of connection state in `hci_encrypt_cfm`
  2020-07-15  8:59 [PATCH] Bluetooth: Fix updating connecton state in `hci_encrypt_cfm` Patrick Steinhardt
  2020-07-15 15:40 ` Luiz Augusto von Dentz
@ 2020-07-15 17:43 ` Patrick Steinhardt
  2020-07-15 18:34   ` Luiz Augusto von Dentz
  2020-07-16  6:40   ` Marcel Holtmann
  1 sibling, 2 replies; 6+ messages in thread
From: Patrick Steinhardt @ 2020-07-15 17:43 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz


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

Starting with the upgrade to v5.8-rc3, I've noticed I wasn't able to
connect to my Bluetooth headset properly anymore. While connecting to
the device would eventually succeed, bluetoothd seemed to be confused
about the current connection state where the state was flapping hence
and forth. Bisecting this issue led to commit 3ca44c16b0dc (Bluetooth:
Consolidate encryption handling in hci_encrypt_cfm, 2020-05-19), which
refactored `hci_encrypt_cfm` to also handle updating the connection
state.

The commit in question changed the code to call `hci_connect_cfm` inside
`hci_encrypt_cfm` and to change the connection state. But with the
conversion, we now only update the connection state if a status was set
already. In fact, the reverse should be true: the status should be
updated if no status is yet set. So let's fix the isuse by reversing the
condition.

Fixes: 3ca44c16b0dc ("Bluetooth: Consolidate encryption handling in hci_encrypt_cfm")
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 include/net/bluetooth/hci_core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index cdd4f1db8670..da3728871e85 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1387,7 +1387,7 @@ static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status)
 	__u8 encrypt;
 
 	if (conn->state == BT_CONFIG) {
-		if (status)
+		if (!status)
 			conn->state = BT_CONNECTED;
 
 		hci_connect_cfm(conn, status);
-- 
2.27.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] Bluetooth: Fix update of connection state in `hci_encrypt_cfm`
  2020-07-15 17:43 ` [PATCH v2] Bluetooth: Fix update of connection " Patrick Steinhardt
@ 2020-07-15 18:34   ` Luiz Augusto von Dentz
  2020-07-16  6:40   ` Marcel Holtmann
  1 sibling, 0 replies; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2020-07-15 18:34 UTC (permalink / raw)
  To: Patrick Steinhardt
  Cc: linux-bluetooth, Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz

Hi Patrick,

On Wed, Jul 15, 2020 at 10:45 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> Starting with the upgrade to v5.8-rc3, I've noticed I wasn't able to
> connect to my Bluetooth headset properly anymore. While connecting to
> the device would eventually succeed, bluetoothd seemed to be confused
> about the current connection state where the state was flapping hence
> and forth. Bisecting this issue led to commit 3ca44c16b0dc (Bluetooth:
> Consolidate encryption handling in hci_encrypt_cfm, 2020-05-19), which
> refactored `hci_encrypt_cfm` to also handle updating the connection
> state.
>
> The commit in question changed the code to call `hci_connect_cfm` inside
> `hci_encrypt_cfm` and to change the connection state. But with the
> conversion, we now only update the connection state if a status was set
> already. In fact, the reverse should be true: the status should be
> updated if no status is yet set. So let's fix the isuse by reversing the
> condition.
>
> Fixes: 3ca44c16b0dc ("Bluetooth: Consolidate encryption handling in hci_encrypt_cfm")
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Acked-by:  Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

> ---
>  include/net/bluetooth/hci_core.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index cdd4f1db8670..da3728871e85 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1387,7 +1387,7 @@ static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status)
>         __u8 encrypt;
>
>         if (conn->state == BT_CONFIG) {
> -               if (status)
> +               if (!status)
>                         conn->state = BT_CONNECTED;
>
>                 hci_connect_cfm(conn, status);
> --
> 2.27.0
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] Bluetooth: Fix update of connection state in `hci_encrypt_cfm`
  2020-07-15 17:43 ` [PATCH v2] Bluetooth: Fix update of connection " Patrick Steinhardt
  2020-07-15 18:34   ` Luiz Augusto von Dentz
@ 2020-07-16  6:40   ` Marcel Holtmann
  1 sibling, 0 replies; 6+ messages in thread
From: Marcel Holtmann @ 2020-07-16  6:40 UTC (permalink / raw)
  To: Patrick Steinhardt
  Cc: Bluetooth Kernel Mailing List, Johan Hedberg, Luiz Augusto von Dentz

Hi Patrick,

> Starting with the upgrade to v5.8-rc3, I've noticed I wasn't able to
> connect to my Bluetooth headset properly anymore. While connecting to
> the device would eventually succeed, bluetoothd seemed to be confused
> about the current connection state where the state was flapping hence
> and forth. Bisecting this issue led to commit 3ca44c16b0dc (Bluetooth:
> Consolidate encryption handling in hci_encrypt_cfm, 2020-05-19), which
> refactored `hci_encrypt_cfm` to also handle updating the connection
> state.
> 
> The commit in question changed the code to call `hci_connect_cfm` inside
> `hci_encrypt_cfm` and to change the connection state. But with the
> conversion, we now only update the connection state if a status was set
> already. In fact, the reverse should be true: the status should be
> updated if no status is yet set. So let's fix the isuse by reversing the
> condition.
> 
> Fixes: 3ca44c16b0dc ("Bluetooth: Consolidate encryption handling in hci_encrypt_cfm")
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> include/net/bluetooth/hci_core.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-15  8:59 [PATCH] Bluetooth: Fix updating connecton state in `hci_encrypt_cfm` Patrick Steinhardt
2020-07-15 15:40 ` Luiz Augusto von Dentz
2020-07-15 17:26   ` Patrick Steinhardt
2020-07-15 17:43 ` [PATCH v2] Bluetooth: Fix update of connection " Patrick Steinhardt
2020-07-15 18:34   ` Luiz Augusto von Dentz
2020-07-16  6:40   ` Marcel Holtmann

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org
	public-inbox-index linux-bluetooth

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git