diff mbox series

[v2,3/3] Bluetooth: hci_sync: Attempt to dequeue connection attempt

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

Commit Message

Luiz Augusto von Dentz Feb. 13, 2024, 9:31 p.m. UTC
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

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(-)

Comments

Jonas Dreßler Feb. 13, 2024, 11:47 p.m. UTC | #1
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;
  }
Luiz Augusto von Dentz Feb. 14, 2024, 6:44 p.m. UTC | #2
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;
>   }
>
Jonas Dreßler Feb. 21, 2024, 10:25 p.m. UTC | #3
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 mbox series

Patch

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;
 }