Hi Pauli,
On Wed, Jul 26, 2023 at 2:37 PM Pauli Virtanen <pav@xxxxxx> 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@xxxxxx>
> ---
> 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.
--
Luiz Augusto von Dentz
