[Bluez PATCH v2 1/2] Monitor: Remove handle before assigning

[Bluez PATCH v2 1/2] Monitor: Remove handle before assigning

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

It is possible to have some handles not removed, for example the host
may decide not to wait for disconnection complete event when it is
suspending. In this case, when the peer device reconnected, we might
have two of the some handle assigned and create problem down the road.

This patch solves the issue by always removing any previous handles
when assigning a new handle if they are the same.

Reviewed-by: Zhengping Jiang <jiangzp@google.com>

---

Changes in v2:
* Return connection pointer when removing handle

 monitor/packet.c | 79 ++++++++++++++++++++++++++----------------------
 1 file changed, 43 insertions(+), 36 deletions(-)

diff --git a/monitor/packet.c b/monitor/packet.c
index 42e711cafa..164cc82bb2 100644
--- a/monitor/packet.c
+++ b/monitor/packet.c
@@ -189,59 +189,66 @@ static struct packet_conn_data *lookup_parent(uint16_t handle)
 	return NULL;
 }
 
-static void assign_handle(uint16_t index, uint16_t handle, uint8_t type,
-					uint8_t *dst, uint8_t dst_type)
+static struct packet_conn_data *release_handle(uint16_t handle)
 {
 	int i;
 
 	for (i = 0; i < MAX_CONN; i++) {
-		if (conn_list[i].handle == 0xffff) {
-			hci_devba(index, (bdaddr_t *)conn_list[i].src);
+		struct packet_conn_data *conn = &conn_list[i];
+
+		if (conn->handle == handle) {
+			if (conn->destroy)
+				conn->destroy(conn->data);
 
-			conn_list[i].index = index;
-			conn_list[i].handle = handle;
-			conn_list[i].type = type;
+			queue_destroy(conn->tx_q, free);
+			queue_destroy(conn->chan_q, free);
+			memset(conn, 0, sizeof(*conn));
+			conn->handle = 0xffff;
+			return conn;
+		}
+	}
 
-			if (!dst) {
-				struct packet_conn_data *p;
+	return NULL;
+}
 
-				/* If destination is not set attempt to use the
-				 * parent one if that exists.
-				 */
-				p = lookup_parent(handle);
-				if (p) {
-					memcpy(conn_list[i].dst, p->dst,
-						sizeof(conn_list[i].dst));
-					conn_list[i].dst_type = p->dst_type;
-				}
+static void assign_handle(uint16_t index, uint16_t handle, uint8_t type,
+					uint8_t *dst, uint8_t dst_type)
+{
+	struct packet_conn_data *conn = release_handle(handle);
+	int i;
 
+	if (!conn) {
+		for (i = 0; i < MAX_CONN; i++) {
+			if (conn_list[i].handle == 0xffff) {
+				conn = &conn_list[i];
 				break;
 			}
-
-			memcpy(conn_list[i].dst, dst, sizeof(conn_list[i].dst));
-			conn_list[i].dst_type = dst_type;
-			break;
 		}
 	}
-}
 
-static void release_handle(uint16_t handle)
-{
-	int i;
+	if (!conn)
+		return;
 
-	for (i = 0; i < MAX_CONN; i++) {
-		struct packet_conn_data *conn = &conn_list[i];
+	hci_devba(index, (bdaddr_t *)conn->src);
 
-		if (conn->handle == handle) {
-			if (conn->destroy)
-				conn->destroy(conn->data);
+	conn->index = index;
+	conn->handle = handle;
+	conn->type = type;
 
-			queue_destroy(conn->tx_q, free);
-			queue_destroy(conn->chan_q, free);
-			memset(conn, 0, sizeof(*conn));
-			conn->handle = 0xffff;
-			break;
+	if (!dst) {
+		struct packet_conn_data *p;
+
+		/* If destination is not set attempt to use the parent one if
+		 * that exists.
+		 */
+		p = lookup_parent(handle);
+		if (p) {
+			memcpy(conn->dst, p->dst, sizeof(conn->dst));
+			conn->dst_type = p->dst_type;
 		}
+	} else {
+		memcpy(conn->dst, dst, sizeof(conn->dst));
+		conn->dst_type = dst_type;
 	}
 }
 
-- 
2.43.0.429.g432eaa2c6b-goog

[Bluez PATCH v2 2/2] Monitor: Avoid printing stale address on connection event

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

We now remove potentially stale handle when assigning a new handle.
However, that is done after printing the handle and the stale address
associated with it.

Directly use print_field instead of print_handle to avoid printing the
stale address. We still print the correct address on the following
line anyway.

---

(no changes since v1)

 monitor/packet.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/monitor/packet.c b/monitor/packet.c
index 164cc82bb2..3c32735b73 100644
--- a/monitor/packet.c
+++ b/monitor/packet.c
@@ -10083,7 +10083,7 @@ static void conn_complete_evt(struct timeval *tv, uint16_t index,
 	const struct bt_hci_evt_conn_complete *evt = data;
 
 	print_status(evt->status);
-	print_handle(evt->handle);
+	print_field("Handle: %d", le16_to_cpu(evt->handle));
 	print_bdaddr(evt->bdaddr);
 	print_link_type(evt->link_type);
 	print_enable("Encryption", evt->encr_mode);
@@ -10655,7 +10655,7 @@ static void sync_conn_complete_evt(struct timeval *tv, uint16_t index,
 	const struct bt_hci_evt_sync_conn_complete *evt = data;
 
 	print_status(evt->status);
-	print_handle(evt->handle);
+	print_field("Handle: %d", le16_to_cpu(evt->handle));
 	print_bdaddr(evt->bdaddr);
 	print_link_type(evt->link_type);
 	print_field("Transmission interval: 0x%2.2x", evt->tx_interval);
@@ -11084,7 +11084,7 @@ static void le_conn_complete_evt(struct timeval *tv, uint16_t index,
 	const struct bt_hci_evt_le_conn_complete *evt = data;
 
 	print_status(evt->status);
-	print_handle(evt->handle);
+	print_field("Handle: %d", le16_to_cpu(evt->handle));
 	print_role(evt->role);
 	print_peer_addr_type("Peer address type", evt->peer_addr_type);
 	print_addr("Peer address", evt->peer_addr, evt->peer_addr_type);
@@ -11213,7 +11213,7 @@ static void le_enhanced_conn_complete_evt(struct timeval *tv, uint16_t index,
 	const struct bt_hci_evt_le_enhanced_conn_complete *evt = data;
 
 	print_status(evt->status);
-	print_handle(evt->handle);
+	print_field("Handle: %d", le16_to_cpu(evt->handle));
 	print_role(evt->role);
 	print_peer_addr_type("Peer address type", evt->peer_addr_type);
 	print_addr("Peer address", evt->peer_addr, evt->peer_addr_type);
-- 
2.43.0.429.g432eaa2c6b-goog

RE: [Bluez,v2,1/2] Monitor: Remove handle before assigning

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1715 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=821233

---Test result---

Test Summary:
CheckPatch                    PASS      0.94 seconds
GitLint                       PASS      0.64 seconds
BuildEll                      PASS      24.40 seconds
BluezMake                     PASS      734.57 seconds
MakeCheck                     PASS      12.14 seconds
MakeDistcheck                 PASS      166.71 seconds
CheckValgrind                 PASS      231.79 seconds
CheckSmatch                   WARNING   337.51 seconds
bluezmakeextell               PASS      109.66 seconds
IncrementalBuild              PASS      1351.96 seconds
ScanBuild                     PASS      941.39 seconds

Details
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
monitor/packet.c: note: in included file:monitor/display.h:82:26: warning: Variable length array is used.monitor/packet.c:1867:26: warning: Variable length array is used.monitor/packet.c: note: in included file:monitor/bt.h:3606:52: warning: array of flexible structuresmonitor/bt.h:3594:40: warning: array of flexible structuresmonitor/packet.c: note: in included file:monitor/display.h:82:26: warning: Variable length array is used.monitor/packet.c:1867:26: warning: Variable length array is used.monitor/packet.c: note: in included file:monitor/bt.h:3606:52: warning: array of flexible structuresmonitor/bt.h:3594:40: warning: array of flexible structures


---
Regards,
Linux Bluetooth

Re: [Bluez PATCH v2 1/2] Monitor: Remove handle before assigning

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Tue, 30 Jan 2024 18:24:59 +0800 you wrote:
> From: Archie Pusaka <apusaka@chromium.org>
> 
> It is possible to have some handles not removed, for example the host
> may decide not to wait for disconnection complete event when it is
> suspending. In this case, when the peer device reconnected, we might
> have two of the some handle assigned and create problem down the road.
> 
> [...]

Here is the summary with links:
  - [Bluez,v2,1/2] Monitor: Remove handle before assigning
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=12ccf5ea0fa5
  - [Bluez,v2,2/2] Monitor: Avoid printing stale address on connection event
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=e98bbe3f1cb2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html