Hi Iulia,
On Wed, Dec 4, 2024 at 7:29 AM Iulia Tanasescu <iulia.tanasescu@xxxxxxx> wrote:
>
> This fixes circular locking dependency warnings, by ensuring
> the hci_dev_lock -> lock_sk order for all ISO functions.
>
> Below is an example of a warning generated because of locking
> dependencies:
>
> [ 75.307983] ======================================================
> [ 75.307984] WARNING: possible circular locking dependency detected
> [ 75.307985] 6.12.0-rc6+ #22 Not tainted
> [ 75.307987] ------------------------------------------------------
> [ 75.307987] kworker/u81:2/2623 is trying to acquire lock:
> [ 75.307988] ffff8fde1769da58 (sk_lock-AF_BLUETOOTH-BTPROTO_ISO)
> at: iso_connect_cfm+0x253/0x840 [bluetooth]
> [ 75.308021]
> but task is already holding lock:
> [ 75.308022] ffff8fdd61a10078 (&hdev->lock)
> at: hci_le_per_adv_report_evt+0x47/0x2f0 [bluetooth]
> [ 75.308053]
> which lock already depends on the new lock.
>
> [ 75.308054]
> the existing dependency chain (in reverse order) is:
> [ 75.308055]
> -> #1 (&hdev->lock){+.+.}-{3:3}:
> [ 75.308057] __mutex_lock+0xad/0xc50
> [ 75.308061] mutex_lock_nested+0x1b/0x30
> [ 75.308063] iso_sock_listen+0x143/0x5c0 [bluetooth]
> [ 75.308085] __sys_listen_socket+0x49/0x60
> [ 75.308088] __x64_sys_listen+0x4c/0x90
> [ 75.308090] x64_sys_call+0x2517/0x25f0
> [ 75.308092] do_syscall_64+0x87/0x150
> [ 75.308095] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [ 75.308098]
> -> #0 (sk_lock-AF_BLUETOOTH-BTPROTO_ISO){+.+.}-{0:0}:
> [ 75.308100] __lock_acquire+0x155e/0x25f0
> [ 75.308103] lock_acquire+0xc9/0x300
> [ 75.308105] lock_sock_nested+0x32/0x90
> [ 75.308107] iso_connect_cfm+0x253/0x840 [bluetooth]
> [ 75.308128] hci_connect_cfm+0x6c/0x190 [bluetooth]
> [ 75.308155] hci_le_per_adv_report_evt+0x27b/0x2f0 [bluetooth]
> [ 75.308180] hci_le_meta_evt+0xe7/0x200 [bluetooth]
> [ 75.308206] hci_event_packet+0x21f/0x5c0 [bluetooth]
> [ 75.308230] hci_rx_work+0x3ae/0xb10 [bluetooth]
> [ 75.308254] process_one_work+0x212/0x740
> [ 75.308256] worker_thread+0x1bd/0x3a0
> [ 75.308258] kthread+0xe4/0x120
> [ 75.308259] ret_from_fork+0x44/0x70
> [ 75.308261] ret_from_fork_asm+0x1a/0x30
> [ 75.308263]
> other info that might help us debug this:
>
> [ 75.308264] Possible unsafe locking scenario:
>
> [ 75.308264] CPU0 CPU1
> [ 75.308265] ---- ----
> [ 75.308265] lock(&hdev->lock);
> [ 75.308267] lock(sk_lock-
> AF_BLUETOOTH-BTPROTO_ISO);
> [ 75.308268] lock(&hdev->lock);
> [ 75.308269] lock(sk_lock-AF_BLUETOOTH-BTPROTO_ISO);
> [ 75.308270]
> *** DEADLOCK ***
>
> [ 75.308271] 4 locks held by kworker/u81:2/2623:
> [ 75.308272] #0: ffff8fdd66e52148 ((wq_completion)hci0#2){+.+.}-{0:0},
> at: process_one_work+0x443/0x740
> [ 75.308276] #1: ffffafb488b7fe48 ((work_completion)(&hdev->rx_work)),
> at: process_one_work+0x1ce/0x740
> [ 75.308280] #2: ffff8fdd61a10078 (&hdev->lock){+.+.}-{3:3}
> at: hci_le_per_adv_report_evt+0x47/0x2f0 [bluetooth]
> [ 75.308304] #3: ffffffffb6ba4900 (rcu_read_lock){....}-{1:2},
> at: hci_connect_cfm+0x29/0x190 [bluetooth]
>
> Signed-off-by: Iulia Tanasescu <iulia.tanasescu@xxxxxxx>
> ---
> net/bluetooth/iso.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index 8ed818254dc8..cb004b678d65 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -1102,6 +1102,7 @@ static int iso_sock_connect(struct socket *sock, struct sockaddr *addr,
> return err;
> }
>
> +/* This function requires the caller to hold sk lock */
> static int iso_listen_bis(struct sock *sk)
> {
> struct hci_dev *hdev;
> @@ -1128,7 +1129,15 @@ static int iso_listen_bis(struct sock *sk)
> if (!hdev)
> return -EHOSTUNREACH;
>
> + /* Prevent sk from being freed whilst unlocked */
> + sock_hold(sk);
> +
> + /* To avoid circular locking dependencies,
> + * hdev should be locked first before sk.
> + */
> + release_sock(sk);
> hci_dev_lock(hdev);
> + lock_sock(sk);
>
> /* Fail if user set invalid QoS */
> if (iso_pi(sk)->qos_user_set && !check_bcast_qos(&iso_pi(sk)->qos)) {
> @@ -1161,7 +1170,13 @@ static int iso_listen_bis(struct sock *sk)
> hci_dev_put(hdev);
>
> unlock:
> + /* Unlock order should be in reverse from lock order. */
> + release_sock(sk);
> hci_dev_unlock(hdev);
> + lock_sock(sk);
> +
> + sock_put(sk);
> +
> return err;
> }
>
> @@ -1417,6 +1432,7 @@ static void iso_conn_defer_accept(struct hci_conn *conn)
> hci_send_cmd(hdev, HCI_OP_LE_ACCEPT_CIS, sizeof(cp), &cp);
> }
>
> +/* This function requires the caller to hold sk lock */
> static void iso_conn_big_sync(struct sock *sk)
> {
> int err;
> @@ -1428,6 +1444,14 @@ static void iso_conn_big_sync(struct sock *sk)
> if (!hdev)
> return;
>
> + /* Prevent sk from being freed whilst unlocked */
> + sock_hold(sk);
> +
> + /* To avoid circular locking dependencies, hdev should be
> + * locked first before sk.
> + */
> + release_sock(sk);
> +
> /* hci_le_big_create_sync requires hdev lock to be held, since
> * it enqueues the HCI LE BIG Create Sync command via
> * hci_cmd_sync_queue_once, which checks hdev flags that might
> @@ -1435,6 +1459,8 @@ static void iso_conn_big_sync(struct sock *sk)
> */
> hci_dev_lock(hdev);
>
> + lock_sock(sk);
> +
> if (!test_and_set_bit(BT_SK_BIG_SYNC, &iso_pi(sk)->flags)) {
> err = hci_le_big_create_sync(hdev, iso_pi(sk)->conn->hcon,
> &iso_pi(sk)->qos,
> @@ -1446,7 +1472,12 @@ static void iso_conn_big_sync(struct sock *sk)
> err);
> }
>
> + /* Unlock order should be in reverse from lock order. */
> + release_sock(sk);
> hci_dev_unlock(hdev);
> + lock_sock(sk);
> +
> + sock_put(sk);
> }
>
> static int iso_sock_recvmsg(struct socket *sock, struct msghdr *msg,
> --
> 2.40.1
We should probably avoid having to do hci_dev_lock while holding
lock_sock to begin with, like we are doing in iso_sock_connect which
calls iso_connect_bis without holding any locks so we don't have
multiple unlock/lock sequences.
--
Luiz Augusto von Dentz
