[PATCH v4] Bluetooth: MGMT: Fix discovery stuck after a failed Stop Discovery

[PATCH v4] Bluetooth: MGMT: Fix discovery stuck after a failed Stop Discovery

[Kevin Dewald]

Stop Discovery moves the discovery to DISCOVERY_STOPPING, and
stop_discovery_complete() only moves it on to DISCOVERY_STOPPED when
stopping succeeded. When it fails, for example because LE Set Scan
Enable times out, nothing moves the discovery out of
DISCOVERY_STOPPING: Stop Discovery is rejected, as the discovery is not
active anymore, and the LE scan timeout was cancelled before the scan
was disabled. Until the controller is powered off, every Start
Discovery then fails with Busy, and hci_update_passive_scan_sync()
skips background scanning, so devices set to auto-connect are not
connected anymore.

Move the discovery back to DISCOVERY_FINDING when stopping it failed
while its inquiry or active LE scan is still running, so that it can be
stopped again, unless it was stopped meanwhile, for example by Inquiry
Cancel when only the passive scan could not be disabled. Do this under
hci_dev_lock(), like the command completions that change these flags,
and only cancel the LE scan timeout once the scan is disabled, so that
the discovery still ends on its own.

A timed out LE Set Scan Enable can still complete afterwards, which
cancels the timeout as well, so also stop the discovery when its scan
is disabled while the timeout is pending, or while the timeout work
waits for the lock.

Fixes: 0e05bba6f6f8 ("Bluetooth: Update stop_discovery to use HCI request")
Assisted-by: Claude:claude-opus-5.5
Signed-off-by: Kevin Dewald <kevin@simpleble.org>
---
Changes in v4:
- Only go back to DISCOVERY_FINDING while the discovery is still
  stopping, and not for the passive scan (Sashiko).
- Check the flags and change the state under hci_dev_lock() (Sashiko).
- Also stop the discovery in le_scan_disable() when the scan was
  disabled while it waited for the lock (Sashiko).
- Rebase on bluetooth-next 1081a6ef8f.
- v3: https://lore.kernel.org/linux-bluetooth/20261027210057.44347-1-kevin@simpleble.org/

Changes in v3:
- Go back to DISCOVERY_FINDING instead of DISCOVERY_STOPPED when
  stopping failed while the inquiry or the LE scan is still running.
- Keep the LE scan timeout until the scan is disabled, so that the
  discovery still ends on its own.
- Stop the discovery when a timed out LE Set Scan Enable completes
  afterwards, as that cancels the timeout too.
- v2: https://lore.kernel.org/linux-bluetooth/20261027172730.86568-1-kevin@simpleble.org/

I did not find an error status that shows by itself that the scan
already stopped: the kernel only disables the scan while HCI_LE_SCAN is
set, and hci_cc_inquiry_cancel() already turns Command Disallowed after
the inquiry completed into success. So this checks HCI_INQUIRY and
HCI_LE_SCAN instead, which are only cleared once the controller
confirms. Going back to DISCOVERY_FINDING sends Discovering (enabled)
again.

Reproduced in test-runner with a new mgmt-tester case, "Stop Discovery -
Scan Disable Failed 1", in which the emulator answers the first LE Set
Scan Enable (disable) with Unspecified Error: without this patch a
second Stop Discovery is rejected and Start Discovery fails with Busy;
with it the second Stop Discovery succeeds, and so does Start Discovery
(3 of 3 runs). Without the second Stop Discovery, the discovery ends at
the LE scan timeout.

The v3 regression reproduces with a BR/EDR discovery while passive
scanning for an auto-connect device, where only disabling the passive
scan fails: with v3 the discovery is enabled again and Start Discovery
fails with Busy (2 of 2 runs), with this version it stays stopped and
Start Discovery succeeds (3 of 3).

Freezing the emulated controller while bluetoothd stops discovering
makes LE Set Scan Enable time out and then complete just after: the
discovery is stopped when it completes, and Start Discovery succeeds
(3 of 3). The other kernel testers pass as before. The two lock
orderings Sashiko raised are not reproduced, only checked in the code.
Only tested with the emulator, not with real controllers. I can send
the mgmt-tester case to BlueZ if you want it.

Not changed: hci_pause_discovery_sync() has the same pattern on suspend,
which I have not reproduced. A simultaneous discovery that is stopped
while it resolves names also goes back to DISCOVERY_FINDING rather than
DISCOVERY_RESOLVING, so its remaining names are not resolved and the LE
scan timeout ends it.

 net/bluetooth/hci_event.c | 12 +++++++++++-
 net/bluetooth/hci_sync.c  | 20 +++++++++++++++++---
 net/bluetooth/mgmt.c      | 15 ++++++++++++++-
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index d05934fef4..80679f1881 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1819,6 +1819,8 @@ static void store_pending_adv_report(struct hci_dev *hdev, bdaddr_t *bdaddr,
 
 static void le_set_scan_enable_complete(struct hci_dev *hdev, u8 enable)
 {
+	bool timeout_pending;
+
 	hci_dev_lock(hdev);
 
 	switch (enable) {
@@ -1848,10 +1850,18 @@ static void le_set_scan_enable_complete(struct hci_dev *hdev, u8 enable)
 		/* Cancel this timer so that we don't try to disable scanning
 		 * when it's already disabled.
 		 */
-		cancel_delayed_work(&hdev->le_scan_disable);
+		timeout_pending = cancel_delayed_work(&hdev->le_scan_disable);
 
 		hci_dev_clear_flag(hdev, HCI_LE_SCAN);
 
+		/* The scan of a discovery was disabled before its timeout,
+		 * for example by a Stop Discovery that timed out and still
+		 * completed, so nothing else would end the discovery.
+		 */
+		if (timeout_pending &&
+		    hdev->discovery.state == DISCOVERY_FINDING)
+			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+
 		if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
 		    hci_test_quirk(hdev, HCI_QUIRK_SIMULTANEOUS_DISCOVERY) &&
 		    !test_bit(HCI_INQUIRY, &hdev->flags) &&
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 9d71ce96bd..0babc776f1 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -376,8 +376,18 @@ static void le_scan_disable(struct work_struct *work)
 	bt_dev_dbg(hdev, "");
 	hci_dev_lock(hdev);
 
-	if (!hci_dev_test_flag(hdev, HCI_LE_SCAN))
+	if (!hci_dev_test_flag(hdev, HCI_LE_SCAN)) {
+		/* The scan of a discovery can be disabled while this work
+		 * waits for the lock, for example by a Stop Discovery that
+		 * timed out and still completed, so end the discovery as
+		 * le_set_scan_enable_complete() does when it cancels this
+		 * work.
+		 */
+		if (hdev->discovery.state == DISCOVERY_FINDING)
+			goto discov_stopped;
+
 		goto _return;
+	}
 
 	status = hci_cmd_sync_queue(hdev, scan_disable_sync, NULL, NULL);
 	if (status) {
@@ -5913,11 +5923,15 @@ int hci_stop_discovery_sync(struct hci_dev *hdev)
 		}
 
 		if (hci_dev_test_flag(hdev, HCI_LE_SCAN)) {
-			cancel_delayed_work(&hdev->le_scan_disable);
-
 			err = hci_scan_disable_sync(hdev);
 			if (err)
 				return err;
+
+			/* Only cancel the LE scan timeout once the scan is
+			 * disabled, so that the discovery still ends if
+			 * stopping it failed.
+			 */
+			cancel_delayed_work(&hdev->le_scan_disable);
 		}
 
 	} else {
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3cb4f9ece8..03f5869c03 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -6322,8 +6322,21 @@ static void stop_discovery_complete(struct hci_dev *hdev, void *data, int err)
 			  cmd->param, 1);
 	mgmt_pending_free(cmd);
 
-	if (!err)
+	hci_dev_lock(hdev);
+
+	/* Go back to DISCOVERY_FINDING when stopping failed while the inquiry
+	 * or the active LE scan of the discovery is still running, unless the
+	 * discovery was stopped meanwhile, so that it can be stopped again.
+	 */
+	if (err && hdev->discovery.state == DISCOVERY_STOPPING &&
+	    (test_bit(HCI_INQUIRY, &hdev->flags) ||
+	     (hci_dev_test_flag(hdev, HCI_LE_SCAN) &&
+	      hdev->le_scan_type == LE_SCAN_ACTIVE)))
+		hci_discovery_set_state(hdev, DISCOVERY_FINDING);
+	else
 		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+
+	hci_dev_unlock(hdev);
 }
 
 static int stop_discovery_sync(struct hci_dev *hdev, void *data)

base-commit: 1081a6ef8f1e0661fd52eb3f878691b0b96cdff9
-- 
2.53.0

RE: [v4] Bluetooth: MGMT: Fix discovery stuck after a failed Stop Discovery

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3452 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/series/1196813/

---Test result---

Test Summary:
CheckPatch                    PASS      1.09 seconds
VerifyFixes                   PASS      0.07 seconds
VerifySignedoff               PASS      0.07 seconds
GitLint                       PASS      0.22 seconds
SubjectPrefix                 PASS      0.06 seconds
BuildKernel                   PASS      31.77 seconds
CheckAllWarning               PASS      39.02 seconds
CheckSparse                   WARNING   40.88 seconds
BuildKernel32                 PASS      32.72 seconds
CheckKernelLLVM               PASS      36.03 seconds
TestRunnerSetup               PASS      744.11 seconds
TestRunner_l2cap-tester       FAIL      10.51 seconds
TestRunner_iso-tester         PASS      10.72 seconds
TestRunner_bnep-tester        PASS      3.11 seconds
TestRunner_mgmt-tester        PASS      87.92 seconds
TestRunner_rfcomm-tester      PASS      5.43 seconds
TestRunner_sco-tester         PASS      5.64 seconds
TestRunner_ioctl-tester       PASS      4.65 seconds
TestRunner_mesh-tester        PASS      5.50 seconds
TestRunner_smp-tester         PASS      4.31 seconds
TestRunner_userchan-tester    PASS      3.19 seconds
TestRunner_6lowpan-tester     PASS      4.41 seconds
IncrementalBuild              PASS      36.79 seconds

Details
##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: TestRunner_l2cap-tester - FAIL
Desc: Run l2cap-tester with test-runner
Output:
Sanitizer errors:
==35==ERROR: AddressSanitizer: heap-use-after-free on address 0x7bc59a8e33d0 at pc 0x561c09478e8e bp 0x7ffe3ee10190 sp 0x7ffe3ee10180
READ of size 4 at 0x7bc59a8e33d0 thread T0
    #0 0x561c09478e8d in timeout_callback src/shared/timeout-mainloop.c:33
    #1 0x561c09479b75 in timeout_callback src/shared/mainloop.c:270
    #2 0x561c0947aaa6 in mainloop_run src/shared/mainloop.c:124
    #3 0x561c0947d3fa in mainloop_run_with_signal src/shared/mainloop-notify.c:196
    #4 0x561c0948264b in tester_run src/shared/tester.c:1223
    #5 0x561c093ac17d in main tools/l2cap-tester.c:3918
    #6 0x7f859b817600 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:59
    #7 0x7f859b817717 in __libc_start_main_impl ../csu/libc-start.c:360
    #8 0x561c093b2c84 in _start (/home/runner/work/bluetooth-next/bluetooth-next/src/bluez/tools/l2cap-tester+0x16cc84) (BuildId: 91c9d49e9766c394468e27ffa29b05e7d348169f)

0x7bc59a8e33d0 is located 0 bytes inside of 40-byte region [0x7bc59a8e33d0,0x7bc59a8e33f8)
freed by thread T0 here:
    #0 0x7f859c1553ff in free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:51
    #1 0x561c0947936a in timeout_destroy src/shared/mainloop.c:251

previously allocated by thread T0 here:
    #0 0x7f859c15660f in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:67
    #1 0x561c094632d5 in util_malloc src/shared/util.c:46

SUMMARY: AddressSanitizer: heap-use-after-free src/shared/timeout-mainloop.c:33 in timeout_callback
No test result found


https://github.com/bluez/bluetooth-next/pull/1028

---
Regards,
Linux Bluetooth