Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1223-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Andrei Volkov <andrey.volkov@mobile-devices.fr>
Message-ID: <810c8fcc-95a5-4117-afbc-91d201398004@munic.io>
Date: Mon, 22 Jan 2024 16:50:33 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: regression in c7f59461f5a78: Bluetooth: Fix a refcnt underflow
 problem for hci_conn
Content-Language: en-US, fr
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: linux-bluetooth@vger.kernel.org
References: <a3a31b2d-6039-4b50-af81-47f7ea1a0461@munic.io>
 <CABBYNZ+9UdG1cMZVmdtN3U2aS16AKMCyTARZZyFX7xTEDWcMOw@mail.gmail.com>
 <747030d8-a06f-4d7d-bf93-0e95f54b9bbf@munic.io>
 <CABBYNZ+4fb3FbEPe41gQuRBNG3jNARakfSqvqB2j8wKZ3J92fQ@mail.gmail.com>
Organization: MUNIC
In-Reply-To: <CABBYNZ+4fb3FbEPe41gQuRBNG3jNARakfSqvqB2j8wKZ3J92fQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Hi Luiz,

Le 22/01/2024 à 16:10, Luiz Augusto von Dentz a écrit :

> Hi Andrei,
>
> On Mon, Jan 22, 2024 at 9:45 AM Andrei Volkov
> <andrey.volkov@mobile-devices.fr> wrote:
>> 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.
> HCI_CONN_SSP_ENABLED is already at conn level, besides that it is too
> late to clear it if the SSP procedure has already taken place and on
> top of it I don't want to change the code too much and risk having
> more regressions like this.
>
> Btw, something tells me that Android is not actually doing the
> HCI_OP_READ_REMOTE_EXT_FEATURES since we do have CI testing SSP and
> this change hasn't cause us any problems, do you know what command it
> uses? Perhaps it tries SSP right-away or does it cache the previous
> response?
>
The real problem is that since the ext. device does not receive a 
response to the IO request (cmd 0x31) from BlueZ, it refuses to continue 
the pairing. But with proposed reverting the ssp check, we are probably 
coming back to the initial href problem.

Btw, for me it's unclear how the additional check in 
"hci_io_capa_request_evt" helps with href undderrun. It looks like the 
original FSM implementation is missing something, and the fix actually 
is masking the problem.

Regards
Andrei VOLKOV

>> 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@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 = 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
>>>>
>>>>
>
>