[RFC] Bluetooth: Fix not sending link key negative reply

[RFC] Bluetooth: Fix not sending link key negative reply

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

If Link Keys are not loaded then HCI_LINK_KEYS is not set
and for HCI Event "Link Key Request" reply is not sent.
--
send as RFC since I did not get why we need this flag really.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/hci_event.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index db58e72..e7de1df 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2611,11 +2611,11 @@ static void hci_link_key_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	BT_DBG("%s", hdev->name);
 
-	if (!test_bit(HCI_LINK_KEYS, &hdev->dev_flags))
-		return;
-
 	hci_dev_lock(hdev);
 
+	if (!test_bit(HCI_LINK_KEYS, &hdev->dev_flags))
+		goto not_found;
+
 	key = hci_find_link_key(hdev, &ev->bdaddr);
 	if (!key) {
 		BT_DBG("%s link key not found for %pMR", hdev->name,
-- 
1.8.1.2

Re: [RFC] Bluetooth: Fix not sending link key negative reply

[Johan Hedberg]

Hi Andrei,

On Mon, May 06, 2013, Andrei Emeltchenko wrote:
> If Link Keys are not loaded then HCI_LINK_KEYS is not set
> and for HCI Event "Link Key Request" reply is not sent.
> --
> send as RFC since I did not get why we need this flag really.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/hci_event.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index db58e72..e7de1df 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2611,11 +2611,11 @@ static void hci_link_key_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
>  
>  	BT_DBG("%s", hdev->name);
>  
> -	if (!test_bit(HCI_LINK_KEYS, &hdev->dev_flags))
> -		return;
> -
>  	hci_dev_lock(hdev);
>  
> +	if (!test_bit(HCI_LINK_KEYS, &hdev->dev_flags))
> +		goto not_found;
> +
>  	key = hci_find_link_key(hdev, &ev->bdaddr);
>  	if (!key) {
>  		BT_DBG("%s link key not found for %pMR", hdev->name,

Nack.

This check is there for backwards compatibility with older user space
versions. Your patch would break all user space versions that do not use
the management interface (and hence do not have the HCI_LINK_KEYS flag
set).

That said, the flag is in a way redundant with the HCI_MGMT flag, so the
check could be changed for that and the HCI_LINK_KEYS flag could then be
completely removed.

Johan

[PATCHv2] Bluetooth: Fix not sending link key negative reply

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

If Link Keys are not loaded during initialization with
MGMT_OP_LOAD_LINK_KEYS command then pairing always fails
since HCI_LINK_KEYS is not set. Check instead for HCI_MGMT
flag.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
*v2 modified patch following Johan's comments

 net/bluetooth/hci_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index db58e72..e547cfd 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2611,7 +2611,7 @@ static void hci_link_key_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	BT_DBG("%s", hdev->name);
 
-	if (!test_bit(HCI_LINK_KEYS, &hdev->dev_flags))
+	if (!test_bit(HCI_MGMT, &hdev->dev_flags))
 		return;
 
 	hci_dev_lock(hdev);
-- 
1.8.1.2

Re: [PATCHv2] Bluetooth: Fix not sending link key negative reply

[Johan Hedberg]

Hi Andrei,

On Tue, May 07, 2013, Andrei Emeltchenko wrote:
> If Link Keys are not loaded during initialization with
> MGMT_OP_LOAD_LINK_KEYS command then pairing always fails
> since HCI_LINK_KEYS is not set. Check instead for HCI_MGMT
> flag.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
> *v2 modified patch following Johan's comments

The existing behavior is intentional with the implicit assumption that a
mgmt_load_link_keys will always be received, if only to set the debug
keys flag in case of no link keys. So I'd word the commit message
differently, something like:

	Bluetooth: Use HCI_MGMT flag for link keys management

	The HCI_MGMT flag should be enough to test for a user space that
	expects the kernel to do link keys management. Checking for the
	special HCI_LINK_KEYS flag is a bit overkill for this since we
	don't strictly speaking need to receive the load_link_keys
	command to know that user space talks mgmt - any mgmt command
	should be enough.

Johan

[PATCH 1/2] Bluetooth: Use HCI_MGMT flag for link keys management

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

The HCI_MGMT flag should be enough to test for a user space that
expects the kernel to do link keys management. Checking for the
special HCI_LINK_KEYS flag is a bit overkill for this since we
don't strictly speaking need to receive the load_link_keys
command to know that user space talks mgmt - any mgmt command
should be enough.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/hci_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index db58e72..e547cfd 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2611,7 +2611,7 @@ static void hci_link_key_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	BT_DBG("%s", hdev->name);
 
-	if (!test_bit(HCI_LINK_KEYS, &hdev->dev_flags))
+	if (!test_bit(HCI_MGMT, &hdev->dev_flags))
 		return;
 
 	hci_dev_lock(hdev);
-- 
1.8.1.2

[PATCH 2/2] Bluetooth: Use HCI_MGMT instead of HCI_LINK_KEYS flag

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Remove HCI_LINK_KEYS flag since standard HCI_MGMT can be used instead.
There is also problem with HCI_LINK_KEYS flag since it is set only
when link keys are loaded. Otherwise kernel assumes that old
interface is used.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/hci.h | 1 -
 net/bluetooth/hci_event.c   | 2 +-
 net/bluetooth/mgmt.c        | 2 --
 3 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index e0512aa..3c592cf 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -107,7 +107,6 @@ enum {
 	HCI_MGMT,
 	HCI_PAIRABLE,
 	HCI_SERVICE_CACHE,
-	HCI_LINK_KEYS,
 	HCI_DEBUG_KEYS,
 	HCI_UNREGISTER,
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index e547cfd..0437200 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2687,7 +2687,7 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_conn_drop(conn);
 	}
 
-	if (test_bit(HCI_LINK_KEYS, &hdev->dev_flags))
+	if (test_bit(HCI_MGMT, &hdev->dev_flags))
 		hci_add_link_key(hdev, conn, 1, &ev->bdaddr, ev->link_key,
 				 ev->key_type, pin_len);
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 8567764..5cd3aee 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1736,8 +1736,6 @@ static int load_link_keys(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	hci_link_keys_clear(hdev);
 
-	set_bit(HCI_LINK_KEYS, &hdev->dev_flags);
-
 	if (cp->debug_keys)
 		set_bit(HCI_DEBUG_KEYS, &hdev->dev_flags);
 	else
-- 
1.8.1.2

Re: [PATCH 2/2] Bluetooth: Use HCI_MGMT instead of HCI_LINK_KEYS flag

[Johan Hedberg]

Hi Andrei,

On Tue, May 07, 2013, Andrei Emeltchenko wrote:
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2687,7 +2687,7 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, struct sk_buff *skb)
>  		hci_conn_drop(conn);
>  	}
>  
> -	if (test_bit(HCI_LINK_KEYS, &hdev->dev_flags))
> +	if (test_bit(HCI_MGMT, &hdev->dev_flags))
>  		hci_add_link_key(hdev, conn, 1, &ev->bdaddr, ev->link_key,
>  				 ev->key_type, pin_len);
>  

It seems to me that this chunk would be more appropriately merged to the
first patch.

Johan

[PATCH] Bluetooth: Use HCI_MGMT instead of HCI_LINK_KEYS flag

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Remove HCI_LINK_KEYS flag since standard HCI_MGMT can be used instead.
There is also problem with HCI_LINK_KEYS flag since it is set only
when link keys are loaded. Otherwise kernel assumes that old
interface is used and pairing always fails.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/hci.h | 1 -
 net/bluetooth/hci_event.c   | 4 ++--
 net/bluetooth/mgmt.c        | 2 --
 3 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index e0512aa..3c592cf 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -107,7 +107,6 @@ enum {
 	HCI_MGMT,
 	HCI_PAIRABLE,
 	HCI_SERVICE_CACHE,
-	HCI_LINK_KEYS,
 	HCI_DEBUG_KEYS,
 	HCI_UNREGISTER,
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index db58e72..0437200 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2611,7 +2611,7 @@ static void hci_link_key_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	BT_DBG("%s", hdev->name);
 
-	if (!test_bit(HCI_LINK_KEYS, &hdev->dev_flags))
+	if (!test_bit(HCI_MGMT, &hdev->dev_flags))
 		return;
 
 	hci_dev_lock(hdev);
@@ -2687,7 +2687,7 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_conn_drop(conn);
 	}
 
-	if (test_bit(HCI_LINK_KEYS, &hdev->dev_flags))
+	if (test_bit(HCI_MGMT, &hdev->dev_flags))
 		hci_add_link_key(hdev, conn, 1, &ev->bdaddr, ev->link_key,
 				 ev->key_type, pin_len);
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 8567764..5cd3aee 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1736,8 +1736,6 @@ static int load_link_keys(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	hci_link_keys_clear(hdev);
 
-	set_bit(HCI_LINK_KEYS, &hdev->dev_flags);
-
 	if (cp->debug_keys)
 		set_bit(HCI_DEBUG_KEYS, &hdev->dev_flags);
 	else
-- 
1.8.1.2

Re: [PATCHv2] Bluetooth: Fix not sending link key negative reply

[Andrei Emeltchenko]

Hi Johan,

On Tue, May 07, 2013 at 02:14:36PM +0300, Johan Hedberg wrote:
> Hi Andrei,
> 
> On Tue, May 07, 2013, Andrei Emeltchenko wrote:
> > If Link Keys are not loaded during initialization with
> > MGMT_OP_LOAD_LINK_KEYS command then pairing always fails
> > since HCI_LINK_KEYS is not set. Check instead for HCI_MGMT
> > flag.
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> > *v2 modified patch following Johan's comments
> 
> The existing behavior is intentional with the implicit assumption that a
> mgmt_load_link_keys will always be received, if only to set the debug
> keys flag in case of no link keys. 

There is a valid case that there is no bonded devices and we leave default
debug flags. In that case pairing always fails.

For example I need to execute "dummy link keys loading" just to be able to pair.

Best regards 
Andrei Emeltchenko 

[PATCHv2 1/2] Bluetooth: Use HCI_MGMT instead of HCI_LINK_KEYS flag

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Use HCI_MGMT flag instead of HCI_LINK_KEYS flag. There is a problem with
HCI_LINK_KEYS flag since it is set only when link keys are loaded. Otherwise
kernel assumes that old interface is used.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/hci_event.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index db58e72..0437200 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2611,7 +2611,7 @@ static void hci_link_key_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	BT_DBG("%s", hdev->name);
 
-	if (!test_bit(HCI_LINK_KEYS, &hdev->dev_flags))
+	if (!test_bit(HCI_MGMT, &hdev->dev_flags))
 		return;
 
 	hci_dev_lock(hdev);
@@ -2687,7 +2687,7 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_conn_drop(conn);
 	}
 
-	if (test_bit(HCI_LINK_KEYS, &hdev->dev_flags))
+	if (test_bit(HCI_MGMT, &hdev->dev_flags))
 		hci_add_link_key(hdev, conn, 1, &ev->bdaddr, ev->link_key,
 				 ev->key_type, pin_len);
 
-- 
1.8.1.2

[PATCHv2 2/2] Bluetooth: Remove unneeded flag

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Remove HCI_LINK_KEYS flag since using HCI_MGMT is enough for test that
user space expects the kernel managing link keys.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/hci.h | 1 -
 net/bluetooth/mgmt.c        | 2 --
 2 files changed, 3 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index e0512aa..3c592cf 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -107,7 +107,6 @@ enum {
 	HCI_MGMT,
 	HCI_PAIRABLE,
 	HCI_SERVICE_CACHE,
-	HCI_LINK_KEYS,
 	HCI_DEBUG_KEYS,
 	HCI_UNREGISTER,
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 8567764..5cd3aee 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1736,8 +1736,6 @@ static int load_link_keys(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	hci_link_keys_clear(hdev);
 
-	set_bit(HCI_LINK_KEYS, &hdev->dev_flags);
-
 	if (cp->debug_keys)
 		set_bit(HCI_DEBUG_KEYS, &hdev->dev_flags);
 	else
-- 
1.8.1.2

Re: [PATCHv2 1/2] Bluetooth: Use HCI_MGMT instead of HCI_LINK_KEYS flag

[Johan Hedberg]

Hi Andrei,

On Tue, May 14, 2013, Andrei Emeltchenko wrote:
> Use HCI_MGMT flag instead of HCI_LINK_KEYS flag. There is a problem with
> HCI_LINK_KEYS flag since it is set only when link keys are loaded. Otherwise
> kernel assumes that old interface is used.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/hci_event.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by: Johan Hedberg <johan.hedberg@intel.com>

Johan

Re: [PATCHv2 2/2] Bluetooth: Remove unneeded flag

[Johan Hedberg]

Hi Andrei,

On Tue, May 14, 2013, Andrei Emeltchenko wrote:
> Remove HCI_LINK_KEYS flag since using HCI_MGMT is enough for test that
> user space expects the kernel managing link keys.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/hci.h | 1 -
>  net/bluetooth/mgmt.c        | 2 --
>  2 files changed, 3 deletions(-)

Acked-by: Johan Hedberg <johan.hedberg@intel.com>

Johan

Re: [PATCHv2 1/2] Bluetooth: Use HCI_MGMT instead of HCI_LINK_KEYS flag

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2013-05-14 11:44:16 +0300]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Use HCI_MGMT flag instead of HCI_LINK_KEYS flag. There is a problem with
> HCI_LINK_KEYS flag since it is set only when link keys are loaded. Otherwise
> kernel assumes that old interface is used.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/hci_event.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Patch has been applied to bluetooth.git. Thanks.

	Gustavo

Re: [PATCHv2 2/2] Bluetooth: Remove unneeded flag

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2013-05-14 11:44:17 +0300]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Remove HCI_LINK_KEYS flag since using HCI_MGMT is enough for test that
> user space expects the kernel managing link keys.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/hci.h | 1 -
>  net/bluetooth/mgmt.c        | 2 --
>  2 files changed, 3 deletions(-)

Since patch one went to bluetooth.git, I'll have to wait for a merge of both
trees to apply this one. Once the bluetooth.git gets merged in
bluetooth-next.git I'll apply this patch.

	Gustavo

Re: [PATCHv2 2/2] Bluetooth: Remove unneeded flag

[Gustavo Padovan]

Hi Andrei,

* Gustavo Padovan <gustavo@padovan.org> [2013-05-20 17:59:41 -0300]:

> Hi Andrei,
> 
> * Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2013-05-14 11:44:17 +0300]:
> 
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > 
> > Remove HCI_LINK_KEYS flag since using HCI_MGMT is enough for test that
> > user space expects the kernel managing link keys.
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  include/net/bluetooth/hci.h | 1 -
> >  net/bluetooth/mgmt.c        | 2 --
> >  2 files changed, 3 deletions(-)
> 
> Since patch one went to bluetooth.git, I'll have to wait for a merge of both
> trees to apply this one. Once the bluetooth.git gets merged in
> bluetooth-next.git I'll apply this patch.

I was told by Johan that this only happens in your experimental code, so both
patches are now applied to bluetooth-next nad not anymore to bluetooth.git.
Thanks.

	Gustavo