Re: [PATCH v2 3/8] Bluetooth: mgmt: take lock and hold reference when handling hci_conn

Hillf Danton
2025-11-02T23:22:27Z

On Sun, 2 Nov 2025 18:19:35 +0200 Pauli Virtanen wrote:
> Take hdev/rcu lock to prevent concurrent deletion of the hci_conn we are
> handling.
>
> When using hci_conn* pointers across critical sections, always take
> refcount to keep the pointer valid.
>
> For hci_abort_conn() only hold refcount, as the function takes
> hdev->lock itself.
>
> Fixes: 227a0cdf4a028 ("Bluetooth: MGMT: Fix not generating command complete for MGMT_OP_DISCONNECT")
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>
> Notes:
> v2:
> - no change
>
> net/bluetooth/mgmt.c | 42 ++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 78b7af8bf45f..535c475c2d25 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -3081,6 +3081,8 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
> struct mgmt_cp_unpair_device *cp = cmd->param;
> struct hci_conn *conn;
>
> + rcu_read_lock();
> +
> if (cp->addr.type == BDADDR_BREDR)
> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> &cp->addr.bdaddr);
> @@ -3088,6 +3090,11 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
> conn = hci_conn_hash_lookup_le(hdev, &cp->addr.bdaddr,
> le_addr_type(cp->addr.type));
>
> + if (conn)
> + hci_conn_get(conn);
> +
> + rcu_read_unlock();
> +
Given RCU in the lookup helpers, nested RCU makes no sense.
Feel free to add the lookup_and_get helper instead to bump the refcnt up
for the conn found.

Another simpler option is -- add conn to hash with refcnt increased and
correspondingly put conn after deleting it from hash.

> if (!conn)
> return 0;
>
> @@ -3095,6 +3102,7 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
> * will clean up the connection no matter the error.
> */
> hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
> + hci_conn_put(conn);
>
> return 0;
> }
> @@ -3242,6 +3250,8 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)
> struct mgmt_cp_disconnect *cp = cmd->param;
> struct hci_conn *conn;
>
> + rcu_read_lock();
> +
> if (cp->addr.type == BDADDR_BREDR)
> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> &cp->addr.bdaddr);
> @@ -3249,6 +3259,11 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)
> conn = hci_conn_hash_lookup_le(hdev, &cp->addr.bdaddr,
> le_addr_type(cp->addr.type));
>
> + if (conn)
> + hci_conn_get(conn);
> +
> + rcu_read_unlock();
> +
> if (!conn)
> return -ENOTCONN;
>
> @@ -3256,6 +3271,7 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)
> * will clean up the connection no matter the error.
> */
> hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
> + hci_conn_put(conn);
>
> return 0;
> }
> @@ -7372,6 +7388,9 @@ static void get_conn_info_complete(struct hci_dev *hdev, void *data, int err)
> rp.max_tx_power = HCI_TX_POWER_INVALID;
> }
>
> + if (conn)
> + hci_conn_put(conn);
> +
> mgmt_cmd_complete(cmd->sk, cmd->hdev->id, MGMT_OP_GET_CONN_INFO, status,
> &rp, sizeof(rp));
>
> @@ -7386,6 +7405,8 @@ static int get_conn_info_sync(struct hci_dev *hdev, void *data)
> int err;
> __le16 handle;
>
> + hci_dev_lock(hdev);
> +
> /* Make sure we are still connected */
> if (cp->addr.type == BDADDR_BREDR)
> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> @@ -7393,12 +7414,16 @@ static int get_conn_info_sync(struct hci_dev *hdev, void *data)
> else
> conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->addr.bdaddr);
>
> - if (!conn || conn->state != BT_CONNECTED)
> + if (!conn || conn->state != BT_CONNECTED) {
> + hci_dev_unlock(hdev);
> return MGMT_STATUS_NOT_CONNECTED;
> + }
>
> - cmd->user_data = conn;
> + cmd->user_data = hci_conn_get(conn);
> handle = cpu_to_le16(conn->handle);
>
> + hci_dev_unlock(hdev);
> +
> /* Refresh RSSI each time */
> err = hci_read_rssi_sync(hdev, handle);
>
> @@ -7532,6 +7557,9 @@ static void get_clock_info_complete(struct hci_dev *hdev, void *data, int err)
> }
>
> complete:
> + if (conn)
> + hci_conn_put(conn);
> +
> mgmt_cmd_complete(cmd->sk, cmd->hdev->id, cmd->opcode, status, &rp,
> sizeof(rp));
>
> @@ -7548,15 +7576,21 @@ static int get_clock_info_sync(struct hci_dev *hdev, void *data)
> memset(&hci_cp, 0, sizeof(hci_cp));
> hci_read_clock_sync(hdev, &hci_cp);
>
> + hci_dev_lock(hdev);
> +
> /* Make sure connection still exists */
> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &cp->addr.bdaddr);
> - if (!conn || conn->state != BT_CONNECTED)
> + if (!conn || conn->state != BT_CONNECTED) {
> + hci_dev_unlock(hdev);
> return MGMT_STATUS_NOT_CONNECTED;
> + }
>
> - cmd->user_data = conn;
> + cmd->user_data = hci_conn_get(conn);
> hci_cp.handle = cpu_to_le16(conn->handle);
> hci_cp.which = 0x01; /* Piconet clock */
>
> + hci_dev_unlock(hdev);
> +
> return hci_read_clock_sync(hdev, &hci_cp);
> }
>
> --
> 2.51.1