[PATCH v3] Bluetooth: defer cleanup of resources in hci_unregister_dev()

[PATCH v3] Bluetooth: defer cleanup of resources in hci_unregister_dev()

[Tetsuo Handa]

syzbot is hitting might_sleep() warning at hci_sock_dev_event()
due to calling lock_sock() with rw spinlock held [1].

It seems that history of this locking problem is a trial and error.

Commit b40df5743ee8aed8 ("[PATCH] bluetooth: fix socket locking in
hci_sock_dev_event()") in 2.6.21-rc4 changed bh_lock_sock() to lock_sock()
as an attempt to fix lockdep warning.

Then, commit 4ce61d1c7a8ef4c1 ("[BLUETOOTH]: Fix locking in
hci_sock_dev_event().") in 2.6.22-rc2 changed lock_sock() to
local_bh_disable() + bh_lock_sock_nested() as an attempt to fix
sleep in atomic context warning.

Then, commit 4b5dd696f81b210c ("Bluetooth: Remove local_bh_disable() from
hci_sock.c") in 3.3-rc1 removed local_bh_disable().

Then, commit e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF
of hdev object") in 5.13-rc5 again changed bh_lock_sock_nested() to
lock_sock() as an attempt to fix CVE-2021-3573.

This difficulty comes from current implementation that
hci_sock_dev_event(HCI_DEV_UNREG) is responsible for dropping all
references from sockets because hci_unregister_dev() immediately reclaims
resources as soon as returning from hci_sock_dev_event(HCI_DEV_UNREG).
But the history suggests that hci_sock_dev_event(HCI_DEV_UNREG) was not
doing what it should do.

Therefore, instead of trying to detach sockets from device, let's accept
not detaching sockets from device at hci_sock_dev_event(HCI_DEV_UNREG),
by moving actual cleanup of resources from hci_unregister_dev() to
hci_cleanup_dev() which is called by bt_host_release() when all references
to this unregistered device (which is a kobject) are gone.

Since hci_sock_dev_event(HCI_DEV_UNREG) no longer resets hci_pi(sk)->hdev,
we need to check whether this device was unregistered and return an error
based on HCI_UNREGISTER flag.

Link: https://syzkaller.appspot.com/bug?extid=a5df189917e79d5e59c9 [1]
Reported-by: syzbot <syzbot+a5df189917e79d5e59c9@syzkaller.appspotmail.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: syzbot <syzbot+a5df189917e79d5e59c9@syzkaller.appspotmail.com>
Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")
---
v3: Unexport hci_unregister_dev because only bt_host_release calls it
    and both hci_core.o and hci_sysfs.o are combined into bluetooth.o.

    Rename hci_release_dev to hci_cleanup_dev and call it only when
    HCI_UNREGISTER was set, for syzbot
    <syzbot+47c6d0efbb7fe2f7a5b8@syzkaller.appspotmail.com> is reporting
    that bt_host_release might be called without hci_register_dev and
    hci_unregister_dev.

    Return -EPIPE instead of 0 if HCI_UNREGISTER was set, in order to
    make sure that userspace finds that the device was unregistred.

v2: Add hci_release_dev and make bt_host_release call it instead of making
    bt_host_release public.

 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_core.c         | 16 ++++----
 net/bluetooth/hci_sock.c         | 67 +++++++++++++++++++++-----------
 net/bluetooth/hci_sysfs.c        |  3 ++
 4 files changed, 56 insertions(+), 31 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a53e94459ecd..db4312e44d47 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1230,6 +1230,7 @@ struct hci_dev *hci_alloc_dev(void);
 void hci_free_dev(struct hci_dev *hdev);
 int hci_register_dev(struct hci_dev *hdev);
 void hci_unregister_dev(struct hci_dev *hdev);
+void hci_cleanup_dev(struct hci_dev *hdev);
 int hci_suspend_dev(struct hci_dev *hdev);
 int hci_resume_dev(struct hci_dev *hdev);
 int hci_reset_dev(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 2560ed2f144d..e1a545c8a69f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3996,14 +3996,10 @@ EXPORT_SYMBOL(hci_register_dev);
 /* Unregister HCI device */
 void hci_unregister_dev(struct hci_dev *hdev)
 {
-	int id;
-
 	BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
 
 	hci_dev_set_flag(hdev, HCI_UNREGISTER);
 
-	id = hdev->id;
-
 	write_lock(&hci_dev_list_lock);
 	list_del(&hdev->list);
 	write_unlock(&hci_dev_list_lock);
@@ -4038,7 +4034,14 @@ void hci_unregister_dev(struct hci_dev *hdev)
 	}
 
 	device_del(&hdev->dev);
+	/* Actual cleanup is deferred until hci_cleanup_dev(). */
+	hci_dev_put(hdev);
+}
+EXPORT_SYMBOL(hci_unregister_dev);
 
+/* Cleanup HCI device */
+void hci_cleanup_dev(struct hci_dev *hdev)
+{
 	debugfs_remove_recursive(hdev->debugfs);
 	kfree_const(hdev->hw_info);
 	kfree_const(hdev->fw_info);
@@ -4063,11 +4066,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
 	hci_blocked_keys_clear(hdev);
 	hci_dev_unlock(hdev);
 
-	hci_dev_put(hdev);
-
-	ida_simple_remove(&hci_index_ida, id);
+	ida_simple_remove(&hci_index_ida, hdev->id);
 }
-EXPORT_SYMBOL(hci_unregister_dev);
 
 /* Suspend HCI device */
 int hci_suspend_dev(struct hci_dev *hdev)
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..39c03a8b762f 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -59,6 +59,17 @@ struct hci_pinfo {
 	char              comm[TASK_COMM_LEN];
 };
 
+static struct hci_dev *hci_hdev_from_sock(struct sock *sk)
+{
+	struct hci_dev *hdev = hci_pi(sk)->hdev;
+
+	if (!hdev)
+		return ERR_PTR(-EBADFD);
+	if (hci_dev_test_flag(hdev, HCI_UNREGISTER))
+		return ERR_PTR(-EPIPE);
+	return hdev;
+}
+
 void hci_sock_set_flag(struct sock *sk, int nr)
 {
 	set_bit(nr, &hci_pi(sk)->flags);
@@ -200,7 +211,7 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
 	sk_for_each(sk, &hci_sk_list.head) {
 		struct sk_buff *nskb;
 
-		if (sk->sk_state != BT_BOUND || hci_pi(sk)->hdev != hdev)
+		if (sk->sk_state != BT_BOUND || hci_hdev_from_sock(sk) != hdev)
 			continue;
 
 		/* Don't send frame to the socket it came from */
@@ -494,6 +505,7 @@ static struct sk_buff *create_monitor_ctrl_open(struct sock *sk)
 	u16 format;
 	u8 ver[3];
 	u32 flags;
+	struct hci_dev *hdev;
 
 	/* No message needed when cookie is not present */
 	if (!hci_pi(sk)->cookie)
@@ -536,8 +548,9 @@ static struct sk_buff *create_monitor_ctrl_open(struct sock *sk)
 
 	hdr = skb_push(skb, HCI_MON_HDR_SIZE);
 	hdr->opcode = cpu_to_le16(HCI_MON_CTRL_OPEN);
-	if (hci_pi(sk)->hdev)
-		hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id);
+	hdev = hci_hdev_from_sock(sk);
+	if (!IS_ERR(hdev))
+		hdr->index = cpu_to_le16(hdev->id);
 	else
 		hdr->index = cpu_to_le16(HCI_DEV_NONE);
 	hdr->len = cpu_to_le16(skb->len - HCI_MON_HDR_SIZE);
@@ -549,6 +562,7 @@ static struct sk_buff *create_monitor_ctrl_close(struct sock *sk)
 {
 	struct hci_mon_hdr *hdr;
 	struct sk_buff *skb;
+	struct hci_dev *hdev;
 
 	/* No message needed when cookie is not present */
 	if (!hci_pi(sk)->cookie)
@@ -574,8 +588,9 @@ static struct sk_buff *create_monitor_ctrl_close(struct sock *sk)
 
 	hdr = skb_push(skb, HCI_MON_HDR_SIZE);
 	hdr->opcode = cpu_to_le16(HCI_MON_CTRL_CLOSE);
-	if (hci_pi(sk)->hdev)
-		hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id);
+	hdev = hci_hdev_from_sock(sk);
+	if (!IS_ERR(hdev))
+		hdr->index = cpu_to_le16(hdev->id);
 	else
 		hdr->index = cpu_to_le16(HCI_DEV_NONE);
 	hdr->len = cpu_to_le16(skb->len - HCI_MON_HDR_SIZE);
@@ -759,19 +774,13 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 	if (event == HCI_DEV_UNREG) {
 		struct sock *sk;
 
-		/* Detach sockets from device */
+		/* Wake up sockets using this dead device */
 		read_lock(&hci_sk_list.lock);
 		sk_for_each(sk, &hci_sk_list.head) {
-			lock_sock(sk);
 			if (hci_pi(sk)->hdev == hdev) {
-				hci_pi(sk)->hdev = NULL;
 				sk->sk_err = EPIPE;
-				sk->sk_state = BT_OPEN;
 				sk->sk_state_change(sk);
-
-				hci_dev_put(hdev);
 			}
-			release_sock(sk);
 		}
 		read_unlock(&hci_sk_list.lock);
 	}
@@ -930,10 +939,10 @@ static int hci_sock_reject_list_del(struct hci_dev *hdev, void __user *arg)
 static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd,
 				unsigned long arg)
 {
-	struct hci_dev *hdev = hci_pi(sk)->hdev;
+	struct hci_dev *hdev = hci_hdev_from_sock(sk);
 
-	if (!hdev)
-		return -EBADFD;
+	if (IS_ERR(hdev))
+		return PTR_ERR(hdev);
 
 	if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
 		return -EBUSY;
@@ -1103,6 +1112,18 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
 
 	lock_sock(sk);
 
+	/* Allow detaching from dead device and attaching to alive device, if
+	 * the caller wants to re-bind (instead of close) this socket in
+	 * response to hci_sock_dev_event(HCI_DEV_UNREG) notification.
+	 */
+	hdev = hci_pi(sk)->hdev;
+	if (hdev && hci_dev_test_flag(hdev, HCI_UNREGISTER)) {
+		hci_pi(sk)->hdev = NULL;
+		sk->sk_state = BT_OPEN;
+		hci_dev_put(hdev);
+		hdev = NULL;
+	}
+
 	if (sk->sk_state == BT_BOUND) {
 		err = -EALREADY;
 		goto done;
@@ -1110,7 +1131,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
 
 	switch (haddr.hci_channel) {
 	case HCI_CHANNEL_RAW:
-		if (hci_pi(sk)->hdev) {
+		if (hdev) {
 			err = -EALREADY;
 			goto done;
 		}
@@ -1157,7 +1178,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
 		break;
 
 	case HCI_CHANNEL_USER:
-		if (hci_pi(sk)->hdev) {
+		if (hdev) {
 			err = -EALREADY;
 			goto done;
 		}
@@ -1379,9 +1400,9 @@ static int hci_sock_getname(struct socket *sock, struct sockaddr *addr,
 
 	lock_sock(sk);
 
-	hdev = hci_pi(sk)->hdev;
-	if (!hdev) {
-		err = -EBADFD;
+	hdev = hci_hdev_from_sock(sk);
+	if (IS_ERR(hdev)) {
+		err = PTR_ERR(hdev);
 		goto done;
 	}
 
@@ -1743,9 +1764,9 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 		goto done;
 	}
 
-	hdev = hci_pi(sk)->hdev;
-	if (!hdev) {
-		err = -EBADFD;
+	hdev = hci_hdev_from_sock(sk);
+	if (IS_ERR(hdev)) {
+		err = PTR_ERR(hdev);
 		goto done;
 	}
 
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 9874844a95a9..b69d88b88d2e 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -83,6 +83,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn)
 static void bt_host_release(struct device *dev)
 {
 	struct hci_dev *hdev = to_hci_dev(dev);
+
+	if (hci_dev_test_flag(hdev, HCI_UNREGISTER))
+		hci_cleanup_dev(hdev);
 	kfree(hdev);
 	module_put(THIS_MODULE);
 }
-- 
2.18.4

Re: [PATCH v3] Bluetooth: defer cleanup of resources in hci_unregister_dev()

[Linus Torvalds]

On Mon, Aug 2, 2021 at 8:19 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> @@ -200,7 +211,7 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
>         sk_for_each(sk, &hci_sk_list.head) {
>                 struct sk_buff *nskb;
>
> -               if (sk->sk_state != BT_BOUND || hci_pi(sk)->hdev != hdev)
> +               if (sk->sk_state != BT_BOUND || hci_hdev_from_sock(sk) != hdev)
>                         continue;
>
>                 /* Don't send frame to the socket it came from */

I'm not convinced this is a good idea.

Here we use hci_hdev_from_sock() outside the socket lock, so now that
HCI_UNREGISTER test is not very logical.

And it's positively stupid to start with a hdev, then walk the socket
list to look up another hdev, and then check "was it the original
hdev, but not unregistered"?

In other words - if you care about HCI_UNREGISTER state, it should be
*above* the whole loop. Not inside of it.

So I think this test should remain just that

     hci_pi(sk)->hdev != hdev

and if HCI_UNREGISTER is relevant - and I don't think it is - it
should have been done earlier.

> @@ -536,8 +548,9 @@ static struct sk_buff *create_monitor_ctrl_open(struct sock *sk)
>
>         hdr = skb_push(skb, HCI_MON_HDR_SIZE);
>         hdr->opcode = cpu_to_le16(HCI_MON_CTRL_OPEN);
> -       if (hci_pi(sk)->hdev)
> -               hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id);
> +       hdev = hci_hdev_from_sock(sk);
> +       if (!IS_ERR(hdev))
> +               hdr->index = cpu_to_le16(hdev->id);

Same deal. The 'id' is valid even if it's unregistered.

So either it should have generated an error earlier, or we just shouldn't care.

The fact that the old code used to do HCI_DEV_NONE seems to be more
about the fact that the ID no longer existed.  I think that if you
want to monitor a socket with a stale hdev, that's likely pointless
but valid.

So I think this should just keep using the hdev->id, even for a
HCI_UNREGISTER case.

That said, the *normal* case seems to be from the socket ioctl code,
so it's either explicitly not yet registered, or it just got
registered with a hdev, so I don't think it even matters. It looks
like the only case that HCI_UNREGISTER case would happen is likely the
magical replay case.

Which - again - I think would actually prefer the now still existing
hdev channel id.


> @@ -574,8 +588,9 @@ static struct sk_buff *create_monitor_ctrl_close(struct sock *sk)
>
>         hdr = skb_push(skb, HCI_MON_HDR_SIZE);
>         hdr->opcode = cpu_to_le16(HCI_MON_CTRL_CLOSE);
> -       if (hci_pi(sk)->hdev)
> -               hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id);
> +       hdev = hci_hdev_from_sock(sk);
> +       if (!IS_ERR(hdev))
> +               hdr->index = cpu_to_le16(hdev->id);
>         else
>                 hdr->index = cpu_to_le16(HCI_DEV_NONE);
>         hdr->len = cpu_to_le16(skb->len - HCI_MON_HDR_SIZE);

I think the monitor_ctrl close case is exactly the same case as the
open case above.

But the others look good to me.

So I *think* that the cases you actually want to catch are just the
ioctl/recvmsg/sendmsg ones. Not the special "monitor the hdev" ones.

That said, if you have some test-case that argues differently, then
hard data is obviously more valid than my blathering above.

                 Linus

Re: [PATCH v3] Bluetooth: defer cleanup of resources in hci_unregister_dev()

[Tetsuo Handa]

On 2021/08/03 2:36, Linus Torvalds wrote:
> On Mon, Aug 2, 2021 at 8:19 AM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>
>> @@ -200,7 +211,7 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
>>         sk_for_each(sk, &hci_sk_list.head) {
>>                 struct sk_buff *nskb;
>>
>> -               if (sk->sk_state != BT_BOUND || hci_pi(sk)->hdev != hdev)
>> +               if (sk->sk_state != BT_BOUND || hci_hdev_from_sock(sk) != hdev)
>>                         continue;
>>
>>                 /* Don't send frame to the socket it came from */
> 
> I'm not convinced this is a good idea.

Well, you are right.

One caller (out of three callers) of hci_send_to_sock() is hci_si_event() from
hci_sock_dev_event(event <= HCI_DEV_DOWN) path.

This change for hci_si_event() path is wrong, for hci_si_event() calls
hci_send_to_sock() with hdev == NULL while hci_hdev_from_sock() != NULL.

  if (sk->sk_state != BT_BOUND || 1)
	continue;

makes whole hci_send_to_sock(NULL) no-op. However,

> 
> Here we use hci_hdev_from_sock() outside the socket lock, so now that
> HCI_UNREGISTER test is not very logical.
> 
> And it's positively stupid to start with a hdev, then walk the socket
> list to look up another hdev, and then check "was it the original
> hdev, but not unregistered"?
> 
> In other words - if you care about HCI_UNREGISTER state, it should be
> *above* the whole loop. Not inside of it.
> 
> So I think this test should remain just that
> 
>      hci_pi(sk)->hdev != hdev
> 
> and if HCI_UNREGISTER is relevant - and I don't think it is - it
> should have been done earlier.

now that this patch is going to remove

	sk->sk_state = BT_OPEN;
	hci_pi(sk)->hdev = NULL;

 from the sk_for_each(sk, &hci_sk_list.head) loop in hci_sock_dev_event(HCI_DEV_UNREG),
hci_sock_dev_event(event <= HCI_DEV_DOWN) after hci_sock_dev_event(HCI_DEV_UNREG) will
fail to hit the "continue;" path.


The other two callers of hci_send_to_sock() are hci_send_frame()
and hci_rx_work(). These hdev are not NULL but are subjected to
hci_sock_dev_event(HCI_DEV_UNREG) race.

Prior to this patch,

  if (sk->sk_state != BT_BOUND || hci_pi(sk)->hdev != hdev)
	continue;

was not hitting "continue;" path before hci_sock_dev_event(HCI_DEV_UNREG)
and was hitting "continue;" path after hci_sock_dev_event(HCI_DEV_UNREG),
and the same problem (i.e. failing to hit "continue;" path) exists.



Do we want to block sending frames to RAW sockets which are using dead device?

If yes, doing

  if (sk->sk_state != BT_BOUND || hci_pi(sk)->hdev != hdev ||
      (hci_pi(sk)->hdev && hci_dev_test_flag(hci_pi(sk)->hdev, HCI_UNREGISTER)))
	continue;

will block sending frames from hci_sock_dev_event(event <= HCI_DEV_DOWN) because
hci_dev_set_flag(hdev, HCI_UNREGISTER) was already called by hci_unregister_dev().

If no, just keeping

  if (sk->sk_state != BT_BOUND || hci_pi(sk)->hdev != hdev)
	continue;

is correct.



> 
>> @@ -536,8 +548,9 @@ static struct sk_buff *create_monitor_ctrl_open(struct sock *sk)
>>
>>         hdr = skb_push(skb, HCI_MON_HDR_SIZE);
>>         hdr->opcode = cpu_to_le16(HCI_MON_CTRL_OPEN);
>> -       if (hci_pi(sk)->hdev)
>> -               hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id);

Prior to this patch, compiler might have generated code that reads
hci_pi(sk)->hdev for two times, and hci_sock_dev_event(HCI_DEV_UNREG)
might reset hci_pi(sk)->hdev to NULL between these two reads; leading to
NULL pointer dereference.

>> +       hdev = hci_hdev_from_sock(sk);
>> +       if (!IS_ERR(hdev))
>> +               hdr->index = cpu_to_le16(hdev->id);
> 
> Same deal. The 'id' is valid even if it's unregistered.
> 
> So either it should have generated an error earlier, or we just shouldn't care.

Well, maybe

	hdev = READ_ONCE(hci_pi(sk)->hdev);
	if (!hdev)
		hdr->index = cpu_to_le16(hdev->id);

is the better for create_monitor_ctrl_open() and create_monitor_ctrl_close(), for
it seems that create_monitor_ctrl_close() should report the same id. However,
create_monitor_ctrl_close() from hci_sock_bind() is always using HCI_DEV_NONE
because it is reachable only if hci_pi(sk)->hdev == NULL.

Only create_monitor_ctrl_close() from hci_sock_release() has possibility
of being able to use hdev->id rather than HCI_DEV_NONE. And

> 
> The fact that the old code used to do HCI_DEV_NONE seems to be more
> about the fact that the ID no longer existed.  I think that if you
> want to monitor a socket with a stale hdev, that's likely pointless
> but valid.
> 
> So I think this should just keep using the hdev->id, even for a
> HCI_UNREGISTER case.
> 
> That said, the *normal* case seems to be from the socket ioctl code,
> so it's either explicitly not yet registered, or it just got
> registered with a hdev, so I don't think it even matters. It looks
> like the only case that HCI_UNREGISTER case would happen is likely the
> magical replay case.
> 
> Which - again - I think would actually prefer the now still existing
> hdev channel id.

prior to this patch, hci_sock_dev_event(HCI_DEV_UNREG) might have suddenly
reset hci_pi(sk)->hdev to NULL, and create_monitor_ctrl_close() was forced
to choose HCI_DEV_NONE rather than the id create_monitor_ctrl_open() used.

Therefore, I think that the userspace needs to be designed to tolerate erroneous
HCI_DEV_NONE and take appropriate recovery action if HCI_DEV_NONE was reported.

So, I can't judge whether changing to

	hdev = hci_hdev_from_sock(sk);
	if (!IS_ERR(hdev))
		hdr->index = cpu_to_le16(hdev->id);

makes things better or worse.


> 
> 
>> @@ -574,8 +588,9 @@ static struct sk_buff *create_monitor_ctrl_close(struct sock *sk)
>>
>>         hdr = skb_push(skb, HCI_MON_HDR_SIZE);
>>         hdr->opcode = cpu_to_le16(HCI_MON_CTRL_CLOSE);
>> -       if (hci_pi(sk)->hdev)
>> -               hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id);
>> +       hdev = hci_hdev_from_sock(sk);
>> +       if (!IS_ERR(hdev))
>> +               hdr->index = cpu_to_le16(hdev->id);
>>         else
>>                 hdr->index = cpu_to_le16(HCI_DEV_NONE);
>>         hdr->len = cpu_to_le16(skb->len - HCI_MON_HDR_SIZE);
> 
> I think the monitor_ctrl close case is exactly the same case as the
> open case above.
> 
> But the others look good to me.
> 
> So I *think* that the cases you actually want to catch are just the
> ioctl/recvmsg/sendmsg ones. Not the special "monitor the hdev" ones.
> 
> That said, if you have some test-case that argues differently, then
> hard data is obviously more valid than my blathering above.

The special "monitor the hdev" is always racy.
But "monitor the hdev" change is too subtle to handle within this patch.
Leaving "monitor the hdev" change to future patches might be the better.

I want opinions from Bluetooth developers.

[PATCH v4] Bluetooth: defer cleanup of resources in hci_unregister_dev()

[Tetsuo Handa]

syzbot is hitting might_sleep() warning at hci_sock_dev_event()
due to calling lock_sock() with rw spinlock held [1].

It seems that history of this locking problem is a trial and error.

Commit b40df5743ee8aed8 ("[PATCH] bluetooth: fix socket locking in
hci_sock_dev_event()") in 2.6.21-rc4 changed bh_lock_sock() to lock_sock()
as an attempt to fix lockdep warning.

Then, commit 4ce61d1c7a8ef4c1 ("[BLUETOOTH]: Fix locking in
hci_sock_dev_event().") in 2.6.22-rc2 changed lock_sock() to
local_bh_disable() + bh_lock_sock_nested() as an attempt to fix
sleep in atomic context warning.

Then, commit 4b5dd696f81b210c ("Bluetooth: Remove local_bh_disable() from
hci_sock.c") in 3.3-rc1 removed local_bh_disable().

Then, commit e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF
of hdev object") in 5.13-rc5 again changed bh_lock_sock_nested() to
lock_sock() as an attempt to fix CVE-2021-3573.

This difficulty comes from current implementation that
hci_sock_dev_event(HCI_DEV_UNREG) is responsible for dropping all
references from sockets because hci_unregister_dev() immediately reclaims
resources as soon as returning from hci_sock_dev_event(HCI_DEV_UNREG).
But the history suggests that hci_sock_dev_event(HCI_DEV_UNREG) was not
doing what it should do.

Therefore, instead of trying to detach sockets from device, let's accept
not detaching sockets from device at hci_sock_dev_event(HCI_DEV_UNREG),
by moving actual cleanup of resources from hci_unregister_dev() to
hci_cleanup_dev() which is called by bt_host_release() when all references
to this unregistered device (which is a kobject) are gone.

Since hci_sock_dev_event(HCI_DEV_UNREG) no longer resets hci_pi(sk)->hdev,
we need to check whether this device was unregistered and return an error
based on HCI_UNREGISTER flag. There might be subtle behavioral difference
in "monitor the hdev" functionality; please report if you found something
went wrong due to this patch.

Link: https://syzkaller.appspot.com/bug?extid=a5df189917e79d5e59c9 [1]
Reported-by: syzbot <syzbot+a5df189917e79d5e59c9@syzkaller.appspotmail.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")
---
v4: Don't touch "monitor the hdev" functions.

v3: Unexport hci_unregister_dev because only bt_host_release calls it
    and both hci_core.o and hci_sysfs.o are combined into bluetooth.o.

    Rename hci_release_dev to hci_cleanup_dev and call it only when
    HCI_UNREGISTER was set, for syzbot
    <syzbot+47c6d0efbb7fe2f7a5b8@syzkaller.appspotmail.com> is reporting
    that bt_host_release might be called without hci_register_dev and
    hci_unregister_dev.

    Return -EPIPE instead of 0 if HCI_UNREGISTER was set, in order to
    make sure that userspace finds that the device was unregistred.

v2: Add hci_release_dev and make bt_host_release call it instead of making
    bt_host_release public.

 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_core.c         | 16 +++++------
 net/bluetooth/hci_sock.c         | 49 +++++++++++++++++++++-----------
 net/bluetooth/hci_sysfs.c        |  3 ++
 4 files changed, 45 insertions(+), 24 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a53e94459ecd..db4312e44d47 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1230,6 +1230,7 @@ struct hci_dev *hci_alloc_dev(void);
 void hci_free_dev(struct hci_dev *hdev);
 int hci_register_dev(struct hci_dev *hdev);
 void hci_unregister_dev(struct hci_dev *hdev);
+void hci_cleanup_dev(struct hci_dev *hdev);
 int hci_suspend_dev(struct hci_dev *hdev);
 int hci_resume_dev(struct hci_dev *hdev);
 int hci_reset_dev(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 2560ed2f144d..e1a545c8a69f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3996,14 +3996,10 @@ EXPORT_SYMBOL(hci_register_dev);
 /* Unregister HCI device */
 void hci_unregister_dev(struct hci_dev *hdev)
 {
-	int id;
-
 	BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
 
 	hci_dev_set_flag(hdev, HCI_UNREGISTER);
 
-	id = hdev->id;
-
 	write_lock(&hci_dev_list_lock);
 	list_del(&hdev->list);
 	write_unlock(&hci_dev_list_lock);
@@ -4038,7 +4034,14 @@ void hci_unregister_dev(struct hci_dev *hdev)
 	}
 
 	device_del(&hdev->dev);
+	/* Actual cleanup is deferred until hci_cleanup_dev(). */
+	hci_dev_put(hdev);
+}
+EXPORT_SYMBOL(hci_unregister_dev);
 
+/* Cleanup HCI device */
+void hci_cleanup_dev(struct hci_dev *hdev)
+{
 	debugfs_remove_recursive(hdev->debugfs);
 	kfree_const(hdev->hw_info);
 	kfree_const(hdev->fw_info);
@@ -4063,11 +4066,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
 	hci_blocked_keys_clear(hdev);
 	hci_dev_unlock(hdev);
 
-	hci_dev_put(hdev);
-
-	ida_simple_remove(&hci_index_ida, id);
+	ida_simple_remove(&hci_index_ida, hdev->id);
 }
-EXPORT_SYMBOL(hci_unregister_dev);
 
 /* Suspend HCI device */
 int hci_suspend_dev(struct hci_dev *hdev)
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..f1128c2134f0 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -59,6 +59,17 @@ struct hci_pinfo {
 	char              comm[TASK_COMM_LEN];
 };
 
+static struct hci_dev *hci_hdev_from_sock(struct sock *sk)
+{
+	struct hci_dev *hdev = hci_pi(sk)->hdev;
+
+	if (!hdev)
+		return ERR_PTR(-EBADFD);
+	if (hci_dev_test_flag(hdev, HCI_UNREGISTER))
+		return ERR_PTR(-EPIPE);
+	return hdev;
+}
+
 void hci_sock_set_flag(struct sock *sk, int nr)
 {
 	set_bit(nr, &hci_pi(sk)->flags);
@@ -759,19 +770,13 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 	if (event == HCI_DEV_UNREG) {
 		struct sock *sk;
 
-		/* Detach sockets from device */
+		/* Wake up sockets using this dead device */
 		read_lock(&hci_sk_list.lock);
 		sk_for_each(sk, &hci_sk_list.head) {
-			lock_sock(sk);
 			if (hci_pi(sk)->hdev == hdev) {
-				hci_pi(sk)->hdev = NULL;
 				sk->sk_err = EPIPE;
-				sk->sk_state = BT_OPEN;
 				sk->sk_state_change(sk);
-
-				hci_dev_put(hdev);
 			}
-			release_sock(sk);
 		}
 		read_unlock(&hci_sk_list.lock);
 	}
@@ -930,10 +935,10 @@ static int hci_sock_reject_list_del(struct hci_dev *hdev, void __user *arg)
 static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd,
 				unsigned long arg)
 {
-	struct hci_dev *hdev = hci_pi(sk)->hdev;
+	struct hci_dev *hdev = hci_hdev_from_sock(sk);
 
-	if (!hdev)
-		return -EBADFD;
+	if (IS_ERR(hdev))
+		return PTR_ERR(hdev);
 
 	if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
 		return -EBUSY;
@@ -1103,6 +1108,18 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
 
 	lock_sock(sk);
 
+	/* Allow detaching from dead device and attaching to alive device, if
+	 * the caller wants to re-bind (instead of close) this socket in
+	 * response to hci_sock_dev_event(HCI_DEV_UNREG) notification.
+	 */
+	hdev = hci_pi(sk)->hdev;
+	if (hdev && hci_dev_test_flag(hdev, HCI_UNREGISTER)) {
+		hci_pi(sk)->hdev = NULL;
+		sk->sk_state = BT_OPEN;
+		hci_dev_put(hdev);
+	}
+	hdev = NULL;
+
 	if (sk->sk_state == BT_BOUND) {
 		err = -EALREADY;
 		goto done;
@@ -1379,9 +1396,9 @@ static int hci_sock_getname(struct socket *sock, struct sockaddr *addr,
 
 	lock_sock(sk);
 
-	hdev = hci_pi(sk)->hdev;
-	if (!hdev) {
-		err = -EBADFD;
+	hdev = hci_hdev_from_sock(sk);
+	if (IS_ERR(hdev)) {
+		err = PTR_ERR(hdev);
 		goto done;
 	}
 
@@ -1743,9 +1760,9 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 		goto done;
 	}
 
-	hdev = hci_pi(sk)->hdev;
-	if (!hdev) {
-		err = -EBADFD;
+	hdev = hci_hdev_from_sock(sk);
+	if (IS_ERR(hdev)) {
+		err = PTR_ERR(hdev);
 		goto done;
 	}
 
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 9874844a95a9..b69d88b88d2e 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -83,6 +83,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn)
 static void bt_host_release(struct device *dev)
 {
 	struct hci_dev *hdev = to_hci_dev(dev);
+
+	if (hci_dev_test_flag(hdev, HCI_UNREGISTER))
+		hci_cleanup_dev(hdev);
 	kfree(hdev);
 	module_put(THIS_MODULE);
 }
-- 
2.18.4

Re: [PATCH v4] Bluetooth: defer cleanup of resources in hci_unregister_dev()

[Luiz Augusto von Dentz]

Hi Tetsuo,

On Wed, Aug 4, 2021 at 3:27 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> syzbot is hitting might_sleep() warning at hci_sock_dev_event()
> due to calling lock_sock() with rw spinlock held [1].
>
> It seems that history of this locking problem is a trial and error.
>
> Commit b40df5743ee8aed8 ("[PATCH] bluetooth: fix socket locking in
> hci_sock_dev_event()") in 2.6.21-rc4 changed bh_lock_sock() to lock_sock()
> as an attempt to fix lockdep warning.
>
> Then, commit 4ce61d1c7a8ef4c1 ("[BLUETOOTH]: Fix locking in
> hci_sock_dev_event().") in 2.6.22-rc2 changed lock_sock() to
> local_bh_disable() + bh_lock_sock_nested() as an attempt to fix
> sleep in atomic context warning.
>
> Then, commit 4b5dd696f81b210c ("Bluetooth: Remove local_bh_disable() from
> hci_sock.c") in 3.3-rc1 removed local_bh_disable().
>
> Then, commit e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF
> of hdev object") in 5.13-rc5 again changed bh_lock_sock_nested() to
> lock_sock() as an attempt to fix CVE-2021-3573.
>
> This difficulty comes from current implementation that
> hci_sock_dev_event(HCI_DEV_UNREG) is responsible for dropping all
> references from sockets because hci_unregister_dev() immediately reclaims
> resources as soon as returning from hci_sock_dev_event(HCI_DEV_UNREG).
> But the history suggests that hci_sock_dev_event(HCI_DEV_UNREG) was not
> doing what it should do.
>
> Therefore, instead of trying to detach sockets from device, let's accept
> not detaching sockets from device at hci_sock_dev_event(HCI_DEV_UNREG),
> by moving actual cleanup of resources from hci_unregister_dev() to
> hci_cleanup_dev() which is called by bt_host_release() when all references
> to this unregistered device (which is a kobject) are gone.
>
> Since hci_sock_dev_event(HCI_DEV_UNREG) no longer resets hci_pi(sk)->hdev,
> we need to check whether this device was unregistered and return an error
> based on HCI_UNREGISTER flag. There might be subtle behavioral difference
> in "monitor the hdev" functionality; please report if you found something
> went wrong due to this patch.
>
> Link: https://syzkaller.appspot.com/bug?extid=a5df189917e79d5e59c9 [1]
> Reported-by: syzbot <syzbot+a5df189917e79d5e59c9@syzkaller.appspotmail.com>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")
> ---
> v4: Don't touch "monitor the hdev" functions.
>
> v3: Unexport hci_unregister_dev because only bt_host_release calls it
>     and both hci_core.o and hci_sysfs.o are combined into bluetooth.o.
>
>     Rename hci_release_dev to hci_cleanup_dev and call it only when
>     HCI_UNREGISTER was set, for syzbot
>     <syzbot+47c6d0efbb7fe2f7a5b8@syzkaller.appspotmail.com> is reporting
>     that bt_host_release might be called without hci_register_dev and
>     hci_unregister_dev.
>
>     Return -EPIPE instead of 0 if HCI_UNREGISTER was set, in order to
>     make sure that userspace finds that the device was unregistred.
>
> v2: Add hci_release_dev and make bt_host_release call it instead of making
>     bt_host_release public.
>
>  include/net/bluetooth/hci_core.h |  1 +
>  net/bluetooth/hci_core.c         | 16 +++++------
>  net/bluetooth/hci_sock.c         | 49 +++++++++++++++++++++-----------
>  net/bluetooth/hci_sysfs.c        |  3 ++
>  4 files changed, 45 insertions(+), 24 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index a53e94459ecd..db4312e44d47 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1230,6 +1230,7 @@ struct hci_dev *hci_alloc_dev(void);
>  void hci_free_dev(struct hci_dev *hdev);
>  int hci_register_dev(struct hci_dev *hdev);
>  void hci_unregister_dev(struct hci_dev *hdev);
> +void hci_cleanup_dev(struct hci_dev *hdev);
>  int hci_suspend_dev(struct hci_dev *hdev);
>  int hci_resume_dev(struct hci_dev *hdev);
>  int hci_reset_dev(struct hci_dev *hdev);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 2560ed2f144d..e1a545c8a69f 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3996,14 +3996,10 @@ EXPORT_SYMBOL(hci_register_dev);
>  /* Unregister HCI device */
>  void hci_unregister_dev(struct hci_dev *hdev)
>  {
> -       int id;
> -
>         BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
>
>         hci_dev_set_flag(hdev, HCI_UNREGISTER);
>
> -       id = hdev->id;
> -
>         write_lock(&hci_dev_list_lock);
>         list_del(&hdev->list);
>         write_unlock(&hci_dev_list_lock);
> @@ -4038,7 +4034,14 @@ void hci_unregister_dev(struct hci_dev *hdev)
>         }
>
>         device_del(&hdev->dev);
> +       /* Actual cleanup is deferred until hci_cleanup_dev(). */
> +       hci_dev_put(hdev);
> +}
> +EXPORT_SYMBOL(hci_unregister_dev);
>
> +/* Cleanup HCI device */
> +void hci_cleanup_dev(struct hci_dev *hdev)
> +{
>         debugfs_remove_recursive(hdev->debugfs);
>         kfree_const(hdev->hw_info);
>         kfree_const(hdev->fw_info);
> @@ -4063,11 +4066,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
>         hci_blocked_keys_clear(hdev);
>         hci_dev_unlock(hdev);
>
> -       hci_dev_put(hdev);
> -
> -       ida_simple_remove(&hci_index_ida, id);
> +       ida_simple_remove(&hci_index_ida, hdev->id);
>  }
> -EXPORT_SYMBOL(hci_unregister_dev);
>
>  /* Suspend HCI device */
>  int hci_suspend_dev(struct hci_dev *hdev)
> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> index b04a5a02ecf3..f1128c2134f0 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -59,6 +59,17 @@ struct hci_pinfo {
>         char              comm[TASK_COMM_LEN];
>  };
>
> +static struct hci_dev *hci_hdev_from_sock(struct sock *sk)
> +{
> +       struct hci_dev *hdev = hci_pi(sk)->hdev;
> +
> +       if (!hdev)
> +               return ERR_PTR(-EBADFD);
> +       if (hci_dev_test_flag(hdev, HCI_UNREGISTER))
> +               return ERR_PTR(-EPIPE);
> +       return hdev;
> +}
> +
>  void hci_sock_set_flag(struct sock *sk, int nr)
>  {
>         set_bit(nr, &hci_pi(sk)->flags);
> @@ -759,19 +770,13 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
>         if (event == HCI_DEV_UNREG) {
>                 struct sock *sk;
>
> -               /* Detach sockets from device */
> +               /* Wake up sockets using this dead device */
>                 read_lock(&hci_sk_list.lock);
>                 sk_for_each(sk, &hci_sk_list.head) {
> -                       lock_sock(sk);
>                         if (hci_pi(sk)->hdev == hdev) {
> -                               hci_pi(sk)->hdev = NULL;
>                                 sk->sk_err = EPIPE;
> -                               sk->sk_state = BT_OPEN;
>                                 sk->sk_state_change(sk);
> -
> -                               hci_dev_put(hdev);
>                         }
> -                       release_sock(sk);
>                 }
>                 read_unlock(&hci_sk_list.lock);
>         }
> @@ -930,10 +935,10 @@ static int hci_sock_reject_list_del(struct hci_dev *hdev, void __user *arg)
>  static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd,
>                                 unsigned long arg)
>  {
> -       struct hci_dev *hdev = hci_pi(sk)->hdev;
> +       struct hci_dev *hdev = hci_hdev_from_sock(sk);
>
> -       if (!hdev)
> -               return -EBADFD;
> +       if (IS_ERR(hdev))
> +               return PTR_ERR(hdev);
>
>         if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
>                 return -EBUSY;
> @@ -1103,6 +1108,18 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
>
>         lock_sock(sk);
>
> +       /* Allow detaching from dead device and attaching to alive device, if
> +        * the caller wants to re-bind (instead of close) this socket in
> +        * response to hci_sock_dev_event(HCI_DEV_UNREG) notification.
> +        */
> +       hdev = hci_pi(sk)->hdev;
> +       if (hdev && hci_dev_test_flag(hdev, HCI_UNREGISTER)) {
> +               hci_pi(sk)->hdev = NULL;
> +               sk->sk_state = BT_OPEN;
> +               hci_dev_put(hdev);
> +       }
> +       hdev = NULL;
> +
>         if (sk->sk_state == BT_BOUND) {
>                 err = -EALREADY;
>                 goto done;
> @@ -1379,9 +1396,9 @@ static int hci_sock_getname(struct socket *sock, struct sockaddr *addr,
>
>         lock_sock(sk);
>
> -       hdev = hci_pi(sk)->hdev;
> -       if (!hdev) {
> -               err = -EBADFD;
> +       hdev = hci_hdev_from_sock(sk);
> +       if (IS_ERR(hdev)) {
> +               err = PTR_ERR(hdev);
>                 goto done;
>         }
>
> @@ -1743,9 +1760,9 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>                 goto done;
>         }
>
> -       hdev = hci_pi(sk)->hdev;
> -       if (!hdev) {
> -               err = -EBADFD;
> +       hdev = hci_hdev_from_sock(sk);
> +       if (IS_ERR(hdev)) {
> +               err = PTR_ERR(hdev);
>                 goto done;
>         }
>
> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 9874844a95a9..b69d88b88d2e 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -83,6 +83,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn)
>  static void bt_host_release(struct device *dev)
>  {
>         struct hci_dev *hdev = to_hci_dev(dev);
> +
> +       if (hci_dev_test_flag(hdev, HCI_UNREGISTER))
> +               hci_cleanup_dev(hdev);
>         kfree(hdev);
>         module_put(THIS_MODULE);
>  }
> --
> 2.18.4

It doesn't apply to bluetooth-next, could you please rebase it.


-- 
Luiz Augusto von Dentz

Re: [PATCH v4] Bluetooth: defer cleanup of resources in hci_unregister_dev()

[Linus Torvalds]

On Thu, Aug 5, 2021 at 11:19 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> It doesn't apply to bluetooth-next, could you please rebase it.

If you're ok with the patch, I think it should go into 5.14 - and be
marked for stable.

The locking right now is very wrong, and causes syzbot warnings and
probably keeps people from finding other things.

And we can't just revert the incorrect locking change, because it was
a security issue.

So this should go in the current release cycle. I agree it's a bit
scary - not because I think the patch is all that complicated, but
because this area has clearly had issues and is subtle - but I'd
rather do it asap and get reports on it early than delay it further.

            Linus

Re: [PATCH v4] Bluetooth: defer cleanup of resources in hci_unregister_dev()

[Luiz Augusto von Dentz]

Hi Linus,

On Thu, Aug 5, 2021 at 11:45 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Aug 5, 2021 at 11:19 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > It doesn't apply to bluetooth-next, could you please rebase it.
>
> If you're ok with the patch, I think it should go into 5.14 - and be
> marked for stable.
>
> The locking right now is very wrong, and causes syzbot warnings and
> probably keeps people from finding other things.
>
> And we can't just revert the incorrect locking change, because it was
> a security issue.
>
> So this should go in the current release cycle. I agree it's a bit
> scary - not because I think the patch is all that complicated, but
> because this area has clearly had issues and is subtle - but I'd
> rather do it asap and get reports on it early than delay it further.

Alright so we are skipping bluetooth-next then, I thought of using
bluetooth-next for further testing these changes but I agree this
should go into 5.14 so Im fine if you take it straight to your tree:

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>


-- 
Luiz Augusto von Dentz

Re: [PATCH v4] Bluetooth: defer cleanup of resources in hci_unregister_dev()

[Linus Torvalds]

On Thu, Aug 5, 2021 at 11:59 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Alright so we are skipping bluetooth-next then, I thought of using
> bluetooth-next for further testing these changes but I agree this
> should go into 5.14 so Im fine if you take it straight to your tree:

Turned your sign-off into an acked-by, since the patch came directly
from Tetsuo to me, but it's commit e04480920d1e ("Bluetooth: defer
cleanup of resources in hci_unregister_dev()") in my tree now.

Let's hope this closes the issue, but keep our ears to the ground in
case something pops up.

Tetsuo, thanks for driving this.

                 Linus