Bluetooth: fix hci_conn lookup RCU usage + holding refcounts
Subsystems (2)
v2 cover Bluetooth: fix hci_conn lookup RCU usage + holding refcounts
Resending parts of
https://lore.kernel.org/linux-bluetooth/[email protected]/
plus additional hci_conn_params usage fix.
v2:
- Take hdev->lock instead of RCU in hci_update_scan_sync() to guard also the hdev->accept_list access.
- In unpair_device/disconnect_sync, take hdev->lock instead of RCU. This avoids potential access to uninitialized struct device.
In __hci_conn_add() it looks like we should move hci_conn_hash_add(hdev, conn); after hci_conn_init_sysfs(conn); so that the pattern
conn = hci_conn_lookup(...)
if (conn)
hci_conn_get(conn) can be done with RCU without needing hdev->lock. But better done in separate patch series.
Pauli Virtanen (4):
Bluetooth: hci_sync: extend conn_hash lookup critical sections
Bluetooth: mgmt: fix locking in unpair_device/disconnect_sync
Bluetooth: mgmt: hold reference for hci_conn in mgmt_pending_cmds
Bluetooth: hci_sync: hold hdev->lock for hci_conn_params lookups
net/bluetooth/hci_sync.c | 62 ++++++++++++++++++++++++++++++++++++---- net/bluetooth/mgmt.c | 42 ++++++++++++++++++++++++--- 2 files changed, 94 insertions(+), 10 deletions(-)
Signature · 1 line
2.55.0
PATCH v2 1/4 Bluetooth: hci_sync: extend conn_hash lookup critical sections
Using RCU-protected pointers outside the critical sections without refcount is incorrect and may result to UAF.
Extend critical section to cover both hci_conn_hash lookup and use of the returned conn.
Add surrounding rcu_read_lock() also when return value is not used, in preparation for RCU lockdep requirement to hci_lookup_le_connect().
This avoids concurrent deletion of the conn before we are done dereferencing it.
Also, make sure to hold hdev->lock when accessing hdev->accept_list.
---
Notes:
v2:
- take hdev->lock instead of RCU in hci_update_scan_sync() to
guard also the hdev->accept_list accessnet/bluetooth/hci_sync.c | 42 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-)
net/bluetooth/hci_sync.c +39 -3
@@ -1028,14 +1028,19 @@ static int hci_set_random_addr_sync(struct hci_dev *hdev, bdaddr_t *rpa)* In this kind of scenario skip the update and let the random * address be updated at the next cycle. */ +rcu_read_lock (); +if (bacmp (&hdev->random_addr,BDADDR_ANY ) && (hci_dev_test_flag (hdev,HCI_LE_ADV ) ||hci_lookup_le_connect (hdev))) {bt_dev_dbg (hdev,"Deferring random address update" );hci_dev_set_flag (hdev,HCI_RPA_EXPIRED ); +rcu_read_unlock ();return 0 ; } +rcu_read_unlock (); +return __hci_cmd_sync_status (hdev,HCI_OP_LE_SET_RANDOM_ADDR ,6 , rpa,HCI_CMD_TIMEOUT ); }
@@ -2621,12 +2626,17 @@ static int hci_pause_addr_resolution(struct hci_dev *hdev)/* Cannot disable addr resolution if scanning is enabled or * when initiating an LE connection. */ +rcu_read_lock (); +if (hci_dev_test_flag (hdev,HCI_LE_SCAN ) ||hci_lookup_le_connect (hdev)) { +rcu_read_unlock ();bt_dev_err (hdev,"Command not allowed when scan/LE connect" );return -EPERM ; } +rcu_read_unlock (); +/* Cannot disable addr resolution if advertising is enabled. */ err =hci_pause_advertising_sync (hdev);if (err) {
@@ -2764,6 +2774,8 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)if (hci_dev_test_flag (hdev,HCI_PA_SYNC )) {struct hci_conn *conn; +rcu_read_lock (); + conn =hci_conn_hash_lookup_create_pa_sync (hdev);if (conn) {struct conn_params pa;
@@ -2773,6 +2785,8 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)bacpy (&pa.addr, &conn->dst); pa.addr_type = conn->dst_type; +rcu_read_unlock (); +/* Clear first since there could be addresses left * behind. */
@@ -2782,6 +2796,8 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev) err =hci_le_add_accept_list_sync (hdev, &pa, &num_entries); goto done; + }else { +rcu_read_unlock (); } }
@@ -2792,10 +2808,13 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)* the controller. */ list_for_each_entry_safe (b, t, &hdev->le_accept_list, list) { -if (hci_conn_hash_lookup_le (hdev, &b->bdaddr, b->bdaddr_type)) +rcu_read_lock (); + +if (hci_conn_hash_lookup_le (hdev, &b->bdaddr, b->bdaddr_type)) { +rcu_read_unlock ();continue ; + } -/* Pointers not dereferenced, no locks needed */ pend_conn =hci_pend_le_action_lookup (&hdev->pend_le_conns, &b->bdaddr, b->bdaddr_type);
@@ -2803,6 +2822,8 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev) &b->bdaddr, b->bdaddr_type); +rcu_read_unlock (); +/* If the device is not likely to connect or report, * remove it from the acceptlist. */
@@ -2929,6 +2950,8 @@ static int hci_le_set_ext_scan_param_sync(struct hci_dev *hdev, u8 type,if (sent) {struct hci_conn *conn; +rcu_read_lock (); + conn =hci_conn_hash_lookup_ba (hdev,PA_LINK , &sent->bdaddr);if (conn) {
@@ -2953,8 +2976,12 @@ static int hci_le_set_ext_scan_param_sync(struct hci_dev *hdev, u8 type, phy++; } +rcu_read_unlock (); +if (num_phy) goto done; + }else { +rcu_read_unlock (); } } }
@@ -3205,12 +3232,16 @@ int hci_update_passive_scan_sync(struct hci_dev *hdev)/* If there is at least one pending LE connection, we should * keep the background scan running. */ +bool exists;/* If controller is connecting, we should not start scanning * since some controllers are not able to scan and connect at * the same time. */ -if (hci_lookup_le_connect (hdev)) +rcu_read_lock (); + exists =hci_lookup_le_connect (hdev); +rcu_read_unlock (); +if (exists)return 0 ;bt_dev_dbg (hdev,"start background scanning" );
@@ -3428,6 +3459,7 @@ int hci_write_fast_connectable_sync(struct hci_dev *hdev, bool enable) }static bool disconnected_accept_list_entries (struct hci_dev *hdev) + __must_hold(&hdev->lock) {struct bdaddr_list *b;
@@ -3468,12 +3500,16 @@ int hci_update_scan_sync(struct hci_dev *hdev)if (hdev->scanning_paused)return 0 ; +hci_dev_lock (hdev); +if (hci_dev_test_flag (hdev,HCI_CONNECTABLE ) ||disconnected_accept_list_entries (hdev)) scan =SCAN_PAGE ;else scan =SCAN_DISABLED ; +hci_dev_unlock (hdev); +if (hci_dev_test_flag (hdev,HCI_DISCOVERABLE )) scan |=SCAN_INQUIRY ;
Signature · 1 line
2.55.0
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=1121912
---Test result---
Test Summary:
CheckPatch PASS 2.79 seconds VerifyFixes PASS 0.12 seconds VerifySignedoff PASS 0.12 seconds GitLint FAIL 2.28 seconds SubjectPrefix PASS 0.47 seconds BuildKernel PASS 25.45 seconds CheckAllWarning PASS 27.97 seconds CheckSparse PASS 27.61 seconds BuildKernel32 PASS 25.65 seconds CheckKernelLLVM SKIP 0.00 seconds TestRunnerSetup PASS 463.78 seconds TestRunner_l2cap-tester PASS 58.97 seconds TestRunner_iso-tester PASS 79.59 seconds TestRunner_bnep-tester PASS 18.91 seconds TestRunner_mgmt-tester FAIL 214.01 seconds TestRunner_rfcomm-tester PASS 25.28 seconds TestRunner_sco-tester PASS 32.49 seconds TestRunner_ioctl-tester PASS 26.07 seconds TestRunner_mesh-tester FAIL 25.98 seconds TestRunner_smp-tester PASS 23.27 seconds TestRunner_userchan-tester PASS 20.07 seconds TestRunner_6lowpan-tester PASS 22.76 seconds IncrementalBuild PASS 34.18 seconds
Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v2,2/4] Bluetooth: mgmt: fix locking in unpair_device/disconnect_sync
16: B2 Line has trailing whitespace: " "
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis Output:
Clang not found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner Output:
Total: 494, Passed: 489 (99.0%), Failed: 1, Not Run: 4
Failed Test Cases
Read Exp Feature - Success Failed 0.248 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0
Failed Test Cases
Mesh - Send cancel - 1 Timed out 2.662 seconds Mesh - Send cancel - 2 Timed out 1.988 seconds
https://github.com/bluez/bluetooth-next/pull/401
---
Regards,
Linux Bluetooth
PATCH v2 2/4 Bluetooth: mgmt: fix locking in unpair_device/disconnect_sync
In reply to Pauli VirtanenDereferencing RCU-protected pointers outside critical sections is invalid and may lead to UAF.
Take hdev->lock for hci_conn lookup and hci_abort_conn(). Don't use RCU to ensure the conn is fully initialized at this point.
---
Notes:
v2:
- take hdev->lock instead of RCU, to make sure the conn object is fully initialized before hci_conn_abort_sync()
hci_conn_abort_sync() may not be called with hdev->lock held when called from hci_sync workqueue.
net/bluetooth/mgmt.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
net/bluetooth/mgmt.c +16 -0
@@ -3091,6 +3091,8 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)struct mgmt_cp_unpair_device *cp = cmd->param;struct hci_conn *conn; +hci_dev_lock (hdev); +if (cp->addr.type ==BDADDR_BREDR ) conn =hci_conn_hash_lookup_ba (hdev,ACL_LINK , &cp->addr.bdaddr);
@@ -3098,6 +3100,11 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data) conn =hci_conn_hash_lookup_le (hdev, &cp->addr.bdaddr,le_addr_type (cp->addr.type)); +if (conn) +hci_conn_get (conn); + +hci_dev_unlock (hdev); +if (!conn)return 0 ;
@@ -3105,6 +3112,7 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)* will clean up the connection no matter the error. */ hci_abort_conn (conn,HCI_ERROR_REMOTE_USER_TERM ); +hci_conn_put (conn);return 0 ; }
@@ -3252,6 +3260,8 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)struct mgmt_cp_disconnect *cp = cmd->param;struct hci_conn *conn; +hci_dev_lock (hdev); +if (cp->addr.type ==BDADDR_BREDR ) conn =hci_conn_hash_lookup_ba (hdev,ACL_LINK , &cp->addr.bdaddr);
@@ -3259,6 +3269,11 @@ static int disconnect_sync(struct hci_dev *hdev, void *data) conn =hci_conn_hash_lookup_le (hdev, &cp->addr.bdaddr,le_addr_type (cp->addr.type)); +if (conn) +hci_conn_get (conn); + +hci_dev_unlock (hdev); +if (!conn)return -ENOTCONN ;
@@ -3266,6 +3281,7 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)* will clean up the connection no matter the error. */ hci_abort_conn (conn,HCI_ERROR_REMOTE_USER_TERM ); +hci_conn_put (conn);return 0 ; }
Signature · 1 line
2.55.0
PATCH v2 3/4 Bluetooth: mgmt: hold reference for hci_conn in mgmt_pending_cmds
In reply to Pauli VirtanenDereferencing RCU-protected pointers outside critical sections is invalid and may lead to UAF. Use of hci_conn in hci_sync callbacks also needs to hold refcount to avoid UAF.
Take appropriate locks for hci_conn lookups, and take refcount for hci_conn pointers stored in mgmt_pending_cmd so that the pointer stays valid.
When accessing conn->state, ensure hdev->lock is held to avoid data race.
---
Notes:
v2:
- no change
net/bluetooth/mgmt.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
net/bluetooth/mgmt.c +22 -4
@@ -7404,6 +7404,9 @@ static void get_conn_info_complete(struct hci_dev *hdev, void *data, int err) rp.max_tx_power =HCI_TX_POWER_INVALID ; } +if (conn) +hci_conn_put (conn); +mgmt_cmd_complete (cmd->sk, cmd->hdev->id,MGMT_OP_GET_CONN_INFO , status, &rp,sizeof (rp));
@@ -7418,6 +7421,8 @@ static int get_conn_info_sync(struct hci_dev *hdev, void *data)int err;__le16 handle; +hci_dev_lock (hdev); +/* Make sure we are still connected */ if (cp->addr.type ==BDADDR_BREDR ) conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
@@ -7425,12 +7430,16 @@ static int get_conn_info_sync(struct hci_dev *hdev, void *data)else conn =hci_conn_hash_lookup_ba (hdev,LE_LINK , &cp->addr.bdaddr); -if (!conn || conn->state !=BT_CONNECTED ) +if (!conn || conn->state !=BT_CONNECTED ) { +hci_dev_unlock (hdev);return MGMT_STATUS_NOT_CONNECTED ; + } - cmd->user_data = conn; + cmd->user_data =hci_conn_get (conn); handle =cpu_to_le16 (conn->handle); +hci_dev_unlock (hdev); +/* Refresh RSSI each time */ err =hci_read_rssi_sync (hdev, handle);
@@ -7564,6 +7573,9 @@ static void get_clock_info_complete(struct hci_dev *hdev, void *data, int err) } complete: +if (conn) +hci_conn_put (conn); +mgmt_cmd_complete (cmd->sk, cmd->hdev->id, cmd->opcode, status, &rp,sizeof (rp));
@@ -7580,15 +7592,21 @@ static int get_clock_info_sync(struct hci_dev *hdev, void *data)memset (&hci_cp,0 ,sizeof (hci_cp));hci_read_clock_sync (hdev, &hci_cp); +hci_dev_lock (hdev); +/* Make sure connection still exists */ conn =hci_conn_hash_lookup_ba (hdev,ACL_LINK , &cp->addr.bdaddr); -if (!conn || conn->state !=BT_CONNECTED ) +if (!conn || conn->state !=BT_CONNECTED ) { +hci_dev_unlock (hdev);return MGMT_STATUS_NOT_CONNECTED ; + } - cmd->user_data = conn; + cmd->user_data =hci_conn_get (conn); hci_cp.handle =cpu_to_le16 (conn->handle); hci_cp.which =0x01 ;/* Piconet clock */ +hci_dev_unlock (hdev); +return hci_read_clock_sync (hdev, &hci_cp); }
Signature · 1 line
2.55.0
PATCH v2 4/4 Bluetooth: hci_sync: hold hdev->lock for hci_conn_params lookups
In reply to Pauli Virtanenhci_conn_params_lookup requires hdev->lock be held, otherwise the list iteration or param access is not safe.
Hold hdev->lock for params lookups in hci_sync.
---
Notes:
v2:
- no change
net/bluetooth/hci_sync.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
net/bluetooth/hci_sync.c +17 -3
@@ -6677,6 +6677,8 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)if (!hci_dev_test_flag (hdev,HCI_LE_SIMULTANEOUS_ROLES ))hci_pause_advertising_sync (hdev); +hci_dev_lock (hdev); + params =hci_conn_params_lookup (hdev, &conn->dst, conn->dst_type);if (params) { conn->le_conn_min_interval = params->conn_min_interval;
@@ -6690,6 +6692,8 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data) conn->le_supv_timeout = hdev->le_supv_timeout; } +hci_dev_unlock (hdev); +/* If controller is scanning, we stop it since some controllers are * not able to scan and connect at the same time. Also set the * HCI_LE_SCAN_INTERRUPTED flag so that the command complete
@@ -7247,13 +7251,13 @@ static void create_pa_complete(struct hci_dev *hdev, void *data, int err) }static int hci_le_past_params_sync (struct hci_dev *hdev,struct hci_conn *conn, -struct hci_conn *acl,struct bt_iso_qos *qos) +u16 acl_handle,struct bt_iso_qos *qos) {struct hci_cp_le_past_params cp;int err;memset (&cp,0 ,sizeof (cp)); - cp.handle =cpu_to_le16 (acl->handle); + cp.handle =cpu_to_le16 (acl_handle);/* An HCI_LE_Periodic_Advertising_Sync_Transfer_Received event is sent * to the Host. HCI_LE_Periodic_Advertising_Report events will be * enabled with duplicate filtering enabled.
@@ -7318,16 +7322,26 @@ static int hci_le_pa_create_sync(struct hci_dev *hdev, void *data)* 2. Check if that HCI_CONN_FLAG_PAST has been set which indicates that * user really intended to use PAST. */ +hci_dev_lock (hdev); + le =hci_conn_hash_lookup_le (hdev, &conn->dst, conn->dst_type);if (le) {struct hci_conn_params *params; +u16 le_handle = le->handle; params =hci_conn_params_lookup (hdev, &le->dst, le->dst_type);if (params && params->flags &HCI_CONN_FLAG_PAST ) { - err =hci_le_past_params_sync (hdev, conn, le, qos); +hci_dev_unlock (hdev); + + err =hci_le_past_params_sync (hdev, conn, le_handle, + qos);if (!err) goto done; + }else { +hci_dev_unlock (hdev); } + }else { +hci_dev_unlock (hdev); }/* SID has not been set listen for HCI_EV_LE_EXT_ADV_REPORT to update
Signature · 1 line
2.55.0
Luiz Augusto von Dentz
maintainerRe: [PATCH v2 4/4] Bluetooth: hci_sync: hold hdev->lock for hci_conn_params lookups
Hi Pauli,
Quoted 31 lines · Pauli Virtanen
hci_conn_params_lookuprequireshdev->lockbe held, otherwise the list iteration or param access is not safe.Hold
hdev->lockfor params lookups inhci_sync.Fixes c530569adc19 ("Bluetooth: hci_core: Introduce HCI_CONN_FLAG_PAST")Signed-off-by Pauli Virtanen <[email protected]>---
Notes:
v2:
- no changenet/bluetooth/hci_sync.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)net/bluetooth/hci_sync.c +2
-0@@ -6677,6 +6677,8 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)if (!hci_dev_test_flag (hdev,HCI_LE_SIMULTANEOUS_ROLES ))hci_pause_advertising_sync (hdev); +hci_dev_lock (hdev); + params =hci_conn_params_lookup (hdev, &conn->dst, conn->dst_type);if (params) { conn->le_conn_min_interval = params->conn_min_interval;
net/bluetooth/hci_sync.c static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data) · line 6695
Quoted 4 lines
net/bluetooth/hci_sync.c +1
-0@@ -6690,6 +6692,8 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data) conn->le_supv_timeout = hdev->le_supv_timeout; } +hci_dev_unlock (hdev);
Couldn't we make the hci_conn_params_lookup behave like hci_pend_le_action_lookup and take a RCU read lock and then make hci_conn_params_del use synchronize_rcu?
Quoted 21 lines
net/bluetooth/hci_sync.c +2
-2/* If controller is scanning, we stop it since some controllers are * not able to scan and connect at the same time. Also set the * HCI_LE_SCAN_INTERRUPTED flag so that the command complete @@ -7247,13 +7251,13 @@ static void create_pa_complete(struct hci_dev *hdev, void *data, int err) }static int hci_le_past_params_sync (struct hci_dev *hdev,struct hci_conn *conn, -struct hci_conn *acl,struct bt_iso_qos *qos) +u16 acl_handle,struct bt_iso_qos *qos) {struct hci_cp_le_past_params cp;int err;memset (&cp,0 ,sizeof (cp)); - cp.handle =cpu_to_le16 (acl->handle); + cp.handle =cpu_to_le16 (acl_handle);/* An HCI_LE_Periodic_Advertising_Sync_Transfer_Received event is sent * to the Host. HCI_LE_Periodic_Advertising_Report events will be * enabled with duplicate filtering enabled. @@ -7318,16 +7322,26 @@ static int hci_le_pa_create_sync(struct hci_dev *hdev, void *data)* 2. Check if that HCI_CONN_FLAG_PAST has been set which indicates that
net/bluetooth/hci_sync.c static int hci_le_pa_create_sync(struct hci_dev *hdev, void *data) · line 7332
Quoted 10 lines
net/bluetooth/hci_sync.c +3
-0* user really intended to use PAST. */ +hci_dev_lock (hdev); + le =hci_conn_hash_lookup_le (hdev, &conn->dst, conn->dst_type);if (le) {struct hci_conn_params *params; +u16 le_handle = le->handle; params =hci_conn_params_lookup (hdev, &le->dst, le->dst_type);
Aside from the RCU, couldn't we just unlock here so we don't have to do it in every branch below?
Quoted 19 lines
net/bluetooth/hci_sync.c +8
-2if (params && params->flags &HCI_CONN_FLAG_PAST ) { - err =hci_le_past_params_sync (hdev, conn, le, qos); +hci_dev_unlock (hdev); + + err =hci_le_past_params_sync (hdev, conn, le_handle, + qos);if (!err) goto done; + }else { +hci_dev_unlock (hdev); } + }else { +hci_dev_unlock (hdev); }/* SID has not been set listen for HCI_EV_LE_EXT_ADV_REPORT to update -- 2.55.0
Signature · 1 line
Luiz Augusto von Dentz
Re: [PATCH v2 4/4] Bluetooth: hci_sync: hold hdev->lock for hci_conn_params lookups
Hi,
ma, 2026-07-06 kello 12:56 -0400, Luiz Augusto von Dentz kirjoitti:
Quoted 37 lines
Hi Pauli,
Quoted 34 lines · Pauli Virtanen
hci_conn_params_lookuprequireshdev->lockbe held, otherwise the list iteration or param access is not safe.Hold
hdev->lockfor params lookups inhci_sync.Fixes c530569adc19 ("Bluetooth: hci_core: Introduce HCI_CONN_FLAG_PAST")Signed-off-by Pauli Virtanen <[email protected]>---
Notes:
v2:
- no changenet/bluetooth/hci_sync.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)net/bluetooth/hci_sync.c +2
-0@@ -6677,6 +6677,8 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)if (!hci_dev_test_flag (hdev,HCI_LE_SIMULTANEOUS_ROLES ))hci_pause_advertising_sync (hdev); +hci_dev_lock (hdev); + params =hci_conn_params_lookup (hdev, &conn->dst, conn->dst_type);if (params) { conn->le_conn_min_interval = params->conn_min_interval;@@ -6690,6 +6692,8 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data) conn->le_supv_timeout = hdev->le_supv_timeout; }
Quoted 5 lines
net/bluetooth/hci_sync.c static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data) · line 6695
Quoted 1 line
net/bluetooth/hci_sync.c +1
-0+hci_dev_unlock (hdev);Couldn't we make the
hci_conn_params_lookupbehave likehci_pend_le_action_lookupand take a RCU read lock and then makehci_conn_params_delusesynchronize_rcu?
Some of these fields are mutable, so to do conversion to RCU someone has to first think through data races in field access. In the hci_pend_le only few fields were accessed (as copied to struct conn_params in hci_sync.c) and it was in code that is rerun if something changes, so lockless READ_ONCE/WRITE_ONCE for mutable fields appeared OK there. Here, we'd maybe need RCU + spinlock for field access.
It seems to me hdev->lock would be the simplest option here.
Quoted 30 lines
Quoted 30 lines
net/bluetooth/hci_sync.c +5
-2/* If controller is scanning, we stop it since some controllers are * not able to scan and connect at the same time. Also set the * HCI_LE_SCAN_INTERRUPTED flag so that the command complete @@ -7247,13 +7251,13 @@ static void create_pa_complete(struct hci_dev *hdev, void *data, int err) }static int hci_le_past_params_sync (struct hci_dev *hdev,struct hci_conn *conn, -struct hci_conn *acl,struct bt_iso_qos *qos) +u16 acl_handle,struct bt_iso_qos *qos) {struct hci_cp_le_past_params cp;int err;memset (&cp,0 ,sizeof (cp)); - cp.handle =cpu_to_le16 (acl->handle); + cp.handle =cpu_to_le16 (acl_handle);/* An HCI_LE_Periodic_Advertising_Sync_Transfer_Received event is sent * to the Host. HCI_LE_Periodic_Advertising_Report events will be * enabled with duplicate filtering enabled. @@ -7318,16 +7322,26 @@ static int hci_le_pa_create_sync(struct hci_dev *hdev, void *data)* 2. Check if that HCI_CONN_FLAG_PAST has been set which indicates that * user really intended to use PAST. */ +hci_dev_lock (hdev); + le =hci_conn_hash_lookup_le (hdev, &conn->dst, conn->dst_type);if (le) {struct hci_conn_params *params; +u16 le_handle = le->handle;
Quoted 4 lines
net/bluetooth/hci_sync.c static int hci_le_pa_create_sync(struct hci_dev *hdev, void *data) · line 7332
Quoted 1 line
net/bluetooth/hci_sync.c +0
-0params =hci_conn_params_lookup (hdev, &le->dst, le->dst_type);Aside from the RCU, couldn't we just unlock here so we don't have to do it in every branch below?
Yes, we could copy params->flags and then unlock to make it simpler
-> v3
Quoted 20 lines
Quoted 19 lines
net/bluetooth/hci_sync.c +8
-2if (params && params->flags &HCI_CONN_FLAG_PAST ) { - err =hci_le_past_params_sync (hdev, conn, le, qos); +hci_dev_unlock (hdev); + + err =hci_le_past_params_sync (hdev, conn, le_handle, + qos);if (!err) goto done; + }else { +hci_dev_unlock (hdev); } + }else { +hci_dev_unlock (hdev); }/* SID has not been set listen for HCI_EV_LE_EXT_ADV_REPORT to update -- 2.55.0
Signature · 1 line
Pauli Virtanen