| Message ID | 20240213213123.403654-3-luiz.dentz@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] Bluetooth: hci_conn: Fix UAF Write in __hci_acl_create_connection_sync | expand |
Hi Luiz, > If connection is still queued/pending in the cmd_sync queue it means no > command has been generated and it should be safe to just dequeue the > callback when it is being aborted. > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > include/net/bluetooth/hci_core.h | 19 ++++++++ > include/net/bluetooth/hci_sync.h | 10 +++-- > net/bluetooth/hci_conn.c | 70 ++++++------------------------ > net/bluetooth/hci_sync.c | 74 ++++++++++++++++++++++++++++---- > 4 files changed, 102 insertions(+), 71 deletions(-) > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index 2bdea85b7c44..317d495cfcf5 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -1083,6 +1083,24 @@ static inline unsigned int hci_conn_count(struct hci_dev *hdev) > return c->acl_num + c->amp_num + c->sco_num + c->le_num + c->iso_num; > } > > +static inline bool hci_conn_valid(struct hci_dev *hdev, struct hci_conn *conn) > +{ > + struct hci_conn_hash *h = &hdev->conn_hash; > + struct hci_conn *c; > + > + rcu_read_lock(); > + > + list_for_each_entry_rcu(c, &h->list, list) { > + if (c == conn) { > + rcu_read_unlock(); > + return true; > + } > + } > + rcu_read_unlock(); > + > + return false; > +} > + > static inline __u8 hci_conn_lookup_type(struct hci_dev *hdev, __u16 handle) > { > struct hci_conn_hash *h = &hdev->conn_hash; > @@ -1493,6 +1511,7 @@ struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst, > struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst, > u8 dst_type, bool dst_resolved, u8 sec_level, > u16 conn_timeout, u8 role); > +void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status); > struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst, > u8 sec_level, u8 auth_type, > enum conn_reasons conn_reason, u16 timeout); > diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h > index 4ff4aa68ee19..6a9d063e9f47 100644 > --- a/include/net/bluetooth/hci_sync.h > +++ b/include/net/bluetooth/hci_sync.h > @@ -48,11 +48,11 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, > void *data, hci_cmd_sync_work_destroy_t destroy); > int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, > void *data, hci_cmd_sync_work_destroy_t destroy); > +int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, > + void *data, hci_cmd_sync_work_destroy_t destroy); > struct hci_cmd_sync_work_entry * > hci_cmd_sync_lookup_entry(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, > void *data, hci_cmd_sync_work_destroy_t destroy); > -int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, > - void *data, hci_cmd_sync_work_destroy_t destroy); > void hci_cmd_sync_cancel_entry(struct hci_dev *hdev, > struct hci_cmd_sync_work_entry *entry); > bool hci_cmd_sync_dequeue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, > @@ -139,8 +139,6 @@ struct hci_conn; > > int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason); > > -int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn); > - > int hci_le_create_cis_sync(struct hci_dev *hdev); > > int hci_le_remove_cig_sync(struct hci_dev *hdev, u8 handle); > @@ -152,3 +150,7 @@ int hci_le_big_terminate_sync(struct hci_dev *hdev, u8 handle); > int hci_le_pa_terminate_sync(struct hci_dev *hdev, u16 handle); > > int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn); > + > +int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn); > + > +int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn); > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > index 587eb27f374c..21e0b4064d05 100644 > --- a/net/bluetooth/hci_conn.c > +++ b/net/bluetooth/hci_conn.c > @@ -68,7 +68,7 @@ static const struct sco_param esco_param_msbc[] = { > }; > > /* This function requires the caller holds hdev->lock */ > -static void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status) > +void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status) > { > struct hci_conn_params *params; > struct hci_dev *hdev = conn->hdev; > @@ -1124,6 +1124,9 @@ void hci_conn_del(struct hci_conn *conn) > * rest of hci_conn_del. > */ > hci_conn_cleanup(conn); > + > + /* Dequeue callbacks using connection pointer as data */ > + hci_cmd_sync_dequeue(hdev, NULL, conn, NULL); > } > > struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src, uint8_t src_type) > @@ -1258,53 +1261,6 @@ u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle) > return 0; > } > > -static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) > -{ > - struct hci_conn *conn; > - u16 handle = PTR_UINT(data); > - > - conn = hci_conn_hash_lookup_handle(hdev, handle); > - if (!conn) > - return; > - > - bt_dev_dbg(hdev, "err %d", err); > - > - hci_dev_lock(hdev); > - > - if (!err) { > - hci_connect_le_scan_cleanup(conn, 0x00); > - goto done; > - } > - > - /* Check if connection is still pending */ > - if (conn != hci_lookup_le_connect(hdev)) > - goto done; > - > - /* Flush to make sure we send create conn cancel command if needed */ > - flush_delayed_work(&conn->le_conn_timeout); > - hci_conn_failed(conn, bt_status(err)); > - > -done: > - hci_dev_unlock(hdev); > -} > - > -static int hci_connect_le_sync(struct hci_dev *hdev, void *data) > -{ > - struct hci_conn *conn; > - u16 handle = PTR_UINT(data); > - > - conn = hci_conn_hash_lookup_handle(hdev, handle); > - if (!conn) > - return 0; > - > - bt_dev_dbg(hdev, "conn %p", conn); > - > - clear_bit(HCI_CONN_SCANNING, &conn->flags); > - conn->state = BT_CONNECT; > - > - return hci_le_create_conn_sync(hdev, conn); > -} > - > struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst, > u8 dst_type, bool dst_resolved, u8 sec_level, > u16 conn_timeout, u8 role) > @@ -1371,9 +1327,7 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst, > conn->sec_level = BT_SECURITY_LOW; > conn->conn_timeout = conn_timeout; > Might want to add a + if (conn->state != BT_OPEN && conn->state != BT_CLOSED) + return conn; before setting the conn->dst_type while at it, similar to how it's in hci_connect_acl(). > - err = hci_cmd_sync_queue(hdev, hci_connect_le_sync, > - UINT_PTR(conn->handle), > - create_le_conn_complete); > + err = hci_connect_le_sync(hdev, conn); > if (err) { > hci_conn_del(conn); > return ERR_PTR(err); > @@ -2909,12 +2863,10 @@ u32 hci_conn_get_phy(struct hci_conn *conn) > > static int abort_conn_sync(struct hci_dev *hdev, void *data) > { > - struct hci_conn *conn; > - u16 handle = PTR_UINT(data); > + struct hci_conn *conn = data; > > - conn = hci_conn_hash_lookup_handle(hdev, handle); > - if (!conn) > - return 0; > + if (!hci_conn_valid(hdev, conn)) > + return -ECANCELED; > > return hci_abort_conn_sync(hdev, conn, conn->abort_reason); > } > @@ -2949,8 +2901,10 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason) > hci_cmd_sync_cancel(hdev, -ECANCELED); > break; > } > + /* Cancel connect attempt if still queued/pending */ > + } else if (!hci_cancel_connect_sync(hdev, conn)) { > + return 0; > } > > - return hci_cmd_sync_queue(hdev, abort_conn_sync, UINT_PTR(conn->handle), > - NULL); > + return hci_cmd_sync_queue_once(hdev, abort_conn_sync, conn, NULL); > } > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > index 5b314bf844f8..b7d8e99e2a30 100644 > --- a/net/bluetooth/hci_sync.c > +++ b/net/bluetooth/hci_sync.c > @@ -6285,12 +6285,21 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev, > conn->conn_timeout, NULL); > } > > -int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn) > +static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data) > { > struct hci_cp_le_create_conn cp; > struct hci_conn_params *params; > u8 own_addr_type; > int err; > + struct hci_conn *conn = data; > + > + if (!hci_conn_valid(hdev, conn)) > + return -ECANCELED; > + > + bt_dev_dbg(hdev, "conn %p", conn); > + > + clear_bit(HCI_CONN_SCANNING, &conn->flags); > + conn->state = BT_CONNECT; > > /* If requested to connect as peripheral use directed advertising */ > if (conn->role == HCI_ROLE_SLAVE) { > @@ -6611,16 +6620,11 @@ int hci_update_adv_data(struct hci_dev *hdev, u8 instance) > > static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data) > { > - struct hci_conn *conn; > - u16 handle = PTR_UINT(data); > + struct hci_conn *conn = data; > struct inquiry_entry *ie; > struct hci_cp_create_conn cp; > int err; > > - conn = hci_conn_hash_lookup_handle(hdev, handle); > - if (!conn) > - return 0; > - > /* Many controllers disallow HCI Create Connection while it is doing > * HCI Inquiry. So we cancel the Inquiry first before issuing HCI Create > * Connection. This may cause the MGMT discovering state to become false > @@ -6679,6 +6683,58 @@ static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data) > > int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn) > { > - return hci_cmd_sync_queue(hdev, hci_acl_create_conn_sync, > - UINT_PTR(conn->handle), NULL); > + return hci_cmd_sync_queue_once(hdev, hci_acl_create_conn_sync, conn, > + NULL); > +} > + > +static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) > +{ > + struct hci_conn *conn = data; > + > + bt_dev_dbg(hdev, "err %d", err); > + > + if (err == -ECANCELED) > + return; > + > + hci_dev_lock(hdev); > + > + if (!err) { > + hci_connect_le_scan_cleanup(conn, 0x00); > + goto done; > + } > + > + /* Check if connection is still pending */ > + if (conn != hci_lookup_le_connect(hdev)) > + goto done; > + > + /* Flush to make sure we send create conn cancel command if needed */ > + flush_delayed_work(&conn->le_conn_timeout); > + hci_conn_failed(conn, bt_status(err)); > + > +done: > + hci_dev_unlock(hdev); > +} > + > +int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn) > +{ > + return hci_cmd_sync_queue_once(hdev, hci_le_create_conn_sync, conn, > + create_le_conn_complete); > +} > + > +int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn) > +{ > + if (conn->state != BT_OPEN) > + return -EINVAL; > + > + switch (conn->type) { > + case ACL_LINK: > + return !hci_cmd_sync_dequeue_once(hdev, > + hci_acl_create_conn_sync, > + conn, NULL); > + case LE_LINK: > + return !hci_cmd_sync_dequeue_once(hdev, hci_le_create_conn_sync, > + conn, create_le_conn_complete); > + } > + > + return -ENOENT; > } > -- > 2.43.0 Thanks for sending those patches, this is pretty much exactly what I had in mind! I came up with a slightly different cancel function for the queued work, one that also cancels the ongoing work. I'm not sure if it makes too much sense, because it means adding careful -ECANCELED handling to those sync_work callbacks. The nice thing about it is that it should also allow getting rid of the hci_cmd_sync_cancel(hdev, -ECANCELED) in hci_abort_conn(). Adding the patch below, feel free to reuse whatever you like! Cheers, Jonas diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index 067d445701..a2b14f6be1 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -154,6 +154,13 @@ struct sk_buff *__hci_cmd_sync_sk(struct hci_dev *hdev, u16 opcode, u32 plen, bt_dev_dbg(hdev, "Opcode 0x%4x", opcode); + if (hdev->cur_hci_sync_work_cancelled) { + hdev->cur_hci_sync_work_cancelled = false; + + return ERR_PTR(-ECANCELED); + } + mutex_unlock(&hdev->cmd_sync_work_lock); + hci_req_init(&req, hdev); hci_cmd_sync_add(&req, opcode, plen, param, event, sk); @@ -303,9 +310,29 @@ static void hci_cmd_sync_work(struct work_struct *work) int err; hci_req_sync_lock(hdev); + + mutex_lock(&hdev->cmd_sync_work_lock); + hdev->cur_hci_sync_work_func = entry->func; + hdev->cur_hci_sync_work_data = entry->data; + mutex_unlock(&hdev->cmd_sync_work_lock); + err = entry->func(hdev, entry->data); if (entry->destroy) entry->destroy(hdev, entry->data, err); + + mutex_lock(&hdev->cmd_sync_work_lock); + hdev->cur_hci_sync_work_func = NULL; + hdev->cur_hci_sync_work_data = NULL; + + if (hdev->cur_hci_sync_work_cancelled) { + /* If cur_hci_sync_work_cancelled is still set at this point, + * no more request was sent and the work func has never been + * notified of our cancellation. + */ + hdev->cur_hci_sync_work_cancelled = false; + } + mutex_unlock(&hdev->cmd_sync_work_lock); + hci_req_sync_unlock(hdev); } @@ -735,6 +762,87 @@ int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, } EXPORT_SYMBOL(hci_cmd_sync_queue); +bool hci_cmd_sync_has_cmd(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, + void *data) +{ + struct hci_cmd_sync_work_entry *entry; + + mutex_lock(&hdev->cmd_sync_work_lock); + if (hdev->cur_hci_sync_work_func == func && + hdev->cur_hci_sync_work_data == data) { + mutex_unlock(&hdev->cmd_sync_work_lock); + return true; + } + + list_for_each_entry(entry, &hdev->cmd_sync_work_list, list) { + if (entry->func == func && entry->data == data) { + mutex_unlock(&hdev->cmd_sync_work_lock); + return true; + } + } + mutex_unlock(&hdev->cmd_sync_work_lock); + + return false; +} + +void hci_cmd_sync_cancel_cmd(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, + void *data) +{ + struct hci_cmd_sync_work_entry *entry; + bool work_already_ongoing; + + mutex_lock(&hdev->cmd_sync_work_lock); + if (hdev->cur_hci_sync_work_func == func && + hdev->cur_hci_sync_work_data == data) { + /* If the command is already ongoing, we check if we're currently + * sending a async HCI request. If we are, we can cancel that + * and hope that the hci_cmd_sync_work_func is handling -ECANCELED. + */ + + if (hdev->req_status == HCI_REQ_PEND) { + /* If we're already executing a request, cancel that request. + * This will signal cancellation to the work func which sent + * the request in the first place. + */ + __hci_cmd_sync_cancel(hdev, -ECANCELED); + } else { + /* Otherwise, just set a flag which will cancel the next + * request. Just as above, this will then signal cancellation + * to the work func. + */ + hdev->cur_hci_sync_work_cancelled = true; + } + + mutex_unlock(&hdev->cmd_sync_work_lock); + + return; + } else { + /* Or is it still queued? Then we remove it from the queue and + * execute the destroy callback. + */ + list_for_each_entry(entry, &hdev->cmd_sync_work_list, list) { + if (entry->func == func && entry->data == data) { + if (entry->destroy) + entry->destroy(hdev, entry->data, -ECANCELED); + list_del(&entry->list); + kfree(entry); + + mutex_unlock(&hdev->cmd_sync_work_lock); + + if (list_empty(&hdev->cmd_sync_work_list)) { + cancel_work_sync(&hdev->cmd_sync_work); + cancel_work_sync(&hdev->reenable_adv_work); + } + + return; + } + } + + } + + mutex_unlock(&hdev->cmd_sync_work_lock); +} + int hci_update_eir_sync(struct hci_dev *hdev) { struct hci_cp_write_eir cp; @@ -6262,15 +6370,20 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn) } /* Pause advertising while doing directed advertising. */ - hci_pause_advertising_sync(hdev); + err = hci_pause_advertising_sync(hdev); + if (err == -ECANCELED) + goto done; err = hci_le_directed_advertising_sync(hdev, conn); goto done; } /* Disable advertising if simultaneous roles is not in use. */ - if (!hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES)) - hci_pause_advertising_sync(hdev); + if (!hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES)) { + err = hci_pause_advertising_sync(hdev); + if (err == -ECANCELED) + goto done; + } params = hci_conn_params_lookup(hdev, &conn->dst, conn->dst_type); if (params) { @@ -6292,7 +6405,10 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn) * state. */ if (hci_dev_test_flag(hdev, HCI_LE_SCAN)) { - hci_scan_disable_sync(hdev); + err = hci_scan_disable_sync(hdev); + if (err == -ECANCELED) + goto done; + hci_dev_set_flag(hdev, HCI_LE_SCAN_INTERRUPTED); } @@ -6336,11 +6452,10 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn) HCI_EV_LE_ENHANCED_CONN_COMPLETE : HCI_EV_LE_CONN_COMPLETE, conn->conn_timeout, NULL); + if (err == -ECANCELED || err == -ETIMEDOUT) + hci_le_connect_cancel_sync(hdev, conn, conn->abort_reason || 0x00); done: - if (err == -ETIMEDOUT) - hci_le_connect_cancel_sync(hdev, conn, 0x00); - /* Re-enable advertising after the connection attempt is finished. */ hci_resume_advertising_sync(hdev); return err; @@ -6586,7 +6701,9 @@ static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data) if (test_bit(HCI_INQUIRY, &hdev->flags)) { err = __hci_cmd_sync_status(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL, HCI_CMD_TIMEOUT); - if (err) + if (err == -ECANCELED) + return -ECANCELED; + else if (err) bt_dev_warn(hdev, "Failed to cancel inquiry %d", err); } @@ -6625,15 +6742,10 @@ static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data) HCI_EV_CONN_COMPLETE, HCI_ACL_CONN_TIMEOUT, NULL); - if (err == -ETIMEDOUT) - hci_abort_conn_sync(hdev, conn, HCI_ERROR_LOCAL_HOST_TERM); + if (err == -ECANCELED || err == -ETIMEDOUT) { + hci_connect_cancel_sync(hdev, conn, conn->abort_reason || HCI_ERROR_LOCAL_HOST_TERM); + return err; + } return err; }
Hi Jonas, On Tue, Feb 13, 2024 at 6:47 PM Jonas Dreßler <verdre@v0yd.nl> wrote: > > Hi Luiz, > > > If connection is still queued/pending in the cmd_sync queue it means no > > command has been generated and it should be safe to just dequeue the > > callback when it is being aborted. > > > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > --- > > include/net/bluetooth/hci_core.h | 19 ++++++++ > > include/net/bluetooth/hci_sync.h | 10 +++-- > > net/bluetooth/hci_conn.c | 70 ++++++------------------------ > > net/bluetooth/hci_sync.c | 74 ++++++++++++++++++++++++++++---- > > 4 files changed, 102 insertions(+), 71 deletions(-) > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > index 2bdea85b7c44..317d495cfcf5 100644 > > --- a/include/net/bluetooth/hci_core.h > > +++ b/include/net/bluetooth/hci_core.h > > @@ -1083,6 +1083,24 @@ static inline unsigned int hci_conn_count(struct hci_dev *hdev) > > return c->acl_num + c->amp_num + c->sco_num + c->le_num + c->iso_num; > > } > > > > +static inline bool hci_conn_valid(struct hci_dev *hdev, struct hci_conn *conn) > > +{ > > + struct hci_conn_hash *h = &hdev->conn_hash; > > + struct hci_conn *c; > > + > > + rcu_read_lock(); > > + > > + list_for_each_entry_rcu(c, &h->list, list) { > > + if (c == conn) { > > + rcu_read_unlock(); > > + return true; > > + } > > + } > > + rcu_read_unlock(); > > + > > + return false; > > +} > > + > > static inline __u8 hci_conn_lookup_type(struct hci_dev *hdev, __u16 handle) > > { > > struct hci_conn_hash *h = &hdev->conn_hash; > > @@ -1493,6 +1511,7 @@ struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst, > > struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst, > > u8 dst_type, bool dst_resolved, u8 sec_level, > > u16 conn_timeout, u8 role); > > +void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status); > > struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst, > > u8 sec_level, u8 auth_type, > > enum conn_reasons conn_reason, u16 timeout); > > diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h > > index 4ff4aa68ee19..6a9d063e9f47 100644 > > --- a/include/net/bluetooth/hci_sync.h > > +++ b/include/net/bluetooth/hci_sync.h > > @@ -48,11 +48,11 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, > > void *data, hci_cmd_sync_work_destroy_t destroy); > > int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, > > void *data, hci_cmd_sync_work_destroy_t destroy); > > +int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, > > + void *data, hci_cmd_sync_work_destroy_t destroy); > > struct hci_cmd_sync_work_entry * > > hci_cmd_sync_lookup_entry(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, > > void *data, hci_cmd_sync_work_destroy_t destroy); > > -int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, > > - void *data, hci_cmd_sync_work_destroy_t destroy); > > void hci_cmd_sync_cancel_entry(struct hci_dev *hdev, > > struct hci_cmd_sync_work_entry *entry); > > bool hci_cmd_sync_dequeue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, > > @@ -139,8 +139,6 @@ struct hci_conn; > > > > int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason); > > > > -int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn); > > - > > int hci_le_create_cis_sync(struct hci_dev *hdev); > > > > int hci_le_remove_cig_sync(struct hci_dev *hdev, u8 handle); > > @@ -152,3 +150,7 @@ int hci_le_big_terminate_sync(struct hci_dev *hdev, u8 handle); > > int hci_le_pa_terminate_sync(struct hci_dev *hdev, u16 handle); > > > > int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn); > > + > > +int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn); > > + > > +int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn); > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > > index 587eb27f374c..21e0b4064d05 100644 > > --- a/net/bluetooth/hci_conn.c > > +++ b/net/bluetooth/hci_conn.c > > @@ -68,7 +68,7 @@ static const struct sco_param esco_param_msbc[] = { > > }; > > > > /* This function requires the caller holds hdev->lock */ > > -static void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status) > > +void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status) > > { > > struct hci_conn_params *params; > > struct hci_dev *hdev = conn->hdev; > > @@ -1124,6 +1124,9 @@ void hci_conn_del(struct hci_conn *conn) > > * rest of hci_conn_del. > > */ > > hci_conn_cleanup(conn); > > + > > + /* Dequeue callbacks using connection pointer as data */ > > + hci_cmd_sync_dequeue(hdev, NULL, conn, NULL); > > } > > > > struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src, uint8_t src_type) > > @@ -1258,53 +1261,6 @@ u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle) > > return 0; > > } > > > > -static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) > > -{ > > - struct hci_conn *conn; > > - u16 handle = PTR_UINT(data); > > - > > - conn = hci_conn_hash_lookup_handle(hdev, handle); > > - if (!conn) > > - return; > > - > > - bt_dev_dbg(hdev, "err %d", err); > > - > > - hci_dev_lock(hdev); > > - > > - if (!err) { > > - hci_connect_le_scan_cleanup(conn, 0x00); > > - goto done; > > - } > > - > > - /* Check if connection is still pending */ > > - if (conn != hci_lookup_le_connect(hdev)) > > - goto done; > > - > > - /* Flush to make sure we send create conn cancel command if needed */ > > - flush_delayed_work(&conn->le_conn_timeout); > > - hci_conn_failed(conn, bt_status(err)); > > - > > -done: > > - hci_dev_unlock(hdev); > > -} > > - > > -static int hci_connect_le_sync(struct hci_dev *hdev, void *data) > > -{ > > - struct hci_conn *conn; > > - u16 handle = PTR_UINT(data); > > - > > - conn = hci_conn_hash_lookup_handle(hdev, handle); > > - if (!conn) > > - return 0; > > - > > - bt_dev_dbg(hdev, "conn %p", conn); > > - > > - clear_bit(HCI_CONN_SCANNING, &conn->flags); > > - conn->state = BT_CONNECT; > > - > > - return hci_le_create_conn_sync(hdev, conn); > > -} > > - > > struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst, > > u8 dst_type, bool dst_resolved, u8 sec_level, > > u16 conn_timeout, u8 role) > > @@ -1371,9 +1327,7 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst, > > conn->sec_level = BT_SECURITY_LOW; > > conn->conn_timeout = conn_timeout; > > > > Might want to add a > > + if (conn->state != BT_OPEN && conn->state != BT_CLOSED) > + return conn; > > before setting the conn->dst_type while at it, similar to how it's > in hci_connect_acl(). Hmm but shall never be the case since we have the following check before it: /* Since the controller supports only one LE connection attempt at a * time, we return -EBUSY if there is any connection attempt running. */ if (hci_lookup_le_connect(hdev)) return ERR_PTR(-EBUSY); > > > - err = hci_cmd_sync_queue(hdev, hci_connect_le_sync, > > - UINT_PTR(conn->handle), > > - create_le_conn_complete); > > + err = hci_connect_le_sync(hdev, conn); > > if (err) { > > hci_conn_del(conn); > > return ERR_PTR(err); > > @@ -2909,12 +2863,10 @@ u32 hci_conn_get_phy(struct hci_conn *conn) > > > > static int abort_conn_sync(struct hci_dev *hdev, void *data) > > { > > - struct hci_conn *conn; > > - u16 handle = PTR_UINT(data); > > + struct hci_conn *conn = data; > > > > - conn = hci_conn_hash_lookup_handle(hdev, handle); > > - if (!conn) > > - return 0; > > + if (!hci_conn_valid(hdev, conn)) > > + return -ECANCELED; > > > > return hci_abort_conn_sync(hdev, conn, conn->abort_reason); > > } > > @@ -2949,8 +2901,10 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason) > > hci_cmd_sync_cancel(hdev, -ECANCELED); > > break; > > } > > + /* Cancel connect attempt if still queued/pending */ > > + } else if (!hci_cancel_connect_sync(hdev, conn)) { > > + return 0; > > } > > > > - return hci_cmd_sync_queue(hdev, abort_conn_sync, UINT_PTR(conn->handle), > > - NULL); > > + return hci_cmd_sync_queue_once(hdev, abort_conn_sync, conn, NULL); > > } > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > > index 5b314bf844f8..b7d8e99e2a30 100644 > > --- a/net/bluetooth/hci_sync.c > > +++ b/net/bluetooth/hci_sync.c > > @@ -6285,12 +6285,21 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev, > > conn->conn_timeout, NULL); > > } > > > > -int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn) > > +static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data) > > { > > struct hci_cp_le_create_conn cp; > > struct hci_conn_params *params; > > u8 own_addr_type; > > int err; > > + struct hci_conn *conn = data; > > + > > + if (!hci_conn_valid(hdev, conn)) > > + return -ECANCELED; > > + > > + bt_dev_dbg(hdev, "conn %p", conn); > > + > > + clear_bit(HCI_CONN_SCANNING, &conn->flags); > > + conn->state = BT_CONNECT; > > > > /* If requested to connect as peripheral use directed advertising */ > > if (conn->role == HCI_ROLE_SLAVE) { > > @@ -6611,16 +6620,11 @@ int hci_update_adv_data(struct hci_dev *hdev, u8 instance) > > > > static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data) > > { > > - struct hci_conn *conn; > > - u16 handle = PTR_UINT(data); > > + struct hci_conn *conn = data; > > struct inquiry_entry *ie; > > struct hci_cp_create_conn cp; > > int err; > > > > - conn = hci_conn_hash_lookup_handle(hdev, handle); > > - if (!conn) > > - return 0; > > - > > /* Many controllers disallow HCI Create Connection while it is doing > > * HCI Inquiry. So we cancel the Inquiry first before issuing HCI Create > > * Connection. This may cause the MGMT discovering state to become false > > @@ -6679,6 +6683,58 @@ static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data) > > > > int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn) > > { > > - return hci_cmd_sync_queue(hdev, hci_acl_create_conn_sync, > > - UINT_PTR(conn->handle), NULL); > > + return hci_cmd_sync_queue_once(hdev, hci_acl_create_conn_sync, conn, > > + NULL); > > +} > > + > > +static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) > > +{ > > + struct hci_conn *conn = data; > > + > > + bt_dev_dbg(hdev, "err %d", err); > > + > > + if (err == -ECANCELED) > > + return; > > + > > + hci_dev_lock(hdev); > > + > > + if (!err) { > > + hci_connect_le_scan_cleanup(conn, 0x00); > > + goto done; > > + } > > + > > + /* Check if connection is still pending */ > > + if (conn != hci_lookup_le_connect(hdev)) > > + goto done; > > + > > + /* Flush to make sure we send create conn cancel command if needed */ > > + flush_delayed_work(&conn->le_conn_timeout); > > + hci_conn_failed(conn, bt_status(err)); > > + > > +done: > > + hci_dev_unlock(hdev); > > +} > > + > > +int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn) > > +{ > > + return hci_cmd_sync_queue_once(hdev, hci_le_create_conn_sync, conn, > > + create_le_conn_complete); > > +} > > + > > +int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn) > > +{ > > + if (conn->state != BT_OPEN) > > + return -EINVAL; > > + > > + switch (conn->type) { > > + case ACL_LINK: > > + return !hci_cmd_sync_dequeue_once(hdev, > > + hci_acl_create_conn_sync, > > + conn, NULL); > > + case LE_LINK: > > + return !hci_cmd_sync_dequeue_once(hdev, hci_le_create_conn_sync, > > + conn, create_le_conn_complete); > > + } > > + > > + return -ENOENT; > > } > > -- > > 2.43.0 > > Thanks for sending those patches, this is pretty much exactly what I had in mind! > > I came up with a slightly different cancel function for the queued work, one that > also cancels the ongoing work. I'm not sure if it makes too much sense, because it > means adding careful -ECANCELED handling to those sync_work callbacks. > > The nice thing about it is that it should also allow getting rid of the > hci_cmd_sync_cancel(hdev, -ECANCELED) in hci_abort_conn(). > > Adding the patch below, feel free to reuse whatever you like! > > Cheers, > Jonas > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > index 067d445701..a2b14f6be1 100644 > --- a/net/bluetooth/hci_sync.c > +++ b/net/bluetooth/hci_sync.c > @@ -154,6 +154,13 @@ struct sk_buff *__hci_cmd_sync_sk(struct hci_dev *hdev, u16 opcode, u32 plen, > > bt_dev_dbg(hdev, "Opcode 0x%4x", opcode); > > + if (hdev->cur_hci_sync_work_cancelled) { > + hdev->cur_hci_sync_work_cancelled = false; > + > + return ERR_PTR(-ECANCELED); > + } > + mutex_unlock(&hdev->cmd_sync_work_lock); > + > hci_req_init(&req, hdev); > > hci_cmd_sync_add(&req, opcode, plen, param, event, sk); > @@ -303,9 +310,29 @@ static void hci_cmd_sync_work(struct work_struct *work) > int err; > > hci_req_sync_lock(hdev); > + > + mutex_lock(&hdev->cmd_sync_work_lock); > + hdev->cur_hci_sync_work_func = entry->func; > + hdev->cur_hci_sync_work_data = entry->data; > + mutex_unlock(&hdev->cmd_sync_work_lock); > + > err = entry->func(hdev, entry->data); > if (entry->destroy) > entry->destroy(hdev, entry->data, err); > + > + mutex_lock(&hdev->cmd_sync_work_lock); > + hdev->cur_hci_sync_work_func = NULL; > + hdev->cur_hci_sync_work_data = NULL; > + > + if (hdev->cur_hci_sync_work_cancelled) { > + /* If cur_hci_sync_work_cancelled is still set at this point, > + * no more request was sent and the work func has never been > + * notified of our cancellation. > + */ > + hdev->cur_hci_sync_work_cancelled = false; > + } > + mutex_unlock(&hdev->cmd_sync_work_lock); Not really following this code, do you want to be able to cancel callbacks that are actually executing, rather than queued? > hci_req_sync_unlock(hdev); > } > > @@ -735,6 +762,87 @@ int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, > } > EXPORT_SYMBOL(hci_cmd_sync_queue); > > +bool hci_cmd_sync_has_cmd(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, > + void *data) > +{ > + struct hci_cmd_sync_work_entry *entry; > + > + mutex_lock(&hdev->cmd_sync_work_lock); > + if (hdev->cur_hci_sync_work_func == func && > + hdev->cur_hci_sync_work_data == data) { > + mutex_unlock(&hdev->cmd_sync_work_lock); > + return true; > + } > + > + list_for_each_entry(entry, &hdev->cmd_sync_work_list, list) { > + if (entry->func == func && entry->data == data) { > + mutex_unlock(&hdev->cmd_sync_work_lock); > + return true; > + } > + } > + mutex_unlock(&hdev->cmd_sync_work_lock); > + > + return false; > +} > + > +void hci_cmd_sync_cancel_cmd(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, > + void *data) > +{ > + struct hci_cmd_sync_work_entry *entry; > + bool work_already_ongoing; > + > + mutex_lock(&hdev->cmd_sync_work_lock); > + if (hdev->cur_hci_sync_work_func == func && > + hdev->cur_hci_sync_work_data == data) { > + /* If the command is already ongoing, we check if we're currently > + * sending a async HCI request. If we are, we can cancel that > + * and hope that the hci_cmd_sync_work_func is handling -ECANCELED. > + */ > + > + if (hdev->req_status == HCI_REQ_PEND) { > + /* If we're already executing a request, cancel that request. > + * This will signal cancellation to the work func which sent > + * the request in the first place. > + */ > + __hci_cmd_sync_cancel(hdev, -ECANCELED); > + } else { > + /* Otherwise, just set a flag which will cancel the next > + * request. Just as above, this will then signal cancellation > + * to the work func. > + */ > + hdev->cur_hci_sync_work_cancelled = true; > + } It might be better to save the executing entry at hdev and then make the lookup_entry return it if the parameters match so it can be cancelled with cancel_entry, that said perhaps it would be better to just cancel the work if -ECANCELED is received so we don't have to keep checking if it is returned on every command the callback generates. > + mutex_unlock(&hdev->cmd_sync_work_lock); > + > + return; > + } else { > + /* Or is it still queued? Then we remove it from the queue and > + * execute the destroy callback. > + */ > + list_for_each_entry(entry, &hdev->cmd_sync_work_list, list) { > + if (entry->func == func && entry->data == data) { > + if (entry->destroy) > + entry->destroy(hdev, entry->data, -ECANCELED); > + list_del(&entry->list); > + kfree(entry); > + > + mutex_unlock(&hdev->cmd_sync_work_lock); > + > + if (list_empty(&hdev->cmd_sync_work_list)) { > + cancel_work_sync(&hdev->cmd_sync_work); > + cancel_work_sync(&hdev->reenable_adv_work); > + } > + > + return; > + } > + } > + > + } > + > + mutex_unlock(&hdev->cmd_sync_work_lock); > +} > + > int hci_update_eir_sync(struct hci_dev *hdev) > { > struct hci_cp_write_eir cp; > @@ -6262,15 +6370,20 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn) > } > > /* Pause advertising while doing directed advertising. */ > - hci_pause_advertising_sync(hdev); > + err = hci_pause_advertising_sync(hdev); > + if (err == -ECANCELED) > + goto done; > > err = hci_le_directed_advertising_sync(hdev, conn); > goto done; > } > > /* Disable advertising if simultaneous roles is not in use. */ > - if (!hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES)) > - hci_pause_advertising_sync(hdev); > + if (!hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES)) { > + err = hci_pause_advertising_sync(hdev); > + if (err == -ECANCELED) > + goto done; > + } > > params = hci_conn_params_lookup(hdev, &conn->dst, conn->dst_type); > if (params) { > @@ -6292,7 +6405,10 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn) > * state. > */ > if (hci_dev_test_flag(hdev, HCI_LE_SCAN)) { > - hci_scan_disable_sync(hdev); > + err = hci_scan_disable_sync(hdev); > + if (err == -ECANCELED) > + goto done; > + > hci_dev_set_flag(hdev, HCI_LE_SCAN_INTERRUPTED); > } > > @@ -6336,11 +6452,10 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn) > HCI_EV_LE_ENHANCED_CONN_COMPLETE : > HCI_EV_LE_CONN_COMPLETE, > conn->conn_timeout, NULL); > + if (err == -ECANCELED || err == -ETIMEDOUT) > + hci_le_connect_cancel_sync(hdev, conn, conn->abort_reason || 0x00); > > done: > - if (err == -ETIMEDOUT) > - hci_le_connect_cancel_sync(hdev, conn, 0x00); > - > /* Re-enable advertising after the connection attempt is finished. */ > hci_resume_advertising_sync(hdev); > return err; > @@ -6586,7 +6701,9 @@ static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data) > if (test_bit(HCI_INQUIRY, &hdev->flags)) { > err = __hci_cmd_sync_status(hdev, HCI_OP_INQUIRY_CANCEL, 0, > NULL, HCI_CMD_TIMEOUT); > - if (err) > + if (err == -ECANCELED) > + return -ECANCELED; > + else if (err) > bt_dev_warn(hdev, "Failed to cancel inquiry %d", err); > } > > @@ -6625,15 +6742,10 @@ static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data) > HCI_EV_CONN_COMPLETE, > HCI_ACL_CONN_TIMEOUT, NULL); > > - if (err == -ETIMEDOUT) > - hci_abort_conn_sync(hdev, conn, HCI_ERROR_LOCAL_HOST_TERM); > + if (err == -ECANCELED || err == -ETIMEDOUT) { > + hci_connect_cancel_sync(hdev, conn, conn->abort_reason || HCI_ERROR_LOCAL_HOST_TERM); > + return err; > + } > > return err; > } >
Hi Luiz, On 14.02.24 19:44, Luiz Augusto von Dentz wrote: > Hi Jonas, > > On Tue, Feb 13, 2024 at 6:47 PM Jonas Dreßler <verdre@v0yd.nl> wrote: >> >> Hi Luiz, >> >>> If connection is still queued/pending in the cmd_sync queue it means no >>> command has been generated and it should be safe to just dequeue the >>> callback when it is being aborted. >>> >>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> >>> --- >>> include/net/bluetooth/hci_core.h | 19 ++++++++ >>> include/net/bluetooth/hci_sync.h | 10 +++-- >>> net/bluetooth/hci_conn.c | 70 ++++++------------------------ >>> net/bluetooth/hci_sync.c | 74 ++++++++++++++++++++++++++++---- >>> 4 files changed, 102 insertions(+), 71 deletions(-) >>> >>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h >>> index 2bdea85b7c44..317d495cfcf5 100644 >>> --- a/include/net/bluetooth/hci_core.h >>> +++ b/include/net/bluetooth/hci_core.h >>> @@ -1083,6 +1083,24 @@ static inline unsigned int hci_conn_count(struct hci_dev *hdev) >>> return c->acl_num + c->amp_num + c->sco_num + c->le_num + c->iso_num; >>> } >>> >>> +static inline bool hci_conn_valid(struct hci_dev *hdev, struct hci_conn *conn) >>> +{ >>> + struct hci_conn_hash *h = &hdev->conn_hash; >>> + struct hci_conn *c; >>> + >>> + rcu_read_lock(); >>> + >>> + list_for_each_entry_rcu(c, &h->list, list) { >>> + if (c == conn) { >>> + rcu_read_unlock(); >>> + return true; >>> + } >>> + } >>> + rcu_read_unlock(); >>> + >>> + return false; >>> +} >>> + >>> static inline __u8 hci_conn_lookup_type(struct hci_dev *hdev, __u16 handle) >>> { >>> struct hci_conn_hash *h = &hdev->conn_hash; >>> @@ -1493,6 +1511,7 @@ struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst, >>> struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst, >>> u8 dst_type, bool dst_resolved, u8 sec_level, >>> u16 conn_timeout, u8 role); >>> +void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status); >>> struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst, >>> u8 sec_level, u8 auth_type, >>> enum conn_reasons conn_reason, u16 timeout); >>> diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h >>> index 4ff4aa68ee19..6a9d063e9f47 100644 >>> --- a/include/net/bluetooth/hci_sync.h >>> +++ b/include/net/bluetooth/hci_sync.h >>> @@ -48,11 +48,11 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, >>> void *data, hci_cmd_sync_work_destroy_t destroy); >>> int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, >>> void *data, hci_cmd_sync_work_destroy_t destroy); >>> +int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, >>> + void *data, hci_cmd_sync_work_destroy_t destroy); >>> struct hci_cmd_sync_work_entry * >>> hci_cmd_sync_lookup_entry(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, >>> void *data, hci_cmd_sync_work_destroy_t destroy); >>> -int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, >>> - void *data, hci_cmd_sync_work_destroy_t destroy); >>> void hci_cmd_sync_cancel_entry(struct hci_dev *hdev, >>> struct hci_cmd_sync_work_entry *entry); >>> bool hci_cmd_sync_dequeue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, >>> @@ -139,8 +139,6 @@ struct hci_conn; >>> >>> int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason); >>> >>> -int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn); >>> - >>> int hci_le_create_cis_sync(struct hci_dev *hdev); >>> >>> int hci_le_remove_cig_sync(struct hci_dev *hdev, u8 handle); >>> @@ -152,3 +150,7 @@ int hci_le_big_terminate_sync(struct hci_dev *hdev, u8 handle); >>> int hci_le_pa_terminate_sync(struct hci_dev *hdev, u16 handle); >>> >>> int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn); >>> + >>> +int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn); >>> + >>> +int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn); >>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c >>> index 587eb27f374c..21e0b4064d05 100644 >>> --- a/net/bluetooth/hci_conn.c >>> +++ b/net/bluetooth/hci_conn.c >>> @@ -68,7 +68,7 @@ static const struct sco_param esco_param_msbc[] = { >>> }; >>> >>> /* This function requires the caller holds hdev->lock */ >>> -static void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status) >>> +void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status) >>> { >>> struct hci_conn_params *params; >>> struct hci_dev *hdev = conn->hdev; >>> @@ -1124,6 +1124,9 @@ void hci_conn_del(struct hci_conn *conn) >>> * rest of hci_conn_del. >>> */ >>> hci_conn_cleanup(conn); >>> + >>> + /* Dequeue callbacks using connection pointer as data */ >>> + hci_cmd_sync_dequeue(hdev, NULL, conn, NULL); >>> } >>> >>> struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src, uint8_t src_type) >>> @@ -1258,53 +1261,6 @@ u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle) >>> return 0; >>> } >>> >>> -static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) >>> -{ >>> - struct hci_conn *conn; >>> - u16 handle = PTR_UINT(data); >>> - >>> - conn = hci_conn_hash_lookup_handle(hdev, handle); >>> - if (!conn) >>> - return; >>> - >>> - bt_dev_dbg(hdev, "err %d", err); >>> - >>> - hci_dev_lock(hdev); >>> - >>> - if (!err) { >>> - hci_connect_le_scan_cleanup(conn, 0x00); >>> - goto done; >>> - } >>> - >>> - /* Check if connection is still pending */ >>> - if (conn != hci_lookup_le_connect(hdev)) >>> - goto done; >>> - >>> - /* Flush to make sure we send create conn cancel command if needed */ >>> - flush_delayed_work(&conn->le_conn_timeout); >>> - hci_conn_failed(conn, bt_status(err)); >>> - >>> -done: >>> - hci_dev_unlock(hdev); >>> -} >>> - >>> -static int hci_connect_le_sync(struct hci_dev *hdev, void *data) >>> -{ >>> - struct hci_conn *conn; >>> - u16 handle = PTR_UINT(data); >>> - >>> - conn = hci_conn_hash_lookup_handle(hdev, handle); >>> - if (!conn) >>> - return 0; >>> - >>> - bt_dev_dbg(hdev, "conn %p", conn); >>> - >>> - clear_bit(HCI_CONN_SCANNING, &conn->flags); >>> - conn->state = BT_CONNECT; >>> - >>> - return hci_le_create_conn_sync(hdev, conn); >>> -} >>> - >>> struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst, >>> u8 dst_type, bool dst_resolved, u8 sec_level, >>> u16 conn_timeout, u8 role) >>> @@ -1371,9 +1327,7 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst, >>> conn->sec_level = BT_SECURITY_LOW; >>> conn->conn_timeout = conn_timeout; >>> >> >> Might want to add a >> >> + if (conn->state != BT_OPEN && conn->state != BT_CLOSED) >> + return conn; >> >> before setting the conn->dst_type while at it, similar to how it's >> in hci_connect_acl(). > > Hmm but shall never be the case since we have the following check before it: > > /* Since the controller supports only one LE connection attempt at a > * time, we return -EBUSY if there is any connection attempt running. > */ > if (hci_lookup_le_connect(hdev)) > return ERR_PTR(-EBUSY); Ahh okay, fair, didn't notice that! > >> >>> - err = hci_cmd_sync_queue(hdev, hci_connect_le_sync, >>> - UINT_PTR(conn->handle), >>> - create_le_conn_complete); >>> + err = hci_connect_le_sync(hdev, conn); >>> if (err) { >>> hci_conn_del(conn); >>> return ERR_PTR(err); >>> @@ -2909,12 +2863,10 @@ u32 hci_conn_get_phy(struct hci_conn *conn) >>> >>> static int abort_conn_sync(struct hci_dev *hdev, void *data) >>> { >>> - struct hci_conn *conn; >>> - u16 handle = PTR_UINT(data); >>> + struct hci_conn *conn = data; >>> >>> - conn = hci_conn_hash_lookup_handle(hdev, handle); >>> - if (!conn) >>> - return 0; >>> + if (!hci_conn_valid(hdev, conn)) >>> + return -ECANCELED; >>> >>> return hci_abort_conn_sync(hdev, conn, conn->abort_reason); >>> } >>> @@ -2949,8 +2901,10 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason) >>> hci_cmd_sync_cancel(hdev, -ECANCELED); >>> break; >>> } >>> + /* Cancel connect attempt if still queued/pending */ >>> + } else if (!hci_cancel_connect_sync(hdev, conn)) { >>> + return 0; >>> } >>> >>> - return hci_cmd_sync_queue(hdev, abort_conn_sync, UINT_PTR(conn->handle), >>> - NULL); >>> + return hci_cmd_sync_queue_once(hdev, abort_conn_sync, conn, NULL); >>> } >>> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c >>> index 5b314bf844f8..b7d8e99e2a30 100644 >>> --- a/net/bluetooth/hci_sync.c >>> +++ b/net/bluetooth/hci_sync.c >>> @@ -6285,12 +6285,21 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev, >>> conn->conn_timeout, NULL); >>> } >>> >>> -int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn) >>> +static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data) >>> { >>> struct hci_cp_le_create_conn cp; >>> struct hci_conn_params *params; >>> u8 own_addr_type; >>> int err; >>> + struct hci_conn *conn = data; >>> + >>> + if (!hci_conn_valid(hdev, conn)) >>> + return -ECANCELED; >>> + >>> + bt_dev_dbg(hdev, "conn %p", conn); >>> + >>> + clear_bit(HCI_CONN_SCANNING, &conn->flags); >>> + conn->state = BT_CONNECT; >>> >>> /* If requested to connect as peripheral use directed advertising */ >>> if (conn->role == HCI_ROLE_SLAVE) { >>> @@ -6611,16 +6620,11 @@ int hci_update_adv_data(struct hci_dev *hdev, u8 instance) >>> >>> static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data) >>> { >>> - struct hci_conn *conn; >>> - u16 handle = PTR_UINT(data); >>> + struct hci_conn *conn = data; >>> struct inquiry_entry *ie; >>> struct hci_cp_create_conn cp; >>> int err; >>> >>> - conn = hci_conn_hash_lookup_handle(hdev, handle); >>> - if (!conn) >>> - return 0; >>> - >>> /* Many controllers disallow HCI Create Connection while it is doing >>> * HCI Inquiry. So we cancel the Inquiry first before issuing HCI Create >>> * Connection. This may cause the MGMT discovering state to become false >>> @@ -6679,6 +6683,58 @@ static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data) >>> >>> int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn) >>> { >>> - return hci_cmd_sync_queue(hdev, hci_acl_create_conn_sync, >>> - UINT_PTR(conn->handle), NULL); >>> + return hci_cmd_sync_queue_once(hdev, hci_acl_create_conn_sync, conn, >>> + NULL); >>> +} >>> + >>> +static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) >>> +{ >>> + struct hci_conn *conn = data; >>> + >>> + bt_dev_dbg(hdev, "err %d", err); >>> + >>> + if (err == -ECANCELED) >>> + return; >>> + >>> + hci_dev_lock(hdev); >>> + >>> + if (!err) { >>> + hci_connect_le_scan_cleanup(conn, 0x00); >>> + goto done; >>> + } >>> + >>> + /* Check if connection is still pending */ >>> + if (conn != hci_lookup_le_connect(hdev)) >>> + goto done; >>> + >>> + /* Flush to make sure we send create conn cancel command if needed */ >>> + flush_delayed_work(&conn->le_conn_timeout); >>> + hci_conn_failed(conn, bt_status(err)); >>> + >>> +done: >>> + hci_dev_unlock(hdev); >>> +} >>> + >>> +int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn) >>> +{ >>> + return hci_cmd_sync_queue_once(hdev, hci_le_create_conn_sync, conn, >>> + create_le_conn_complete); >>> +} >>> + >>> +int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn) >>> +{ >>> + if (conn->state != BT_OPEN) >>> + return -EINVAL; >>> + >>> + switch (conn->type) { >>> + case ACL_LINK: >>> + return !hci_cmd_sync_dequeue_once(hdev, >>> + hci_acl_create_conn_sync, >>> + conn, NULL); >>> + case LE_LINK: >>> + return !hci_cmd_sync_dequeue_once(hdev, hci_le_create_conn_sync, >>> + conn, create_le_conn_complete); >>> + } >>> + >>> + return -ENOENT; >>> } >>> -- >>> 2.43.0 >> >> Thanks for sending those patches, this is pretty much exactly what I had in mind! >> >> I came up with a slightly different cancel function for the queued work, one that >> also cancels the ongoing work. I'm not sure if it makes too much sense, because it >> means adding careful -ECANCELED handling to those sync_work callbacks. >> >> The nice thing about it is that it should also allow getting rid of the >> hci_cmd_sync_cancel(hdev, -ECANCELED) in hci_abort_conn(). >> >> Adding the patch below, feel free to reuse whatever you like! >> >> Cheers, >> Jonas >> >> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c >> index 067d445701..a2b14f6be1 100644 >> --- a/net/bluetooth/hci_sync.c >> +++ b/net/bluetooth/hci_sync.c >> @@ -154,6 +154,13 @@ struct sk_buff *__hci_cmd_sync_sk(struct hci_dev *hdev, u16 opcode, u32 plen, >> >> bt_dev_dbg(hdev, "Opcode 0x%4x", opcode); >> >> + if (hdev->cur_hci_sync_work_cancelled) { >> + hdev->cur_hci_sync_work_cancelled = false; >> + >> + return ERR_PTR(-ECANCELED); >> + } >> + mutex_unlock(&hdev->cmd_sync_work_lock); >> + >> hci_req_init(&req, hdev); >> >> hci_cmd_sync_add(&req, opcode, plen, param, event, sk); >> @@ -303,9 +310,29 @@ static void hci_cmd_sync_work(struct work_struct *work) >> int err; >> >> hci_req_sync_lock(hdev); >> + >> + mutex_lock(&hdev->cmd_sync_work_lock); >> + hdev->cur_hci_sync_work_func = entry->func; >> + hdev->cur_hci_sync_work_data = entry->data; >> + mutex_unlock(&hdev->cmd_sync_work_lock); >> + >> err = entry->func(hdev, entry->data); >> if (entry->destroy) >> entry->destroy(hdev, entry->data, err); >> + >> + mutex_lock(&hdev->cmd_sync_work_lock); >> + hdev->cur_hci_sync_work_func = NULL; >> + hdev->cur_hci_sync_work_data = NULL; >> + >> + if (hdev->cur_hci_sync_work_cancelled) { >> + /* If cur_hci_sync_work_cancelled is still set at this point, >> + * no more request was sent and the work func has never been >> + * notified of our cancellation. >> + */ >> + hdev->cur_hci_sync_work_cancelled = false; >> + } >> + mutex_unlock(&hdev->cmd_sync_work_lock); > > Not really following this code, do you want to be able to cancel > callbacks that are actually executing, rather than queued? Yup exactly, I'm using __hci_cmd_sync_cancel(-ECANCELED) for that, and in the case where there's not currently a hci req ongoing, I'm setting a flag so that the next hci req will return -ECANCELED immediately. It's not too necessary to cancel the ongoing callback too I think, but since we have __hci_cmd_sync_cancel() already it seemed to make sense. And IMO it's a lot more elegant than the check for hci_skb_event() that hci_abort_conn() currently does. > >> hci_req_sync_unlock(hdev); >> } >> >> @@ -735,6 +762,87 @@ int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, >> } >> EXPORT_SYMBOL(hci_cmd_sync_queue); >> >> +bool hci_cmd_sync_has_cmd(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, >> + void *data) >> +{ >> + struct hci_cmd_sync_work_entry *entry; >> + >> + mutex_lock(&hdev->cmd_sync_work_lock); >> + if (hdev->cur_hci_sync_work_func == func && >> + hdev->cur_hci_sync_work_data == data) { >> + mutex_unlock(&hdev->cmd_sync_work_lock); >> + return true; >> + } >> + >> + list_for_each_entry(entry, &hdev->cmd_sync_work_list, list) { >> + if (entry->func == func && entry->data == data) { >> + mutex_unlock(&hdev->cmd_sync_work_lock); >> + return true; >> + } >> + } >> + mutex_unlock(&hdev->cmd_sync_work_lock); >> + >> + return false; >> +} >> + >> +void hci_cmd_sync_cancel_cmd(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, >> + void *data) >> +{ >> + struct hci_cmd_sync_work_entry *entry; >> + bool work_already_ongoing; >> + >> + mutex_lock(&hdev->cmd_sync_work_lock); >> + if (hdev->cur_hci_sync_work_func == func && >> + hdev->cur_hci_sync_work_data == data) { >> + /* If the command is already ongoing, we check if we're currently >> + * sending a async HCI request. If we are, we can cancel that >> + * and hope that the hci_cmd_sync_work_func is handling -ECANCELED. >> + */ >> + >> + if (hdev->req_status == HCI_REQ_PEND) { >> + /* If we're already executing a request, cancel that request. >> + * This will signal cancellation to the work func which sent >> + * the request in the first place. >> + */ >> + __hci_cmd_sync_cancel(hdev, -ECANCELED); >> + } else { >> + /* Otherwise, just set a flag which will cancel the next >> + * request. Just as above, this will then signal cancellation >> + * to the work func. >> + */ >> + hdev->cur_hci_sync_work_cancelled = true; >> + } > > It might be better to save the executing entry at hdev and then make > the lookup_entry return it if the parameters match so it can be > cancelled with cancel_entry, Ahh yeah, now that lookup_entry() is a thing, that seems nicer indeed. > that said perhaps it would be better to > just cancel the work if -ECANCELED is received so we don't have to > keep checking if it is returned on every command the callback > generates Hmm, I don't see how it makes sense to cancel the cmd_sync_work, and we can't cancel the callback execution from outside, can we? I don't think there's a way around checking -ECANCELED all the time as long as __hci_cmd_sync_cancel()/hci_cmd_sync_cancel() are a thing, we should probably ensure that every hci_sync callback handles this properly anyway. Cheers Jonas > >> + mutex_unlock(&hdev->cmd_sync_work_lock); >> + >> + return; >> + } else { >> + /* Or is it still queued? Then we remove it from the queue and >> + * execute the destroy callback. >> + */ >> + list_for_each_entry(entry, &hdev->cmd_sync_work_list, list) { >> + if (entry->func == func && entry->data == data) { >> + if (entry->destroy) >> + entry->destroy(hdev, entry->data, -ECANCELED); >> + list_del(&entry->list); >> + kfree(entry); >> + >> + mutex_unlock(&hdev->cmd_sync_work_lock); >> + >> + if (list_empty(&hdev->cmd_sync_work_list)) { >> + cancel_work_sync(&hdev->cmd_sync_work); >> + cancel_work_sync(&hdev->reenable_adv_work); >> + } >> + >> + return; >> + } >> + } >> + >> + } >> + >> + mutex_unlock(&hdev->cmd_sync_work_lock); >> +} >> + >> int hci_update_eir_sync(struct hci_dev *hdev) >> { >> struct hci_cp_write_eir cp; >> @@ -6262,15 +6370,20 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn) >> } >> >> /* Pause advertising while doing directed advertising. */ >> - hci_pause_advertising_sync(hdev); >> + err = hci_pause_advertising_sync(hdev); >> + if (err == -ECANCELED) >> + goto done; >> >> err = hci_le_directed_advertising_sync(hdev, conn); >> goto done; >> } >> >> /* Disable advertising if simultaneous roles is not in use. */ >> - if (!hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES)) >> - hci_pause_advertising_sync(hdev); >> + if (!hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES)) { >> + err = hci_pause_advertising_sync(hdev); >> + if (err == -ECANCELED) >> + goto done; >> + } >> >> params = hci_conn_params_lookup(hdev, &conn->dst, conn->dst_type); >> if (params) { >> @@ -6292,7 +6405,10 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn) >> * state. >> */ >> if (hci_dev_test_flag(hdev, HCI_LE_SCAN)) { >> - hci_scan_disable_sync(hdev); >> + err = hci_scan_disable_sync(hdev); >> + if (err == -ECANCELED) >> + goto done; >> + >> hci_dev_set_flag(hdev, HCI_LE_SCAN_INTERRUPTED); >> } >> >> @@ -6336,11 +6452,10 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn) >> HCI_EV_LE_ENHANCED_CONN_COMPLETE : >> HCI_EV_LE_CONN_COMPLETE, >> conn->conn_timeout, NULL); >> + if (err == -ECANCELED || err == -ETIMEDOUT) >> + hci_le_connect_cancel_sync(hdev, conn, conn->abort_reason || 0x00); >> >> done: >> - if (err == -ETIMEDOUT) >> - hci_le_connect_cancel_sync(hdev, conn, 0x00); >> - >> /* Re-enable advertising after the connection attempt is finished. */ >> hci_resume_advertising_sync(hdev); >> return err; >> @@ -6586,7 +6701,9 @@ static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data) >> if (test_bit(HCI_INQUIRY, &hdev->flags)) { >> err = __hci_cmd_sync_status(hdev, HCI_OP_INQUIRY_CANCEL, 0, >> NULL, HCI_CMD_TIMEOUT); >> - if (err) >> + if (err == -ECANCELED) >> + return -ECANCELED; >> + else if (err) >> bt_dev_warn(hdev, "Failed to cancel inquiry %d", err); >> } >> >> @@ -6625,15 +6742,10 @@ static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data) >> HCI_EV_CONN_COMPLETE, >> HCI_ACL_CONN_TIMEOUT, NULL); >> >> - if (err == -ETIMEDOUT) >> - hci_abort_conn_sync(hdev, conn, HCI_ERROR_LOCAL_HOST_TERM); >> + if (err == -ECANCELED || err == -ETIMEDOUT) { >> + hci_connect_cancel_sync(hdev, conn, conn->abort_reason || HCI_ERROR_LOCAL_HOST_TERM); >> + return err; >> + } >> >> return err; >> } >> > >
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 2bdea85b7c44..317d495cfcf5 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -1083,6 +1083,24 @@ static inline unsigned int hci_conn_count(struct hci_dev *hdev) return c->acl_num + c->amp_num + c->sco_num + c->le_num + c->iso_num; } +static inline bool hci_conn_valid(struct hci_dev *hdev, struct hci_conn *conn) +{ + struct hci_conn_hash *h = &hdev->conn_hash; + struct hci_conn *c; + + rcu_read_lock(); + + list_for_each_entry_rcu(c, &h->list, list) { + if (c == conn) { + rcu_read_unlock(); + return true; + } + } + rcu_read_unlock(); + + return false; +} + static inline __u8 hci_conn_lookup_type(struct hci_dev *hdev, __u16 handle) { struct hci_conn_hash *h = &hdev->conn_hash; @@ -1493,6 +1511,7 @@ struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst, struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst, u8 dst_type, bool dst_resolved, u8 sec_level, u16 conn_timeout, u8 role); +void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status); struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst, u8 sec_level, u8 auth_type, enum conn_reasons conn_reason, u16 timeout); diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h index 4ff4aa68ee19..6a9d063e9f47 100644 --- a/include/net/bluetooth/hci_sync.h +++ b/include/net/bluetooth/hci_sync.h @@ -48,11 +48,11 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, void *data, hci_cmd_sync_work_destroy_t destroy); int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, void *data, hci_cmd_sync_work_destroy_t destroy); +int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, + void *data, hci_cmd_sync_work_destroy_t destroy); struct hci_cmd_sync_work_entry * hci_cmd_sync_lookup_entry(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, void *data, hci_cmd_sync_work_destroy_t destroy); -int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, - void *data, hci_cmd_sync_work_destroy_t destroy); void hci_cmd_sync_cancel_entry(struct hci_dev *hdev, struct hci_cmd_sync_work_entry *entry); bool hci_cmd_sync_dequeue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func, @@ -139,8 +139,6 @@ struct hci_conn; int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason); -int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn); - int hci_le_create_cis_sync(struct hci_dev *hdev); int hci_le_remove_cig_sync(struct hci_dev *hdev, u8 handle); @@ -152,3 +150,7 @@ int hci_le_big_terminate_sync(struct hci_dev *hdev, u8 handle); int hci_le_pa_terminate_sync(struct hci_dev *hdev, u16 handle); int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn); + +int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn); + +int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn); diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c index 587eb27f374c..21e0b4064d05 100644 --- a/net/bluetooth/hci_conn.c +++ b/net/bluetooth/hci_conn.c @@ -68,7 +68,7 @@ static const struct sco_param esco_param_msbc[] = { }; /* This function requires the caller holds hdev->lock */ -static void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status) +void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status) { struct hci_conn_params *params; struct hci_dev *hdev = conn->hdev; @@ -1124,6 +1124,9 @@ void hci_conn_del(struct hci_conn *conn) * rest of hci_conn_del. */ hci_conn_cleanup(conn); + + /* Dequeue callbacks using connection pointer as data */ + hci_cmd_sync_dequeue(hdev, NULL, conn, NULL); } struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src, uint8_t src_type) @@ -1258,53 +1261,6 @@ u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle) return 0; } -static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) -{ - struct hci_conn *conn; - u16 handle = PTR_UINT(data); - - conn = hci_conn_hash_lookup_handle(hdev, handle); - if (!conn) - return; - - bt_dev_dbg(hdev, "err %d", err); - - hci_dev_lock(hdev); - - if (!err) { - hci_connect_le_scan_cleanup(conn, 0x00); - goto done; - } - - /* Check if connection is still pending */ - if (conn != hci_lookup_le_connect(hdev)) - goto done; - - /* Flush to make sure we send create conn cancel command if needed */ - flush_delayed_work(&conn->le_conn_timeout); - hci_conn_failed(conn, bt_status(err)); - -done: - hci_dev_unlock(hdev); -} - -static int hci_connect_le_sync(struct hci_dev *hdev, void *data) -{ - struct hci_conn *conn; - u16 handle = PTR_UINT(data); - - conn = hci_conn_hash_lookup_handle(hdev, handle); - if (!conn) - return 0; - - bt_dev_dbg(hdev, "conn %p", conn); - - clear_bit(HCI_CONN_SCANNING, &conn->flags); - conn->state = BT_CONNECT; - - return hci_le_create_conn_sync(hdev, conn); -} - struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst, u8 dst_type, bool dst_resolved, u8 sec_level, u16 conn_timeout, u8 role) @@ -1371,9 +1327,7 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst, conn->sec_level = BT_SECURITY_LOW; conn->conn_timeout = conn_timeout; - err = hci_cmd_sync_queue(hdev, hci_connect_le_sync, - UINT_PTR(conn->handle), - create_le_conn_complete); + err = hci_connect_le_sync(hdev, conn); if (err) { hci_conn_del(conn); return ERR_PTR(err); @@ -2909,12 +2863,10 @@ u32 hci_conn_get_phy(struct hci_conn *conn) static int abort_conn_sync(struct hci_dev *hdev, void *data) { - struct hci_conn *conn; - u16 handle = PTR_UINT(data); + struct hci_conn *conn = data; - conn = hci_conn_hash_lookup_handle(hdev, handle); - if (!conn) - return 0; + if (!hci_conn_valid(hdev, conn)) + return -ECANCELED; return hci_abort_conn_sync(hdev, conn, conn->abort_reason); } @@ -2949,8 +2901,10 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason) hci_cmd_sync_cancel(hdev, -ECANCELED); break; } + /* Cancel connect attempt if still queued/pending */ + } else if (!hci_cancel_connect_sync(hdev, conn)) { + return 0; } - return hci_cmd_sync_queue(hdev, abort_conn_sync, UINT_PTR(conn->handle), - NULL); + return hci_cmd_sync_queue_once(hdev, abort_conn_sync, conn, NULL); } diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index 5b314bf844f8..b7d8e99e2a30 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -6285,12 +6285,21 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev, conn->conn_timeout, NULL); } -int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn) +static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data) { struct hci_cp_le_create_conn cp; struct hci_conn_params *params; u8 own_addr_type; int err; + struct hci_conn *conn = data; + + if (!hci_conn_valid(hdev, conn)) + return -ECANCELED; + + bt_dev_dbg(hdev, "conn %p", conn); + + clear_bit(HCI_CONN_SCANNING, &conn->flags); + conn->state = BT_CONNECT; /* If requested to connect as peripheral use directed advertising */ if (conn->role == HCI_ROLE_SLAVE) { @@ -6611,16 +6620,11 @@ int hci_update_adv_data(struct hci_dev *hdev, u8 instance) static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data) { - struct hci_conn *conn; - u16 handle = PTR_UINT(data); + struct hci_conn *conn = data; struct inquiry_entry *ie; struct hci_cp_create_conn cp; int err; - conn = hci_conn_hash_lookup_handle(hdev, handle); - if (!conn) - return 0; - /* Many controllers disallow HCI Create Connection while it is doing * HCI Inquiry. So we cancel the Inquiry first before issuing HCI Create * Connection. This may cause the MGMT discovering state to become false @@ -6679,6 +6683,58 @@ static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data) int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn) { - return hci_cmd_sync_queue(hdev, hci_acl_create_conn_sync, - UINT_PTR(conn->handle), NULL); + return hci_cmd_sync_queue_once(hdev, hci_acl_create_conn_sync, conn, + NULL); +} + +static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) +{ + struct hci_conn *conn = data; + + bt_dev_dbg(hdev, "err %d", err); + + if (err == -ECANCELED) + return; + + hci_dev_lock(hdev); + + if (!err) { + hci_connect_le_scan_cleanup(conn, 0x00); + goto done; + } + + /* Check if connection is still pending */ + if (conn != hci_lookup_le_connect(hdev)) + goto done; + + /* Flush to make sure we send create conn cancel command if needed */ + flush_delayed_work(&conn->le_conn_timeout); + hci_conn_failed(conn, bt_status(err)); + +done: + hci_dev_unlock(hdev); +} + +int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn) +{ + return hci_cmd_sync_queue_once(hdev, hci_le_create_conn_sync, conn, + create_le_conn_complete); +} + +int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn) +{ + if (conn->state != BT_OPEN) + return -EINVAL; + + switch (conn->type) { + case ACL_LINK: + return !hci_cmd_sync_dequeue_once(hdev, + hci_acl_create_conn_sync, + conn, NULL); + case LE_LINK: + return !hci_cmd_sync_dequeue_once(hdev, hci_le_create_conn_sync, + conn, create_le_conn_complete); + } + + return -ENOENT; }