[PATCH v4] Bluetooth: Fix Device Scan and connection collision

[PATCH v4] Bluetooth: Fix Device Scan and connection collision

[ramm]

From: Ram Malovany <ramm@ti.com>

During search of devices, HCI Remote Name Request Command is sent for
every device which name was not included in inquiry result. But the
same command is also sent during incoming connection establishing
procedure. Function hci_check_pending_name() was fixed in order to
handle this situation which led to a kernel crash when initiating
an incoming connection from a device that was not found in the
inquiry while doing a search. There is no need to continue resolving
the next name if we get the request from the incoming connection
procedure as it will be done upon receiving another remote name
request complete event

Signed-off-by: Ram Malovany <ramm@ti.com>
---
 net/bluetooth/hci_event.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 41ff978..2814ea4 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1365,7 +1365,7 @@ static bool hci_resolve_next_name(struct hci_dev *hdev)
 		return false;
 
 	e = hci_inquiry_cache_lookup_resolve(hdev, BDADDR_ANY, NAME_NEEDED);
-	if (hci_resolve_name(hdev, e) == 0) {
+	if (e && hci_resolve_name(hdev, e) == 0) {
 		e->name_state = NAME_PENDING;
 		return true;
 	}
@@ -1379,6 +1379,9 @@ static void hci_check_pending_name(struct hci_dev *hdev, struct hci_conn *conn,
 	struct discovery_state *discov = &hdev->discovery;
 	struct inquiry_entry *e;
 
+	BT_DBG("name %p, name_len %d, discov->state %d",
+		name, name_len, discov->state);
+
 	if (conn && !test_and_set_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags))
 		mgmt_device_connected(hdev, bdaddr, ACL_LINK, 0x00, 0, name,
 				      name_len, conn->dev_class);
@@ -1393,12 +1396,16 @@ static void hci_check_pending_name(struct hci_dev *hdev, struct hci_conn *conn,
 		return;
 
 	e = hci_inquiry_cache_lookup_resolve(hdev, bdaddr, NAME_PENDING);
-	if (e) {
+	if (!e)
+		return;
+
+	list_del(&e->list);
+	if (name) {
 		e->name_state = NAME_KNOWN;
-		list_del(&e->list);
-		if (name)
-			mgmt_remote_name(hdev, bdaddr, ACL_LINK, 0x00,
-					 e->data.rssi, name, name_len);
+		mgmt_remote_name(hdev, bdaddr, ACL_LINK, 0x00,
+				 e->data.rssi, name, name_len);
+	} else {
+		e->name_state = NAME_NOT_KNOWN;
 	}
 
 	if (hci_resolve_next_name(hdev))
@@ -1996,7 +2003,7 @@ static void hci_remote_name_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	struct hci_ev_remote_name *ev = (void *) skb->data;
 	struct hci_conn *conn;
 
-	BT_DBG("%s", hdev->name);
+	BT_DBG("%s, status 0x%2.2x", hdev->name, ev->status);
 
 	hci_conn_check_pending(hdev);
 
-- 
1.7.4.1

Re: [PATCH v4] Bluetooth: Fix Device Scan and connection collision

[Johan Hedberg]

Hi Ram,

On Wed, Jul 18, 2012, ramm@ti.com wrote:
> During search of devices, HCI Remote Name Request Command is sent for
> every device which name was not included in inquiry result. But the
> same command is also sent during incoming connection establishing
> procedure. Function hci_check_pending_name() was fixed in order to
> handle this situation which led to a kernel crash when initiating
> an incoming connection from a device that was not found in the
> inquiry while doing a search. There is no need to continue resolving
> the next name if we get the request from the incoming connection
> procedure as it will be done upon receiving another remote name
> request complete event
> 
> Signed-off-by: Ram Malovany <ramm@ti.com>
> ---
>  net/bluetooth/hci_event.c |   21 ++++++++++++++-------
>  1 files changed, 14 insertions(+), 7 deletions(-)

The fixes in this patch seem correct to me but there seems to be three
of them and each one could be considered independently. I'd therefore
split this into three separate patches:

>  	e = hci_inquiry_cache_lookup_resolve(hdev, BDADDR_ANY, NAME_NEEDED);
> -	if (hci_resolve_name(hdev, e) == 0) {
> +	if (e && hci_resolve_name(hdev, e) == 0) {
>  		e->name_state = NAME_PENDING;
>  		return true;
>  	}

This missing NULL check is the first fix. You might actually consider
doing:

	if (!e)
		return false;

	...

>  	e = hci_inquiry_cache_lookup_resolve(hdev, bdaddr, NAME_PENDING);
> -	if (e) {
> +	if (!e)
> +		return;

This is the second fix. Since it's not directly clear why it's safe to
risk not setting DISCOVERY_STOPPED in this case I'd also add a code
comment explaining it (that if there's no matching entry in the cache it
must mean that there's another pending command ongoing which we can
safely wait for).

> +	} else {
> +		e->name_state = NAME_NOT_KNOWN;
>  	}

This missing setting back to NAME_NOT_KNOWN when name resolving fails is
the third fix.

Please let me know if I misunderstood something or if some of these
fixes really must go into the same patch (e.g. if applying just one of
them will make the code more broken than it is now).

Johan

RE: [PATCH v4] Bluetooth: Fix Device Scan and connection collision

[Malovany, Ram]

> -----Original Message-----
> From: Johan Hedberg [mailto:johan.hedberg@gmail.com]
> Sent: Wednesday, July 18, 2012 11:24 AM
> To: Malovany, Ram
> Cc: linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH v4] Bluetooth: Fix Device Scan and connection collision
> 
> Hi Ram,
> 
> On Wed, Jul 18, 2012, ramm@ti.com wrote:
> > During search of devices, HCI Remote Name Request Command is sent for
> > every device which name was not included in inquiry result. But the
> > same command is also sent during incoming connection establishing
> > procedure. Function hci_check_pending_name() was fixed in order to
> > handle this situation which led to a kernel crash when initiating
> > an incoming connection from a device that was not found in the
> > inquiry while doing a search. There is no need to continue resolving
> > the next name if we get the request from the incoming connection
> > procedure as it will be done upon receiving another remote name
> > request complete event
> >
> > Signed-off-by: Ram Malovany <ramm@ti.com>
> > ---
> >  net/bluetooth/hci_event.c |   21 ++++++++++++++-------
> >  1 files changed, 14 insertions(+), 7 deletions(-)
> 
> The fixes in this patch seem correct to me but there seems to be three
> of them and each one could be considered independently. I'd therefore
> split this into three separate patches:
> 
> >  	e = hci_inquiry_cache_lookup_resolve(hdev, BDADDR_ANY, NAME_NEEDED);
> > -	if (hci_resolve_name(hdev, e) == 0) {
> > +	if (e && hci_resolve_name(hdev, e) == 0) {
> >  		e->name_state = NAME_PENDING;
> >  		return true;
> >  	}
> 
> This missing NULL check is the first fix. You might actually consider
> doing:
> 
> 	if (!e)
> 		return false;
> 

I didn't want to change the coding in this specific file , as you can see in the function:

hci_inquiry_complete_evt() , calling it the same way. 
But I can changed them both if needed. Do you want me to ?

> 	...
> 
> >  	e = hci_inquiry_cache_lookup_resolve(hdev, bdaddr, NAME_PENDING);
> > -	if (e) {
> > +	if (!e)
> > +		return;
> 
> This is the second fix. Since it's not directly clear why it's safe to
> risk not setting DISCOVERY_STOPPED in this case I'd also add a code
> comment explaining it (that if there's no matching entry in the cache it
> must mean that there's another pending command ongoing which we can
> safely wait for).

Correct I will add a short comment.

> 
> > +	} else {
> > +		e->name_state = NAME_NOT_KNOWN;
> >  	}
> 
> This missing setting back to NAME_NOT_KNOWN when name resolving fails is
> the third fix.
> 
> Please let me know if I misunderstood something or if some of these
> fixes really must go into the same patch (e.g. if applying just one of
> them will make the code more broken than it is now).
> 

This patch can be split to 3 ways , and no applying only one of them would not
Make the code more broken that it is right now , but it's important to integrate 
all of them in order to make the code more stable.

I will send a new set of patches that include your comments, but I don't know if I will
Change the first fix here due to the current code implementation.

> Johan

Thanks,
Ram

Re: [PATCH v4] Bluetooth: Fix Device Scan and connection collision

[Johan Hedberg]

Hi Ram,

On Wed, Jul 18, 2012, Malovany, Ram wrote:
> > >  	e = hci_inquiry_cache_lookup_resolve(hdev, BDADDR_ANY, NAME_NEEDED);
> > > -	if (hci_resolve_name(hdev, e) == 0) {
> > > +	if (e && hci_resolve_name(hdev, e) == 0) {
> > >  		e->name_state = NAME_PENDING;
> > >  		return true;
> > >  	}
> > 
> > This missing NULL check is the first fix. You might actually consider
> > doing:
> > 
> > 	if (!e)
> > 		return false;
> > 
> 
> I didn't want to change the coding in this specific file , as you can
> see in the function:
> 
> hci_inquiry_complete_evt() , calling it the same way. 
> But I can changed them both if needed. Do you want me to ?

The code in hci_inquiry_complete_evt() is a bit different though. There
you have something else instead of a simple return statement if e is
NULL (set the discovery state and unlock hdev) so changing that part
wouldn't really simplify the code much. In your case the code does
however get a bit simpler since all you need to do is return false.

Johan

RE: [PATCH v4] Bluetooth: Fix Device Scan and connection collision

[Malovany, Ram]

Hi Johan,

> -----Original Message-----
> From: Johan Hedberg [mailto:johan.hedberg@gmail.com]
> Sent: Wednesday, July 18, 2012 1:20 PM
> To: Malovany, Ram
> Cc: linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH v4] Bluetooth: Fix Device Scan and connection collision
> 
> Hi Ram,
> 
> On Wed, Jul 18, 2012, Malovany, Ram wrote:
> > > >  	e = hci_inquiry_cache_lookup_resolve(hdev, BDADDR_ANY,
> NAME_NEEDED);
> > > > -	if (hci_resolve_name(hdev, e) == 0) {
> > > > +	if (e && hci_resolve_name(hdev, e) == 0) {
> > > >  		e->name_state = NAME_PENDING;
> > > >  		return true;
> > > >  	}
> > >
> > > This missing NULL check is the first fix. You might actually consider
> > > doing:
> > >
> > > 	if (!e)
> > > 		return false;
> > >
> >
> > I didn't want to change the coding in this specific file , as you can
> > see in the function:
> >
> > hci_inquiry_complete_evt() , calling it the same way.
> > But I can changed them both if needed. Do you want me to ?
> 
> The code in hci_inquiry_complete_evt() is a bit different though. There
> you have something else instead of a simple return statement if e is
> NULL (set the discovery state and unlock hdev) so changing that part
> wouldn't really simplify the code much. In your case the code does
> however get a bit simpler since all you need to do is return false.
> 


OK  - will update it according to your comments. Thanks

> Johan


Regards,

Ram