Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1220-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <a3a31b2d-6039-4b50-af81-47f7ea1a0461@munic.io>
In-Reply-To: <a3a31b2d-6039-4b50-af81-47f7ea1a0461@munic.io>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Mon, 22 Jan 2024 09:02:11 -0500
Message-ID: <CABBYNZ+9UdG1cMZVmdtN3U2aS16AKMCyTARZZyFX7xTEDWcMOw@mail.gmail.com>
Subject: Re: regression in c7f59461f5a78: Bluetooth: Fix a refcnt underflow
 problem for hci_conn
To: Andrei Volkov <andrey.volkov@mobile-devices.fr>
Cc: linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Andrei,

On Mon, Jan 22, 2024 at 7:18=E2=80=AFAM Andrei Volkov
<andrey.volkov@mobile-devices.fr> 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 =3D 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 =3D 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
>
>


--=20
Luiz Augusto von Dentz