| Message ID | 20241204122849.9000-2-iulia.tanasescu@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Bluetooth: iso: Fix warnings | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | fail | TestRunner_iso-tester: Total: 125, Passed: 120 (96.0%), Failed: 1, Not Run: 4 |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | fail | TestRunner_mgmt-tester: Total: 492, Passed: 486 (98.8%), Failed: 2, Not Run: 4 |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
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
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.
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,
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(+)