Hi Luiz,
Wouldn't it be better to add a 'yet-another' flag as an addition to
HCI_CONN_SSP_ENABLED, and clear it unconditionally at the top of
'hci_remote_ext_features_evt', before
" conn = hci_conn_hash_lookup_handle "
check?
In this case broken ext_features response (with ev->status != 0 or conn
== NULL) will not indirectly enable the SSP feature.
Regards
Andrei VOLKOV
Le 22/01/2024 à 15:02, Luiz Augusto von Dentz a écrit :
Hi Andrei,
On Mon, Jan 22, 2024 at 7:18 AM Andrei Volkov
<andrey.volkov@xxxxxxxxxxxxxxxxx> wrote:
Hello,
Lately we've bumped with regression introduced by commit:
c7f59461f5a78 ("Bluetooth: Fix a refcnt underflow problem for
hci_conn", 2023-10-04)
The regression related with adding "hci_conn_ssp_enabled()" check in
"hci_io_capa_request_evt()" handler, and broke pairing process initiated
by the external device.
Precisely, some ext. devices, like any phone equipped with Android ver <
14 (we have not latest one, so we didn't check), always send "IO
Capability Request" before "Read Remote Extended Features" command, as
consequence the flag "HCI_CONN_SSP_ENABLED" not yet activated at the
time of "hci_io_capa_request_evt()" execution and
"hci_conn_ssp_enabled()" always returns false in time of the pairing.
As a result, pairing always fails. The quick and dirty fix is revert the
ssp check introduced in the subj. commit, like below:
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -5329,7 +5329,7 @@ static void hci_io_capa_request_evt(struct hci_dev
*hdev, void *data,
hci_dev_lock(hdev);
conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr);
- if (!conn || !hci_conn_ssp_enabled(conn))
+ if (!conn)
goto unlock;
hci_conn_hold(conn);
However, a more thorough and correct fix requires discussion and
testing. Therefore, I would like to get any comments/suggestion from the
community before doing this.
I think we need to do something like this, so we consider only the
local SSP flag when evaluating if we should proceed to respond or just
drop:
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 6130c969f361..a15924db83d9 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -5327,9 +5327,12 @@ static void hci_io_capa_request_evt(struct
hci_dev *hdev, void *data,
hci_dev_lock(hdev);
conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr);
- if (!conn || !hci_conn_ssp_enabled(conn))
+ if (!conn || !hci_dev_test_flag(hdev, HCI_SSP_ENABLED))
goto unlock;
+ /* Assume remote supports SSP since it has triggered this event */
+ set_bit(HCI_CONN_SSP_ENABLED, &conn->flags);
+
hci_conn_hold(conn);
if (!hci_dev_test_flag(hdev, HCI_MGMT))
Regards
Andrey VOLKOV
