This patch adds some fixes for the iso_listen_bis function:
The first commit fixes the circular locking dependency warning
caused by iso_listen_bis attempting to lock the hdev while the
socket is locked from the caller function.
The second commit fixes the fact that hci_dev_put is only called
if iso_listen_bis returns successfully. Otherwise, the hdev will
still be held from hci_get_route.
Iulia Tanasescu (2):
Bluetooth: ISO: Fix circular locking dependency warning
Bluetooth: ISO: Always release hdev at the end of iso_listen_bis
net/bluetooth/iso.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
--
2.39.2
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 <[email protected]>
---
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 5ca7bb0806b0..336c28cea3f7 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1128,10 +1128,10 @@ static int iso_listen_bis(struct sock *sk)
goto unlock;
}
- hci_dev_put(hdev);
-
unlock:
hci_dev_unlock(hdev);
+ hci_dev_put(hdev);
+
return err;
}
--
2.39.2
This fixes the circular locking dependency warning caused
by iso_listen_bis acquiring the hdev lock while the socket
has been locked in the caller function.
======================================================
WARNING: possible circular locking dependency detected
6.8.0-rc5+ #1 Not tainted
------------------------------------------------------
iso-tester/2950 is trying to acquire lock:
ffff88817a048080 (&hdev->lock){+.+.}-{3:3}, at: iso_sock_listen+0x305/0x8d0
but task is already holding lock:
ffff888197c39278 (sk_lock-AF_BLUETOOTH-BTPROTO_ISO){+.+.}-{0:0},
at: iso_sock_listen+0x91/0x8d0
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (sk_lock-AF_BLUETOOTH-BTPROTO_ISO){+.+.}-{0:0}:
lock_sock_nested+0x3b/0xb0
iso_connect_cis+0x185/0x540
iso_sock_connect+0x445/0x7e0
__sys_connect_file+0xd5/0x100
__sys_connect+0x11e/0x150
__x64_sys_connect+0x42/0x60
do_syscall_64+0x8d/0x150
entry_SYSCALL_64_after_hwframe+0x6e/0x76
-> #0 (&hdev->lock){+.+.}-{3:3}:
__lock_acquire+0x208f/0x3720
lock_acquire+0x16d/0x3f0
__mutex_lock+0x155/0x1310
mutex_lock_nested+0x1b/0x30
iso_sock_listen+0x305/0x8d0
__sys_listen+0x106/0x190
__x64_sys_listen+0x30/0x40
do_syscall_64+0x8d/0x150
entry_SYSCALL_64_after_hwframe+0x6e/0x76
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(sk_lock-AF_BLUETOOTH-BTPROTO_ISO);
lock(&hdev->lock);
lock(sk_lock-AF_BLUETOOTH-BTPROTO_ISO);
lock(&hdev->lock);
*** DEADLOCK ***
1 lock held by iso-tester/2950:
0: ffff888197c39278 (sk_lock-AF_BLUETOOTH-BTPROTO_ISO){+.+.}-{0:0},
at: iso_sock_listen+0x91/0x8d0
Signed-off-by: Iulia Tanasescu <[email protected]>
---
net/bluetooth/iso.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 8af75d37b14c..5ca7bb0806b0 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1095,7 +1095,10 @@ static int iso_listen_bis(struct sock *sk)
if (!hdev)
return -EHOSTUNREACH;
- hci_dev_lock(hdev);
+ /* To avoid circular locking dependency warnings,
+ * use nested lock for hdev.
+ */
+ mutex_lock_nested(&hdev->lock, SINGLE_DEPTH_NESTING);
/* Fail if user set invalid QoS */
if (iso_pi(sk)->qos_user_set && !check_bcast_qos(&iso_pi(sk)->qos)) {
--
2.39.2
Hi,
ti, 2024-03-05 kello 16:09 +0200, Iulia Tanasescu kirjoitti:
> This fixes the circular locking dependency warning caused
> by iso_listen_bis acquiring the hdev lock while the socket
> has been locked in the caller function.
I think the commit message and the comment should explain why there is
no deadlock, and that the lockdep warning is a false positive.
E.g. fuzzer calling connect() and listen() at the same time seems it
would be dangerous here?
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 6.8.0-rc5+ #1 Not tainted
> ------------------------------------------------------
> iso-tester/2950 is trying to acquire lock:
> ffff88817a048080 (&hdev->lock){+.+.}-{3:3}, at: iso_sock_listen+0x305/0x8d0
>
> but task is already holding lock:
> ffff888197c39278 (sk_lock-AF_BLUETOOTH-BTPROTO_ISO){+.+.}-{0:0},
> at: iso_sock_listen+0x91/0x8d0
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (sk_lock-AF_BLUETOOTH-BTPROTO_ISO){+.+.}-{0:0}:
> lock_sock_nested+0x3b/0xb0
> iso_connect_cis+0x185/0x540
> iso_sock_connect+0x445/0x7e0
> __sys_connect_file+0xd5/0x100
> __sys_connect+0x11e/0x150
> __x64_sys_connect+0x42/0x60
> do_syscall_64+0x8d/0x150
> entry_SYSCALL_64_after_hwframe+0x6e/0x76
>
> -> #0 (&hdev->lock){+.+.}-{3:3}:
> __lock_acquire+0x208f/0x3720
> lock_acquire+0x16d/0x3f0
> __mutex_lock+0x155/0x1310
> mutex_lock_nested+0x1b/0x30
> iso_sock_listen+0x305/0x8d0
> __sys_listen+0x106/0x190
> __x64_sys_listen+0x30/0x40
> do_syscall_64+0x8d/0x150
> entry_SYSCALL_64_after_hwframe+0x6e/0x76
>
> other info that might help us debug this:
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(sk_lock-AF_BLUETOOTH-BTPROTO_ISO);
> lock(&hdev->lock);
> lock(sk_lock-AF_BLUETOOTH-BTPROTO_ISO);
> lock(&hdev->lock);
>
> *** DEADLOCK ***
>
> 1 lock held by iso-tester/2950:
> 0: ffff888197c39278 (sk_lock-AF_BLUETOOTH-BTPROTO_ISO){+.+.}-{0:0},
> at: iso_sock_listen+0x91/0x8d0
>
> Signed-off-by: Iulia Tanasescu <[email protected]>
> ---
> net/bluetooth/iso.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index 8af75d37b14c..5ca7bb0806b0 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -1095,7 +1095,10 @@ static int iso_listen_bis(struct sock *sk)
> if (!hdev)
> return -EHOSTUNREACH;
>
> - hci_dev_lock(hdev);
> + /* To avoid circular locking dependency warnings,
> + * use nested lock for hdev.
> + */
> + mutex_lock_nested(&hdev->lock, SINGLE_DEPTH_NESTING);
>
> /* Fail if user set invalid QoS */
> if (iso_pi(sk)->qos_user_set && !check_bcast_qos(&iso_pi(sk)->qos)) {
--
Pauli Virtanen