Re: [PATCH] Bluetooth: SCO: fix ABBA deadlock in sco_connect_cfm

[Pauli Virtanen]

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@xxxxxxxxx>
> ---
>  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);
> --

-- 
Pauli Virtanen