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;
Message-ID: <15a7a714e1f16a2b215accf451927378417c0929.camel@iki.fi>
Subject: Re: [PATCH RFC 6/6] Bluetooth: ISO: handle bound CIS cleanup via
 hci_conn
From:   Pauli Virtanen <pav@iki.fi>
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     linux-bluetooth@vger.kernel.org
Date:   Thu, 27 Jul 2023 21:35:27 +0000
In-Reply-To: <CABBYNZK-im2S7KnbBuP7Bq4V8yvJa-KZ5fZkW7zySepDY9DNTA@mail.gmail.com>
References: <cover.1690399379.git.pav@iki.fi>
         <f02203d5565bbe78c2406ca45a5a72336a1315ea.1690399379.git.pav@iki.fi>
         <CABBYNZK-im2S7KnbBuP7Bq4V8yvJa-KZ5fZkW7zySepDY9DNTA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Precedence: bulk

Hi Luiz,

to, 2023-07-27 kello 14:14 -0700, Luiz Augusto von Dentz kirjoitti:
> On Wed, Jul 26, 2023 at 2:37 PM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > 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 *hdev, bdaddr_t *dst,
> >                 return ERR_PTR(-EINVAL);
> >         }
> > 
> > +       hci_conn_hold(cis);
> > +
> >         cis->iso_qos = *qos;
> >         cis->state = 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 = 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 the 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 = 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.

-- 
Pauli Virtanen