Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1410-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240126144120.Bluez.1.If74ccbca4d541c5f576765a3a78cb8923b5f85b1@changeid>
In-Reply-To: <20240126144120.Bluez.1.If74ccbca4d541c5f576765a3a78cb8923b5f85b1@changeid>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Fri, 26 Jan 2024 14:01:35 -0500
Message-ID: <CABBYNZL3yZ1_7wVyGg6fMZ=a3thqia2i+WSBFFyvBxTt4KPEgA@mail.gmail.com>
Subject: Re: [Bluez PATCH] Monitor: Remove handle before assigning
To: Archie Pusaka <apusaka@google.com>
Cc: linux-bluetooth <linux-bluetooth@vger.kernel.org>, 
	Johan Hedberg <johan.hedberg@gmail.com>, Marcel Holtmann <marcel@holtmann.org>, 
	CrosBT Upstreaming <chromeos-bluetooth-upstreaming@chromium.org>, 
	Archie Pusaka <apusaka@chromium.org>, Zhengping Jiang <jiangzp@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Archie,

On Fri, Jan 26, 2024 at 1:42=E2=80=AFAM Archie Pusaka <apusaka@google.com> =
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 same 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>
>
> ---
>
>  monitor/packet.c | 50 +++++++++++++++++++++++++-----------------------
>  1 file changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/monitor/packet.c b/monitor/packet.c
> index 42e711cafa..b5dea6384a 100644
> --- a/monitor/packet.c
> +++ b/monitor/packet.c
> @@ -189,11 +189,33 @@ static struct packet_conn_data *lookup_parent(uint1=
6_t handle)
>         return NULL;
>  }
>
> +static void release_handle(uint16_t handle)
> +{
> +       int i;
> +
> +       for (i =3D 0; i < MAX_CONN; i++) {
> +               struct packet_conn_data *conn =3D &conn_list[i];
> +
> +               if (conn->handle =3D=3D handle) {
> +                       if (conn->destroy)
> +                               conn->destroy(conn->data);
> +
> +                       queue_destroy(conn->tx_q, free);
> +                       queue_destroy(conn->chan_q, free);
> +                       memset(conn, 0, sizeof(*conn));
> +                       conn->handle =3D 0xffff;
> +                       break;
> +               }
> +       }
> +}

We might as well return if we find out there is a packet_conn_data,
that way we don't need to do 2 lookups in a row.

>  static void assign_handle(uint16_t index, uint16_t handle, uint8_t type,
>                                         uint8_t *dst, uint8_t dst_type)
>  {
>         int i;
>
> +       release_handle(handle);
> +
>         for (i =3D 0; i < MAX_CONN; i++) {
>                 if (conn_list[i].handle =3D=3D 0xffff) {
>                         hci_devba(index, (bdaddr_t *)conn_list[i].src);
> @@ -225,26 +247,6 @@ static void assign_handle(uint16_t index, uint16_t h=
andle, uint8_t type,
>         }
>  }
>
> -static void release_handle(uint16_t handle)
> -{
> -       int i;
> -
> -       for (i =3D 0; i < MAX_CONN; i++) {
> -               struct packet_conn_data *conn =3D &conn_list[i];
> -
> -               if (conn->handle =3D=3D handle) {
> -                       if (conn->destroy)
> -                               conn->destroy(conn->data);
> -
> -                       queue_destroy(conn->tx_q, free);
> -                       queue_destroy(conn->chan_q, free);
> -                       memset(conn, 0, sizeof(*conn));
> -                       conn->handle =3D 0xffff;
> -                       break;
> -               }
> -       }
> -}
> -
>  struct packet_conn_data *packet_get_conn_data(uint16_t handle)
>  {
>         int i;
> @@ -10076,7 +10078,7 @@ static void conn_complete_evt(struct timeval *tv,=
 uint16_t index,
>         const struct bt_hci_evt_conn_complete *evt =3D 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);
> @@ -10648,7 +10650,7 @@ static void sync_conn_complete_evt(struct timeval=
 *tv, uint16_t index,
>         const struct bt_hci_evt_sync_conn_complete *evt =3D 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);
> @@ -11077,7 +11079,7 @@ static void le_conn_complete_evt(struct timeval *=
tv, uint16_t index,
>         const struct bt_hci_evt_le_conn_complete *evt =3D 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);
> @@ -11206,7 +11208,7 @@ static void le_enhanced_conn_complete_evt(struct =
timeval *tv, uint16_t index,
>         const struct bt_hci_evt_le_enhanced_conn_complete *evt =3D 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);

Are these changes really intentional? Or perhaps we don't want to
attempt to resolve the address since these are the event that would
assign them in the first place? In that case I had these fixed
separately with a proper patch description.

> --
> 2.43.0.429.g432eaa2c6b-goog
>


--=20
Luiz Augusto von Dentz