[PATCH 1/2] Bluetooth: Fix index added after unregister

[PATCH 1/2] Bluetooth: Fix index added after unregister

[Abhishek Pandit-Subedi]

From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

When a userchannel socket is released, we should check whether the hdev
is already unregistered before sending out an IndexAdded.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
This happened when the firmware crashed or the controller was lost for
some other reason.

For testing, I emualated this using:
echo 0 > $(readlink -f /sys/class/bluetooth/hci0)/../../authorized

   = Close Index: F8:E4:E3:D9:9E:45                     [hci0] 682.178794
    @ MGMT Event: Index Removed (0x0005) plen 0 {0x0002} [hci0] 682.178809
    @ MGMT Event: Index Removed (0x0005) plen 0 {0x0001} [hci0] 682.178809
    = Delete Index: F8:E4:E3:D9:9E:45                    [hci0] 682.178821
    @ USER Close: bt_stack_manage               {0x0003} [hci0] 682.397653
    @ MGMT Event: Index Added (0x0004) plen 0   {0x0002} [hci0] 682.397667
    @ MGMT Event: Index Added (0x0004) plen 0   {0x0001} [hci0] 682.397667
    @ MGMT Close: bt_stack_manage               {0x0002} 682.397793
    @ MGMT Open: bt_stack_manage (privileged) version 1.14     {0x0003} 682.437223
    @ MGMT Command: Read Controller Index List (0x0003) plen 0 {0x0003} 682.437230
    @ MGMT Event: Command Complete (0x0001) plen 5             {0x0003} 682.437232
          Read Controller Index List (0x0003) plen 2
            Status: Success (0x00)
            Controllers: 0

Tested on ChromeOS kernel and compiled with allmodconfig on
bluetooth-next.

 net/bluetooth/hci_sock.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 189e3115c8c6..bd8358b44aa4 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -869,7 +869,8 @@ static int hci_sock_release(struct socket *sock)
 
 	hdev = hci_pi(sk)->hdev;
 	if (hdev) {
-		if (hci_pi(sk)->channel == HCI_CHANNEL_USER) {
+		if (hci_pi(sk)->channel == HCI_CHANNEL_USER &&
+		    !hci_dev_test_flag(hdev, HCI_UNREGISTER)) {
 			/* When releasing a user channel exclusive access,
 			 * call hci_dev_do_close directly instead of calling
 			 * hci_dev_close to ensure the exclusive access will
@@ -878,6 +879,11 @@ static int hci_sock_release(struct socket *sock)
 			 * The checking of HCI_AUTO_OFF is not needed in this
 			 * case since it will have been cleared already when
 			 * opening the user channel.
+			 *
+			 * Make sure to also check that we haven't already
+			 * unregistered since all the cleanup will have already
+			 * been complete and hdev will get released when we put
+			 * below.
 			 */
 			hci_dev_do_close(hdev);
 			hci_dev_clear_flag(hdev, HCI_USER_CHANNEL);
-- 
2.36.1.255.ge46751e96f-goog

[PATCH 2/2] Bluetooth: Unregister suspend with userchannel

[Abhishek Pandit-Subedi]

From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

When HCI_USERCHANNEL is used, unregister the suspend notifier when
binding and register when releasing. The userchannel socket should be
left alone after open is completed.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Currently, the suspend notifier is harmless when used since no traffic
is actually passed to controller with HCI_USERCHANNEL. When I suspend,
all I see is the MGMT Controller Suspended and Controller Resumed
events. However, for correctness, I've opted to unregister the suspend
notifier entirely when using HCI_USERCHANNEL (similar to how the
HCI_QUIRK_NO_SUSPEND_NOTIFIER works).

The alternative to this approach would be to always keep the notifier
registered and exit early in hci_suspend_notifier. Please let me know
which you prefer.

Tested on ChromeOS kernel and compiled with allmodconfig on
bluetooth-next.

 include/net/bluetooth/hci_core.h |  2 ++
 net/bluetooth/hci_core.c         | 33 ++++++++++++++++++++++++--------
 net/bluetooth/hci_sock.c         |  3 +++
 3 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 5a52a2018b56..5b92a9abe141 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1289,6 +1289,8 @@ 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_release_dev(struct hci_dev *hdev);
+int hci_register_suspend_notifier(struct hci_dev *hdev);
+int hci_unregister_suspend_notifier(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 5abb2ca5b129..ab647a63830d 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2639,12 +2639,8 @@ int hci_register_dev(struct hci_dev *hdev)
 	hci_sock_dev_event(hdev, HCI_DEV_REG);
 	hci_dev_hold(hdev);
 
-	if (!test_bit(HCI_QUIRK_NO_SUSPEND_NOTIFIER, &hdev->quirks)) {
-		hdev->suspend_notifier.notifier_call = hci_suspend_notifier;
-		error = register_pm_notifier(&hdev->suspend_notifier);
-		if (error)
-			goto err_wqueue;
-	}
+	if (hci_register_suspend_notifier(hdev))
+		goto err_wqueue;
 
 	queue_work(hdev->req_workqueue, &hdev->power_on);
 
@@ -2677,8 +2673,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
 
 	hci_cmd_sync_clear(hdev);
 
-	if (!test_bit(HCI_QUIRK_NO_SUSPEND_NOTIFIER, &hdev->quirks))
-		unregister_pm_notifier(&hdev->suspend_notifier);
+	hci_unregister_suspend_notifier(hdev);
 
 	msft_unregister(hdev);
 
@@ -2742,6 +2737,28 @@ void hci_release_dev(struct hci_dev *hdev)
 }
 EXPORT_SYMBOL(hci_release_dev);
 
+int hci_register_suspend_notifier(struct hci_dev *hdev)
+{
+	int ret = 0;
+
+	if (!test_bit(HCI_QUIRK_NO_SUSPEND_NOTIFIER, &hdev->quirks)) {
+		hdev->suspend_notifier.notifier_call = hci_suspend_notifier;
+		ret = register_pm_notifier(&hdev->suspend_notifier);
+	}
+
+	return ret;
+}
+
+int hci_unregister_suspend_notifier(struct hci_dev *hdev)
+{
+	int ret = 0;
+
+	if (!test_bit(HCI_QUIRK_NO_SUSPEND_NOTIFIER, &hdev->quirks))
+		ret = unregister_pm_notifier(&hdev->suspend_notifier);
+
+	return ret;
+}
+
 /* 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 bd8358b44aa4..0d015d4a8e41 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -887,6 +887,7 @@ static int hci_sock_release(struct socket *sock)
 			 */
 			hci_dev_do_close(hdev);
 			hci_dev_clear_flag(hdev, HCI_USER_CHANNEL);
+			hci_register_suspend_notifier(hdev);
 			mgmt_index_added(hdev);
 		}
 
@@ -1215,6 +1216,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
 		}
 
 		mgmt_index_removed(hdev);
+		hci_unregister_suspend_notifier(hdev);
 
 		err = hci_dev_open(hdev->id);
 		if (err) {
@@ -1229,6 +1231,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
 				err = 0;
 			} else {
 				hci_dev_clear_flag(hdev, HCI_USER_CHANNEL);
+				hci_register_suspend_notifier(hdev);
 				mgmt_index_added(hdev);
 				hci_dev_put(hdev);
 				goto done;
-- 
2.36.1.255.ge46751e96f-goog

RE: [1/2] Bluetooth: Fix index added after unregister

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PASS      3.66 seconds
GitLint                       FAIL      1.03 seconds
SubjectPrefix                 PASS      1.74 seconds
BuildKernel                   PASS      31.77 seconds
BuildKernel32                 PASS      28.08 seconds
Incremental Build with patchesPASS      65.60 seconds
TestRunner: Setup             PASS      475.78 seconds
TestRunner: l2cap-tester      PASS      17.44 seconds
TestRunner: bnep-tester       PASS      6.11 seconds
TestRunner: mgmt-tester       PASS      101.81 seconds
TestRunner: rfcomm-tester     PASS      9.61 seconds
TestRunner: sco-tester        PASS      9.37 seconds
TestRunner: smp-tester        PASS      9.40 seconds
TestRunner: userchan-tester   PASS      6.41 seconds

Details
##############################
Test: GitLint - FAIL - 1.03 seconds
Run gitlint with rule in .gitlint
[1/2] Bluetooth: Fix index added after unregister
24: B1 Line exceeds max length (82>80): "    @ MGMT Open: bt_stack_manage (privileged) version 1.14     {0x0003} 682.437223"
25: B1 Line exceeds max length (82>80): "    @ MGMT Command: Read Controller Index List (0x0003) plen 0 {0x0003} 682.437230"
26: B1 Line exceeds max length (82>80): "    @ MGMT Event: Command Complete (0x0001) plen 5             {0x0003} 682.437232"




---
Regards,
Linux Bluetooth

Re: [PATCH 1/2] Bluetooth: Fix index added after unregister

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Marcel Holtmann <marcel@holtmann.org>:

On Thu,  2 Jun 2022 09:46:49 -0700 you wrote:
> From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> 
> When a userchannel socket is released, we should check whether the hdev
> is already unregistered before sending out an IndexAdded.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> 
> [...]

Here is the summary with links:
  - [1/2] Bluetooth: Fix index added after unregister
    https://git.kernel.org/bluetooth/bluetooth-next/c/8d4b73539cca
  - [2/2] Bluetooth: Unregister suspend with userchannel
    https://git.kernel.org/bluetooth/bluetooth-next/c/d6bb2a91f95b

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