2021-08-04 13:05:18

by Ammar Faizi

[permalink] [raw]
Subject: Re: WARNING: possible circular locking dependency detected (hci_sock_dev_event+0x17d/0x1f0)

See the original report here:
https://lore.kernel.org/lkml/[email protected]/

On 8/4/21 3:56 PM, Hillf Danton wrote:
> On Mon, 2 Aug 2021 20:17:02 +0700 Ammar Faizi wrote:
>> Hi everyone,
>>
>> I found a potential deadlock at hci_sock_dev_event.
>
> Thanks for your report.
>
> Can you try and see if reverting e305509e678b3a4af2b3cfd410f409f7cdaabb52
> ("Bluetooth: use correct lock to prevent UAF of hdev object") is a cure.
>

Hi Hillf,

Thank you for your response.

Here is what I have tried:
1. Reverted e305509e678b3a4af2b3cfd410f409f7cdaabb52.
2. Compile and install reverted version.
3. Rebooted 5.14.0-rc3 (c7d102232649, before revert) and try to
reproduce again. At this point I am able to reproduce it.
4. Booted 5.14.0-rc3 (the reverted version). The deadlock warning is
gone after reverting it. No more deadlock warning.

------------------------------------------------------------
[ 51207ee38ab65db86554655300a912e8c661525e is my local revert. ]
Revert diff:
ammarfaizi2@integral:~/project/now/linux$ git diff
c7d102232649226a69dddd58a4942cf13cff4f7c
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..590467345c5f 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -762,7 +762,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
/* Detach sockets from device */
read_lock(&hci_sk_list.lock);
sk_for_each(sk, &hci_sk_list.head) {
- lock_sock(sk);
+ bh_lock_sock_nested(sk);
if (hci_pi(sk)->hdev == hdev) {
hci_pi(sk)->hdev = NULL;
sk->sk_err = EPIPE;
@@ -771,7 +771,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)

hci_dev_put(hdev);
}
- release_sock(sk);
+ bh_unlock_sock(sk);
}
read_unlock(&hci_sk_list.lock);
}
ammarfaizi2@integral:~/project/now/linux$
------------------------------------------------------------

Summary:
Reverting e305509e678b3a4af2b3cfd410f409f7cdaabb52 is a cure.

Although it fixes the deadlock, there is probably something still goes
wrong regarding "prevent UAF" as the commit message says.

Will anyone create a patch for this?
I want to be the tester, I am new at kernel development and want to
start contributing, so please guide me :)

I also attached dmesg log before and after revert if you want to see the
full log.

Adding Lin as the author of e305509e678b3a4af2b3cfd410f409f7cdaabb52 to
CC list...


--
Ammar


Attachments:
dmesg_after_revert.txt (220.86 kB)
dmesg_before_revert.txt (228.17 kB)
Download all attachments

2021-08-06 12:01:40

by Anand K. Mistry

[permalink] [raw]
Subject: Re: WARNING: possible circular locking dependency detected (hci_sock_dev_event+0x17d/0x1f0)

On Wed, 4 Aug 2021 at 21:43, Ammar Faizi <[email protected]> wrote:
>
> See the original report here:
> https://lore.kernel.org/lkml/[email protected]/
>
> On 8/4/21 3:56 PM, Hillf Danton wrote:
> > On Mon, 2 Aug 2021 20:17:02 +0700 Ammar Faizi wrote:
> >> Hi everyone,
> >>
> >> I found a potential deadlock at hci_sock_dev_event.
> >
> > Thanks for your report.
> >
> > Can you try and see if reverting e305509e678b3a4af2b3cfd410f409f7cdaabb52
> > ("Bluetooth: use correct lock to prevent UAF of hdev object") is a cure.
> >
>
> Hi Hillf,
>
> Thank you for your response.
>
> Here is what I have tried:
> 1. Reverted e305509e678b3a4af2b3cfd410f409f7cdaabb52.
> 2. Compile and install reverted version.
> 3. Rebooted 5.14.0-rc3 (c7d102232649, before revert) and try to
> reproduce again. At this point I am able to reproduce it.
> 4. Booted 5.14.0-rc3 (the reverted version). The deadlock warning is
> gone after reverting it. No more deadlock warning.

Can you try patching in
https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=58ce6d5b271ab25fb2056f84a8e5546945eb5fc9
and see if that fixes it.

>
> ------------------------------------------------------------
> [ 51207ee38ab65db86554655300a912e8c661525e is my local revert. ]
> Revert diff:
> ammarfaizi2@integral:~/project/now/linux$ git diff
> c7d102232649226a69dddd58a4942cf13cff4f7c
> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> index b04a5a02ecf3..590467345c5f 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -762,7 +762,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
> /* Detach sockets from device */
> read_lock(&hci_sk_list.lock);
> sk_for_each(sk, &hci_sk_list.head) {
> - lock_sock(sk);
> + bh_lock_sock_nested(sk);
> if (hci_pi(sk)->hdev == hdev) {
> hci_pi(sk)->hdev = NULL;
> sk->sk_err = EPIPE;
> @@ -771,7 +771,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
>
> hci_dev_put(hdev);
> }
> - release_sock(sk);
> + bh_unlock_sock(sk);
> }
> read_unlock(&hci_sk_list.lock);
> }
> ammarfaizi2@integral:~/project/now/linux$
> ------------------------------------------------------------
>
> Summary:
> Reverting e305509e678b3a4af2b3cfd410f409f7cdaabb52 is a cure.
>
> Although it fixes the deadlock, there is probably something still goes
> wrong regarding "prevent UAF" as the commit message says.
>
> Will anyone create a patch for this?
> I want to be the tester, I am new at kernel development and want to
> start contributing, so please guide me :)
>
> I also attached dmesg log before and after revert if you want to see the
> full log.
>
> Adding Lin as the author of e305509e678b3a4af2b3cfd410f409f7cdaabb52 to
> CC list...
>
>
> --
> Ammar
>


--
Anand K. Mistry
Software Engineer
Google Australia