[PATCH 0/1] Bluetooth: iso: Allow BIG re-sync

[PATCH 0/1] Bluetooth: iso: Allow BIG re-sync

[Iulia Tanasescu]

A Broadcast Sink might require BIG sync to be terminated and
re-established multiple times, while keeping the same PA sync
handle active. This can be possible if the configuration of the
listening (PA sync) socket is reset once all bound BISes are
established and accepted by the user space:

1. The DEFER setup flag needs to be reset on the parent socket,
to allow another BIG create sync procedure to be started on socket
read.

2. The BT_SK_BIG_SYNC flag needs to be cleared on the parent socket,
to allow another BIG create sync command to be sent.

3. The socket state needs to transition from BT_LISTEN to BT_CONNECTED,
to mark that the listening process has completed and another one can
be started if needed.

Iulia Tanasescu (1):
  Bluetooth: iso: Allow BIG re-sync

 net/bluetooth/iso.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)


base-commit: ad0d88dc33bb226d530886e2722e8eced0db49b1
-- 
2.43.0

[PATCH 1/1] Bluetooth: iso: Allow BIG re-sync

[Iulia Tanasescu]

A Broadcast Sink might require BIG sync to be terminated and
re-established multiple times, while keeping the same PA sync
handle active. This can be possible if the configuration of the
listening (PA sync) socket is reset once all bound BISes are
established and accepted by the user space:

1. The DEFER setup flag needs to be reset on the parent socket,
to allow another BIG create sync procedure to be started on socket
read.

2. The BT_SK_BIG_SYNC flag needs to be cleared on the parent socket,
to allow another BIG create sync command to be sent.

3. The socket state needs to transition from BT_LISTEN to BT_CONNECTED,
to mark that the listening process has completed and another one can
be started if needed.

Signed-off-by: Iulia Tanasescu <iulia.tanasescu@nxp.com>
---
 net/bluetooth/iso.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 1b40fd2b2f02..013edb19c4a1 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1268,6 +1268,42 @@ static int iso_sock_accept(struct socket *sock, struct socket *newsock,
 
 	BT_DBG("new socket %p", ch);
 
+	/* A Broadcast Sink might require BIG sync to be terminated
+	 * and re-established multiple times, while keeping the same
+	 * PA sync handle active. To allow this, once all BIS
+	 * connections have been accepted on a PA sync parent socket,
+	 * "reset" socket state, to allow future BIG re-sync procedures.
+	 */
+	if (test_bit(BT_SK_PA_SYNC, &iso_pi(sk)->flags)) {
+		/* Iterate through the list of bound BIS indices
+		 * and clear each BIS as they are accepted by the
+		 * user space, one by one.
+		 */
+		for (int i = 0; i < iso_pi(sk)->bc_num_bis; i++) {
+			if (iso_pi(sk)->bc_bis[i] > 0) {
+				iso_pi(sk)->bc_bis[i] = 0;
+				iso_pi(sk)->bc_num_bis--;
+				break;
+			}
+		}
+
+		if (iso_pi(sk)->bc_num_bis == 0) {
+			/* Once the last BIS was accepted, reset parent
+			 * socket parameters to mark that the listening
+			 * process for BIS connections has been completed:
+			 *
+			 * 1. Reset the DEFER setup flag on the parent sk.
+			 * 2. Clear the flag marking that the BIG create
+			 *    sync command is pending.
+			 * 3. Transition socket state from BT_LISTEN to
+			 *    BT_CONNECTED.
+			 */
+			set_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags);
+			clear_bit(BT_SK_BIG_SYNC, &iso_pi(sk)->flags);
+			sk->sk_state = BT_CONNECTED;
+		}
+	}
+
 done:
 	release_sock(sk);
 	return err;
-- 
2.43.0

RE: Bluetooth: iso: Allow BIG re-sync

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1902 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=913156

---Test result---

Test Summary:
CheckPatch                    PENDING   0.33 seconds
GitLint                       PENDING   0.23 seconds
SubjectPrefix                 PASS      0.09 seconds
BuildKernel                   PASS      25.39 seconds
CheckAllWarning               PASS      27.06 seconds
CheckSparse                   PASS      30.36 seconds
BuildKernel32                 PASS      24.44 seconds
TestRunnerSetup               PASS      438.63 seconds
TestRunner_l2cap-tester       PASS      20.99 seconds
TestRunner_iso-tester         FAIL      32.56 seconds
TestRunner_bnep-tester        PASS      4.82 seconds
TestRunner_mgmt-tester        PASS      122.28 seconds
TestRunner_rfcomm-tester      PASS      7.63 seconds
TestRunner_sco-tester         PASS      9.58 seconds
TestRunner_ioctl-tester       PASS      8.25 seconds
TestRunner_mesh-tester        PASS      6.08 seconds
TestRunner_smp-tester         PASS      7.15 seconds
TestRunner_userchan-tester    PASS      5.08 seconds
IncrementalBuild              PENDING   0.48 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:
WARNING: possible circular locking dependency detected
Total: 124, Passed: 120 (96.8%), Failed: 0, Not Run: 4
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH 1/1] Bluetooth: iso: Allow BIG re-sync

[Luiz Augusto von Dentz]

Hi Iulia,

On Thu, Nov 28, 2024 at 10:54 AM Iulia Tanasescu
<iulia.tanasescu@nxp.com> wrote:
>
> A Broadcast Sink might require BIG sync to be terminated and
> re-established multiple times, while keeping the same PA sync
> handle active. This can be possible if the configuration of the
> listening (PA sync) socket is reset once all bound BISes are
> established and accepted by the user space:
>
> 1. The DEFER setup flag needs to be reset on the parent socket,
> to allow another BIG create sync procedure to be started on socket
> read.
>
> 2. The BT_SK_BIG_SYNC flag needs to be cleared on the parent socket,
> to allow another BIG create sync command to be sent.
>
> 3. The socket state needs to transition from BT_LISTEN to BT_CONNECTED,
> to mark that the listening process has completed and another one can
> be started if needed.
>
> Signed-off-by: Iulia Tanasescu <iulia.tanasescu@nxp.com>
> ---
>  net/bluetooth/iso.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index 1b40fd2b2f02..013edb19c4a1 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -1268,6 +1268,42 @@ static int iso_sock_accept(struct socket *sock, struct socket *newsock,
>
>         BT_DBG("new socket %p", ch);
>
> +       /* A Broadcast Sink might require BIG sync to be terminated
> +        * and re-established multiple times, while keeping the same
> +        * PA sync handle active. To allow this, once all BIS
> +        * connections have been accepted on a PA sync parent socket,
> +        * "reset" socket state, to allow future BIG re-sync procedures.
> +        */
> +       if (test_bit(BT_SK_PA_SYNC, &iso_pi(sk)->flags)) {
> +               /* Iterate through the list of bound BIS indices
> +                * and clear each BIS as they are accepted by the
> +                * user space, one by one.
> +                */
> +               for (int i = 0; i < iso_pi(sk)->bc_num_bis; i++) {
> +                       if (iso_pi(sk)->bc_bis[i] > 0) {
> +                               iso_pi(sk)->bc_bis[i] = 0;
> +                               iso_pi(sk)->bc_num_bis--;
> +                               break;
> +                       }
> +               }
> +
> +               if (iso_pi(sk)->bc_num_bis == 0) {
> +                       /* Once the last BIS was accepted, reset parent
> +                        * socket parameters to mark that the listening
> +                        * process for BIS connections has been completed:
> +                        *
> +                        * 1. Reset the DEFER setup flag on the parent sk.
> +                        * 2. Clear the flag marking that the BIG create
> +                        *    sync command is pending.
> +                        * 3. Transition socket state from BT_LISTEN to
> +                        *    BT_CONNECTED.
> +                        */
> +                       set_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags);
> +                       clear_bit(BT_SK_BIG_SYNC, &iso_pi(sk)->flags);
> +                       sk->sk_state = BT_CONNECTED;
> +               }
> +       }
> +
>  done:
>         release_sock(sk);
>         return err;
> --
> 2.43.0

While testing this Ive run into the following problem:

======================================================
WARNING: possible circular locking dependency detected
6.12.0-rc6-g47ebf099106e #7428 Not tainted
------------------------------------------------------
kworker/u5:2/38 is trying to acquire lock:
ffff88800224a248 (sk_lock-AF_BLUETOOTH-BTPROTO_ISO){+.+.}-{0:0}, at:
iso_connect_cfm+0x563/0x13e0

but task is already holding lock:
ffffffffb0438668 (hci_cb_list_lock){+.+.}-{4:4}, at:
hci_le_per_adv_report_evt+0x494/0x6d0

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #2 (hci_cb_list_lock){+.+.}-{4:4}:
       lock_acquire+0x1b6/0x510
       __mutex_lock+0x180/0x1a60
       hci_le_per_adv_report_evt+0x494/0x6d0
       hci_event_packet+0x54a/0xec0
       hci_rx_work+0x76c/0x11c0
       process_one_work+0x7d9/0x13d0
       worker_thread+0x5b7/0xf90
       kthread+0x293/0x360
       ret_from_fork+0x2f/0x70
       ret_from_fork_asm+0x1a/0x30

-> #1 (&hdev->lock){+.+.}-{4:4}:
       lock_acquire+0x1b6/0x510
       __mutex_lock+0x180/0x1a60
       iso_sock_listen+0x2d4/0xe00
       __sys_listen+0x163/0x240
       __x64_sys_listen+0x4e/0x70
       do_syscall_64+0x71/0x140
       entry_SYSCALL_64_after_hwframe+0x76/0x7e

-> #0 (sk_lock-AF_BLUETOOTH-BTPROTO_ISO){+.+.}-{0:0}:
       check_prev_add+0x1b5/0x23f0
       __lock_acquire+0x2bdf/0x4580
       lock_acquire+0x1b6/0x510
       lock_sock_nested+0x36/0xd0
       iso_connect_cfm+0x563/0x13e0
       hci_le_per_adv_report_evt+0x4f8/0x6d0
       hci_event_packet+0x54a/0xec0
       hci_rx_work+0x76c/0x11c0
       process_one_work+0x7d9/0x13d0
       worker_thread+0x5b7/0xf90
       kthread+0x293/0x360
       ret_from_fork+0x2f/0x70
       ret_from_fork_asm+0x1a/0x30

other info that might help us debug this:

Chain exists of:
  sk_lock-AF_BLUETOOTH-BTPROTO_ISO --> &hdev->lock --> hci_cb_list_lock

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(hci_cb_list_lock);
                               lock(&hdev->lock);
                               lock(hci_cb_list_lock);
  lock(sk_lock-AF_BLUETOOTH-BTPROTO_ISO);

 *** DEADLOCK ***

4 locks held by kworker/u5:2/38:
 #0: ffff888002213948 ((wq_completion)hci0#2){+.+.}-{0:0}, at:
process_one_work+0xce8/0x13d0
 #1: ffff8880024a7da0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0},
at: process_one_work+0x758/0x13d0
 #2: ffff888002300078 (&hdev->lock){+.+.}-{4:4}, at:
hci_le_per_adv_report_evt+0x152/0x6d0
 #3: ffffffffb0438668 (hci_cb_list_lock){+.+.}-{4:4}, at:
hci_le_per_adv_report_evt+0x494/0x6d0

While this doesn't seem to be new I think it is a good idea to try and
solve it so we don't run into deadlocks.

-- 
Luiz Augusto von Dentz

Re: [PATCH 1/1] Bluetooth: iso: Allow BIG re-sync

[Luiz Augusto von Dentz]

Hi Iulia,

On Mon, Dec 2, 2024 at 5:00 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Iulia,
>
> On Thu, Nov 28, 2024 at 10:54 AM Iulia Tanasescu
> <iulia.tanasescu@nxp.com> wrote:
> >
> > A Broadcast Sink might require BIG sync to be terminated and
> > re-established multiple times, while keeping the same PA sync
> > handle active. This can be possible if the configuration of the
> > listening (PA sync) socket is reset once all bound BISes are
> > established and accepted by the user space:
> >
> > 1. The DEFER setup flag needs to be reset on the parent socket,
> > to allow another BIG create sync procedure to be started on socket
> > read.
> >
> > 2. The BT_SK_BIG_SYNC flag needs to be cleared on the parent socket,
> > to allow another BIG create sync command to be sent.
> >
> > 3. The socket state needs to transition from BT_LISTEN to BT_CONNECTED,
> > to mark that the listening process has completed and another one can
> > be started if needed.
> >
> > Signed-off-by: Iulia Tanasescu <iulia.tanasescu@nxp.com>
> > ---
> >  net/bluetooth/iso.c | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > index 1b40fd2b2f02..013edb19c4a1 100644
> > --- a/net/bluetooth/iso.c
> > +++ b/net/bluetooth/iso.c
> > @@ -1268,6 +1268,42 @@ static int iso_sock_accept(struct socket *sock, struct socket *newsock,
> >
> >         BT_DBG("new socket %p", ch);
> >
> > +       /* A Broadcast Sink might require BIG sync to be terminated
> > +        * and re-established multiple times, while keeping the same
> > +        * PA sync handle active. To allow this, once all BIS
> > +        * connections have been accepted on a PA sync parent socket,
> > +        * "reset" socket state, to allow future BIG re-sync procedures.
> > +        */
> > +       if (test_bit(BT_SK_PA_SYNC, &iso_pi(sk)->flags)) {
> > +               /* Iterate through the list of bound BIS indices
> > +                * and clear each BIS as they are accepted by the
> > +                * user space, one by one.
> > +                */
> > +               for (int i = 0; i < iso_pi(sk)->bc_num_bis; i++) {
> > +                       if (iso_pi(sk)->bc_bis[i] > 0) {
> > +                               iso_pi(sk)->bc_bis[i] = 0;
> > +                               iso_pi(sk)->bc_num_bis--;
> > +                               break;
> > +                       }
> > +               }
> > +
> > +               if (iso_pi(sk)->bc_num_bis == 0) {
> > +                       /* Once the last BIS was accepted, reset parent
> > +                        * socket parameters to mark that the listening
> > +                        * process for BIS connections has been completed:
> > +                        *
> > +                        * 1. Reset the DEFER setup flag on the parent sk.
> > +                        * 2. Clear the flag marking that the BIG create
> > +                        *    sync command is pending.
> > +                        * 3. Transition socket state from BT_LISTEN to
> > +                        *    BT_CONNECTED.
> > +                        */
> > +                       set_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags);
> > +                       clear_bit(BT_SK_BIG_SYNC, &iso_pi(sk)->flags);
> > +                       sk->sk_state = BT_CONNECTED;
> > +               }
> > +       }
> > +
> >  done:
> >         release_sock(sk);
> >         return err;
> > --
> > 2.43.0
>
> While testing this Ive run into the following problem:
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 6.12.0-rc6-g47ebf099106e #7428 Not tainted
> ------------------------------------------------------
> kworker/u5:2/38 is trying to acquire lock:
> ffff88800224a248 (sk_lock-AF_BLUETOOTH-BTPROTO_ISO){+.+.}-{0:0}, at:
> iso_connect_cfm+0x563/0x13e0
>
> but task is already holding lock:
> ffffffffb0438668 (hci_cb_list_lock){+.+.}-{4:4}, at:
> hci_le_per_adv_report_evt+0x494/0x6d0
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #2 (hci_cb_list_lock){+.+.}-{4:4}:
>        lock_acquire+0x1b6/0x510
>        __mutex_lock+0x180/0x1a60
>        hci_le_per_adv_report_evt+0x494/0x6d0
>        hci_event_packet+0x54a/0xec0
>        hci_rx_work+0x76c/0x11c0
>        process_one_work+0x7d9/0x13d0
>        worker_thread+0x5b7/0xf90
>        kthread+0x293/0x360
>        ret_from_fork+0x2f/0x70
>        ret_from_fork_asm+0x1a/0x30
>
> -> #1 (&hdev->lock){+.+.}-{4:4}:
>        lock_acquire+0x1b6/0x510
>        __mutex_lock+0x180/0x1a60
>        iso_sock_listen+0x2d4/0xe00
>        __sys_listen+0x163/0x240
>        __x64_sys_listen+0x4e/0x70
>        do_syscall_64+0x71/0x140
>        entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> -> #0 (sk_lock-AF_BLUETOOTH-BTPROTO_ISO){+.+.}-{0:0}:
>        check_prev_add+0x1b5/0x23f0
>        __lock_acquire+0x2bdf/0x4580
>        lock_acquire+0x1b6/0x510
>        lock_sock_nested+0x36/0xd0
>        iso_connect_cfm+0x563/0x13e0
>        hci_le_per_adv_report_evt+0x4f8/0x6d0
>        hci_event_packet+0x54a/0xec0
>        hci_rx_work+0x76c/0x11c0
>        process_one_work+0x7d9/0x13d0
>        worker_thread+0x5b7/0xf90
>        kthread+0x293/0x360
>        ret_from_fork+0x2f/0x70
>        ret_from_fork_asm+0x1a/0x30
>
> other info that might help us debug this:
>
> Chain exists of:
>   sk_lock-AF_BLUETOOTH-BTPROTO_ISO --> &hdev->lock --> hci_cb_list_lock
>
>  Possible unsafe locking scenario:
>
>        CPU0                    CPU1
>        ----                    ----
>   lock(hci_cb_list_lock);
>                                lock(&hdev->lock);
>                                lock(hci_cb_list_lock);
>   lock(sk_lock-AF_BLUETOOTH-BTPROTO_ISO);
>
>  *** DEADLOCK ***
>
> 4 locks held by kworker/u5:2/38:
>  #0: ffff888002213948 ((wq_completion)hci0#2){+.+.}-{0:0}, at:
> process_one_work+0xce8/0x13d0
>  #1: ffff8880024a7da0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0},
> at: process_one_work+0x758/0x13d0
>  #2: ffff888002300078 (&hdev->lock){+.+.}-{4:4}, at:
> hci_le_per_adv_report_evt+0x152/0x6d0
>  #3: ffffffffb0438668 (hci_cb_list_lock){+.+.}-{4:4}, at:
> hci_le_per_adv_report_evt+0x494/0x6d0
>
> While this doesn't seem to be new I think it is a good idea to try and
> solve it so we don't run into deadlocks.

Looks like this is due our usage of a mutex as locking for
hci_cb_list_lock, we could perhaps use a rwlock_t like its done for
hci_dev_list_lock but I think we may still run into possible unsafe
locking if there are multiple writers, so perhaps we could attempt to
convert use RCU which does allows us to do certain operations
lockless:

https://docs.kernel.org/next/RCU/listRCU.html

>
> --
> Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH 0/1] Bluetooth: iso: Allow BIG re-sync

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Thu, 28 Nov 2024 17:54:04 +0200 you wrote:
> A Broadcast Sink might require BIG sync to be terminated and
> re-established multiple times, while keeping the same PA sync
> handle active. This can be possible if the configuration of the
> listening (PA sync) socket is reset once all bound BISes are
> established and accepted by the user space:
> 
> 1. The DEFER setup flag needs to be reset on the parent socket,
> to allow another BIG create sync procedure to be started on socket
> read.
> 
> [...]

Here is the summary with links:
  - [1/1] Bluetooth: iso: Allow BIG re-sync
    https://git.kernel.org/bluetooth/bluetooth-next/c/47ebf099106e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH 1/1] Bluetooth: iso: Allow BIG re-sync

[Iulia Tanasescu]

Hi Luiz,

> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> Sent: Tuesday, December 3, 2024 12:29 AM
> To: Iulia Tanasescu <iulia.tanasescu@nxp.com>
> Cc: linux-bluetooth@vger.kernel.org; Claudia Cristina Draghicescu
> <claudia.rosu@nxp.com>; Mihai-Octavian Urzica <mihai-
> octavian.urzica@nxp.com>; Andrei Istodorescu
> <andrei.istodorescu@nxp.com>
> Subject: Re: [PATCH 1/1] Bluetooth: iso: Allow BIG re-sync
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> Hi Iulia,
> 
> On Mon, Dec 2, 2024 at 5:00 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Iulia,
> >
> > On Thu, Nov 28, 2024 at 10:54 AM Iulia Tanasescu
> > <iulia.tanasescu@nxp.com> wrote:
> > >
> > > A Broadcast Sink might require BIG sync to be terminated and
> > > re-established multiple times, while keeping the same PA sync handle
> > > active. This can be possible if the configuration of the listening
> > > (PA sync) socket is reset once all bound BISes are established and
> > > accepted by the user space:
> > >
> > > 1. The DEFER setup flag needs to be reset on the parent socket, to
> > > allow another BIG create sync procedure to be started on socket
> > > read.
> > >
> > > 2. The BT_SK_BIG_SYNC flag needs to be cleared on the parent socket,
> > > to allow another BIG create sync command to be sent.
> > >
> > > 3. The socket state needs to transition from BT_LISTEN to
> > > BT_CONNECTED, to mark that the listening process has completed and
> > > another one can be started if needed.
> > >
> > > Signed-off-by: Iulia Tanasescu <iulia.tanasescu@nxp.com>
> > > ---
> > >  net/bluetooth/iso.c | 36 ++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 36 insertions(+)
> > >
> > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c index
> > > 1b40fd2b2f02..013edb19c4a1 100644
> > > --- a/net/bluetooth/iso.c
> > > +++ b/net/bluetooth/iso.c
> > > @@ -1268,6 +1268,42 @@ static int iso_sock_accept(struct socket
> > > *sock, struct socket *newsock,
> > >
> > >         BT_DBG("new socket %p", ch);
> > >
> > > +       /* A Broadcast Sink might require BIG sync to be terminated
> > > +        * and re-established multiple times, while keeping the same
> > > +        * PA sync handle active. To allow this, once all BIS
> > > +        * connections have been accepted on a PA sync parent socket,
> > > +        * "reset" socket state, to allow future BIG re-sync procedures.
> > > +        */
> > > +       if (test_bit(BT_SK_PA_SYNC, &iso_pi(sk)->flags)) {
> > > +               /* Iterate through the list of bound BIS indices
> > > +                * and clear each BIS as they are accepted by the
> > > +                * user space, one by one.
> > > +                */
> > > +               for (int i = 0; i < iso_pi(sk)->bc_num_bis; i++) {
> > > +                       if (iso_pi(sk)->bc_bis[i] > 0) {
> > > +                               iso_pi(sk)->bc_bis[i] = 0;
> > > +                               iso_pi(sk)->bc_num_bis--;
> > > +                               break;
> > > +                       }
> > > +               }
> > > +
> > > +               if (iso_pi(sk)->bc_num_bis == 0) {
> > > +                       /* Once the last BIS was accepted, reset parent
> > > +                        * socket parameters to mark that the listening
> > > +                        * process for BIS connections has been completed:
> > > +                        *
> > > +                        * 1. Reset the DEFER setup flag on the parent sk.
> > > +                        * 2. Clear the flag marking that the BIG create
> > > +                        *    sync command is pending.
> > > +                        * 3. Transition socket state from BT_LISTEN to
> > > +                        *    BT_CONNECTED.
> > > +                        */
> > > +                       set_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags);
> > > +                       clear_bit(BT_SK_BIG_SYNC, &iso_pi(sk)->flags);
> > > +                       sk->sk_state = BT_CONNECTED;
> > > +               }
> > > +       }
> > > +
> > >  done:
> > >         release_sock(sk);
> > >         return err;
> > > --
> > > 2.43.0
> >
> > While testing this Ive run into the following problem:
> >
> > ======================================================
> > WARNING: possible circular locking dependency detected
> > 6.12.0-rc6-g47ebf099106e #7428 Not tainted
> > ------------------------------------------------------
> > kworker/u5:2/38 is trying to acquire lock:
> > ffff88800224a248 (sk_lock-AF_BLUETOOTH-BTPROTO_ISO){+.+.}-{0:0}, at:
> > iso_connect_cfm+0x563/0x13e0
> >
> > but task is already holding lock:
> > ffffffffb0438668 (hci_cb_list_lock){+.+.}-{4:4}, at:
> > hci_le_per_adv_report_evt+0x494/0x6d0
> >
> > which lock already depends on the new lock.
> >
> >
> > the existing dependency chain (in reverse order) is:
> >
> > -> #2 (hci_cb_list_lock){+.+.}-{4:4}:
> >        lock_acquire+0x1b6/0x510
> >        __mutex_lock+0x180/0x1a60
> >        hci_le_per_adv_report_evt+0x494/0x6d0
> >        hci_event_packet+0x54a/0xec0
> >        hci_rx_work+0x76c/0x11c0
> >        process_one_work+0x7d9/0x13d0
> >        worker_thread+0x5b7/0xf90
> >        kthread+0x293/0x360
> >        ret_from_fork+0x2f/0x70
> >        ret_from_fork_asm+0x1a/0x30
> >
> > -> #1 (&hdev->lock){+.+.}-{4:4}:
> >        lock_acquire+0x1b6/0x510
> >        __mutex_lock+0x180/0x1a60
> >        iso_sock_listen+0x2d4/0xe00
> >        __sys_listen+0x163/0x240
> >        __x64_sys_listen+0x4e/0x70
> >        do_syscall_64+0x71/0x140
> >        entry_SYSCALL_64_after_hwframe+0x76/0x7e
> >
> > -> #0 (sk_lock-AF_BLUETOOTH-BTPROTO_ISO){+.+.}-{0:0}:
> >        check_prev_add+0x1b5/0x23f0
> >        __lock_acquire+0x2bdf/0x4580
> >        lock_acquire+0x1b6/0x510
> >        lock_sock_nested+0x36/0xd0
> >        iso_connect_cfm+0x563/0x13e0
> >        hci_le_per_adv_report_evt+0x4f8/0x6d0
> >        hci_event_packet+0x54a/0xec0
> >        hci_rx_work+0x76c/0x11c0
> >        process_one_work+0x7d9/0x13d0
> >        worker_thread+0x5b7/0xf90
> >        kthread+0x293/0x360
> >        ret_from_fork+0x2f/0x70
> >        ret_from_fork_asm+0x1a/0x30
> >
> > other info that might help us debug this:
> >
> > Chain exists of:
> >   sk_lock-AF_BLUETOOTH-BTPROTO_ISO --> &hdev->lock -->
> > hci_cb_list_lock
> >
> >  Possible unsafe locking scenario:
> >
> >        CPU0                    CPU1
> >        ----                    ----
> >   lock(hci_cb_list_lock);
> >                                lock(&hdev->lock);
> >                                lock(hci_cb_list_lock);
> >   lock(sk_lock-AF_BLUETOOTH-BTPROTO_ISO);
> >
> >  *** DEADLOCK ***
> >
> > 4 locks held by kworker/u5:2/38:
> >  #0: ffff888002213948 ((wq_completion)hci0#2){+.+.}-{0:0}, at:
> > process_one_work+0xce8/0x13d0
> >  #1: ffff8880024a7da0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0},
> > at: process_one_work+0x758/0x13d0
> >  #2: ffff888002300078 (&hdev->lock){+.+.}-{4:4}, at:
> > hci_le_per_adv_report_evt+0x152/0x6d0
> >  #3: ffffffffb0438668 (hci_cb_list_lock){+.+.}-{4:4}, at:
> > hci_le_per_adv_report_evt+0x494/0x6d0
> >
> > While this doesn't seem to be new I think it is a good idea to try and
> > solve it so we don't run into deadlocks.
> 
> Looks like this is due our usage of a mutex as locking for hci_cb_list_lock, we
> could perhaps use a rwlock_t like its done for hci_dev_list_lock but I think we
> may still run into possible unsafe locking if there are multiple writers, so
> perhaps we could attempt to convert use RCU which does allows us to do
> certain operations
> lockless:
> 
> https://docs.ke/
> rnel.org%2Fnext%2FRCU%2FlistRCU.html&data=05%7C02%7Ciulia.tanasescu
> %40nxp.com%7C493e0b198a26497d5e2f08dd1320bfb8%7C686ea1d3bc2b4c
> 6fa92cd99c5c301635%7C0%7C0%7C638687753582549789%7CUnknown%7CT
> WFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXa
> W4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=Eyu7
> %2BLOcRhCHYA7wEpkikFHsUgJQKFHRAZlPFXcPn8s%3D&reserved=0
>

Even if I use RCU instead of mutex, I still get the following warning:

[   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)
               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)
                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]

I think the root issue comes from an A->B/B->A type lock order with hdev
and sk. I submitted a patch series with a fix for this, and also
2 other issues that I found:
https://patchwork.kernel.org/project/bluetooth/cover/20241204122849.9000-1-iulia.tanasescu@nxp.com/

> >
> > --
> > Luiz Augusto von Dentz
> 
> 
> 
> --
> Luiz Augusto von Dentz

Regards,
Iulia