Re: Bluetooth: Add framework for device found filtering based on UUID

Re: Bluetooth: Add framework for device found filtering based on UUID

[Geert Uytterhoeven]

On Thu, Dec 11, 2014 at 11:48 PM, Linux Kernel Mailing List
<linux-kernel@vger.kernel.org> wrote:
> Gitweb:     http://git.kernel.org/linus/;a=commit;h=b487b9ce9340a6e98d7f8277399304b23b7be456
> Commit:     b487b9ce9340a6e98d7f8277399304b23b7be456
> Parent:     bda157a40077447b25a1172a17b8ef81a2905cb7
> Refname:    refs/heads/master
> Author:     Marcel Holtmann <marcel@holtmann.org>
> AuthorDate: Fri Dec 5 10:55:57 2014 +0100
> Committer:  Johan Hedberg <johan.hedberg@intel.com>
> CommitDate: Fri Dec 5 12:37:35 2014 +0200
>
>     Bluetooth: Add framework for device found filtering based on UUID
>
>     Using Start Service Discovery provides the option to specifiy a list
>     of UUID that are used to filter out device found events. This patch
>     provides the framework for hooking up the UUID filter.
>
>     Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>     Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/mgmt.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 1715c91..e39190a 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -6802,6 +6802,11 @@ void mgmt_read_local_oob_data_complete(struct hci_dev *hdev, u8 *hash192,
>         mgmt_pending_remove(cmd);
>  }
>
> +static bool eir_has_uuids(u8 *eir, u16 eir_len, u16 uuid_count, u8 (*uuids)[16])
> +{
> +       return false;
> +}
> +
>  void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>                        u8 addr_type, u8 *dev_class, s8 rssi, u32 flags,
>                        u8 *eir, u16 eir_len, u8 *scan_rsp, u8 scan_rsp_len)
> @@ -6809,6 +6814,7 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>         char buf[512];
>         struct mgmt_ev_device_found *ev = (void *) buf;
>         size_t ev_size;
> +       bool match;

net/bluetooth/mgmt.c: In function ‘mgmt_device_found’:
net/bluetooth/mgmt.c:6993: warning: ‘match’ may be used uninitialized
in this function

All tests for "match" are protected by "if (hdev->discovery.uuid_count > 0)",
so we only have to care about that case.

>         /* Don't send events for a non-kernel initiated discovery. With
>          * LE one exception is if we have pend_le_reports > 0 in which
> @@ -6843,15 +6849,59 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>         ev->rssi = rssi;
>         ev->flags = cpu_to_le32(flags);
>
> -       if (eir_len > 0)
> +       if (eir_len > 0) {
> +               /* When using service discovery and a list of UUID is
> +                * provided, results with no matching UUID should be
> +                * dropped. In case there is a match the result is
> +                * kept and checking possible scan response data
> +                * will be skipped.
> +                */
> +               if (hdev->discovery.uuid_count > 0) {
> +                       match = eir_has_uuids(eir, eir_len,
> +                                             hdev->discovery.uuid_count,
> +                                             hdev->discovery.uuids);
> +                       if (!match)
> +                               return;
> +               }

If we get here and if (hdev->discovery.uuid_count > 0), "match" must be true.

> +
> +               /* Copy EIR or advertising data into event */
>                 memcpy(ev->eir, eir, eir_len);
> +       } else {
> +               /* When using service discovery and a list of UUID is
> +                * provided, results with empty EIR or advertising data
> +                * should be dropped since they do not match any UUID.
> +                */
> +               if (hdev->discovery.uuid_count > 0)
> +                       return;

We will never get here if (hdev->discovery.uuid_count > 0).

> +       }

Hence if we get here and if (hdev->discovery.uuid_count > 0),
"match" is always true.

>         if (dev_class && !eir_has_data_type(ev->eir, eir_len, EIR_CLASS_OF_DEV))
>                 eir_len = eir_append_data(ev->eir, eir_len, EIR_CLASS_OF_DEV,
>                                           dev_class, 3);
>
> -       if (scan_rsp_len > 0)
> +       if (scan_rsp_len > 0) {
> +               /* When using service discovery and a list of UUID is
> +                * provided, results with no matching UUID should be
> +                * dropped if there is no previous match from the
> +                * advertising data.
> +                */
> +               if (hdev->discovery.uuid_count > 0) {
> +                       if (!match && !eir_has_uuids(scan_rsp, scan_rsp_len,

"match" is true, so this whole block is useless.

> +                                                    hdev->discovery.uuid_count,
> +                                                    hdev->discovery.uuids))
> +                               return;
> +               }
> +
> +               /* Append scan response data to event */
>                 memcpy(ev->eir + eir_len, scan_rsp, scan_rsp_len);
> +       } else {
> +               /* When using service discovery and a list of UUID is
> +                * provided, results with empty scan response and no
> +                * previous matched advertising data should be dropped.
> +                */
> +               if (hdev->discovery.uuid_count > 0 && !match)

"match" is true, so this whole block is useless.

> +                       return;
> +       }
>
>         ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
>         ev_size = sizeof(*ev) + eir_len + scan_rsp_len;

Is my analysis correct?
Am I missing something?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Bluetooth: Add framework for device found filtering based on UUID

[Marcel Holtmann]

Hi Geert,

>> Gitweb:     http://git.kernel.org/linus/;a=commit;h=b487b9ce9340a6e98d7f8277399304b23b7be456
>> Commit:     b487b9ce9340a6e98d7f8277399304b23b7be456
>> Parent:     bda157a40077447b25a1172a17b8ef81a2905cb7
>> Refname:    refs/heads/master
>> Author:     Marcel Holtmann <marcel@holtmann.org>
>> AuthorDate: Fri Dec 5 10:55:57 2014 +0100
>> Committer:  Johan Hedberg <johan.hedberg@intel.com>
>> CommitDate: Fri Dec 5 12:37:35 2014 +0200
>> 
>>    Bluetooth: Add framework for device found filtering based on UUID
>> 
>>    Using Start Service Discovery provides the option to specifiy a list
>>    of UUID that are used to filter out device found events. This patch
>>    provides the framework for hooking up the UUID filter.
>> 
>>    Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>>    Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
>> ---
>> net/bluetooth/mgmt.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 52 insertions(+), 2 deletions(-)
>> 
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 1715c91..e39190a 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -6802,6 +6802,11 @@ void mgmt_read_local_oob_data_complete(struct hci_dev *hdev, u8 *hash192,
>>        mgmt_pending_remove(cmd);
>> }
>> 
>> +static bool eir_has_uuids(u8 *eir, u16 eir_len, u16 uuid_count, u8 (*uuids)[16])
>> +{
>> +       return false;
>> +}
>> +
>> void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>>                       u8 addr_type, u8 *dev_class, s8 rssi, u32 flags,
>>                       u8 *eir, u16 eir_len, u8 *scan_rsp, u8 scan_rsp_len)
>> @@ -6809,6 +6814,7 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>>        char buf[512];
>>        struct mgmt_ev_device_found *ev = (void *) buf;
>>        size_t ev_size;
>> +       bool match;
> 
> net/bluetooth/mgmt.c: In function ‘mgmt_device_found’:
> net/bluetooth/mgmt.c:6993: warning: ‘match’ may be used uninitialized
> in this function

I did not get this warning, but I did consider it happening and my conclusion was that it will not and thus no need for initializing this to false.

> All tests for "match" are protected by "if (hdev->discovery.uuid_count > 0)",
> so we only have to care about that case.
> 
>>        /* Don't send events for a non-kernel initiated discovery. With
>>         * LE one exception is if we have pend_le_reports > 0 in which
>> @@ -6843,15 +6849,59 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>>        ev->rssi = rssi;
>>        ev->flags = cpu_to_le32(flags);
>> 
>> -       if (eir_len > 0)
>> +       if (eir_len > 0) {
>> +               /* When using service discovery and a list of UUID is
>> +                * provided, results with no matching UUID should be
>> +                * dropped. In case there is a match the result is
>> +                * kept and checking possible scan response data
>> +                * will be skipped.
>> +                */
>> +               if (hdev->discovery.uuid_count > 0) {
>> +                       match = eir_has_uuids(eir, eir_len,
>> +                                             hdev->discovery.uuid_count,
>> +                                             hdev->discovery.uuids);
>> +                       if (!match)
>> +                               return;
>> +               }
> 
> If we get here and if (hdev->discovery.uuid_count > 0), "match" must be true.

We need to keep the match state to optimize the later branch. There is no point in running through the second list if the first one already has a match.

Actually this is the bogus one here. The only valid reason to leave here is when also !scan_rsp_len.

>> +
>> +               /* Copy EIR or advertising data into event */
>>                memcpy(ev->eir, eir, eir_len);
>> +       } else {
>> +               /* When using service discovery and a list of UUID is
>> +                * provided, results with empty EIR or advertising data
>> +                * should be dropped since they do not match any UUID.
>> +                */
>> +               if (hdev->discovery.uuid_count > 0)
>> +                       return;
> 
> We will never get here if (hdev->discovery.uuid_count > 0).

> 
>> +       }
> 
> Hence if we get here and if (hdev->discovery.uuid_count > 0),
> "match" is always true.

> 
>>        if (dev_class && !eir_has_data_type(ev->eir, eir_len, EIR_CLASS_OF_DEV))
>>                eir_len = eir_append_data(ev->eir, eir_len, EIR_CLASS_OF_DEV,
>>                                          dev_class, 3);
>> 
>> -       if (scan_rsp_len > 0)
>> +       if (scan_rsp_len > 0) {
>> +               /* When using service discovery and a list of UUID is
>> +                * provided, results with no matching UUID should be
>> +                * dropped if there is no previous match from the
>> +                * advertising data.
>> +                */
>> +               if (hdev->discovery.uuid_count > 0) {
>> +                       if (!match && !eir_has_uuids(scan_rsp, scan_rsp_len,
> 
> "match" is true, so this whole block is useless.

This was meant to not bother checking the second data block since we already had a match in the first one. However we need to append the second data block to the first one before sending out the event.

> 
>> +                                                    hdev->discovery.uuid_count,
>> +                                                    hdev->discovery.uuids))
>> +                               return;
>> +               }
>> +
>> +               /* Append scan response data to event */
>>                memcpy(ev->eir + eir_len, scan_rsp, scan_rsp_len);
>> +       } else {
>> +               /* When using service discovery and a list of UUID is
>> +                * provided, results with empty scan response and no
>> +                * previous matched advertising data should be dropped.
>> +                */
>> +               if (hdev->discovery.uuid_count > 0 && !match)
> 
> "match" is true, so this whole block is useless.

This was suppose to do what the comment says, but we never get here :(

> 
>> +                       return;
>> +       }
>> 
>>        ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
>>        ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
> 
> Is my analysis correct?
> Am I missing something?

Your analysis seems correct and even while putting in tons of comments, my brain still messed it up.

I looked at the whole code again and this is would should make all statements work correctly.

@@ -7085,7 +7085,7 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
                        match = eir_has_uuids(eir, eir_len,
                                              hdev->discovery.uuid_count,
                                              hdev->discovery.uuids);
-                       if (!match)
+                       if (!match && !scan_rsp_len)
                                return;
                }

Regards

Marcel

Re: Bluetooth: Add framework for device found filtering based on UUID

[Geert Uytterhoeven]

Hi Marcel,

On Mon, Dec 15, 2014 at 1:27 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>>> --- a/net/bluetooth/mgmt.c
>>> +++ b/net/bluetooth/mgmt.c
>>> @@ -6809,6 +6814,7 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>>>        char buf[512];
>>>        struct mgmt_ev_device_found *ev = (void *) buf;
>>>        size_t ev_size;
>>> +       bool match;
>>
>> net/bluetooth/mgmt.c: In function ‘mgmt_device_found’:
>> net/bluetooth/mgmt.c:6993: warning: ‘match’ may be used uninitialized
>> in this function
>
> I did not get this warning, but I did consider it happening and my conclusion was that it will not and thus no need for initializing this to false.

It's highly compiler-dependent, and it's the reason I'll stick to gcc
4.1.2 as long it
helps me finding several real issues during each merge window.

>> All tests for "match" are protected by "if (hdev->discovery.uuid_count > 0)",
>> so we only have to care about that case.
>>
>>>        /* Don't send events for a non-kernel initiated discovery. With
>>>         * LE one exception is if we have pend_le_reports > 0 in which
>>> @@ -6843,15 +6849,59 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>>>        ev->rssi = rssi;
>>>        ev->flags = cpu_to_le32(flags);
>>>
>>> -       if (eir_len > 0)
>>> +       if (eir_len > 0) {
>>> +               /* When using service discovery and a list of UUID is
>>> +                * provided, results with no matching UUID should be
>>> +                * dropped. In case there is a match the result is
>>> +                * kept and checking possible scan response data
>>> +                * will be skipped.
>>> +                */
>>> +               if (hdev->discovery.uuid_count > 0) {
>>> +                       match = eir_has_uuids(eir, eir_len,
>>> +                                             hdev->discovery.uuid_count,
>>> +                                             hdev->discovery.uuids);
>>> +                       if (!match)
>>> +                               return;
>>> +               }
>>
>> If we get here and if (hdev->discovery.uuid_count > 0), "match" must be true.
>
> We need to keep the match state to optimize the later branch. There is no point in running through the second list if the first one already has a match.
>
> Actually this is the bogus one here. The only valid reason to leave here is when also !scan_rsp_len.

OK.

>> Is my analysis correct?
>> Am I missing something?
>
> Your analysis seems correct and even while putting in tons of comments, my brain still messed it up.
>
> I looked at the whole code again and this is would should make all statements work correctly.
>
> @@ -7085,7 +7085,7 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>                         match = eir_has_uuids(eir, eir_len,
>                                               hdev->discovery.uuid_count,
>                                               hdev->discovery.uuids);
> -                       if (!match)
> +                       if (!match && !scan_rsp_len)
>                                 return;
>                 }

Thanks, that indeed changes all further checks for match to make sense.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds