* [PATCH v2 1/2] Bluetooth: iso: Fix circular lock in iso_listen_bis
2024-12-09 9:42 [PATCH v2 0/2] Bluetooth: iso: Fix circular lock warnings Iulia Tanasescu
@ 2024-12-09 9:42 ` Iulia Tanasescu
2024-12-09 10:33 ` Bluetooth: iso: Fix circular lock warnings bluez.test.bot
2024-12-09 9:42 ` [PATCH v2 2/2] Bluetooth: iso: Fix circular lock in iso_conn_big_sync Iulia Tanasescu
2024-12-11 16:50 ` [PATCH v2 0/2] Bluetooth: iso: Fix circular lock warnings patchwork-bot+bluetooth
2 siblings, 1 reply; 5+ messages in thread
From: Iulia Tanasescu @ 2024-12-09 9:42 UTC (permalink / raw)
To: linux-bluetooth
Cc: claudia.rosu, mihai-octavian.urzica, andrei.istodorescu,
luiz.dentz, Iulia Tanasescu
This fixes the circular locking dependency warning below, by
releasing the socket lock before enterning iso_listen_bis, to
avoid any potential deadlock with hdev lock.
[ 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]
Fixes: 02171da6e86a ("Bluetooth: ISO: Add hcon for listening bis sk")
Signed-off-by: Iulia Tanasescu <iulia.tanasescu@nxp.com>
---
net/bluetooth/iso.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 5ca381a3b04b..ed559c82d353 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1129,6 +1129,7 @@ static int iso_listen_bis(struct sock *sk)
return -EHOSTUNREACH;
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)) {
@@ -1159,6 +1160,7 @@ static int iso_listen_bis(struct sock *sk)
}
unlock:
+ release_sock(sk);
hci_dev_unlock(hdev);
hci_dev_put(hdev);
return err;
@@ -1187,6 +1189,7 @@ static int iso_sock_listen(struct socket *sock, int backlog)
BT_DBG("sk %p backlog %d", sk, backlog);
+ sock_hold(sk);
lock_sock(sk);
if (sk->sk_state != BT_BOUND) {
@@ -1199,10 +1202,16 @@ static int iso_sock_listen(struct socket *sock, int backlog)
goto done;
}
- if (!bacmp(&iso_pi(sk)->dst, BDADDR_ANY))
+ if (!bacmp(&iso_pi(sk)->dst, BDADDR_ANY)) {
err = iso_listen_cis(sk);
- else
+ } else {
+ /* Drop sock lock to avoid potential
+ * deadlock with the hdev lock.
+ */
+ release_sock(sk);
err = iso_listen_bis(sk);
+ lock_sock(sk);
+ }
if (err)
goto done;
@@ -1214,6 +1223,7 @@ static int iso_sock_listen(struct socket *sock, int backlog)
done:
release_sock(sk);
+ sock_put(sk);
return err;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* RE: Bluetooth: iso: Fix circular lock warnings
2024-12-09 9:42 ` [PATCH v2 1/2] Bluetooth: iso: Fix circular lock in iso_listen_bis Iulia Tanasescu
@ 2024-12-09 10:33 ` bluez.test.bot
0 siblings, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2024-12-09 10:33 UTC (permalink / raw)
To: linux-bluetooth, iulia.tanasescu
[-- Attachment #1: Type: text/plain, Size: 1680 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=915843
---Test result---
Test Summary:
CheckPatch PENDING 0.28 seconds
GitLint PENDING 0.21 seconds
SubjectPrefix PASS 0.23 seconds
BuildKernel PASS 25.78 seconds
CheckAllWarning PASS 27.60 seconds
CheckSparse PASS 31.26 seconds
BuildKernel32 PASS 25.38 seconds
TestRunnerSetup PASS 439.26 seconds
TestRunner_l2cap-tester PASS 20.54 seconds
TestRunner_iso-tester PASS 27.79 seconds
TestRunner_bnep-tester PASS 4.91 seconds
TestRunner_mgmt-tester PASS 120.37 seconds
TestRunner_rfcomm-tester PASS 7.56 seconds
TestRunner_sco-tester PASS 9.29 seconds
TestRunner_ioctl-tester PASS 8.06 seconds
TestRunner_mesh-tester PASS 6.95 seconds
TestRunner_smp-tester PASS 7.01 seconds
TestRunner_userchan-tester PASS 4.98 seconds
IncrementalBuild PENDING 0.90 seconds
Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:
##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] Bluetooth: iso: Fix circular lock in iso_conn_big_sync
2024-12-09 9:42 [PATCH v2 0/2] Bluetooth: iso: Fix circular lock warnings Iulia Tanasescu
2024-12-09 9:42 ` [PATCH v2 1/2] Bluetooth: iso: Fix circular lock in iso_listen_bis Iulia Tanasescu
@ 2024-12-09 9:42 ` Iulia Tanasescu
2024-12-11 16:50 ` [PATCH v2 0/2] Bluetooth: iso: Fix circular lock warnings patchwork-bot+bluetooth
2 siblings, 0 replies; 5+ messages in thread
From: Iulia Tanasescu @ 2024-12-09 9:42 UTC (permalink / raw)
To: linux-bluetooth
Cc: claudia.rosu, mihai-octavian.urzica, andrei.istodorescu,
luiz.dentz, Iulia Tanasescu
This fixes the circular locking dependency warning below, by reworking
iso_sock_recvmsg, to ensure that the socket lock is always released
before calling a function that locks hdev.
[ 561.670344] ======================================================
[ 561.670346] WARNING: possible circular locking dependency detected
[ 561.670349] 6.12.0-rc6+ #26 Not tainted
[ 561.670351] ------------------------------------------------------
[ 561.670353] iso-tester/3289 is trying to acquire lock:
[ 561.670355] ffff88811f600078 (&hdev->lock){+.+.}-{3:3},
at: iso_conn_big_sync+0x73/0x260 [bluetooth]
[ 561.670405]
but task is already holding lock:
[ 561.670407] ffff88815af58258 (sk_lock-AF_BLUETOOTH){+.+.}-{0:0},
at: iso_sock_recvmsg+0xbf/0x500 [bluetooth]
[ 561.670450]
which lock already depends on the new lock.
[ 561.670452]
the existing dependency chain (in reverse order) is:
[ 561.670453]
-> #2 (sk_lock-AF_BLUETOOTH){+.+.}-{0:0}:
[ 561.670458] lock_acquire+0x7c/0xc0
[ 561.670463] lock_sock_nested+0x3b/0xf0
[ 561.670467] bt_accept_dequeue+0x1a5/0x4d0 [bluetooth]
[ 561.670510] iso_sock_accept+0x271/0x830 [bluetooth]
[ 561.670547] do_accept+0x3dd/0x610
[ 561.670550] __sys_accept4+0xd8/0x170
[ 561.670553] __x64_sys_accept+0x74/0xc0
[ 561.670556] x64_sys_call+0x17d6/0x25f0
[ 561.670559] do_syscall_64+0x87/0x150
[ 561.670563] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 561.670567]
-> #1 (sk_lock-AF_BLUETOOTH-BTPROTO_ISO){+.+.}-{0:0}:
[ 561.670571] lock_acquire+0x7c/0xc0
[ 561.670574] lock_sock_nested+0x3b/0xf0
[ 561.670577] iso_sock_listen+0x2de/0xf30 [bluetooth]
[ 561.670617] __sys_listen_socket+0xef/0x130
[ 561.670620] __x64_sys_listen+0xe1/0x190
[ 561.670623] x64_sys_call+0x2517/0x25f0
[ 561.670626] do_syscall_64+0x87/0x150
[ 561.670629] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 561.670632]
-> #0 (&hdev->lock){+.+.}-{3:3}:
[ 561.670636] __lock_acquire+0x32ad/0x6ab0
[ 561.670639] lock_acquire.part.0+0x118/0x360
[ 561.670642] lock_acquire+0x7c/0xc0
[ 561.670644] __mutex_lock+0x18d/0x12f0
[ 561.670647] mutex_lock_nested+0x1b/0x30
[ 561.670651] iso_conn_big_sync+0x73/0x260 [bluetooth]
[ 561.670687] iso_sock_recvmsg+0x3e9/0x500 [bluetooth]
[ 561.670722] sock_recvmsg+0x1d5/0x240
[ 561.670725] sock_read_iter+0x27d/0x470
[ 561.670727] vfs_read+0x9a0/0xd30
[ 561.670731] ksys_read+0x1a8/0x250
[ 561.670733] __x64_sys_read+0x72/0xc0
[ 561.670736] x64_sys_call+0x1b12/0x25f0
[ 561.670738] do_syscall_64+0x87/0x150
[ 561.670741] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 561.670744]
other info that might help us debug this:
[ 561.670745] Chain exists of:
&hdev->lock --> sk_lock-AF_BLUETOOTH-BTPROTO_ISO --> sk_lock-AF_BLUETOOTH
[ 561.670751] Possible unsafe locking scenario:
[ 561.670753] CPU0 CPU1
[ 561.670754] ---- ----
[ 561.670756] lock(sk_lock-AF_BLUETOOTH);
[ 561.670758] lock(sk_lock
AF_BLUETOOTH-BTPROTO_ISO);
[ 561.670761] lock(sk_lock-AF_BLUETOOTH);
[ 561.670764] lock(&hdev->lock);
[ 561.670767]
*** DEADLOCK ***
Fixes: 07a9342b94a9 ("Bluetooth: ISO: Send BIG Create Sync via hci_sync")
Signed-off-by: Iulia Tanasescu <iulia.tanasescu@nxp.com>
---
net/bluetooth/iso.c | 34 +++++++++++++++++++++++++++-------
1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index ed559c82d353..44acddf58a0c 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1447,6 +1447,7 @@ static void iso_conn_big_sync(struct sock *sk)
* change.
*/
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,
@@ -1459,6 +1460,7 @@ static void iso_conn_big_sync(struct sock *sk)
err);
}
+ release_sock(sk);
hci_dev_unlock(hdev);
}
@@ -1467,39 +1469,57 @@ static int iso_sock_recvmsg(struct socket *sock, struct msghdr *msg,
{
struct sock *sk = sock->sk;
struct iso_pinfo *pi = iso_pi(sk);
+ bool early_ret = false;
+ int err = 0;
BT_DBG("sk %p", sk);
if (test_and_clear_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
+ sock_hold(sk);
lock_sock(sk);
+
switch (sk->sk_state) {
case BT_CONNECT2:
if (test_bit(BT_SK_PA_SYNC, &pi->flags)) {
+ release_sock(sk);
iso_conn_big_sync(sk);
+ lock_sock(sk);
+
sk->sk_state = BT_LISTEN;
} else {
iso_conn_defer_accept(pi->conn->hcon);
sk->sk_state = BT_CONFIG;
}
- release_sock(sk);
- return 0;
+
+ early_ret = true;
+ break;
case BT_CONNECTED:
if (test_bit(BT_SK_PA_SYNC, &iso_pi(sk)->flags)) {
+ release_sock(sk);
iso_conn_big_sync(sk);
+ lock_sock(sk);
+
sk->sk_state = BT_LISTEN;
- release_sock(sk);
- return 0;
+ early_ret = true;
}
- release_sock(sk);
break;
case BT_CONNECT:
release_sock(sk);
- return iso_connect_cis(sk);
+ err = iso_connect_cis(sk);
+ lock_sock(sk);
+
+ early_ret = true;
+ break;
default:
- release_sock(sk);
break;
}
+
+ release_sock(sk);
+ sock_put(sk);
+
+ if (early_ret)
+ return err;
}
return bt_sock_recvmsg(sock, msg, len, flags);
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2 0/2] Bluetooth: iso: Fix circular lock warnings
2024-12-09 9:42 [PATCH v2 0/2] Bluetooth: iso: Fix circular lock warnings Iulia Tanasescu
2024-12-09 9:42 ` [PATCH v2 1/2] Bluetooth: iso: Fix circular lock in iso_listen_bis Iulia Tanasescu
2024-12-09 9:42 ` [PATCH v2 2/2] Bluetooth: iso: Fix circular lock in iso_conn_big_sync Iulia Tanasescu
@ 2024-12-11 16:50 ` patchwork-bot+bluetooth
2 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+bluetooth @ 2024-12-11 16:50 UTC (permalink / raw)
To: Iulia Tanasescu
Cc: linux-bluetooth, claudia.rosu, mihai-octavian.urzica,
andrei.istodorescu, luiz.dentz
Hello:
This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:
On Mon, 9 Dec 2024 11:42:16 +0200 you wrote:
> This patch fixes the circular locking dependency warnings in iso.c,
> caused by A->B/B->A type lock ordering with sk and hdev.
>
> The first commit fixes the warning caused by iso_listen_bis, by
> releasing sk lock before entering the function and acquiring hdev
> lock.
>
> [...]
Here is the summary with links:
- [v2,1/2] Bluetooth: iso: Fix circular lock in iso_listen_bis
https://git.kernel.org/bluetooth/bluetooth-next/c/c9f33b7cbdb3
- [v2,2/2] Bluetooth: iso: Fix circular lock in iso_conn_big_sync
https://git.kernel.org/bluetooth/bluetooth-next/c/ea4f8778d364
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread