Skip to content

Commit 75ff93a

Browse files
lylezhu2012nashif
lylezhu2012
authored and
nashif
committed
Bluetooth: Classic: HFP: Fix SCO conn cannot be released issue
There is an issue found that the sco conn cannot be released when the SCO has been disconnected. The sco conn count is referred in the sco connected callback due to the `sco_conn` is NULL while `sco_conn` should not be NULL when the sco connected callback is triggered. It is a race condition issue where the sco connected callback is triggered before `sco_conn` is updated. The sco conn reference count has been referred incorrectly, actually it should not be referred. Replace direct pointer access to `sco_conn` with atomic operations to prevent race conditions in concurrent SCO connection scenarios. If the value of `sco_conn` is not NULL when trying to set the return value of `bt_conn_create_sco()` to `sco_conn`, it means the race condition occurs. In this situation, discount the sco conn reference count to fix the issue. Signed-off-by: Lyle Zhu <[email protected]>
1 parent 764f0d1 commit 75ff93a

File tree

4 files changed

+79
-44
lines changed

4 files changed

+79
-44
lines changed

‎subsys/bluetooth/host/classic/hfp_ag.c‎

Lines changed: 50 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -772,7 +772,7 @@ static void bt_hfp_ag_set_call_state(struct bt_hfp_ag_call *call, bt_hfp_call_st
772772

773773
static void hfp_ag_close_sco(struct bt_hfp_ag *ag)
774774
{
775-
struct bt_conn *sco = NULL;
775+
struct bt_conn *sco;
776776
int call_count;
777777

778778
LOG_DBG("");
@@ -785,9 +785,9 @@ static void hfp_ag_close_sco(struct bt_hfp_ag *ag)
785785
return;
786786
}
787787

788-
if (ag->sco_conn != NULL) {
789-
bt_conn_unref(ag->sco_conn);
790-
ag->sco_conn = NULL;
788+
sco = atomic_ptr_set(&ag->sco_conn, NULL);
789+
if (sco != NULL) {
790+
bt_conn_unref(sco);
791791
sco = ag->sco_chan.sco;
792792
}
793793
hfp_ag_unlock(ag);
@@ -1450,8 +1450,8 @@ static void hfp_ag_sco_connected(struct bt_sco_chan *chan)
14501450
{
14511451
struct bt_hfp_ag *ag = CONTAINER_OF(chan, struct bt_hfp_ag, sco_chan);
14521452

1453-
if (ag->sco_conn == NULL) {
1454-
ag->sco_conn = bt_conn_ref(chan->sco);
1453+
if (atomic_ptr_cas(&ag->sco_conn, NULL, chan->sco)) {
1454+
bt_conn_ref(chan->sco);
14551455
}
14561456

14571457
if ((bt_ag) && bt_ag->sco_connected) {
@@ -1462,14 +1462,15 @@ static void hfp_ag_sco_connected(struct bt_sco_chan *chan)
14621462
static void hfp_ag_sco_disconnected(struct bt_sco_chan *chan, uint8_t reason)
14631463
{
14641464
struct bt_hfp_ag *ag = CONTAINER_OF(chan, struct bt_hfp_ag, sco_chan);
1465+
struct bt_conn *sco;
14651466

14661467
if ((bt_ag != NULL) && bt_ag->sco_disconnected) {
14671468
bt_ag->sco_disconnected(chan->sco, reason);
14681469
}
14691470

1470-
if (ag->sco_conn != NULL) {
1471-
bt_conn_unref(ag->sco_conn);
1472-
ag->sco_conn = NULL;
1471+
sco = atomic_ptr_set(&ag->sco_conn, NULL);
1472+
if (sco != NULL) {
1473+
bt_conn_unref(sco);
14731474
}
14741475
}
14751476

@@ -1506,32 +1507,51 @@ static struct bt_conn *bt_hfp_ag_create_sco(struct bt_hfp_ag *ag)
15061507
.connected = hfp_ag_sco_connected,
15071508
.disconnected = hfp_ag_sco_disconnected,
15081509
};
1510+
struct bt_conn *sco;
1511+
int err;
1512+
bool updated;
15091513

15101514
LOG_DBG("");
15111515

1512-
if (ag->sco_conn == NULL) {
1513-
int err;
1516+
sco = atomic_ptr_get(&ag->sco_conn);
1517+
if (sco != NULL) {
1518+
return sco;
1519+
}
15141520

1515-
ag->sco_chan.ops = &ops;
1521+
ag->sco_chan.ops = &ops;
15161522

1517-
err = hfp_ag_set_voice_setting(ag);
1518-
if (err < 0) {
1519-
LOG_ERR("Fail to set voice setting :(%d)", err);
1520-
return NULL;
1523+
err = hfp_ag_set_voice_setting(ag);
1524+
if (err < 0) {
1525+
LOG_ERR("Fail to set voice setting :(%d)", err);
1526+
return NULL;
1527+
}
1528+
1529+
/* create SCO connection*/
1530+
sco = bt_conn_create_sco(&ag->acl_conn->br.dst, &ag->sco_chan);
1531+
updated = atomic_ptr_cas(&ag->sco_conn, NULL, sco);
1532+
if (!updated) {
1533+
LOG_WRN("SCO is not NULL (%p), target (%p)", atomic_ptr_get(&ag->sco_conn), sco);
1534+
__ASSERT(atomic_ptr_get(&ag->sco_conn) == sco,
1535+
"Concurrent SCO connection creation detected");
1536+
/* The `ag->sco_conn` has been updated in callback `hfp_ag_sco_connected()`.
1537+
* The reference count has been increased in callback `hfp_ag_sco_connected()`.
1538+
* The reference count should be decreased in this case.
1539+
*/
1540+
if (sco != NULL) {
1541+
LOG_DBG("Unreference SCO connection %p", sco);
1542+
bt_conn_unref(sco);
15211543
}
1544+
}
15221545

1523-
/* create SCO connection*/
1524-
ag->sco_conn = bt_conn_create_sco(&ag->acl_conn->br.dst, &ag->sco_chan);
1525-
if (ag->sco_conn != NULL) {
1526-
LOG_DBG("Created sco %p", ag->sco_conn);
1527-
if (ag->sco_chan.sco == NULL) {
1528-
/* SCO connection exists */
1529-
LOG_WRN("SCO conn has been created outside");
1530-
}
1546+
if (sco != NULL) {
1547+
LOG_DBG("Created sco %p", sco);
1548+
if (ag->sco_chan.sco == NULL) {
1549+
/* SCO connection exists */
1550+
LOG_WRN("SCO conn has been created outside");
15311551
}
15321552
}
15331553

1534-
return ag->sco_conn;
1554+
return sco;
15351555
}
15361556

15371557
static int hfp_ag_open_sco(struct bt_hfp_ag *ag, struct bt_hfp_ag_call *call)
@@ -1544,7 +1564,7 @@ static int hfp_ag_open_sco(struct bt_hfp_ag *ag, struct bt_hfp_ag_call *call)
15441564
}
15451565

15461566
hfp_ag_lock(ag);
1547-
create_sco = (ag->sco_conn == NULL) ? true : false;
1567+
create_sco = atomic_ptr_get(&ag->sco_conn) == NULL ? true : false;
15481568
if (create_sco) {
15491569
atomic_set_bit(ag->flags, BT_HFP_AG_CREATING_SCO);
15501570
}
@@ -2702,7 +2722,7 @@ static int bt_hfp_ag_bcc_handler(struct bt_hfp_ag *ag, struct net_buf *buf)
27022722
return -ENOTSUP;
27032723
}
27042724

2705-
if (ag->sco_conn != NULL) {
2725+
if (atomic_ptr_get(&ag->sco_conn) != NULL) {
27062726
hfp_ag_unlock(ag);
27072727
return -ECONNREFUSED;
27082728
}
@@ -4300,9 +4320,8 @@ static void ag_sco_disconnected(struct bt_conn *conn, uint8_t reason)
43004320
__ASSERT(conn != NULL, "Invalid SCO conn");
43014321

43024322
ARRAY_FOR_EACH(bt_hfp_ag_pool, i) {
4303-
if (bt_hfp_ag_pool[i].sco_conn == conn) {
4304-
bt_conn_unref(bt_hfp_ag_pool[i].sco_conn);
4305-
bt_hfp_ag_pool[i].sco_conn = NULL;
4323+
if (atomic_ptr_cas(&bt_hfp_ag_pool[i].sco_conn, conn, NULL)) {
4324+
bt_conn_unref(conn);
43064325
}
43074326
}
43084327
}
@@ -5127,7 +5146,7 @@ int bt_hfp_ag_audio_connect(struct bt_hfp_ag *ag, uint8_t id)
51275146
}
51285147
}
51295148

5130-
if (ag->sco_conn != NULL) {
5149+
if (atomic_ptr_get(&ag->sco_conn) != NULL) {
51315150
LOG_ERR("Audio conenction has been connected");
51325151
hfp_ag_unlock(ag);
51335152
return -ECONNREFUSED;

‎subsys/bluetooth/host/classic/hfp_ag_internal.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,7 @@ struct bt_hfp_ag {
291291
struct bt_sco_chan sco_chan;
292292

293293
/* SCO connect */
294-
struct bt_conn *sco_conn;
294+
atomic_ptr_t sco_conn;
295295

296296
/* SDP discover params */
297297
struct bt_sdp_discover_params sdp_param;

‎subsys/bluetooth/host/classic/hfp_hf.c‎

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3446,8 +3446,8 @@ static void hfp_hf_sco_connected(struct bt_sco_chan *chan)
34463446
{
34473447
struct bt_hfp_hf *hf = CONTAINER_OF(chan, struct bt_hfp_hf, chan);
34483448

3449-
if (hf->sco_conn == NULL) {
3450-
hf->sco_conn = bt_conn_ref(chan->sco);
3449+
if (atomic_ptr_cas(&hf->sco_conn, NULL, chan->sco)) {
3450+
bt_conn_ref(chan->sco);
34513451
}
34523452

34533453
if ((bt_hf != NULL) && (bt_hf->sco_connected != NULL)) {
@@ -3458,14 +3458,15 @@ static void hfp_hf_sco_connected(struct bt_sco_chan *chan)
34583458
static void hfp_hf_sco_disconnected(struct bt_sco_chan *chan, uint8_t reason)
34593459
{
34603460
struct bt_hfp_hf *hf = CONTAINER_OF(chan, struct bt_hfp_hf, chan);
3461+
struct bt_conn *sco;
34613462

34623463
if ((bt_hf != NULL) && (bt_hf->sco_disconnected != NULL)) {
34633464
bt_hf->sco_disconnected(chan->sco, reason);
34643465
}
34653466

3466-
if (hf->sco_conn != NULL) {
3467-
bt_conn_unref(hf->sco_conn);
3468-
hf->sco_conn = NULL;
3467+
sco = atomic_ptr_set(&hf->sco_conn, NULL);
3468+
if (sco != NULL) {
3469+
bt_conn_unref(sco);
34693470
}
34703471
}
34713472

@@ -3512,7 +3513,9 @@ static int hfp_hf_create_sco(struct bt_hfp_hf *hf)
35123513
.connected = hfp_hf_sco_connected,
35133514
.disconnected = hfp_hf_sco_disconnected,
35143515
};
3516+
struct bt_conn *sco;
35153517
int err;
3518+
bool updated;
35163519

35173520
LOG_DBG("Creating SCO connection");
35183521

@@ -3524,8 +3527,22 @@ static int hfp_hf_create_sco(struct bt_hfp_hf *hf)
35243527
return err;
35253528
}
35263529

3527-
hf->sco_conn = bt_conn_create_sco(&hf->acl->br.dst, &hf->chan);
3528-
if (hf->sco_conn == NULL) {
3530+
sco = bt_conn_create_sco(&hf->acl->br.dst, &hf->chan);
3531+
updated = atomic_ptr_cas(&hf->sco_conn, NULL, sco);
3532+
if (!updated) {
3533+
LOG_WRN("SCO is not NULL (%p), target (%p)", atomic_ptr_get(&hf->sco_conn), sco);
3534+
__ASSERT(atomic_ptr_get(&hf->sco_conn) == sco,
3535+
"Concurrent SCO connection creation detected");
3536+
/* The `hf->sco_conn` has been udpated in callback `hfp_hf_sco_connected()`.
3537+
* The refernce count has been updated in callback `hfp_hf_sco_connected()`.
3538+
* The refernce count should be unreferred in this case.
3539+
*/
3540+
if (sco != NULL) {
3541+
bt_conn_unref(sco);
3542+
}
3543+
}
3544+
3545+
if (sco == NULL) {
35293546
LOG_ERR("Failed to create SCO");
35303547
return -ENOMEM;
35313548
}
@@ -3544,7 +3561,7 @@ int bt_hfp_hf_audio_connect(struct bt_hfp_hf *hf)
35443561
return -ENOTCONN;
35453562
}
35463563

3547-
if (hf->sco_conn != NULL) {
3564+
if (atomic_ptr_get(&hf->sco_conn) != NULL) {
35483565
LOG_ERR("Audio conenction has been connected");
35493566
return -ECONNREFUSED;
35503567
}
@@ -4487,9 +4504,8 @@ static void hf_sco_disconnected(struct bt_conn *conn, uint8_t reason)
44874504
__ASSERT(conn != NULL, "Invalid SCO conn");
44884505

44894506
ARRAY_FOR_EACH(bt_hfp_hf_pool, i) {
4490-
if (bt_hfp_hf_pool[i].sco_conn == conn) {
4491-
bt_conn_unref(bt_hfp_hf_pool[i].sco_conn);
4492-
bt_hfp_hf_pool[i].sco_conn = NULL;
4507+
if (atomic_ptr_cas(&bt_hfp_hf_pool[i].sco_conn, conn, NULL)) {
4508+
bt_conn_unref(conn);
44934509
}
44944510
}
44954511
}

‎subsys/bluetooth/host/classic/hfp_hf_internal.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ struct bt_hfp_hf {
215215
/* SCO Channel */
216216
struct bt_sco_chan chan;
217217
/* SCO connect */
218-
struct bt_conn *sco_conn;
218+
atomic_ptr_t sco_conn;
219219

220220
/* SDP discover params */
221221
struct bt_sdp_discover_params sdp_param;

0 commit comments

Comments
 (0)