On Wed, Nov 19, 2014 at 11:37 AM, Johan Hedberg <johan.hedberg@xxxxxxxxx> wrote:
> Hi Jakub,
>
> On Mon, Nov 10, 2014, Jakub Pawlowski wrote:
>> /* Default LE RPA expiry time, 15 minutes */
>> @@ -307,6 +313,8 @@ struct hci_dev {
>> struct discovery_state discovery;
>> struct hci_conn_hash conn_hash;
>>
>> + struct list_head discov_uuid_filter;
>
> Would it not make sense to include this inside the discovery_state
> struct?
>
>> +void mgmt_clean_up_service_discovery(struct hci_dev *hdev)
>> +{
>> + struct uuid_filter *filter;
>> + struct uuid_filter *tmp;
>> +
>> + if (!test_bit(HCI_SERVICE_FILTER, &hdev->dev_flags))
>> + return;
>> +
>> + clear_bit(HCI_SERVICE_FILTER, &hdev->dev_flags);
>
> The above two test_bit & clear_bit calls could be combined into a single
> atomic test_and_clear_bit call:
>
> if (!test_and_clear_bit(HCI_SERVICE_FILTER, &hdev->dev_flags)
> return;
>
>> +int init_service_discovery(struct hci_dev *hdev,
>> + struct mgmt_uuid_filter *filters, __le16 filter_cnt)
>
> Should this function not be declared static?
>
>> +{
>> + int i;
>> +
>> + for (i = 0; i < filter_cnt; i++) {
>
> filter_cnt is a little endian value so this is completely broken on any
> big endian architecture. The general principle we follow is to try to
> keep the protocol endianness only in the protocol PDUs and everywhere
> else in host endianness. So you should probably change this function to
> receive a u16 instead.
>
>> static int generic_start_discovery(struct sock *sk, struct hci_dev *hdev,
>> void *data, u16 len, uint16_t opcode)
>> {
>> - struct mgmt_cp_start_discovery *cp = data;
>> struct pending_cmd *cmd;
>> struct hci_cp_le_set_scan_param param_cp;
>> struct hci_cp_le_set_scan_enable enable_cp;
>> struct hci_cp_inquiry inq_cp;
>> struct hci_request req;
>> + struct mgmt_uuid_filter *serv_filters = NULL;
>> /* General inquiry access code (GIAC) */
>> u8 lap[3] = { 0x33, 0x8b, 0x9e };
>> u8 status, own_addr_type, type;
>> int err;
>> + __le16 serv_filter_cnt = 0;
>>
>> BT_DBG("%s", hdev->name);
>>
>> - type = cp->type;
>> + if (opcode == MGMT_OP_START_SERVICE_DISCOVERY) {
>> + struct mgmt_cp_start_service_discovery *cp = data;
>> + u16 expected_len, filter_count;
>> +
>> + type = cp->type;
>> + filter_count = __le16_to_cpu(cp->filter_count);
>
> Ok, here I see the problem with this filter_count. Your code might
> actually work because you do have the endianness conversion here, but
> you have the wrong type for this variable. Since you're converting to
> host endianness you can't assign to a __le16 variable. Instead this
> variable should be declared u16. Static analyzers (e.g. sparse) should
> have given you a warning here.
>
>> + if (serv_filters != NULL) {
>> + err = init_service_discovery(hdev, serv_filters,
>> + serv_filter_cnt);
>> + if (err != 0)
>> + goto failed;
>
> I think "if (err)" is more common than "if (err != 0)".
>
>> +void free_uuids_list(struct list_head *uuids)
>
> Shouldn't this be declared static (you should be getting compiler
> warnings because of it).
>
>> +uint8_t find_match(struct uuid_filter *filter, u8 *uuid, s8 rssi)
>
> Why suddenly uint8_t (which btw should be u8) when you've everywhere
> else used "int" for the match type? Also, shouldn't this function be
> declared static? The uuid is probably better declared as "u8 uuid[16]"
> to make it clear that it has a fixed expected length.
I also found similar error in patch 2/3 introducing generic start/stop
discovery which I fixed (I was using uint16_t instead of u16)
>
>> +int find_matches(struct hci_dev *hdev, s8 rssi, struct list_head *uuids)
>
> Missing static declaration again?
>
>> 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)
>> @@ -6820,6 +7061,9 @@ 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;
>> + LIST_HEAD(uuids);
>> + int ret = 0;
>> + u8 match_type;
>>
>> /* Don't send events for a non-kernel initiated discovery. With
>> * LE one exception is if we have pend_le_reports > 0 in which
>> @@ -6858,7 +7102,29 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>> ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
>> ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
>>
>> - mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
>> + if (!test_bit(HCI_SERVICE_FILTER, &hdev->dev_flags)) {
>> + mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
>> + return;
>> + }
>> +
>> + ret = eir_parse(eir, eir_len, &uuids);
>> + if (list_empty(&uuids) || ret != 0)
>> + return;
>
> I think I already mentioned this in a previous review but you've got a
> memory leak here if the list is not empty but eir_parse still fails (ret
> is not 0). Btw, please be consistent with your error variables - you
> used "err" in an earlier place (which I prefer) and the check can be "if
> (err)" instead of "if (err != 0)".
The other memory leak was in find_matches, and it's already fixed.
>
> Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
