[PATCH 0/3] Bluetooth: hci_conn: fix SCO connection reuse

[PATCH 0/3] Bluetooth: hci_conn: fix SCO connection reuse

[Iaroslav Voitovych]

An HFP codec change can leave a headset silent. The new SCO socket
reports a successful connect(), but no new SCO link is established.

PipeWire closes the old SCO socket and immediately connects another
to the same headset. If the headset replies to AT+BCS before the old
link's Disconnection Complete arrives, hci_connect_sco() reuses the
connection being torn down.

That connection remains BT_CONNECTED while its Disconnect is pending.
No Setup Synchronous Connection is sent, yet the new socket reports
success. When the old link's Disconnection Complete arrives, the new
socket is closed with ECONNABORTED.

This series fixes three related problems.

Patch 1 corrects hold ownership when reusing a SCO connection whose
link is still up. A second connect() correctly returns EBUSY, but
also drops the first socket's hold and disconnects its working link.
Give the second caller its own hold and release the extra ACL hold,
following the CIS fix e06d549fcd4a. EBUSY remains the result, but
the existing link survives.

Patch 2 prevents reuse of a connection already committed to
teardown. If abort_reason is set, return -ECONNABORTED immediately
instead of handing the dying connection to a new socket.

Patch 3 closes the remaining race before abort_reason is set. A
running hci_conn_timeout() may already have observed that no holder
remains when a new connect() takes a hold. Wait for that worker and
check abort_reason again before using the connection.

A connect() arriving before the worker starts cancels the pending
disconnect with the hold patch 1 gives it, and keeps the live link.
Patches 2 and 3 leave that case unchanged.

Testing

The series was tested on bluetooth-next 27731ba09195 in QEMU with
BlueZ's emulator, on uniprocessor, SMP and KCSAN kernels, with
lockdep enabled. The accompanying BlueZ patch adds three sco-tester
cases:

  "Reconnect After Disconnect"
  "Reconnect During Disconnect"
  "Connect Twice"

"Reconnect During Disconnect" and "Connect Twice" fail without the
series on the uniprocessor and SMP kernels, and pass with it in three
runs each on the uniprocessor, SMP and KCSAN kernels. "Reconnect
After Disconnect" passes with and without the series. In the full
sco-tester, iso-tester, l2cap-tester and mgmt-tester runs, the only
other differences were two mgmt-tester cases that timed out
intermittently on SMP. Neither involves SCO, and one also timed out
without the series.

Patch 3's race was reproduced by a debug-only change that pauses
hci_conn_timeout() after it observes no holder; the change widens
the window and is not part of this series. Both host-initiated and
remote-initiated SCO links were tested. With patches 1 and 2 alone,
the new socket is still reported connected and then aborted. With
patch 3, connect() returns -ECONNABORTED.

The series also applies unchanged to bluetooth-next 47d4d9f67e92,
the base of this posting. On that base, sco-tester passes all 33
cases with the series; without it, only "Reconnect During
Disconnect" and "Connect Twice" fail. The other testers were not
rerun on this base.

Hardware testing

The series was also run on the QCA9377 laptop where the bug was
found, with PipeWire 1.0.5 and three headsets. The laptop ran
Ubuntu 24.04's 7.0.0-38 kernel with its Bluetooth module rebuilt
with abd93c85c8667, 816fb1590a4c and this series; the two
prerequisites are explained below.

Over more than a day and a half of ordinary use, from 23 to 25
October, three codec changes on earbuds that reply to AT+BCS before
the old link's Disconnection Complete returned ECONNABORTED instead
of reporting a connected socket without a link. On two other
headsets, which reply after that event, all eight codec changes
established a new link.

All 36 SCO links in the HCI capture of those days ended with a
successful Disconnection Complete for their handle, and no HCI
command timeout was logged. A search of the boot's kernel log found
no WARNING, BUG, Oops, Call Trace, refcount or use-after-free
report. Its only error-level lines are "SCO packet for unknown
connection handle" and "corrupted SCO packet", which this
controller also logs without the series.

Patch 1's case was tested separately on 26 October. While a
headset's eSCO link carried audio, three attempts to connect a
second SCO socket to the same headset each returned EBUSY at once.
The HCI capture shows no new setup or disconnect during the
attempts, and SCO packets kept flowing in both directions.

Patch 3's race was not reproduced on hardware; it is covered by the
emulator test described above.

Remaining codec-switch delay

The series stops connect() from succeeding on a connection already
being torn down. It does not, by itself, make PipeWire's codec
change seamless.

PipeWire 1.0.5 retries ECONNABORTED at once, while the old link is
still being disconnected, so those retries fail too. With a stream
playing on the laptop, audio returned about five seconds later,
when WirePlumber suspended the failed nodes and the next connect
established a new link.

In an earlier reproduction without the series, on Ubuntu 7.0.0-34,
the same codec change produced no Setup Synchronous Connection, and
sounds started 2 to 26 seconds later were still silent. That was a
historical observation, not a same-build control.

A separate prototype defers the new connect until the old link has
been torn down. It lets the retry succeed in the emulator tests but
has not been run on hardware. It builds on this series and changes
connect() semantics, so I can post it as an RFC if that approach is
preferred.

Related work

The series is independent of Hitalo Souza's pending SCO setup
series and applies with or without it:
https://patchwork.kernel.org/project/bluetooth/list/?series=1181908

Jeremy Dean Clifton's pending "hci_core: Fix queuing disc_work while
workqueue is drained" (syzbot's "WARNING in hci_connect_sco") is on
the same path but does not overlap: it changes hci_conn_drop() in
hci_core.h.

Stable and downstream kernels

For stable, patch 1 applies alone on 6.6.y and 6.12.y, which have
the 8a5b0135d4a5 backport and the reuse path but not e6720779ae61.
Patches 2 and 3 apply on top of it on 6.18.y and 7.2.y. Each of
those heads was built and run with sco-tester without and with the
patches it needs: the cases for its bugs fail without them and pass
with them. 6.1.y has no reuse path, hence the "# 6.6.x" on patch 1.

Trees with e6720779ae61 but without 816fb1590a4c, such as Ubuntu
24.04's 7.0 kernel, need these two commits first:

  abd93c85c8667 ("Bluetooth: SCO: give the socket its own sco_conn
                 reference")
  816fb1590a4c ("Bluetooth: SCO: serialise sco_conn lifetime against
                sco_recv_scodata()")

Without them, sco_connect() does not put the sco_conn reference that
sco_conn_add() took when sco_chan_add() fails with EBUSY, and that
reference holds the hci_conn: once patch 1 stops the disconnect, the
SCO link stays up after both sockets are closed. On Ubuntu
7.0.0-38's Bluetooth sources with those two commits, "Reconnect
During Disconnect" and "Connect Twice" fail without the series and
pass with it, uniprocessor and SMP.

Iaroslav Voitovych (3):
  Bluetooth: hci_conn: hold a reused SCO connection for its caller
  Bluetooth: hci_conn: don't reuse an aborting SCO connection
  Bluetooth: hci_conn: synchronize with SCO disconnect work before reuse

 net/bluetooth/hci_conn.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)


base-commit: 47d4d9f67e92d56ef3965f2727f9c98a5a611368
-- 
2.43.0

[PATCH 1/3] Bluetooth: hci_conn: hold a reused SCO connection for its caller

[Iaroslav Voitovych]

Connecting a second SCO socket to a device whose link is already
in use correctly fails with EBUSY, but also disconnects the first
socket's working link.

hci_connect_sco() must return a connection with a hold owned by
its caller. sco_connect() drops that hold when it no longer needs
it, including when sco_chan_add() fails.

For a new link, hci_conn_link() takes the required SCO hold.
Since commit 06149746e720 ("Bluetooth: hci_conn: Add support for
linking multiple hcon"), a connection whose link already exists
is returned without one: hci_connect_sco() hands the reused SCO
connection back without a caller hold, while keeping the extra
ACL hold that hci_connect_acl() took for the link.

Commit 8a5b0135d4a5 ("Bluetooth: SCO: fix race conditions in
sco_sock_connect()") added the drop on sco_chan_add() failure.
When the second socket gets EBUSY, that drop consumes the first
socket's hold. hci_conn_timeout() then finds no holder and
disconnects the link.

When the SCO connection already has a link, take a hold for the
caller and drop the extra ACL hold, which the existing link
already owns. Keep the hci_conn_link() path for connections that
do not yet have a link.

This follows the ownership correction made for CIS by
commit e06d549fcd4a ("Bluetooth: hci_conn: fix CIS hold ownership
on reuse"). The second connect() still fails with EBUSY, but no
longer disconnects the first socket's link.

Fixes: 8a5b0135d4a5 ("Bluetooth: SCO: fix race conditions in sco_sock_connect()")
Cc: stable@vger.kernel.org # 6.6.x
Signed-off-by: Iaroslav Voitovych <yaroslav.voytovych@gmail.com>
---
 net/bluetooth/hci_conn.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b007f623b2ecc..7a55aaf197138 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1911,11 +1911,20 @@ struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
 		}
 	}
 
-	link = hci_conn_link(acl, sco);
-	if (!link) {
+	if (sco->link) {
+		/* The existing link already owns the hold on the ACL, and the
+		 * caller needs its own hold on the SCO connection, which
+		 * hci_conn_link() only takes for a new link.
+		 */
 		hci_conn_drop(acl);
-		hci_conn_drop(sco);
-		return ERR_PTR(-ENOLINK);
+		hci_conn_hold(sco);
+	} else {
+		link = hci_conn_link(acl, sco);
+		if (!link) {
+			hci_conn_drop(acl);
+			hci_conn_drop(sco);
+			return ERR_PTR(-ENOLINK);
+		}
 	}
 
 	sco->setting = setting;
-- 
2.43.0

RE: Bluetooth: hci_conn: fix SCO connection reuse

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1605 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/1194813/

---Test result---

Test Summary:
CheckPatch                    PASS      1.59 seconds
VerifyFixes                   PASS      0.11 seconds
VerifySignedoff               PASS      0.11 seconds
GitLint                       PASS      0.80 seconds
SubjectPrefix                 PASS      7.43 seconds
BuildKernel                   PASS      19.56 seconds
CheckAllWarning               PASS      23.02 seconds
CheckSparse                   PASS      24.40 seconds
BuildKernel32                 PASS      19.74 seconds
CheckKernelLLVM               PASS      20.25 seconds
TestRunnerSetup               PASS      447.58 seconds
TestRunner_l2cap-tester       PASS      18.63 seconds
TestRunner_iso-tester         PASS      9.25 seconds
TestRunner_bnep-tester        PASS      2.79 seconds
TestRunner_mgmt-tester        PASS      82.86 seconds
TestRunner_rfcomm-tester      PASS      4.12 seconds
TestRunner_sco-tester         PASS      5.04 seconds
TestRunner_ioctl-tester       PASS      4.14 seconds
TestRunner_mesh-tester        PASS      5.02 seconds
TestRunner_smp-tester         PASS      3.92 seconds
TestRunner_userchan-tester    PASS      2.81 seconds
TestRunner_6lowpan-tester     PASS      3.99 seconds
IncrementalBuild              PASS      22.61 seconds



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

---
Regards,
Linux Bluetooth

[PATCH 2/3] Bluetooth: hci_conn: don't reuse an aborting SCO connection

[Iaroslav Voitovych]

A new SCO socket can report a successful connect() to a link that
is already being disconnected. No new link is established, and the
socket is closed with ECONNABORTED when the old link's
Disconnection Complete arrives. PipeWire hits this when it closes
and reconnects a SCO socket during an HFP codec change.

hci_connect_sco() looks up an existing connection by type and
address. It sends Setup Synchronous Connection only when that
connection is BT_OPEN or BT_CLOSED.

An aborting connection is neither: hci_abort_conn() sets
abort_reason and queues the Disconnect, but leaves the state
BT_CONNECTED until the Disconnection Complete. A new connect in
that interval therefore reuses the old connection without sending
a setup command.

Before commit e6720779ae61 ("Bluetooth: SCO: Use kref to track
lifetime of sco_conn"), the closed socket stayed attached in
BT_DISCONN until that event, so sco_chan_add() rejected the new
socket with EBUSY. Since that commit, the closed socket is
detached at once, and the new socket takes its place: sco_connect()
reports it connected, and the Disconnection Complete then closes
it with ECONNABORTED.

Reject a connection whose abort_reason is set with -ECONNABORTED,
and drop the ACL hold that hci_connect_acl() took. The caller
learns at once, with the error the socket would otherwise get
later, and can connect again once the old link is gone.

hci_connect_sco() runs under hdev->lock, but hci_conn_timeout()
sets abort_reason through hci_abort_conn() without it. Use
READ_ONCE() for the new check and WRITE_ONCE() for that write.

Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn")
Cc: stable@vger.kernel.org
Signed-off-by: Iaroslav Voitovych <yaroslav.voytovych@gmail.com>
---
 net/bluetooth/hci_conn.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 7a55aaf197138..6b92fd0177857 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1909,6 +1909,14 @@ struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
 			hci_conn_drop(acl);
 			return sco;
 		}
+	} else if (READ_ONCE(sco->abort_reason)) {
+		/* The connection is being disconnected and stays BT_CONNECTED
+		 * until the Disconnection Complete event, so no setup would
+		 * be sent for it and it would be closed under the new socket
+		 * as soon as the event arrives.
+		 */
+		hci_conn_drop(acl);
+		return ERR_PTR(-ECONNABORTED);
 	}
 
 	if (sco->link) {
@@ -3272,7 +3280,7 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
 
 	bt_dev_dbg(hdev, "handle 0x%2.2x reason 0x%2.2x", conn->handle, reason);
 
-	conn->abort_reason = reason;
+	WRITE_ONCE(conn->abort_reason, reason);
 
 	/* Cancel the connect attempt. A return of 0 means the create command
 	 * was still queued and got dequeued, so there is nothing to disconnect.
-- 
2.43.0

[PATCH 3/3] Bluetooth: hci_conn: synchronize with SCO disconnect work before reuse

[Iaroslav Voitovych]

Checking abort_reason before reusing a SCO connection does not
close the race with a disconnect worker that is already running:
connect() can still report success just before that worker aborts
the connection.

hci_conn_hold() uses cancel_delayed_work(), which cancels pending
work but does not wait for work already running.
hci_conn_timeout() reads refcnt without hdev->lock and, after
seeing no holder, calls hci_abort_conn(), which sets abort_reason.

A new connect() can check abort_reason and take its hold between
those two steps. It then gets a connection that the worker is
about to abort, and the Disconnection Complete closes the new
socket with ECONNABORTED. This applies both to a connection whose
link exists and to one the remote device set up, which gets its
link from hci_conn_link() here.

After taking the hold, wait for a running hci_conn_timeout() with
cancel_delayed_work_sync() and check abort_reason again. If the
worker has aborted the connection, drop the caller's hold and
return -ECONNABORTED.

The wait is safe under hdev->lock. For a SCO connection,
hci_conn_timeout() does not take that lock: hci_proto_disconn_ind()
returns a constant, and hci_abort_conn() only queues the
Disconnect. hci_conn_del() already waits for disc_work with
hdev->lock held. For a new connection the work has never been
queued, so nothing is waited for.

Once the wait returns, the work has finished and cannot be queued
again while the caller's hold is taken, so the second check does
not race with it. If the connection had no link before, the link
hci_conn_link() just made stays with the aborting connection and
is released with its ACL hold by hci_conn_del(), as for any linked
SCO connection.

A connect() that comes before the work has started at all still
cancels it with its hold and keeps the live link, as it does
without this patch; that case is not changed here.

Reproduced by delaying hci_conn_timeout() after it has seen no
holder (a debug-only change, not part of this patch), for a link
set up by the host and for one set up by the remote device:
without this patch connect() returns 0 and the socket is then
closed with ECONNABORTED; with it, connect() fails with
-ECONNABORTED.

Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn")
Cc: stable@vger.kernel.org
Signed-off-by: Iaroslav Voitovych <yaroslav.voytovych@gmail.com>
---
 net/bluetooth/hci_conn.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 6b92fd0177857..c1c8d2b29e3c0 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1935,6 +1935,17 @@ struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
 		}
 	}
 
+	/* Taking the hold cannot stop a hci_conn_timeout() that is already
+	 * running: it may have seen no holder and be about to abort the
+	 * connection. It does not take hdev->lock, so wait for it and check
+	 * abort_reason again.
+	 */
+	cancel_delayed_work_sync(&sco->disc_work);
+	if (sco->abort_reason) {
+		hci_conn_drop(sco);
+		return ERR_PTR(-ECONNABORTED);
+	}
+
 	sco->setting = setting;
 	sco->codec = *codec;
 
-- 
2.43.0