linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Bluetooth: ISO: iso_listen_bis fixes
@ 2024-03-05 14:09 Iulia Tanasescu
  2024-03-05 14:09 ` [PATCH v2 1/2] Bluetooth: ISO: Fix circular locking dependency warning Iulia Tanasescu
  2024-03-05 14:09 ` [PATCH v2 2/2] Bluetooth: ISO: Always release hdev at the end of iso_listen_bis Iulia Tanasescu
  0 siblings, 2 replies; 7+ messages in thread
From: Iulia Tanasescu @ 2024-03-05 14:09 UTC (permalink / raw)
  To: linux-bluetooth
  Cc: claudia.rosu, mihai-octavian.urzica, silviu.barbulescu,
	vlad.pruteanu, andrei.istodorescu, luiz.dentz, Iulia Tanasescu

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


^ permalink raw reply	[flat|nested] 7+ messages in thread
* [PATCH v3 1/2] Bluetooth: ISO: Fix circular locking dependency warning
@ 2024-03-07 13:58 Iulia Tanasescu
  2024-03-07 14:32 ` Bluetooth: ISO: iso_listen_bis fixes bluez.test.bot
  0 siblings, 1 reply; 7+ messages in thread
From: Iulia Tanasescu @ 2024-03-07 13:58 UTC (permalink / raw)
  To: linux-bluetooth
  Cc: claudia.rosu, mihai-octavian.urzica, silviu.barbulescu,
	vlad.pruteanu, andrei.istodorescu, luiz.dentz, Iulia Tanasescu

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 <iulia.tanasescu@nxp.com>
---
 net/bluetooth/iso.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 8af75d37b14c..d1608387fd85 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1069,6 +1069,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;
@@ -1095,7 +1096,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)) {
@@ -1128,7 +1137,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;
 }
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [1/2] Bluetooth: ISO: Fix circular locking dependency warning
@ 2024-03-04 16:50 Iulia Tanasescu
  2024-03-04 17:32 ` Bluetooth: ISO: iso_listen_bis fixes bluez.test.bot
  0 siblings, 1 reply; 7+ messages in thread
From: Iulia Tanasescu @ 2024-03-04 16:50 UTC (permalink / raw)
  To: linux-bluetooth
  Cc: claudia.rosu, mihai-octavian.urzica, silviu.barbulescu,
	vlad.pruteanu, andrei.istodorescu, luiz.dentz, Iulia Tanasescu

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 <iulia.tanasescu@nxp.com>
---
 net/bluetooth/iso.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 30c777c469f9..163b07db575d 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1069,6 +1069,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;
@@ -1095,7 +1096,12 @@ static int iso_listen_bis(struct sock *sk)
 	if (!hdev)
 		return -EHOSTUNREACH;
 
+	/* 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)) {
@@ -1128,7 +1134,10 @@ 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);
 	return err;
 }
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2024-03-07 14:32 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-05 14:09 [PATCH v2 0/2] Bluetooth: ISO: iso_listen_bis fixes Iulia Tanasescu
2024-03-05 14:09 ` [PATCH v2 1/2] Bluetooth: ISO: Fix circular locking dependency warning Iulia Tanasescu
2024-03-05 14:32   ` Bluetooth: ISO: iso_listen_bis fixes bluez.test.bot
2024-03-06 16:11   ` [PATCH v2 1/2] Bluetooth: ISO: Fix circular locking dependency warning Pauli Virtanen
2024-03-05 14:09 ` [PATCH v2 2/2] Bluetooth: ISO: Always release hdev at the end of iso_listen_bis Iulia Tanasescu
  -- strict thread matches above, loose matches on Subject: below --
2024-03-07 13:58 [PATCH v3 1/2] Bluetooth: ISO: Fix circular locking dependency warning Iulia Tanasescu
2024-03-07 14:32 ` Bluetooth: ISO: iso_listen_bis fixes bluez.test.bot
2024-03-04 16:50 [1/2] Bluetooth: ISO: Fix circular locking dependency warning Iulia Tanasescu
2024-03-04 17:32 ` Bluetooth: ISO: iso_listen_bis fixes bluez.test.bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).