Hi Luiz,
> Commit d5ebaa7c5f6f6 introduces checks for handle range
> (e.g HCI_CONN_HANDLE_MAX) but controllers don't seem to respect the
> valid range int case of error status:
>
>> HCI Event: Connect Complete (0x03) plen 11
> Status: Page Timeout (0x04)
> Handle: 65535
> Address: 94:DB:56:XX:XX:XX (Sony Home Entertainment&
> Sound Products Inc)
> Link type: ACL (0x01)
> Encryption: Disabled (0x00)
> [1644965.827560] Bluetooth: hci0: Ignoring HCI_Connection_Complete for
> invalid handle
so the problem is that with BR/EDR the lookup is by BD_ADDR. I think the check for valid handle is wrong at the beginning of connect complete handler.
The problem is really the fact the we trying a big hammer at the beginning. The hci_conn_add in case of auto-connect should validate status and handle. We are not even validating the status right now assuming we always get a status == 0 on auto-connect.
The second handle validation should only occur if we have !status in the bottom half of that function.
> Because of it is impossible to cleanup the connections properly since
> the stack would attempt to cancel the connection which is no longer in
> progress causing the following trace:
>
> < HCI Command: Create Connection Cancel (0x01|0x0008) plen 6
> Address: 94:DB:56:XX:XX:XX (Sony Home Entertainment&
> Sound Products Inc)
> = bluetoothd: src/profile.c:record_cb() Unable to get Hands-Free Voice
> gateway SDP record: Connection timed out
>> HCI Event: Command Complete (0x0e) plen 10
> Create Connection Cancel (0x01|0x0008) ncmd 1
> Status: Unknown Connection Identifier (0x02)
> Address: 94:DB:56:XX:XX:XX (Sony Home Entertainment&
> Sound Products Inc)
> < HCI Command: Create Connection Cancel (0x01|0x0008) plen 6
> Address: 94:DB:56:XX:XX:XX (Sony Home Entertainment&
> Sound Products Inc)
Can we get details about which controller uses 0xffff instead of 0 for the handle?
>
> Fixes: d5ebaa7c5f6f6 ("Bluetooth: hci_event: Ignore multiple conn complete events")
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> ---
> 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 abaabfae19cc..1cc5a712459e 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3068,7 +3068,7 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
> struct hci_ev_conn_complete *ev = data;
> struct hci_conn *conn;
>
> - if (__le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
> + if (!status && __le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
> bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for invalid handle");
> return;
> }
See comments above.
> @@ -4690,7 +4690,7 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev, void *data,
> return;
> }
>
> - if (__le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
> + if (!status && __le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
> bt_dev_err(hdev, "Ignoring HCI_Sync_Conn_Complete for invalid handle");
> return;
> }
This is also in the wrong position. Fundamentally we need to check the validity of the handle before we assign it and not just globally.
> @@ -5527,7 +5527,7 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status,
> struct smp_irk *irk;
> u8 addr_type;
>
> - if (handle > HCI_CONN_HANDLE_MAX) {
> + if (!status && handle > HCI_CONN_HANDLE_MAX) {
> bt_dev_err(hdev, "Ignoring HCI_LE_Connection_Complete for invalid handle");
> return;
> }
Same here. The global check is pointless. Check before using it.
Regards
Marcel
