diff mbox series

[RFC,6/6] Bluetooth: ISO: handle bound CIS cleanup via hci_conn

Message ID f02203d5565bbe78c2406ca45a5a72336a1315ea.1690399379.git.pav@iki.fi
State Accepted
Commit 2dfe76d58d3ac6d7e89195826787acafe2870518
Headers show
Series Locking in hci_sync | expand

Commit Message

Pauli Virtanen July 26, 2023, 9:25 p.m. UTC 
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(-)

Comments

Luiz Augusto von Dentz July 27, 2023, 9:14 p.m. UTC | #1 
Hi Pauli,

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.
Pauli Virtanen July 27, 2023, 9:35 p.m. UTC | #2 
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.
Luiz Augusto von Dentz July 28, 2023, 12:27 a.m. UTC | #3 
Hi Pauli,

On Thu, Jul 27, 2023 at 2:35 PM 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 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.

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.
Pauli Virtanen July 28, 2023, 10:07 a.m. UTC | #4 
Hi Luiz,

to, 2023-07-27 kello 17:27 -0700, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Thu, Jul 27, 2023 at 2:35 PM 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 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.
> 
> 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.

Would it be simpler to hold hci_conn_get, and then in the hci_sync
callback check if the conn is still in conn_hash? The purpose of the
HCI_CONN_DELETED flag in the series is to make this check easy.

Also, I don't think just the handle thing protects you from races, as
noted in the cover letter. AFAIK, hci_sync.c callbacks and hci_event.c
processing can run at the same time on different CPU (since they run
from different workqueues), so you have to lock.

Moreover, handle lookup might pick up different connection than
intended if a handle was reused. hci_abort_conn_sync might run quite a
bit after hci_abort_conn queues it and there might be stuff like
another abort and Set CIG Parametrers in the queue...
diff mbox series

Patch

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: