| 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
| ||
|
Message-ID: <75dea485-9bdf-355f-0aff-a26de0998b80@molgen.mpg.de>
Date: Mon, 24 Jul 2023 14:26:43 +0200
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Howard Chung <howardchung@...gle.com>
Cc: linux-bluetooth@...r.kernel.org, marcel@...tmann.org,
Archie Pusaka <apusaka@...gle.com>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Johan Hedberg <johan.hedberg@...il.com>,
Luiz Augusto von Dentz <luiz.dentz@...il.com>,
Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org
Subject: Re: [PATCH v1] Bluetooth: Add timeout in disconnect when power off
Dear Howard,
Thank you for your patch. Some minor nits.
Am 24.07.23 um 13:12 schrieb Howard Chung:
> For some controllers, it is known that when the HCI disconnect and HCI
> Reset are too close to each other, the LMP disconnect command might not
> been sent out yet and the command will be dropped by the controoler when
1. s/been/be/ or *have been*?
2. controller
> it is asked to reset itself. This could happen on powering off adapter.
>
> One possible issue is that if a connection exists, and then powering off
> and on adapter within a short time, then our host stack assumes the
I do not understand the part of the first comma.
> conntection was disconnected but this might not be true, so if we issue
connection
> a connection to the peer, it will fail with ACL Already Connected error.
>
> This CL makes the host stack to wait for |HCI_EV_DISCONN_COMPLETE| when
> powering off with a configurable timeout unless the timeout is set to 0.
>
> Reviewed-by: Archie Pusaka <apusaka@...gle.com>
> Signed-off-by: Howard Chung <howardchung@...gle.com>
> ---
> Hi upstream maintainers, this is tested with an AX211 device and Logi
> K580 keyboard by the following procedures:
> 1. pair the peer and stay connected.
> 2. power off and on immediately
> 3. observe that the btsnoop log is consistent with the configured
> timeout.
It’d be great to have this in the commit message.
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_core.c | 2 +-
> net/bluetooth/hci_sync.c | 38 +++++++++++++++++++++++---------
> net/bluetooth/mgmt_config.c | 6 +++++
> 4 files changed, 35 insertions(+), 12 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 8200a6689b39..ce44f9c60059 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -432,6 +432,7 @@ struct hci_dev {
> __u16 advmon_allowlist_duration;
> __u16 advmon_no_filter_duration;
> __u8 enable_advmon_interleave_scan;
> + __u16 discon_on_poweroff_timeout;
I’d append the unit to the variable name: `discon_on_poweroff_timeout_ms`.
>
> __u16 devid_source;
> __u16 devid_vendor;
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 0fefa6788911..769865494f45 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2479,7 +2479,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv)
> hdev->adv_instance_cnt = 0;
> hdev->cur_adv_instance = 0x00;
> hdev->adv_instance_timeout = 0;
> -
> + hdev->discon_on_poweroff_timeout = 0; /* Default to no timeout */
> hdev->advmon_allowlist_duration = 300;
> hdev->advmon_no_filter_duration = 500;
> hdev->enable_advmon_interleave_scan = 0x00; /* Default to disable */
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 3348a1b0e3f7..260e9f05359c 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -5250,6 +5250,8 @@ static int hci_disconnect_sync(struct hci_dev *hdev, struct hci_conn *conn,
> u8 reason)
> {
> struct hci_cp_disconnect cp;
> + unsigned long timeout;
Ditto.
> + int err;
>
> if (conn->type == AMP_LINK)
> return hci_disconnect_phy_link_sync(hdev, conn->handle, reason);
> @@ -5258,19 +5260,33 @@ static int hci_disconnect_sync(struct hci_dev *hdev, struct hci_conn *conn,
> cp.handle = cpu_to_le16(conn->handle);
> cp.reason = reason;
>
> - /* Wait for HCI_EV_DISCONN_COMPLETE, not HCI_EV_CMD_STATUS, when the
> - * reason is anything but HCI_ERROR_REMOTE_POWER_OFF. This reason is
> - * used when suspending or powering off, where we don't want to wait
> - * for the peer's response.
> + /* The HCI_ERROR_REMOTE_POWER_OFF is used when suspending or powering off,
> + * so we don't want to waste time waiting for the reply of the peer.
> + * However, if the configuration specified, we'll wait some time to give the
“if the configuration specified” sounds strange to me.
Kind regards,
Paul
> + * controller chance to actually send the disconnect command.
> */
> - if (reason != HCI_ERROR_REMOTE_POWER_OFF)
> - return __hci_cmd_sync_status_sk(hdev, HCI_OP_DISCONNECT,
> - sizeof(cp), &cp,
> - HCI_EV_DISCONN_COMPLETE,
> - HCI_CMD_TIMEOUT, NULL);
> + if (reason == HCI_ERROR_REMOTE_POWER_OFF && !hdev->discon_on_poweroff_timeout) {
> + return __hci_cmd_sync_status(hdev, HCI_OP_DISCONNECT,
> + sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> + }
>
> - return __hci_cmd_sync_status(hdev, HCI_OP_DISCONNECT, sizeof(cp), &cp,
> - HCI_CMD_TIMEOUT);
> + if (reason == HCI_ERROR_REMOTE_POWER_OFF)
> + timeout = msecs_to_jiffies(hdev->discon_on_poweroff_timeout);
> + else
> + timeout = HCI_CMD_TIMEOUT;
> +
> + err = __hci_cmd_sync_status_sk(hdev, HCI_OP_DISCONNECT,
> + sizeof(cp), &cp,
> + HCI_EV_DISCONN_COMPLETE,
> + timeout, NULL);
> +
> + /* Ignore the error in suspending or powering off case to avoid the procedure being
> + * aborted.
> + */
> + if (reason == HCI_ERROR_REMOTE_POWER_OFF)
> + return 0;
> +
> + return err;
> }
>
> static int hci_le_connect_cancel_sync(struct hci_dev *hdev,
> diff --git a/net/bluetooth/mgmt_config.c b/net/bluetooth/mgmt_config.c
> index 6ef701c27da4..f3194e3642d9 100644
> --- a/net/bluetooth/mgmt_config.c
> +++ b/net/bluetooth/mgmt_config.c
> @@ -78,6 +78,7 @@ int read_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
> HDEV_PARAM_U16(advmon_allowlist_duration);
> HDEV_PARAM_U16(advmon_no_filter_duration);
> HDEV_PARAM_U8(enable_advmon_interleave_scan);
> + HDEV_PARAM_U16(discon_on_poweroff_timeout);
> } __packed rp = {
> TLV_SET_U16(0x0000, def_page_scan_type),
> TLV_SET_U16(0x0001, def_page_scan_int),
> @@ -111,6 +112,7 @@ int read_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
> TLV_SET_U16(0x001d, advmon_allowlist_duration),
> TLV_SET_U16(0x001e, advmon_no_filter_duration),
> TLV_SET_U8(0x001f, enable_advmon_interleave_scan),
> + TLV_SET_U16(0x0020, discon_on_poweroff_timeout),
> };
>
> bt_dev_dbg(hdev, "sock %p", sk);
> @@ -186,6 +188,7 @@ int set_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
> case 0x001b:
> case 0x001d:
> case 0x001e:
> + case 0x0020:
> exp_type_len = sizeof(u16);
> break;
> case 0x001f:
> @@ -314,6 +317,9 @@ int set_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
> case 0x0001f:
> hdev->enable_advmon_interleave_scan = TLV_GET_U8(buffer);
> break;
> + case 0x00020:
> + hdev->discon_on_poweroff_timeout = TLV_GET_LE16(buffer);
> + break;
> default:
> bt_dev_warn(hdev, "unsupported parameter %u", type);
> break;
Powered by blists - more mailing lists