[PATCH 0/3] Bluetooth: iso: Fix warnings

[PATCH 0/3] Bluetooth: iso: Fix warnings

[Iulia Tanasescu]

This adds some fixes for locking related warnings in the ISO module.

Iulia Tanasescu (3):
  Bluetooth: iso: Fix circular locking dependency warnings
  Bluetooth: iso: Always release hdev at the end of iso_listen_bis
  Bluetooth: iso: Fix recursive locking warning

 net/bluetooth/iso.c | 43 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)


base-commit: 47ebf099106eb021cf7dd677720e6469a38785a5
-- 
2.40.1

[PATCH 1/3] Bluetooth: iso: Fix circular locking dependency warnings

[Iulia Tanasescu]

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@nxp.com>
---
 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

[PATCH 2/3] Bluetooth: iso: Always release hdev at the end of iso_listen_bis

[Iulia Tanasescu]

Since hci_get_route holds the device before returning, the hdev
should be released with hci_dev_put at the end of iso_listen_bis
even if the function returns with an error.

Signed-off-by: Iulia Tanasescu <iulia.tanasescu@nxp.com>
---
 net/bluetooth/iso.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index cb004b678d65..df523a09ca9a 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1167,14 +1167,14 @@ static int iso_listen_bis(struct sock *sk)
 		goto unlock;
 	}
 
-	hci_dev_put(hdev);
-
 unlock:
 	/* Unlock order should be in reverse from lock order. */
 	release_sock(sk);
 	hci_dev_unlock(hdev);
 	lock_sock(sk);
 
+	hci_dev_put(hdev);
+
 	sock_put(sk);
 
 	return err;
-- 
2.40.1

[PATCH 3/3] Bluetooth: iso: Fix recursive locking warning

[Iulia Tanasescu]

This updates iso_sock_accept to use nested locking for the parent
socket, to avoid lockdep warnings caused because the parent and
child sockets are locked by the same thread:

[   41.585683] ============================================
[   41.585688] WARNING: possible recursive locking detected
[   41.585694] 6.12.0-rc6+ #22 Not tainted
[   41.585701] --------------------------------------------
[   41.585705] iso-tester/3139 is trying to acquire lock:
[   41.585711] ffff988b29530a58 (sk_lock-AF_BLUETOOTH)
               at: bt_accept_dequeue+0xe3/0x280 [bluetooth]
[   41.585905]
               but task is already holding lock:
[   41.585909] ffff988b29533a58 (sk_lock-AF_BLUETOOTH)
               at: iso_sock_accept+0x61/0x2d0 [bluetooth]
[   41.586064]
               other info that might help us debug this:
[   41.586069]  Possible unsafe locking scenario:

[   41.586072]        CPU0
[   41.586076]        ----
[   41.586079]   lock(sk_lock-AF_BLUETOOTH);
[   41.586086]   lock(sk_lock-AF_BLUETOOTH);
[   41.586093]
                *** DEADLOCK ***

[   41.586097]  May be due to missing lock nesting notation

[   41.586101] 1 lock held by iso-tester/3139:
[   41.586107]  #0: ffff988b29533a58 (sk_lock-AF_BLUETOOTH)
                at: iso_sock_accept+0x61/0x2d0 [bluetooth]

Signed-off-by: Iulia Tanasescu <iulia.tanasescu@nxp.com>
---
 net/bluetooth/iso.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index df523a09ca9a..fb15f53e2ce5 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1241,7 +1241,11 @@ static int iso_sock_accept(struct socket *sock, struct socket *newsock,
 	long timeo;
 	int err = 0;
 
-	lock_sock(sk);
+	/* Use explicit nested locking to avoid lockdep warnings generated
+	 * because the parent socket and the child socket are locked on the
+	 * same thread.
+	 */
+	lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
 
 	timeo = sock_rcvtimeo(sk, arg->flags & O_NONBLOCK);
 
@@ -1272,7 +1276,7 @@ static int iso_sock_accept(struct socket *sock, struct socket *newsock,
 		release_sock(sk);
 
 		timeo = wait_woken(&wait, TASK_INTERRUPTIBLE, timeo);
-		lock_sock(sk);
+		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
 	}
 	remove_wait_queue(sk_sleep(sk), &wait);
 
-- 
2.40.1

RE: Bluetooth: iso: Fix warnings

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2294 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=914536

---Test result---

Test Summary:
CheckPatch                    PENDING   0.31 seconds
GitLint                       PENDING   0.22 seconds
SubjectPrefix                 PASS      0.30 seconds
BuildKernel                   PASS      25.32 seconds
CheckAllWarning               PASS      27.56 seconds
CheckSparse                   PASS      30.86 seconds
BuildKernel32                 PASS      24.37 seconds
TestRunnerSetup               PASS      439.51 seconds
TestRunner_l2cap-tester       PASS      20.67 seconds
TestRunner_iso-tester         FAIL      35.49 seconds
TestRunner_bnep-tester        PASS      6.83 seconds
TestRunner_mgmt-tester        FAIL      119.58 seconds
TestRunner_rfcomm-tester      PASS      7.58 seconds
TestRunner_sco-tester         PASS      9.45 seconds
TestRunner_ioctl-tester       PASS      8.13 seconds
TestRunner_mesh-tester        PASS      6.09 seconds
TestRunner_smp-tester         PASS      7.06 seconds
TestRunner_userchan-tester    PASS      5.03 seconds
IncrementalBuild              PENDING   0.47 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
Total: 125, Passed: 120 (96.0%), Failed: 1, Not Run: 4

Failed Test Cases
ISO Connect2 Suspend - Success                       Failed       4.242 seconds
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 492, Passed: 486 (98.8%), Failed: 2, Not Run: 4

Failed Test Cases
LL Privacy - Start Discovery 2 (Disable RL)          Failed       0.186 seconds
LL Privacy - Set Device Flag 1 (Device Privacy)      Failed       0.147 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH 1/3] Bluetooth: iso: Fix circular locking dependency warnings

[Luiz Augusto von Dentz]

Hi Iulia,

On Wed, Dec 4, 2024 at 7:29 AM Iulia Tanasescu <iulia.tanasescu@nxp.com> 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@nxp.com>
> ---
>  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

Re: [PATCH 0/3] Bluetooth: iso: Fix warnings

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Wed,  4 Dec 2024 14:28:46 +0200 you wrote:
> This adds some fixes for locking related warnings in the ISO module.
> 
> Iulia Tanasescu (3):
>   Bluetooth: iso: Fix circular locking dependency warnings
>   Bluetooth: iso: Always release hdev at the end of iso_listen_bis
>   Bluetooth: iso: Fix recursive locking warning
> 
> [...]

Here is the summary with links:
  - [1/3] Bluetooth: iso: Fix circular locking dependency warnings
    (no matching commit)
  - [2/3] Bluetooth: iso: Always release hdev at the end of iso_listen_bis
    (no matching commit)
  - [3/3] Bluetooth: iso: Fix recursive locking warning
    https://git.kernel.org/bluetooth/bluetooth-next/c/269f7b219173

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html