Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1222-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
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>
In-Reply-To: <747030d8-a06f-4d7d-bf93-0e95f54b9bbf@munic.io>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Mon, 22 Jan 2024 10:10:56 -0500
Message-ID: <CABBYNZ+4fb3FbEPe41gQuRBNG3jNARakfSqvqB2j8wKZ3J92fQ@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 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 conn
> =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?

> 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 initiat=
ed
> >> 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 t=
he
> >> 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_d=
ev
> >> *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 t=
he
> >> 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