diff mbox series

Bluetooth: SCO: fix ABBA deadlock in sco_connect_cfm

Message ID 20250222114809.11634-1-aha310510@gmail.com
State New
Headers show
Series Bluetooth: SCO: fix ABBA deadlock in sco_connect_cfm | expand

Commit Message

Jeongjun Park Feb. 22, 2025, 11:48 a.m. UTC 
Caused by previous commit 405280887f8f causes ABBA deadlock. So we need to 
change the lock order to prevent deadlock.

Fixes: 405280887f8f ("Bluetooth: Reduce critical section in sco_conn_ready")
Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
 net/bluetooth/sco.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--

Comments

Pauli Virtanen Feb. 22, 2025, 12:21 p.m. UTC | #1 
Hi,

la, 2025-02-22 kello 20:48 +0900, Jeongjun Park kirjoitti:
> Caused by previous commit 405280887f8f causes ABBA deadlock. So we need to 
> change the lock order to prevent deadlock.

The lock ordering eg. in sco_conn_del() or sco_sock_close() is

	hdev_lock > lock_sock > sco_conn_lock

IIRC this is the lock ordering assumed elsewhere in this file, except
the part touched by 405280887f8f (however, it's locking the parent
socket there so not clear if this causes deadlock, can you clarify if
you have observation/proof of deadlock).

In this patch you have

	sco_conn_lock > lock_sock

Does that also cause a deadlock?

> 
> Fixes: 405280887f8f ("Bluetooth: Reduce critical section in sco_conn_ready")
> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> ---
>  net/bluetooth/sco.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index aa7bfe26cb40..8f1377f4a27c 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -289,13 +289,11 @@ static int sco_chan_add(struct sco_conn *conn, struct sock *sk,
>  {
>  	int err = 0;
>  
> -	sco_conn_lock(conn);
>  	if (conn->sk)
>  		err = -EBUSY;
>  	else
>  		__sco_chan_add(conn, sk, parent);
>  
> -	sco_conn_unlock(conn);
>  	return err;
>  }
>  
> @@ -343,11 +341,13 @@ static int sco_connect(struct sock *sk)
>  		goto unlock;
>  	}
>  
> +	sco_conn_lock(conn);
>  	lock_sock(sk);
>  
>  	err = sco_chan_add(conn, sk, NULL);
>  	if (err) {
>  		release_sock(sk);
> +		sco_conn_unlock(conn);
>  		goto unlock;
>  	}
>  
> @@ -363,6 +363,7 @@ static int sco_connect(struct sock *sk)
>  	}
>  
>  	release_sock(sk);
> +	sco_conn_unlock(conn);
>  
>  unlock:
>  	hci_dev_unlock(hdev);
> --
diff mbox series

Patch

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index aa7bfe26cb40..8f1377f4a27c 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -289,13 +289,11 @@  static int sco_chan_add(struct sco_conn *conn, struct sock *sk,
 {
 	int err = 0;
 
-	sco_conn_lock(conn);
 	if (conn->sk)
 		err = -EBUSY;
 	else
 		__sco_chan_add(conn, sk, parent);
 
-	sco_conn_unlock(conn);
 	return err;
 }
 
@@ -343,11 +341,13 @@  static int sco_connect(struct sock *sk)
 		goto unlock;
 	}
 
+	sco_conn_lock(conn);
 	lock_sock(sk);
 
 	err = sco_chan_add(conn, sk, NULL);
 	if (err) {
 		release_sock(sk);
+		sco_conn_unlock(conn);
 		goto unlock;
 	}
 
@@ -363,6 +363,7 @@  static int sco_connect(struct sock *sk)
 	}
 
 	release_sock(sk);
+	sco_conn_unlock(conn);
 
 unlock:
 	hci_dev_unlock(hdev);