[PATCH-stable] Bluetooth: hci_conn: Fix hci_connect_le_sync

[PATCH-stable] Bluetooth: hci_conn: Fix hci_connect_le_sync

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

The handling of connection failures shall be handled by the request
completion callback as already done by hci_cs_le_create_conn, also make
sure to use hci_conn_failed instead of hci_le_conn_failed as the later
don't actually call hci_conn_del to cleanup.

Link: https://github.com/bluez/bluez/issues/340
Fixes: 8e8b92ee60de5 ("Bluetooth: hci_sync: Add hci_le_create_conn_sync")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/hci_conn.c  | 5 +++--
 net/bluetooth/hci_event.c | 8 +++++---
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 882a7df13005..ac06c9724c7f 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -943,10 +943,11 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
 
 	bt_dev_err(hdev, "request failed to create LE connection: err %d", err);
 
-	if (!conn)
+	/* Check if connection is still pending */
+	if (conn != hci_lookup_le_connect(hdev))
 		goto done;
 
-	hci_le_conn_failed(conn, err);
+	hci_conn_failed(conn, err);
 
 done:
 	hci_dev_unlock(hdev);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 0270e597c285..af17dfb20e01 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -5632,10 +5632,12 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status,
 		status = HCI_ERROR_INVALID_PARAMETERS;
 	}
 
-	if (status) {
-		hci_conn_failed(conn, status);
+	/* All connection failure handling is taken care of by the
+	 * hci_conn_failed function which is triggered by the HCI
+	 * request completion callbacks used for connecting.
+	 */
+	if (status)
 		goto unlock;
-	}
 
 	if (conn->dst_type == ADDR_LE_DEV_PUBLIC)
 		addr_type = BDADDR_LE_PUBLIC;
-- 
2.35.1

RE: [PATCH-stable] Bluetooth: hci_conn: Fix hci_connect_le_sync

[bluez.test.bot]

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

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=643739

---Test result---

Test Summary:
CheckPatch                    PASS      1.85 seconds
GitLint                       PASS      1.06 seconds
SubjectPrefix                 PASS      0.89 seconds
BuildKernel                   PASS      37.24 seconds
BuildKernel32                 PASS      33.03 seconds
Incremental Build with patchesPASS      43.64 seconds
TestRunner: Setup             PASS      538.97 seconds
TestRunner: l2cap-tester      PASS      18.95 seconds
TestRunner: bnep-tester       PASS      6.88 seconds
TestRunner: mgmt-tester       PASS      111.80 seconds
TestRunner: rfcomm-tester     PASS      10.68 seconds
TestRunner: sco-tester        PASS      10.41 seconds
TestRunner: smp-tester        PASS      10.40 seconds
TestRunner: userchan-tester   PASS      7.14 seconds



---
Regards,
Linux Bluetooth

Re: [PATCH-stable] Bluetooth: hci_conn: Fix hci_connect_le_sync

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Fri, 20 May 2022 11:37:13 -0700 you wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> The handling of connection failures shall be handled by the request
> completion callback as already done by hci_cs_le_create_conn, also make
> sure to use hci_conn_failed instead of hci_le_conn_failed as the later
> don't actually call hci_conn_del to cleanup.
> 
> [...]

Here is the summary with links:
  - [PATCH-stable] Bluetooth: hci_conn: Fix hci_connect_le_sync
    https://git.kernel.org/bluetooth/bluetooth-next/c/c9f73a2178c1

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH-stable] Bluetooth: hci_conn: Fix hci_connect_le_sync

[Ahmad Fatoum]

On 20.05.22 20:37, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> The handling of connection failures shall be handled by the request
> completion callback as already done by hci_cs_le_create_conn, also make
> sure to use hci_conn_failed instead of hci_le_conn_failed as the later
> don't actually call hci_conn_del to cleanup.
> 
> Link: https://github.com/bluez/bluez/issues/340
> Fixes: 8e8b92ee60de5 ("Bluetooth: hci_sync: Add hci_le_create_conn_sync")
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

A bit late, as I am not subscribed to linux-bluetooth and didn't notice this
patch, but FWIW: Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

  Bluetooth: hci0: Opcode 0x200d failed: -110
  Bluetooth: hci0: request failed to create LE connection: err -110

now, whereas before it crashed the kernel.

Cheers,
Ahmad

> ---
>  net/bluetooth/hci_conn.c  | 5 +++--
>  net/bluetooth/hci_event.c | 8 +++++---
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 882a7df13005..ac06c9724c7f 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -943,10 +943,11 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
>  
>  	bt_dev_err(hdev, "request failed to create LE connection: err %d", err);
>  
> -	if (!conn)
> +	/* Check if connection is still pending */
> +	if (conn != hci_lookup_le_connect(hdev))
>  		goto done;
>  
> -	hci_le_conn_failed(conn, err);
> +	hci_conn_failed(conn, err);
>  
>  done:
>  	hci_dev_unlock(hdev);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 0270e597c285..af17dfb20e01 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -5632,10 +5632,12 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status,
>  		status = HCI_ERROR_INVALID_PARAMETERS;
>  	}
>  
> -	if (status) {
> -		hci_conn_failed(conn, status);
> +	/* All connection failure handling is taken care of by the
> +	 * hci_conn_failed function which is triggered by the HCI
> +	 * request completion callbacks used for connecting.
> +	 */
> +	if (status)
>  		goto unlock;
> -	}
>  
>  	if (conn->dst_type == ADDR_LE_DEV_PUBLIC)
>  		addr_type = BDADDR_LE_PUBLIC;


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH-stable] Bluetooth: hci_conn: Fix hci_connect_le_sync

[Ahmad Fatoum]

Hello Luiz,

On 24.05.22 16:48, Ahmad Fatoum wrote:
> On 20.05.22 20:37, Luiz Augusto von Dentz wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> The handling of connection failures shall be handled by the request
>> completion callback as already done by hci_cs_le_create_conn, also make
>> sure to use hci_conn_failed instead of hci_le_conn_failed as the later
>> don't actually call hci_conn_del to cleanup.
>>
>> Link: https://github.com/bluez/bluez/issues/340
>> Fixes: 8e8b92ee60de5 ("Bluetooth: hci_sync: Add hci_le_create_conn_sync")
>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> A bit late, as I am not subscribed to linux-bluetooth and didn't notice this
> patch, but FWIW: Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
>   Bluetooth: hci0: Opcode 0x200d failed: -110
>   Bluetooth: hci0: request failed to create LE connection: err -110
> 
> now, whereas before it crashed the kernel.

I see now that this fix doesn't build for v5.17 because hci_conn_failed
was only introduced in v5.18. Can the hci_conn.c hunk be safely dropped?

Thanks,
Ahmad

> 
> Cheers,
> Ahmad
> 
>> ---
>>  net/bluetooth/hci_conn.c  | 5 +++--
>>  net/bluetooth/hci_event.c | 8 +++++---
>>  2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> index 882a7df13005..ac06c9724c7f 100644
>> --- a/net/bluetooth/hci_conn.c
>> +++ b/net/bluetooth/hci_conn.c
>> @@ -943,10 +943,11 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
>>  
>>  	bt_dev_err(hdev, "request failed to create LE connection: err %d", err);
>>  
>> -	if (!conn)
>> +	/* Check if connection is still pending */
>> +	if (conn != hci_lookup_le_connect(hdev))
>>  		goto done;
>>  
>> -	hci_le_conn_failed(conn, err);
>> +	hci_conn_failed(conn, err);
>>  
>>  done:
>>  	hci_dev_unlock(hdev);
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 0270e597c285..af17dfb20e01 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -5632,10 +5632,12 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status,
>>  		status = HCI_ERROR_INVALID_PARAMETERS;
>>  	}
>>  
>> -	if (status) {
>> -		hci_conn_failed(conn, status);
>> +	/* All connection failure handling is taken care of by the
>> +	 * hci_conn_failed function which is triggered by the HCI
>> +	 * request completion callbacks used for connecting.
>> +	 */
>> +	if (status)
>>  		goto unlock;
>> -	}
>>  
>>  	if (conn->dst_type == ADDR_LE_DEV_PUBLIC)
>>  		addr_type = BDADDR_LE_PUBLIC;
> 
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH-stable] Bluetooth: hci_conn: Fix hci_connect_le_sync

[Luiz Augusto von Dentz]

Hi Ahmad,

On Tue, May 24, 2022 at 8:55 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Hello Luiz,
>
> On 24.05.22 16:48, Ahmad Fatoum wrote:
> > On 20.05.22 20:37, Luiz Augusto von Dentz wrote:
> >> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >>
> >> The handling of connection failures shall be handled by the request
> >> completion callback as already done by hci_cs_le_create_conn, also make
> >> sure to use hci_conn_failed instead of hci_le_conn_failed as the later
> >> don't actually call hci_conn_del to cleanup.
> >>
> >> Link: https://github.com/bluez/bluez/issues/340
> >> Fixes: 8e8b92ee60de5 ("Bluetooth: hci_sync: Add hci_le_create_conn_sync")
> >> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
> > A bit late, as I am not subscribed to linux-bluetooth and didn't notice this
> > patch, but FWIW: Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >
> >   Bluetooth: hci0: Opcode 0x200d failed: -110
> >   Bluetooth: hci0: request failed to create LE connection: err -110
> >
> > now, whereas before it crashed the kernel.
>
> I see now that this fix doesn't build for v5.17 because hci_conn_failed
> was only introduced in v5.18. Can the hci_conn.c hunk be safely dropped?

Are you talking about:

        if (status) {
-               hci_le_conn_failed(conn, status);
+               hci_conn_failed(conn, status);
                goto unlock;
        }

You just need to replace hci_conn_failed with hci_le_conn_failed or
well in the code above the end result is the same since it is not
supposed to cleanup in the event handler.

> Thanks,
> Ahmad
>
> >
> > Cheers,
> > Ahmad
> >
> >> ---
> >>  net/bluetooth/hci_conn.c  | 5 +++--
> >>  net/bluetooth/hci_event.c | 8 +++++---
> >>  2 files changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> >> index 882a7df13005..ac06c9724c7f 100644
> >> --- a/net/bluetooth/hci_conn.c
> >> +++ b/net/bluetooth/hci_conn.c
> >> @@ -943,10 +943,11 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
> >>
> >>      bt_dev_err(hdev, "request failed to create LE connection: err %d", err);
> >>
> >> -    if (!conn)
> >> +    /* Check if connection is still pending */
> >> +    if (conn != hci_lookup_le_connect(hdev))
> >>              goto done;
> >>
> >> -    hci_le_conn_failed(conn, err);
> >> +    hci_conn_failed(conn, err);
> >>
> >>  done:
> >>      hci_dev_unlock(hdev);
> >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >> index 0270e597c285..af17dfb20e01 100644
> >> --- a/net/bluetooth/hci_event.c
> >> +++ b/net/bluetooth/hci_event.c
> >> @@ -5632,10 +5632,12 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status,
> >>              status = HCI_ERROR_INVALID_PARAMETERS;
> >>      }
> >>
> >> -    if (status) {
> >> -            hci_conn_failed(conn, status);
> >> +    /* All connection failure handling is taken care of by the
> >> +     * hci_conn_failed function which is triggered by the HCI
> >> +     * request completion callbacks used for connecting.
> >> +     */
> >> +    if (status)
> >>              goto unlock;
> >> -    }
> >>
> >>      if (conn->dst_type == ADDR_LE_DEV_PUBLIC)
> >>              addr_type = BDADDR_LE_PUBLIC;
> >
> >
>
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



-- 
Luiz Augusto von Dentz

Re: [PATCH-stable] Bluetooth: hci_conn: Fix hci_connect_le_sync

[Ahmad Fatoum]

Hello Luiz,

On 24.05.22 20:08, Luiz Augusto von Dentz wrote:
> On Tue, May 24, 2022 at 8:55 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>> On 24.05.22 16:48, Ahmad Fatoum wrote:
>> I see now that this fix doesn't build for v5.17 because hci_conn_failed
>> was only introduced in v5.18. Can the hci_conn.c hunk be safely dropped?
> 
> Are you talking about:
> 
>         if (status) {
> -               hci_le_conn_failed(conn, status);
> +               hci_conn_failed(conn, status);
>                 goto unlock;
>         }
> 
> You just need to replace hci_conn_failed with hci_le_conn_failed or
> well in the code above the end result is the same since it is not
> supposed to cleanup in the event handler.

Yes, that cleanup in le_conn_complete_evt() needs to be removed.
I am talking about the other hunk in hci_conn.c:

  -    if (!conn)
  +    /* Check if connection is still pending */
  +    if (conn != hci_lookup_le_connect(hdev))
               goto done;
 
   -    hci_le_conn_failed(conn, err);
   +    hci_conn_failed(conn, err);
 
    done:
        hci_dev_unlock(hdev);


Can this be dropped for v5.17?

Cheers,
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH-stable] Bluetooth: hci_conn: Fix hci_connect_le_sync

[Luiz Augusto von Dentz]

Hi Ahmad,

On Tue, May 24, 2022 at 3:01 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Hello Luiz,
>
> On 24.05.22 20:08, Luiz Augusto von Dentz wrote:
> > On Tue, May 24, 2022 at 8:55 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> >> On 24.05.22 16:48, Ahmad Fatoum wrote:
> >> I see now that this fix doesn't build for v5.17 because hci_conn_failed
> >> was only introduced in v5.18. Can the hci_conn.c hunk be safely dropped?
> >
> > Are you talking about:
> >
> >         if (status) {
> > -               hci_le_conn_failed(conn, status);
> > +               hci_conn_failed(conn, status);
> >                 goto unlock;
> >         }
> >
> > You just need to replace hci_conn_failed with hci_le_conn_failed or
> > well in the code above the end result is the same since it is not
> > supposed to cleanup in the event handler.
>
> Yes, that cleanup in le_conn_complete_evt() needs to be removed.
> I am talking about the other hunk in hci_conn.c:
>
>   -    if (!conn)
>   +    /* Check if connection is still pending */
>   +    if (conn != hci_lookup_le_connect(hdev))
>                goto done;
>
>    -    hci_le_conn_failed(conn, err);
>    +    hci_conn_failed(conn, err);
>
>     done:
>         hci_dev_unlock(hdev);
>
>
> Can this be dropped for v5.17?

I guess it should be alright but perhaps keep if (conn !=
hci_lookup_le_connect(hdev)) just in case.

> Cheers,
> Ahmad
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



-- 
Luiz Augusto von Dentz