Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <cover.1690399379.git.pav@iki.fi> <f02203d5565bbe78c2406ca45a5a72336a1315ea.1690399379.git.pav@iki.fi>
 <CABBYNZK-im2S7KnbBuP7Bq4V8yvJa-KZ5fZkW7zySepDY9DNTA@mail.gmail.com> <15a7a714e1f16a2b215accf451927378417c0929.camel@iki.fi>
In-Reply-To: <15a7a714e1f16a2b215accf451927378417c0929.camel@iki.fi>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 27 Jul 2023 17:27:20 -0700
Message-ID: <CABBYNZ+9-dxfumDqnQyx7Rd3dUsNiKk-jVsxAkgdgN=xjNF4Rw@mail.gmail.com>
Subject: Re: [PATCH RFC 6/6] Bluetooth: ISO: handle bound CIS cleanup via hci_conn
To:     Pauli Virtanen <pav@iki.fi>
Cc:     linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Pauli,

On Thu, Jul 27, 2023 at 2:35=E2=80=AFPM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> to, 2023-07-27 kello 14:14 -0700, Luiz Augusto von Dentz kirjoitti:
> > On Wed, Jul 26, 2023 at 2:37=E2=80=AFPM Pauli Virtanen <pav@iki.fi> wro=
te:
> > >
> > > Calling hci_conn_del in __iso_sock_close is invalid. It needs
> > > hdev->lock, but it cannot be acquired there due to lock ordering.
> > >
> > > Fix this by doing cleanup via hci_conn_drop.
> > >
> > > Return hci_conn with refcount 1 from hci_bind_cis and hci_connect_cis=
,
> > > so that the iso_conn always holds one reference.  This also fixes
> > > refcounting when error handling.
> > >
> > > Since hci_conn_abort shall handle termination of connections in any
> > > state properly, we can handle BT_CONNECT socket state in the same way=
 as
> > > BT_CONNECTED.
> > >
> > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > ---
> > >  net/bluetooth/hci_conn.c |  5 +++++
> > >  net/bluetooth/iso.c      | 14 +-------------
> > >  2 files changed, 6 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > > index ba339a0eb851..33fbdc8e0748 100644
> > > --- a/net/bluetooth/hci_conn.c
> > > +++ b/net/bluetooth/hci_conn.c
> > > @@ -1901,6 +1901,8 @@ struct hci_conn *hci_bind_cis(struct hci_dev *h=
dev, bdaddr_t *dst,
> > >                 return ERR_PTR(-EINVAL);
> > >         }
> > >
> > > +       hci_conn_hold(cis);
> > > +
> > >         cis->iso_qos =3D *qos;
> > >         cis->state =3D BT_BOUND;
> > >
> > > @@ -2254,6 +2256,9 @@ struct hci_conn *hci_connect_cis(struct hci_dev=
 *hdev, bdaddr_t *dst,
> > >                 return ERR_PTR(-ENOLINK);
> > >         }
> > >
> > > +       /* Link takes the refcount */
> > > +       hci_conn_drop(cis);
> > > +
> > >         cis->state =3D BT_CONNECT;
> > >
> > >         hci_le_create_cis_pending(hdev);
> > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > > index cbe3299b4a41..358954bfbb32 100644
> > > --- a/net/bluetooth/iso.c
> > > +++ b/net/bluetooth/iso.c
> > > @@ -628,6 +628,7 @@ static void __iso_sock_close(struct sock *sk)
> > >                 iso_sock_cleanup_listen(sk);
> > >                 break;
> > >
> > > +       case BT_CONNECT:
> > >         case BT_CONNECTED:
> > >         case BT_CONFIG:
> > >                 if (iso_pi(sk)->conn->hcon) {
> > > @@ -643,19 +644,6 @@ static void __iso_sock_close(struct sock *sk)
> > >                 break;
> > >
> > >         case BT_CONNECT2:
> > > -               iso_chan_del(sk, ECONNRESET);
> > > -               break;
> > > -       case BT_CONNECT:
> > > -               /* In case of DEFER_SETUP the hcon would be bound to =
CIG which
> > > -                * needs to be removed so just call hci_conn_del so t=
he cleanup
> > > -                * callback do what is needed.
> > > -                */
> > > -               if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags) &&
> > > -                   iso_pi(sk)->conn->hcon) {
> > > -                       hci_conn_del(iso_pi(sk)->conn->hcon);
> > > -                       iso_pi(sk)->conn->hcon =3D NULL;
> > > -               }
> > > -
> > >                 iso_chan_del(sk, ECONNRESET);
> > >                 break;
> > >         case BT_DISCONN:
> > > --
> > > 2.41.0
> >
> > I guess this sort of fix can be sent separately which I guess helps
> > here since we can prioritize the ones that don't have side effects.
>
> Right, I can send these separately in the actual patch series.
>
> This one requires hci_conn_abort deletes conns with no handle yet
> though, otherwise it introduces failure to cleanup in a race condition.

I thought we need to lookup by handle to avoid races as well, or are
you doing that because the handle could be updated in the meantime?
Perhaps we could store the temporary handles in case the connection
handles get updated then it can still be looked up by its temporary
handle, either that or we disregard updates to handle when they're in
the process of being aborted.


--=20
Luiz Augusto von Dentz