Bluetooth: avoid concurrent deletion of hci_conn

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

Posted by Pauli Virtanen 3 months, 1 week ago

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

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

Posted by Hillf Danton 3 months, 1 week ago

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

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

Posted by Pauli Virtanen 3 months, 1 week ago

ma, 2025-11-03 kello 07:21 +0800, Hillf Danton kirjoitti:
> 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.

RCU must be held until the refcount is increased. I don't think there
is difference in whether it's done by nesting or by duplicating all the
helpers without the rcu_read_lock().

The rcu_read_lock() in the helpers is probaly something that should be
done away with --- the caller needs to hold RCU or suitable other lock
if it wants to use the returned value safely.

> 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

-- 
Pauli Virtanen