Hi Yun-hao,
On Mon, Apr 8, 2024 at 10:03 AM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi,
>
> On Mon, Apr 8, 2024 at 8:23 AM Yun-hao Chung <howardchung@xxxxxxxxxx> wrote:
> >
> > Hi Luiz,
> >
> > Thanks for the response.
> >
> > This issue was found in ChromeOS edition BlueZ. I'm not sure if any
> > official BlueZ would do the same discovery procedure or not.
> >
> > I extracted relevant part of the trace as below
> > ==========================
> > < ACL Data TX: Handle 2048 flags 0x00 dlen 11
> >
> > #43961 [hci0]
> > 2024-03-05 07:33:55.936283
> > ATT: Read By Group Type Request (0x10) len 6
> > Handle range: 0x001f-0xffff
> > Attribute group type: Primary Service (0x2800)
> > ...
> > > ACL Data RX: Handle 2048 flags 0x02 dlen 27 #43963 [hci0] 2024-03-05 07:33:55.967020
> > > ACL Data RX: Handle 2048 flags 0x01 dlen 3 #43964 [hci0] 2024-03-05 07:33:55.967022
> > ATT: Read By Group Type Response (0x11) len 25
> > Attribute data length: 6
> > Attribute group list: 4 entries
> > Handle range: 0x001f-0x0035
> > UUID: Human Interface Device (0x1812)
> > Handle range: 0x0036-0x0044
> > UUID: Human Interface Device (0x1812)
> > Handle range: 0x0045-0x0055
> > UUID: Human Interface Device (0x1812)
> > Handle range: 0x0056-0x0062
> > UUID: Logitech International SA (0xfd72)
> > …
> > < ACL Data TX: Handle 2048 flags 0x00 dlen 11
> >
> > #44018 [hci0]
> > 2024-03-05 07:33:56.281436
> > ATT: Read By Type Request (0x08) len 6
> > Handle range: 0x0021-0xffff
> > Attribute type: Characteristic (0x2803)
> > ...
> > > ACL Data RX: Handle 2048 flags 0x02 dlen 27 #44023 [hci0] 2024-03-05 07:33:56.311026
> > ATT: Read By Type Response (0x09) len 22
> > Attribute data length: 7
> > Attribute data list: 3 entries
> > Handle: 0x0022
> > Value: 122300332a
> > Properties: 0x12
> > Read (0x02)
> > Notify (0x10)
> > Value Handle: 0x0023
> > Value UUID: Boot Mouse Input Report (0x2a33)
> > Handle: 0x0025
> > Value: 0226004b2a
> > Properties: 0x02
> > Read (0x02)
> > Value Handle: 0x0026
> > Value UUID: Report Map (0x2a4b)
> > Handle: 0x0027
> > Value: 1228004d2a
> > Properties: 0x12
> > Read (0x02)
> > Notify (0x10)
> > Value Handle: 0x0028
> > Value UUID: Report (0x2a4d)
> > ...
> > < ACL Data TX: Handle 2048 flags 0x00 dlen 11
> >
> > #44024 [hci0]
> > 2024-03-05 07:33:56.311458
> > ATT: Read By Type Request (0x08) len 6
> > Handle range: 0x0028-0xffff
> > Attribute type: Characteristic (0x2803)
> > …
> > > ACL Data RX: Handle 2048 flags 0x02 dlen 27 #44029 [hci0] 2024-03-05 07:33:56.341025
> > ATT: Read By Type Response (0x09) len 22
> > Attribute data length: 7
> > Attribute data list: 3 entries
> > Handle: 0x002b
> > Value: 122c004d2a
> > Properties: 0x12
> > Read (0x02)
> > Notify (0x10)
> > Value Handle: 0x002c
> > Value UUID: Report (0x2a4d)
> > Handle: 0x002f
> > Value: 0e30004d2a
> > Properties: 0x0e
> > Read (0x02)
> > Write Without Response (0x04)
> > Write (0x08)
> > Value Handle: 0x0030
> > Value UUID: Report (0x2a4d)
> > Handle: 0x0032
> > Value: 0433004c2a
> > Properties: 0x04
> > Write Without Response (0x04)
> > Value Handle: 0x0033
> > Value UUID: HID Control Point (0x2a4c)
> > …
> > < ACL Data TX: Handle 2048 flags 0x00 dlen 9
> >
> > #44071 [hci0]
> > 2024-03-05 07:33:56.702334
> > ATT: Find Information Request (0x04) len 4
> > Handle range: 0x0029-0x002a
> > …
> > > ACL Data RX: Handle 2048 flags 0x02 dlen 14 #44073 [hci0] 2024-03-05 07:33:56.731069
> > ATT: Find Information Response (0x05) len 9
> > Format: UUID-16 (0x01)
> > Handle: 0x0029
> > UUID: Client Characteristic Configuration (0x2902)
> > Handle: 0x002a
> > UUID: Report Reference (0x2908)
> > =============================
> >
> > When decoding packet #44073, we can't append descriptors 0x0029 and
> > 0x002a at the tail of the |service->attributes| since the
> > characteristics 0x002b,0x002f and 0x0032 are already in the array.
>
> So bluetoothd works but the monitor doesn't? They both are using
> gatt_db_insert_descriptor which uses servvice_insert_descriptor, so
> where exactly is it failing at? The only difference I see is that in
> case of gatt-client.c we do:
>
> attr = gatt_db_get_attribute(client->db, handle);
> if (attr && !bt_uuid_cmp(&uuid,
> gatt_db_attribute_get_type(attr)))
> continue;
>
>
> The daemon does actually insert the CCC automatically in case there is
> only one handle left for the it:
>
> if (desc_start == chrc_data->end_handle &&
> (chrc_data->properties & BT_GATT_CHRC_PROP_NOTIFY ||
> chrc_data->properties & BT_GATT_CHRC_PROP_INDICATE)) {
> bt_uuid_t ccc_uuid;
>
> /* If there is only one descriptor that must be the CCC
> * in case either notify or indicate are supported.
> */
> bt_uuid16_create(&ccc_uuid,
> GATT_CLIENT_CHARAC_CFG_UUID);
> attr = gatt_db_insert_descriptor(client->db, desc_start,
> &ccc_uuid, 0, NULL,
> NULL, NULL);
> if (attr) {
> free(chrc_data);
> continue;
> }
> }
>
> That said perhaps we need to revise the implementation of
> get_attribute_index, it does attempt to take the first free index
> which is incorrect when we are inserting characteristics it shall not
> allocate the indexes one after another, instead the index shall be
> handle - service->attribute[0].handle when the handle is set.
Here is a tentative of fixing the index logic:
https://patchwork.kernel.org/project/bluetooth/patch/20240408155949.3622429-1-luiz.dentz@xxxxxxxxx/
> >
> >
> > On Wed, Apr 3, 2024 at 10:37 PM Luiz Augusto von Dentz
> > <luiz.dentz@xxxxxxxxx> wrote:
> > >
> > > Hi Howard,
> > >
> > > On Wed, Apr 3, 2024 at 7:06 AM Howard Chung <howardchung@xxxxxxxxxx> wrote:
> > > >
> > > > service->attributes has an assumption that it should look like:
> > > > |char_uuid|char_1|desc_1_1|desc_1_2|char_uuid|char_2|desc_2_1|char_uuid|...
> > > > where desc_x_y means a descriptor of char_x.
> > >
> > > Will need to check the trace but I believe BlueZ always discover all
> > > characteristics before moving to descriptors, if that is not happening
> > > they there is probably a bug somewhere, that said perhaps you are
> > > doing the discovery procedure with another stack which doesn't employ
> > > the same logic, although inefficient it is possible to discover out of
> > > order but that would require reassigning the descriptors to
> > > characteristics once they are found, which is also inefficient but I
> > > would understand if you after supporting other stacks.
> > >
> > > > In monitor, an attribute is inserted as soon as it is found. It is not
> > > > guranteed that all the descriptors of a characteristic would be found
> > > > before another characteristic is found.
> > >
> > > You might want to include such a trace, don't recall seeing any stack
> > > that does out-order discover.
> > >
> > > > This adds a function to insert the descriptor with the given handle to
> > > > an appropriate place in its service attribute list and make monitor to
> > > > use this function when a descripter is found.
> > > >
> > > > Reviewed-by: Archie Pusaka <apusaka@xxxxxxxxxxxx>
> > > > ---
> > > >
> > > > monitor/att.c | 2 +-
> > > > src/shared/gatt-db.c | 66 ++++++++++++++++++++++++++++++++++++++++++++
> > > > src/shared/gatt-db.h | 9 ++++++
> > > > 3 files changed, 76 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/monitor/att.c b/monitor/att.c
> > > > index ddeb54d9e..503099745 100644
> > > > --- a/monitor/att.c
> > > > +++ b/monitor/att.c
> > > > @@ -4153,7 +4153,7 @@ static struct gatt_db_attribute *insert_desc(const struct l2cap_frame *frame,
> > > > if (!db)
> > > > return NULL;
> > > >
> > > > - return gatt_db_append_descriptor(db, handle, uuid, 0, NULL, NULL, NULL);
> > > > + return gatt_db_insert_descriptor(db, handle, uuid, 0, NULL, NULL, NULL);
> > > > }
> > > >
> > > > static void att_find_info_rsp_16(const struct l2cap_frame *frame)
> > > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> > > > index 39bba9b54..f08c5afaa 100644
> > > > --- a/src/shared/gatt-db.c
> > > > +++ b/src/shared/gatt-db.c
> > > > @@ -1002,6 +1002,72 @@ service_append_descriptor(struct gatt_db_service *service,
> > > > return service->attributes[i];
> > > > }
> > > >
> > > > +struct gatt_db_attribute *
> > > > +gatt_db_insert_descriptor(struct gatt_db *db,
> > > > + uint16_t handle,
> > > > + const bt_uuid_t *uuid,
> > > > + uint32_t permissions,
> > > > + gatt_db_read_t read_func,
> > > > + gatt_db_write_t write_func,
> > > > + void *user_data)
> > > > +{
> > > > + struct gatt_db_attribute *attrib, *iter_attr, *char_attr = NULL;
> > > > + struct gatt_db_service *service;
> > > > + int i, j, num_index, char_index;
> > > > +
> > > > + attrib = gatt_db_get_service(db, handle);
> > > > + if (!attrib)
> > > > + return NULL;
> > > > +
> > > > + service = attrib->service;
> > > > + num_index = get_attribute_index(service, 0);
> > > > + if (!num_index)
> > > > + return NULL;
> > > > +
> > > > + // Find the characteristic the descriptor belongs to.
> > > > + for (i = 0; i < num_index; i++) {
> > > > + iter_attr = service->attributes[i];
> > > > + if (bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid))
> > > > + continue;
> > > > +
> > > > + if (iter_attr->handle > handle)
> > > > + continue;
> > > > +
> > > > + // Find the characteristic with the largest handle among those
> > > > + // with handles less than the descriptor's handle.
> > > > + if (!char_attr || iter_attr->handle > char_attr->handle) {
> > > > + char_attr = iter_attr;
> > > > + char_index = i;
> > > > + }
> > > > + }
> > > > +
> > > > + // There is no characteristic contain this descriptor. Something went
> > > > + // wrong
> > > > + if (!char_attr)
> > > > + return NULL;
> > > > +
> > > > + // Find the end of this characteristic
> > > > + for (i = char_index + 1; i < num_index; i++) {
> > > > + iter_attr = service->attributes[i];
> > > > +
> > > > + if (!bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid) ||
> > > > + !bt_uuid_cmp(&included_service_uuid, &iter_attr->uuid))
> > > > + break;
> > > > + }
> > > > +
> > > > + // Move all of the attributes after the end of this characteristic to
> > > > + // its next index.
> > > > + for (j = num_index; j > i; j--)
> > > > + service->attributes[j] = service->attributes[j - 1];
> > > > +
> > > > + service->attributes[i] = new_attribute(service, handle, uuid, NULL, 0);
> > > > +
> > > > + set_attribute_data(service->attributes[i], read_func, write_func,
> > > > + permissions, user_data);
> > > > +
> > > > + return service->attributes[i];
> > > > +}
> > > > +
> > > > struct gatt_db_attribute *
> > > > gatt_db_append_descriptor(struct gatt_db *db,
> > > > uint16_t handle,
> > > > diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> > > > index ec0eccdfc..4c4e88d87 100644
> > > > --- a/src/shared/gatt-db.h
> > > > +++ b/src/shared/gatt-db.h
> > > > @@ -80,6 +80,15 @@ gatt_db_append_characteristic(struct gatt_db *db,
> > > > gatt_db_write_t write_func,
> > > > void *user_data);
> > > >
> > > > +struct gatt_db_attribute *
> > > > +gatt_db_insert_descriptor(struct gatt_db *db,
> > > > + uint16_t handle,
> > > > + const bt_uuid_t *uuid,
> > > > + uint32_t permissions,
> > > > + gatt_db_read_t read_func,
> > > > + gatt_db_write_t write_func,
> > > > + void *user_data);
> > > > +
> > > > struct gatt_db_attribute *
> > > > gatt_db_append_descriptor(struct gatt_db *db,
> > > > uint16_t handle,
> > > > --
> > > > 2.44.0.478.gd926399ef9-goog
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
