Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1225-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
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>
 <810c8fcc-95a5-4117-afbc-91d201398004@munic.io>
In-Reply-To: <810c8fcc-95a5-4117-afbc-91d201398004@munic.io>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Mon, 22 Jan 2024 11:12:25 -0500
Message-ID: <CABBYNZ+TujgGCTt93gg-tJryCELuzwyz4vwoxM3ibHZvNvQwvA@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 10:50=E2=80=AFAM Andrei Volkov
<andrey.volkov@mobile-devices.fr> wrote:
>
> Hi Luiz,
>
> Le 22/01/2024 =C3=A0 16:10, Luiz Augusto von Dentz a =C3=A9crit :
>
> > Hi Andrei,
> >
> > On Mon, Jan 22, 2024 at 9:45=E2=80=AFAM 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 =3D hci_conn_hash_lookup_handle"
> >>
> >> check?
> >>
> >> In this case broken ext_features response (with ev->status !=3D 0 or c=
onn
> >> =3D=3D 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.

That is a totally different problem, most likely related to
conn->disc_work not being cancelled properly or something, so this
probably was only masking it for some reason but it is not a proper
fix, anyway it is still a good idea to check if hdev has SSP enabled
in any case, since it can be enabled/disabled at runtime thus why I'm
not completely reverting it.

If the conn_timeout problem comes back then we need to investigate
what is really causing that, but there is a high chance this has been
fixed since we have been reworking some of the code paths related to
hci_conn_del, etc.

> Regards
> Andrei VOLKOV
>
> >> Regards
> >> Andrei VOLKOV
> >>
> >> Le 22/01/2024 =C3=A0 15:02, Luiz Augusto von Dentz a =C3=A9crit :
> >>
> >>> 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 initi=
ated
> >>>> by the external device.
> >>>>
> >>>> Precisely, some ext. devices, like any phone equipped with Android v=
er <
> >>>> 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->bda=
ddr);
> >>>> -       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 jus=
t
> >>> 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->bdadd=
r);
> >>> -       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 eve=
nt */
> >>> +       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