Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2292-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240304165050.3430-1-iulia.tanasescu@nxp.com> <20240304165050.3430-2-iulia.tanasescu@nxp.com>
In-Reply-To: <20240304165050.3430-2-iulia.tanasescu@nxp.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Mon, 4 Mar 2024 14:00:41 -0500
Message-ID: <CABBYNZJT=TM1-nrSn6GE-3wx5SKxiKBvddJTVVbkSG2UT__FWQ@mail.gmail.com>
Subject: Re: [1/2] Bluetooth: ISO: Fix circular locking dependency warning
To: Iulia Tanasescu <iulia.tanasescu@nxp.com>
Cc: linux-bluetooth@vger.kernel.org, claudia.rosu@nxp.com, 
	mihai-octavian.urzica@nxp.com, silviu.barbulescu@nxp.com, 
	vlad.pruteanu@nxp.com, andrei.istodorescu@nxp.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Iulia,

On Mon, Mar 4, 2024 at 11:51=E2=80=AFAM Iulia Tanasescu <iulia.tanasescu@nx=
p.com> wrote:
>
> 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.
>
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D
> 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/0x8=
d0
>
>                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, st=
ruct 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);

Don't really like the idea of having to release the lock here since
that in the meantime can cause it to destroy the socket while waiting
for hci_dev_lock so perhaps we need to look into mutex_lock_nested.

>         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
>


--=20
Luiz Augusto von Dentz