| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 8 Jan 2024 14:11:14 -0500
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Jonas Dreßler <verdre@...d.nl>
Cc: Marcel Holtmann <marcel@...tmann.org>, Johan Hedberg <johan.hedberg@...il.com>,
linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org
Subject: Re: [PATCH v2 3/4] Bluetooth: hci_conn: Only do ACL connections sequentially
Hi Jonas,
On Mon, Jan 8, 2024 at 1:39 PM Jonas Dreßler <verdre@...d.nl> wrote:
>
> Pretty much all bluetooth chipsets only support paging a single device at
> a time, and if they don't reject a secondary "Create Connection" request
> while another is still ongoing, they'll most likely serialize those
> requests in the firware.
>
> With commit 4c67bc74f016 ("[Bluetooth] Support concurrent connect
> requests") we started adding some serialization of our own in case the
> adapter returns "Command Disallowed" HCI error.
>
> This commit was using the BT_CONNECT2 state for the serialization, this
> state is also used for a few more things (most notably to indicate we're
> waiting for an inquiry to cancel) and therefore a bit unreliable. Also
> not all BT firwares would respond with "Command Disallowed" on too many
> connection requests, some will also respond with "Hardware Failure"
> (BCM4378), and others will error out later and send a "Connect Complete"
> event with error "Rejected Limited Resources" (Marvell 88W8897).
>
> We can clean things up a bit and also make the serialization more reliable
> by using our hci_sync machinery to always do "Create Connection" requests
> in a sequential manner.
>
> This is very similar to what we're already doing for establishing LE
> connections, and it works well there.
> ---
> include/net/bluetooth/hci.h | 1 +
> net/bluetooth/hci_conn.c | 37 ++++++++++++++++++++++++++-----------
> 2 files changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index fef723afd..f2bbc0a14 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -427,6 +427,7 @@ enum {
> #define HCI_ACL_TX_TIMEOUT msecs_to_jiffies(45000) /* 45 seconds */
> #define HCI_AUTO_OFF_TIMEOUT msecs_to_jiffies(2000) /* 2 seconds */
> #define HCI_POWER_OFF_TIMEOUT msecs_to_jiffies(5000) /* 5 seconds */
> +#define HCI_ACL_CONN_TIMEOUT msecs_to_jiffies(20000) /* 20 seconds */
> #define HCI_LE_CONN_TIMEOUT msecs_to_jiffies(20000) /* 20 seconds */
> #define HCI_LE_AUTOCONN_TIMEOUT msecs_to_jiffies(4000) /* 4 seconds */
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 76222565e..541d55301 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -229,11 +229,12 @@ static void hci_connect_le_scan_remove(struct hci_conn *conn)
> schedule_work(&conn->le_scan_cleanup);
> }
>
> -static void hci_acl_create_connection(struct hci_conn *conn)
> +static int hci_acl_create_connection_sync(struct hci_dev *hdev, void *data)
Move the above function to hci_sync.c so it is simpler to reuse it in
the future.
> {
> - struct hci_dev *hdev = conn->hdev;
> + struct hci_conn *conn = data;
> struct inquiry_entry *ie;
> struct hci_cp_create_conn cp;
> + int err;
>
> BT_DBG("hcon %p", conn);
>
> @@ -246,12 +247,10 @@ static void hci_acl_create_connection(struct hci_conn *conn)
> * request for discovery again when this flag becomes false.
> */
> if (test_bit(HCI_INQUIRY, &hdev->flags)) {
> - /* Put this connection to "pending" state so that it will be
> - * executed after the inquiry cancel command complete event.
> - */
> - conn->state = BT_CONNECT2;
> - hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
> - return;
> + err = __hci_cmd_sync_status(hdev, HCI_OP_INQUIRY_CANCEL, 0,
> + NULL, HCI_CMD_TIMEOUT);
> + if (err)
> + bt_dev_warn(hdev, "Failed to cancel inquiry %d", err);
> }
>
> conn->state = BT_CONNECT;
> @@ -284,7 +283,15 @@ static void hci_acl_create_connection(struct hci_conn *conn)
> else
> cp.role_switch = 0x00;
>
> - hci_send_cmd(hdev, HCI_OP_CREATE_CONN, sizeof(cp), &cp);
> + err = __hci_cmd_sync_status_sk(hdev, HCI_OP_CREATE_CONN,
> + sizeof(cp), &cp,
> + HCI_EV_CONN_COMPLETE,
> + HCI_ACL_CONN_TIMEOUT, NULL);
> +
> + if (err == -ETIMEDOUT)
> + hci_abort_conn(conn, HCI_ERROR_LOCAL_HOST_TERM);
> +
> + return err;
> }
>
> int hci_disconnect(struct hci_conn *conn, __u8 reason)
> @@ -1622,10 +1629,18 @@ struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
>
> acl->conn_reason = conn_reason;
> if (acl->state == BT_OPEN || acl->state == BT_CLOSED) {
> + int err;
> +
> acl->sec_level = BT_SECURITY_LOW;
> acl->pending_sec_level = sec_level;
> acl->auth_type = auth_type;
> - hci_acl_create_connection(acl);
> +
> + err = hci_cmd_sync_queue(hdev, hci_acl_create_connection_sync,
> + acl, NULL);
> + if (err) {
> + hci_conn_del(acl);
> + return ERR_PTR(err);
> + }
> }
>
> return acl;
> @@ -2530,7 +2545,7 @@ void hci_conn_check_pending(struct hci_dev *hdev)
>
> conn = hci_conn_hash_lookup_state(hdev, ACL_LINK, BT_CONNECT2);
> if (conn)
> - hci_acl_create_connection(conn);
> + hci_cmd_sync_queue(hdev, hci_acl_create_connection_sync, conn, NULL);
>
> hci_dev_unlock(hdev);
> }
> --
> 2.43.0
>
--
Luiz Augusto von Dentz
Powered by blists - more mailing lists