1. Patchew
  2. linux
  3. View series

[PATCH] Bluetooth: core: Fix deadlock due to `cancel_work_sync(&hdev->power_on)` from hci_power_on_sync.

Vasyl Vavrychuk posted 1 patch 3 years, 9 months ago 
net/bluetooth/hci_core.c | 3 +++
net/bluetooth/hci_sync.c | 1 -
2 files changed, 3 insertions(+), 1 deletion(-)
[PATCH] Bluetooth: core: Fix deadlock due to `cancel_work_sync(&hdev->power_on)` from hci_power_on_sync.
Posted by Vasyl Vavrychuk 3 years, 9 months ago 
`cancel_work_sync(&hdev->power_on)` was moved to hci_dev_close_sync in
commit [1] to ensure that power_on work is canceled after HCI interface
down.

But, in certain cases power_on work function may call hci_dev_close_sync
itself: hci_power_on -> hci_dev_do_close -> hci_dev_close_sync ->
cancel_work_sync(&hdev->power_on), causing deadlock. In particular, this
happens when device is rfkilled on boot. To avoid deadlock, move
power_on work canceling out of hci_dev_do_close/hci_dev_close_sync.

Deadlock introduced by commit [1] was reported in [2,3] as broken
suspend. Suspend did not work because `hdev->req_lock` held as result of
`power_on` work deadlock. In fact, other BT features were not working.
It was not observed when testing [1] since it was verified without
rfkill in place.

NOTE: It is not needed to cancel power_on work from other places where
hci_dev_do_close/hci_dev_close_sync is called in case:
* Requests were serialized due to `hdev->req_workqueue`. The power_on
work is first in that workqueue.
* hci_rfkill_set_block which won't close device anyway until HCI_SETUP
is on.
* hci_sock_release which runs after hci_sock_bind which ensures
HCI_SETUP was cleared.

As result, behaviour is the same as in pre-dd06ed7 commit, except
power_on work cancel added to hci_dev_close.

[1]: commit dd06ed7ad057 ("Bluetooth: core: Fix missing power_on work cancel on HCI close")
[2]: https://lore.kernel.org/lkml/20220614181706.26513-1-max.oss.09@gmail.com/
[2]: https://lore.kernel.org/lkml/1236061d-95dd-c3ad-a38f-2dae7aae51ef@o2.pl/

Fixes: commit dd06ed7ad057 ("Bluetooth: core: Fix missing power_on work cancel on HCI close")
Signed-off-by: Vasyl Vavrychuk <vasyl.vavrychuk@opensynergy.com>
Reported-by: Max Krummenacher <max.krummenacher@toradex.com>
Reported-by: Mateusz Jonczyk <mat.jonczyk@o2.pl>
---
 net/bluetooth/hci_core.c | 3 +++
 net/bluetooth/hci_sync.c | 1 -
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 59a5c1341c26..a0f99baafd35 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -571,6 +571,7 @@ int hci_dev_close(__u16 dev)
 		goto done;
 	}
 
+	cancel_work_sync(&hdev->power_on);
 	if (hci_dev_test_and_clear_flag(hdev, HCI_AUTO_OFF))
 		cancel_delayed_work(&hdev->power_off);
 
@@ -2675,6 +2676,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
 	list_del(&hdev->list);
 	write_unlock(&hci_dev_list_lock);
 
+	cancel_work_sync(&hdev->power_on);
+
 	hci_cmd_sync_clear(hdev);
 
 	if (!test_bit(HCI_QUIRK_NO_SUSPEND_NOTIFIER, &hdev->quirks))
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 286d6767f017..1739e8cb3291 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -4088,7 +4088,6 @@ int hci_dev_close_sync(struct hci_dev *hdev)
 
 	bt_dev_dbg(hdev, "");
 
-	cancel_work_sync(&hdev->power_on);
 	cancel_delayed_work(&hdev->power_off);
 	cancel_delayed_work(&hdev->ncmd_timer);
 
-- 
2.30.2
Re: [PATCH] Bluetooth: core: Fix deadlock due to `cancel_work_sync(&hdev->power_on)` from hci_power_on_sync.
Posted by patchwork-bot+netdevbpf@kernel.org 3 years, 9 months ago 
Hello:

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

On Tue,  5 Jul 2022 15:59:31 +0300 you wrote:
> `cancel_work_sync(&hdev->power_on)` was moved to hci_dev_close_sync in
> commit [1] to ensure that power_on work is canceled after HCI interface
> down.
> 
> But, in certain cases power_on work function may call hci_dev_close_sync
> itself: hci_power_on -> hci_dev_do_close -> hci_dev_close_sync ->
> cancel_work_sync(&hdev->power_on), causing deadlock. In particular, this
> happens when device is rfkilled on boot. To avoid deadlock, move
> power_on work canceling out of hci_dev_do_close/hci_dev_close_sync.
> 
> [...]

Here is the summary with links:
  - Bluetooth: core: Fix deadlock due to `cancel_work_sync(&hdev->power_on)` from hci_power_on_sync.
    https://git.kernel.org/netdev/net/c/e36bea6e78ab

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [PATCH] Bluetooth: core: Fix deadlock due to `cancel_work_sync(&hdev->power_on)` from hci_power_on_sync.
Posted by Mateusz Jończyk 3 years, 9 months ago 
W dniu 5.07.2022 o 14:59, Vasyl Vavrychuk pisze:
> `cancel_work_sync(&hdev->power_on)` was moved to hci_dev_close_sync in
> commit [1] to ensure that power_on work is canceled after HCI interface
> down.
>
> But, in certain cases power_on work function may call hci_dev_close_sync
> itself: hci_power_on -> hci_dev_do_close -> hci_dev_close_sync ->
> cancel_work_sync(&hdev->power_on), causing deadlock. In particular, this
> happens when device is rfkilled on boot. To avoid deadlock, move
> power_on work canceling out of hci_dev_do_close/hci_dev_close_sync.
>
> Deadlock introduced by commit [1] was reported in [2,3] as broken
> suspend. Suspend did not work because `hdev->req_lock` held as result of
> `power_on` work deadlock. In fact, other BT features were not working.
> It was not observed when testing [1] since it was verified without
> rfkill in place.
>
> NOTE: It is not needed to cancel power_on work from other places where
> hci_dev_do_close/hci_dev_close_sync is called in case:
> * Requests were serialized due to `hdev->req_workqueue`. The power_on
> work is first in that workqueue.
> * hci_rfkill_set_block which won't close device anyway until HCI_SETUP
> is on.
> * hci_sock_release which runs after hci_sock_bind which ensures
> HCI_SETUP was cleared.
>
> As result, behaviour is the same as in pre-dd06ed7 commit, except
> power_on work cancel added to hci_dev_close.
>
> [1]: commit dd06ed7ad057 ("Bluetooth: core: Fix missing power_on work cancel on HCI close")
> [2]: https://lore.kernel.org/lkml/20220614181706.26513-1-max.oss.09@gmail.com/
> [2]: https://lore.kernel.org/lkml/1236061d-95dd-c3ad-a38f-2dae7aae51ef@o2.pl/
>
> Fixes: commit dd06ed7ad057 ("Bluetooth: core: Fix missing power_on work cancel on HCI close")
> Signed-off-by: Vasyl Vavrychuk <vasyl.vavrychuk@opensynergy.com>
> Reported-by: Max Krummenacher <max.krummenacher@toradex.com>
> Reported-by: Mateusz Jonczyk <mat.jonczyk@o2.pl>

Works well: suspend (with bluetooth on and also off), hibernation, sending files, rfkill.

Thank you.

Reported-and-tested-by: Mateusz Jończyk <mat.jonczyk@o2.pl>

Greetings,

Mateusz Jończyk
Re: [PATCH] Bluetooth: core: Fix deadlock due to `cancel_work_sync(&hdev->power_on)` from hci_power_on_sync.
Posted by Francesco Dolcini 3 years, 9 months ago 
Hello Vasyl,

On Tue, Jul 05, 2022 at 03:59:31PM +0300, Vasyl Vavrychuk wrote:
> Fixes: commit dd06ed7ad057 ("Bluetooth: core: Fix missing power_on work cancel on HCI close")

This fixes tag is broken, dd06ed7ad057 does not exist on
torvalds/master, and the `commit` word should be removed.

Should be:

Fixes: ff7f2926114d ("Bluetooth: core: Fix missing power_on work cancel on HCI close")


Francesco
Re: [PATCH] Bluetooth: core: Fix deadlock due to `cancel_work_sync(&hdev->power_on)` from hci_power_on_sync.
Posted by Luiz Augusto von Dentz 3 years, 9 months ago 
Hi,

On Tue, Jul 5, 2022 at 8:14 AM Francesco Dolcini
<francesco.dolcini@toradex.com> wrote:
>
> Hello Vasyl,
>
> On Tue, Jul 05, 2022 at 03:59:31PM +0300, Vasyl Vavrychuk wrote:
> > Fixes: commit dd06ed7ad057 ("Bluetooth: core: Fix missing power_on work cancel on HCI close")
>
> This fixes tag is broken, dd06ed7ad057 does not exist on
> torvalds/master, and the `commit` word should be removed.
>
> Should be:
>
> Fixes: ff7f2926114d ("Bluetooth: core: Fix missing power_on work cancel on HCI close")

Ive rebased the patch on top of bluetooth-next and fixed the hash,
lets see if passes CI I might just go ahead and push it.

-- 
Luiz Augusto von Dentz
Re: [PATCH] Bluetooth: core: Fix deadlock due to `cancel_work_sync(&hdev->power_on)` from hci_power_on_sync.
Posted by Jakub Kicinski 3 years, 9 months ago 
On Tue, 5 Jul 2022 10:26:08 -0700 Luiz Augusto von Dentz wrote:
> On Tue, Jul 5, 2022 at 8:14 AM Francesco Dolcini
> <francesco.dolcini@toradex.com> wrote:
> >
> > Hello Vasyl,
> >
> > On Tue, Jul 05, 2022 at 03:59:31PM +0300, Vasyl Vavrychuk wrote:  
> > > Fixes: commit dd06ed7ad057 ("Bluetooth: core: Fix missing power_on work cancel on HCI close")  
> >
> > This fixes tag is broken, dd06ed7ad057 does not exist on
> > torvalds/master, and the `commit` word should be removed.
> >
> > Should be:
> >
> > Fixes: ff7f2926114d ("Bluetooth: core: Fix missing power_on work cancel on HCI close")  
> 
> Ive rebased the patch on top of bluetooth-next and fixed the hash,
> lets see if passes CI I might just go ahead and push it.

Thanks for pushing it along, the final version can got thru bluetooth ->
-> net and into 5.19, right?
Re: [PATCH] Bluetooth: core: Fix deadlock due to `cancel_work_sync(&hdev->power_on)` from hci_power_on_sync.
Posted by Luiz Augusto von Dentz 3 years, 9 months ago 
Hi Jakub,

On Tue, Jul 5, 2022 at 11:38 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 5 Jul 2022 10:26:08 -0700 Luiz Augusto von Dentz wrote:
> > On Tue, Jul 5, 2022 at 8:14 AM Francesco Dolcini
> > <francesco.dolcini@toradex.com> wrote:
> > >
> > > Hello Vasyl,
> > >
> > > On Tue, Jul 05, 2022 at 03:59:31PM +0300, Vasyl Vavrychuk wrote:
> > > > Fixes: commit dd06ed7ad057 ("Bluetooth: core: Fix missing power_on work cancel on HCI close")
> > >
> > > This fixes tag is broken, dd06ed7ad057 does not exist on
> > > torvalds/master, and the `commit` word should be removed.
> > >
> > > Should be:
> > >
> > > Fixes: ff7f2926114d ("Bluetooth: core: Fix missing power_on work cancel on HCI close")
> >
> > Ive rebased the patch on top of bluetooth-next and fixed the hash,
> > lets see if passes CI I might just go ahead and push it.
>
> Thanks for pushing it along, the final version can got thru bluetooth ->
> -> net and into 5.19, right?

Yep, I will send the pull request in a moment.

-- 
Luiz Augusto von Dentz
Re: [PATCH] Bluetooth: core: Fix deadlock due to `cancel_work_sync(&hdev->power_on)` from hci_power_on_sync.
Posted by Jakub Kicinski 3 years, 9 months ago 
On Tue, 5 Jul 2022 12:00:43 -0700 Luiz Augusto von Dentz wrote:
> > > Ive rebased the patch on top of bluetooth-next and fixed the hash,
> > > lets see if passes CI I might just go ahead and push it.  
> >
> > Thanks for pushing it along, the final version can got thru bluetooth ->  
> > -> net and into 5.19, right?  
> 
> Yep, I will send the pull request in a moment.

Perfect, thank you!!
Re: [PATCH] Bluetooth: core: Fix deadlock due to `cancel_work_sync(&hdev->power_on)` from hci_power_on_sync.
Posted by Max Krummenacher 3 years, 9 months ago 
On Tue, Jul 5, 2022 at 3:00 PM Vasyl Vavrychuk
<vasyl.vavrychuk@opensynergy.com> wrote:
>
> `cancel_work_sync(&hdev->power_on)` was moved to hci_dev_close_sync in
> commit [1] to ensure that power_on work is canceled after HCI interface
> down.
>
> But, in certain cases power_on work function may call hci_dev_close_sync
> itself: hci_power_on -> hci_dev_do_close -> hci_dev_close_sync ->
> cancel_work_sync(&hdev->power_on), causing deadlock. In particular, this
> happens when device is rfkilled on boot. To avoid deadlock, move
> power_on work canceling out of hci_dev_do_close/hci_dev_close_sync.
>
> Deadlock introduced by commit [1] was reported in [2,3] as broken
> suspend. Suspend did not work because `hdev->req_lock` held as result of
> `power_on` work deadlock. In fact, other BT features were not working.
> It was not observed when testing [1] since it was verified without
> rfkill in place.
>
> NOTE: It is not needed to cancel power_on work from other places where
> hci_dev_do_close/hci_dev_close_sync is called in case:
> * Requests were serialized due to `hdev->req_workqueue`. The power_on
> work is first in that workqueue.
> * hci_rfkill_set_block which won't close device anyway until HCI_SETUP
> is on.
> * hci_sock_release which runs after hci_sock_bind which ensures
> HCI_SETUP was cleared.
>
> As result, behaviour is the same as in pre-dd06ed7 commit, except
> power_on work cancel added to hci_dev_close.
>
> [1]: commit dd06ed7ad057 ("Bluetooth: core: Fix missing power_on work cancel on HCI close")
> [2]: https://lore.kernel.org/lkml/20220614181706.26513-1-max.oss.09@gmail.com/
> [2]: https://lore.kernel.org/lkml/1236061d-95dd-c3ad-a38f-2dae7aae51ef@o2.pl/
>
> Fixes: commit dd06ed7ad057 ("Bluetooth: core: Fix missing power_on work cancel on HCI close")
> Signed-off-by: Vasyl Vavrychuk <vasyl.vavrychuk@opensynergy.com>
> Reported-by: Max Krummenacher <max.krummenacher@toradex.com>
> Reported-by: Mateusz Jonczyk <mat.jonczyk@o2.pl>
> ---
>  net/bluetooth/hci_core.c | 3 +++
>  net/bluetooth/hci_sync.c | 1 -
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 59a5c1341c26..a0f99baafd35 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -571,6 +571,7 @@ int hci_dev_close(__u16 dev)
>                 goto done;
>         }
>
> +       cancel_work_sync(&hdev->power_on);
>         if (hci_dev_test_and_clear_flag(hdev, HCI_AUTO_OFF))
>                 cancel_delayed_work(&hdev->power_off);
>
> @@ -2675,6 +2676,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
>         list_del(&hdev->list);
>         write_unlock(&hci_dev_list_lock);
>
> +       cancel_work_sync(&hdev->power_on);
> +
>         hci_cmd_sync_clear(hdev);
>
>         if (!test_bit(HCI_QUIRK_NO_SUSPEND_NOTIFIER, &hdev->quirks))
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 286d6767f017..1739e8cb3291 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -4088,7 +4088,6 @@ int hci_dev_close_sync(struct hci_dev *hdev)
>
>         bt_dev_dbg(hdev, "");
>
> -       cancel_work_sync(&hdev->power_on);
>         cancel_delayed_work(&hdev->power_off);
>         cancel_delayed_work(&hdev->ncmd_timer);
>
> --
> 2.30.2
>

This fixes the issue I described in [1]. I.e. The kernel no longer
freezes while going to suspend.
Tested-by: Max Krummenacher <max.krummenacher@toradex.com>

Thanks!
Max

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.