Whenever the controller starts/stops monitoring a bt device, it sends
MSFT Monitor Device event. Add handler to read this vendor event.
Test performed:
- Verified by logs that the MSFT Monitor Device event is received from
the controller whenever it starts/stops monitoring a device.
Signed-off-by: Manish Mandlik <[email protected]>
Reviewed-by: Miao-chen Chou <[email protected]>
---
Hello Bt-Maintainers,
As mentioned in the bluez patch series [1], we need to capture the 'MSFT
Monitor Device' event from the controller and pass on the necessary
information to the bluetoothd.
This is required to further optimize the power consumption by avoiding
handling of RSSI thresholds and timeouts in the user space and let the
controller do the RSSI tracking.
This patch series adds support to read the MSFT vendor event
HCI_VS_MSFT_LE_Monitor_Device_Event and introduces new MGMT events
MGMT_EV_ADV_MONITOR_DEVICE_FOUND and MGMT_EV_ADV_MONITOR_DEVICE_LOST to
indicate that the controller has started/stopped tracking a particular
device.
Please let me know what you think about this or if you have any further
questions.
[1] https://patchwork.kernel.org/project/bluetooth/list/?series=583423
Thanks,
Manish.
Changes in v9:
- Fix compiler error.
Changes in v8:
- Fix use-after-free in msft_le_cancel_monitor_advertisement_cb().
- Use skb_pull_data() instead of skb_pull().
Changes in v6:
- Fix compiler warning bt_dev_err() missing argument.
Changes in v5:
- Split v4 into two patches.
- Buffer controller Device Found event and maintain the device tracking
state in the kernel.
Changes in v4:
- Add Advertisement Monitor Device Found event and update addr type.
Changes in v3:
- Discard changes to the Device Found event and notify bluetoothd only
when the controller stops monitoring the device via new Device Lost
event.
Changes in v2:
- Instead of creating a new 'Device Tracking' event, add a flag 'Device
Tracked' in the existing 'Device Found' event and add a new 'Device
Lost' event to indicate that the controller has stopped tracking that
device.
include/net/bluetooth/hci_core.h | 11 +++
net/bluetooth/hci_core.c | 1 +
net/bluetooth/msft.c | 150 +++++++++++++++++++++++++++++--
3 files changed, 154 insertions(+), 8 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 4d69dcfebd63..c2a8b1163c30 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -258,6 +258,15 @@ struct adv_info {
#define HCI_ADV_TX_POWER_NO_PREFERENCE 0x7F
+struct monitored_device {
+ struct list_head list;
+
+ bdaddr_t bdaddr;
+ __u8 addr_type;
+ __u16 handle;
+ bool notified;
+};
+
struct adv_pattern {
struct list_head list;
__u8 ad_type;
@@ -590,6 +599,8 @@ struct hci_dev {
struct delayed_work interleave_scan;
+ struct list_head monitored_devices;
+
#if IS_ENABLED(CONFIG_BT_LEDS)
struct led_trigger *power_led;
#endif
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 38063bf1fdc5..c67e6d40c97d 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2503,6 +2503,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv)
INIT_LIST_HEAD(&hdev->conn_hash.list);
INIT_LIST_HEAD(&hdev->adv_instances);
INIT_LIST_HEAD(&hdev->blocked_keys);
+ INIT_LIST_HEAD(&hdev->monitored_devices);
INIT_LIST_HEAD(&hdev->local_codecs);
INIT_WORK(&hdev->rx_work, hci_rx_work);
diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
index 6a943634b31a..a2e1cfb5d5d0 100644
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -80,6 +80,14 @@ struct msft_rp_le_set_advertisement_filter_enable {
__u8 sub_opcode;
} __packed;
+#define MSFT_EV_LE_MONITOR_DEVICE 0x02
+struct msft_ev_le_monitor_device {
+ __u8 addr_type;
+ bdaddr_t bdaddr;
+ __u8 monitor_handle;
+ __u8 monitor_state;
+} __packed;
+
struct msft_monitor_advertisement_handle_data {
__u8 msft_handle;
__u16 mgmt_handle;
@@ -266,6 +274,7 @@ static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
struct msft_data *msft = hdev->msft_data;
int err;
bool pending;
+ struct monitored_device *dev, *tmp;
if (status)
goto done;
@@ -294,6 +303,15 @@ static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
if (monitor && !msft->suspending)
hci_free_adv_monitor(hdev, monitor);
+ /* Clear any monitored devices by this Adv Monitor */
+ list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices,
+ list) {
+ if (dev->handle == handle_data->mgmt_handle) {
+ list_del(&dev->list);
+ kfree(dev);
+ }
+ }
+
list_del(&handle_data->list);
kfree(handle_data);
}
@@ -538,6 +556,7 @@ void msft_do_close(struct hci_dev *hdev)
struct msft_data *msft = hdev->msft_data;
struct msft_monitor_advertisement_handle_data *handle_data, *tmp;
struct adv_monitor *monitor;
+ struct monitored_device *dev, *tmp_dev;
if (!msft)
return;
@@ -557,6 +576,16 @@ void msft_do_close(struct hci_dev *hdev)
list_del(&handle_data->list);
kfree(handle_data);
}
+
+ hci_dev_lock(hdev);
+
+ /* Clear any devices that are being monitored */
+ list_for_each_entry_safe(dev, tmp_dev, &hdev->monitored_devices, list) {
+ list_del(&dev->list);
+ kfree(dev);
+ }
+
+ hci_dev_unlock(hdev);
}
void msft_register(struct hci_dev *hdev)
@@ -590,10 +619,103 @@ void msft_unregister(struct hci_dev *hdev)
kfree(msft);
}
+/* This function requires the caller holds hdev->lock */
+static void msft_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr,
+ __u8 addr_type, __u16 mgmt_handle)
+{
+ struct monitored_device *dev;
+
+ dev = kmalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ bt_dev_err(hdev, "MSFT vendor event %u: no memory",
+ MSFT_EV_LE_MONITOR_DEVICE);
+ return;
+ }
+
+ bacpy(&dev->bdaddr, bdaddr);
+ dev->addr_type = addr_type;
+ dev->handle = mgmt_handle;
+ dev->notified = false;
+
+ INIT_LIST_HEAD(&dev->list);
+ list_add(&dev->list, &hdev->monitored_devices);
+}
+
+/* This function requires the caller holds hdev->lock */
+static void msft_device_lost(struct hci_dev *hdev, bdaddr_t *bdaddr,
+ __u8 addr_type, __u16 mgmt_handle)
+{
+ struct monitored_device *dev, *tmp;
+
+ list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, list) {
+ if (dev->handle == mgmt_handle) {
+ list_del(&dev->list);
+ kfree(dev);
+
+ break;
+ }
+ }
+}
+
+static void *msft_skb_pull(struct hci_dev *hdev, struct sk_buff *skb,
+ u8 ev, size_t len)
+{
+ void *data;
+
+ data = skb_pull_data(skb, len);
+ if (!data)
+ bt_dev_err(hdev, "Malformed MSFT vendor event: 0x%02x", ev);
+
+ return data;
+}
+
+/* This function requires the caller holds hdev->lock */
+static void msft_monitor_device_evt(struct hci_dev *hdev, struct sk_buff *skb)
+{
+ struct msft_ev_le_monitor_device *ev;
+ struct msft_monitor_advertisement_handle_data *handle_data;
+ u8 addr_type;
+
+ ev = msft_skb_pull(hdev, skb, MSFT_EV_LE_MONITOR_DEVICE, sizeof(*ev));
+ if (!ev)
+ return;
+
+ bt_dev_dbg(hdev,
+ "MSFT vendor event 0x%02x: handle 0x%04x state %d addr %pMR",
+ MSFT_EV_LE_MONITOR_DEVICE, ev->monitor_handle,
+ ev->monitor_state, &ev->bdaddr);
+
+ handle_data = msft_find_handle_data(hdev, ev->monitor_handle, false);
+
+ switch (ev->addr_type) {
+ case ADDR_LE_DEV_PUBLIC:
+ addr_type = BDADDR_LE_PUBLIC;
+ break;
+
+ case ADDR_LE_DEV_RANDOM:
+ addr_type = BDADDR_LE_RANDOM;
+ break;
+
+ default:
+ bt_dev_err(hdev,
+ "MSFT vendor event 0x%02x: unknown addr type 0x%02x",
+ MSFT_EV_LE_MONITOR_DEVICE, ev->addr_type);
+ return;
+ }
+
+ if (ev->monitor_state)
+ msft_device_found(hdev, &ev->bdaddr, addr_type,
+ handle_data->mgmt_handle);
+ else
+ msft_device_lost(hdev, &ev->bdaddr, addr_type,
+ handle_data->mgmt_handle);
+}
+
void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
{
struct msft_data *msft = hdev->msft_data;
- u8 event;
+ u8 *evt_prefix;
+ u8 *evt;
if (!msft)
return;
@@ -602,13 +724,12 @@ void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
* matches, and otherwise just return.
*/
if (msft->evt_prefix_len > 0) {
- if (skb->len < msft->evt_prefix_len)
+ evt_prefix = msft_skb_pull(hdev, skb, 0, msft->evt_prefix_len);
+ if (!evt_prefix)
return;
- if (memcmp(skb->data, msft->evt_prefix, msft->evt_prefix_len))
+ if (memcmp(evt_prefix, msft->evt_prefix, msft->evt_prefix_len))
return;
-
- skb_pull(skb, msft->evt_prefix_len);
}
/* Every event starts at least with an event code and the rest of
@@ -617,10 +738,23 @@ void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
if (skb->len < 1)
return;
- event = *skb->data;
- skb_pull(skb, 1);
+ hci_dev_lock(hdev);
+
+ evt = msft_skb_pull(hdev, skb, 0, sizeof(*evt));
+ if (!evt)
+ return;
+
+ switch (*evt) {
+ case MSFT_EV_LE_MONITOR_DEVICE:
+ msft_monitor_device_evt(hdev, skb);
+ break;
- bt_dev_dbg(hdev, "MSFT vendor event %u", event);
+ default:
+ bt_dev_dbg(hdev, "MSFT vendor event 0x%02x", *evt);
+ break;
+ }
+
+ hci_dev_unlock(hdev);
}
__u64 msft_get_features(struct hci_dev *hdev)
--
2.34.1.173.g76aa8bc2d0-goog
This patch introduces two new MGMT events for notifying the bluetoothd
whenever the controller starts/stops monitoring a device.
Test performed:
- Verified by logs that the MSFT Monitor Device is received from the
controller and the bluetoothd is notified whenever the controller
starts/stops monitoring a device.
Signed-off-by: Manish Mandlik <[email protected]>
Reviewed-by: Miao-chen Chou <[email protected]>
---
(no changes since v8)
Changes in v8:
- Refactor mgmt_adv_monitor_device_found() to make use of
skb_put/skb_put_data.
Changes in v7:
- Refactor mgmt_device_found() to fix stack frame size limit
Changes in v6:
- Fix compiler warning for mgmt_adv_monitor_device_found().
Changes in v5:
- New patch in the series. Split previous patch into two.
- Update the Device Found logic to send existing Device Found event or
Adv Monitor Device Found event depending on the active scanning state.
include/net/bluetooth/hci_core.h | 3 +
include/net/bluetooth/mgmt.h | 16 +++++
net/bluetooth/mgmt.c | 115 +++++++++++++++++++++++++++++--
net/bluetooth/msft.c | 15 +++-
4 files changed, 143 insertions(+), 6 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index c2a8b1163c30..290b8a1ac0f9 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -600,6 +600,7 @@ struct hci_dev {
struct delayed_work interleave_scan;
struct list_head monitored_devices;
+ bool advmon_pend_notify;
#if IS_ENABLED(CONFIG_BT_LEDS)
struct led_trigger *power_led;
@@ -1852,6 +1853,8 @@ void mgmt_adv_monitor_removed(struct hci_dev *hdev, u16 handle);
int mgmt_phy_configuration_changed(struct hci_dev *hdev, struct sock *skip);
int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
+void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
+ bdaddr_t *bdaddr, u8 addr_type);
u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
u16 to_multiplier);
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 107b25deae68..99266f7aebdc 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -1104,3 +1104,19 @@ struct mgmt_ev_controller_resume {
#define MGMT_WAKE_REASON_NON_BT_WAKE 0x0
#define MGMT_WAKE_REASON_UNEXPECTED 0x1
#define MGMT_WAKE_REASON_REMOTE_WAKE 0x2
+
+#define MGMT_EV_ADV_MONITOR_DEVICE_FOUND 0x002f
+struct mgmt_ev_adv_monitor_device_found {
+ __le16 monitor_handle;
+ struct mgmt_addr_info addr;
+ __s8 rssi;
+ __le32 flags;
+ __le16 eir_len;
+ __u8 eir[0];
+} __packed;
+
+#define MGMT_EV_ADV_MONITOR_DEVICE_LOST 0x0030
+struct mgmt_ev_adv_monitor_device_lost {
+ __le16 monitor_handle;
+ struct mgmt_addr_info addr;
+} __packed;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index e931e417d3e1..c65247b5896c 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -174,6 +174,8 @@ static const u16 mgmt_events[] = {
MGMT_EV_ADV_MONITOR_REMOVED,
MGMT_EV_CONTROLLER_SUSPEND,
MGMT_EV_CONTROLLER_RESUME,
+ MGMT_EV_ADV_MONITOR_DEVICE_FOUND,
+ MGMT_EV_ADV_MONITOR_DEVICE_LOST,
};
static const u16 mgmt_untrusted_commands[] = {
@@ -9562,12 +9564,116 @@ static bool is_filter_match(struct hci_dev *hdev, s8 rssi, u8 *eir,
return true;
}
+void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
+ bdaddr_t *bdaddr, u8 addr_type)
+{
+ struct mgmt_ev_adv_monitor_device_lost ev;
+
+ ev.monitor_handle = cpu_to_le16(handle);
+ bacpy(&ev.addr.bdaddr, bdaddr);
+ ev.addr.type = addr_type;
+
+ mgmt_event(MGMT_EV_ADV_MONITOR_DEVICE_LOST, hdev, &ev, sizeof(ev),
+ NULL);
+}
+
+static void mgmt_adv_monitor_device_found(struct hci_dev *hdev,
+ bdaddr_t *bdaddr, bool report_device,
+ struct sk_buff *skb,
+ struct sock *skip_sk)
+{
+ struct sk_buff *advmon_skb;
+ size_t advmon_skb_len;
+ __le16 *monitor_handle;
+ struct monitored_device *dev, *tmp;
+ bool matched = false;
+ bool notify = false;
+
+ /* We have received the Advertisement Report because:
+ * 1. the kernel has initiated active discovery
+ * 2. if not, we have pend_le_reports > 0 in which case we are doing
+ * passive scanning
+ * 3. if none of the above is true, we have one or more active
+ * Advertisement Monitor
+ *
+ * For case 1 and 2, report all advertisements via MGMT_EV_DEVICE_FOUND
+ * and report ONLY one advertisement per device for the matched Monitor
+ * via MGMT_EV_ADV_MONITOR_DEVICE_FOUND event.
+ *
+ * For case 3, since we are not active scanning and all advertisements
+ * received are due to a matched Advertisement Monitor, report all
+ * advertisements ONLY via MGMT_EV_ADV_MONITOR_DEVICE_FOUND event.
+ */
+ if (report_device && !hdev->advmon_pend_notify) {
+ mgmt_event_skb(skb, skip_sk);
+ return;
+ }
+
+ advmon_skb_len = (sizeof(struct mgmt_ev_adv_monitor_device_found) -
+ sizeof(struct mgmt_ev_device_found)) + skb->len;
+ advmon_skb = mgmt_alloc_skb(hdev, MGMT_EV_ADV_MONITOR_DEVICE_FOUND,
+ advmon_skb_len);
+ if (!advmon_skb) {
+ if (report_device)
+ mgmt_event_skb(skb, skip_sk);
+ else
+ kfree_skb(skb);
+ return;
+ }
+
+ /* ADV_MONITOR_DEVICE_FOUND is similar to DEVICE_FOUND event except
+ * that it also has 'monitor_handle'. Make a copy of DEVICE_FOUND and
+ * store monitor_handle of the matched monitor.
+ */
+ monitor_handle = skb_put(advmon_skb, sizeof(*monitor_handle));
+ skb_put_data(advmon_skb, skb->data, skb->len);
+
+ hdev->advmon_pend_notify = false;
+
+ list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, list) {
+ if (!bacmp(&dev->bdaddr, bdaddr)) {
+ matched = true;
+
+ if (!dev->notified) {
+ *monitor_handle = cpu_to_le16(dev->handle);
+ notify = true;
+ dev->notified = true;
+ }
+ }
+
+ if (!dev->notified)
+ hdev->advmon_pend_notify = true;
+ }
+
+ if (!report_device &&
+ ((matched && !notify) || !msft_monitor_supported(hdev))) {
+ /* Handle 0 indicates that we are not active scanning and this
+ * is a subsequent advertisement report for an already matched
+ * Advertisement Monitor or the controller offloading support
+ * is not available.
+ */
+ *monitor_handle = 0;
+ notify = true;
+ }
+
+ if (report_device)
+ mgmt_event_skb(skb, skip_sk);
+ else
+ kfree_skb(skb);
+
+ if (notify)
+ mgmt_event_skb(advmon_skb, skip_sk);
+ else
+ kfree_skb(advmon_skb);
+}
+
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)
{
struct sk_buff *skb;
struct mgmt_ev_device_found *ev;
+ bool report_device = hci_discovery_active(hdev);
/* Don't send events for a non-kernel initiated discovery. With
* LE one exception is if we have pend_le_reports > 0 in which
@@ -9576,11 +9682,10 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
if (!hci_discovery_active(hdev)) {
if (link_type == ACL_LINK)
return;
- if (link_type == LE_LINK &&
- list_empty(&hdev->pend_le_reports) &&
- !hci_is_adv_monitoring(hdev)) {
+ if (link_type == LE_LINK && !list_empty(&hdev->pend_le_reports))
+ report_device = true;
+ else if (!hci_is_adv_monitoring(hdev))
return;
- }
}
if (hdev->discovery.result_filtering) {
@@ -9645,7 +9750,7 @@ 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);
- mgmt_event_skb(skb, NULL);
+ mgmt_adv_monitor_device_found(hdev, bdaddr, report_device, skb, NULL);
}
void mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
index a2e1cfb5d5d0..d507faa9626a 100644
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -579,8 +579,16 @@ void msft_do_close(struct hci_dev *hdev)
hci_dev_lock(hdev);
- /* Clear any devices that are being monitored */
+ /* Clear any devices that are being monitored and notify device lost */
+
+ hdev->advmon_pend_notify = false;
+
list_for_each_entry_safe(dev, tmp_dev, &hdev->monitored_devices, list) {
+ if (dev->notified)
+ mgmt_adv_monitor_device_lost(hdev, dev->handle,
+ &dev->bdaddr,
+ dev->addr_type);
+
list_del(&dev->list);
kfree(dev);
}
@@ -639,6 +647,7 @@ static void msft_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr,
INIT_LIST_HEAD(&dev->list);
list_add(&dev->list, &hdev->monitored_devices);
+ hdev->advmon_pend_notify = true;
}
/* This function requires the caller holds hdev->lock */
@@ -649,6 +658,10 @@ static void msft_device_lost(struct hci_dev *hdev, bdaddr_t *bdaddr,
list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, list) {
if (dev->handle == mgmt_handle) {
+ if (dev->notified)
+ mgmt_adv_monitor_device_lost(hdev, mgmt_handle,
+ bdaddr, addr_type);
+
list_del(&dev->list);
kfree(dev);
--
2.34.1.173.g76aa8bc2d0-goog
Use correct sizeof() parameter while allocating skb.
Signed-off-by: Manish Mandlik <[email protected]>
---
(no changes since v8)
Changes in v8:
- New patch in the series.
net/bluetooth/mgmt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index c65247b5896c..5fd29bd399f1 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -9709,7 +9709,7 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
/* Allocate skb. The 5 extra bytes are for the potential CoD field */
skb = mgmt_alloc_skb(hdev, MGMT_EV_DEVICE_FOUND,
- sizeof(ev) + eir_len + scan_rsp_len + 5);
+ sizeof(*ev) + eir_len + scan_rsp_len + 5);
if (!skb)
return;
--
2.34.1.173.g76aa8bc2d0-goog
Hi Manish,
On Thu, Dec 16, 2021 at 4:50 AM Manish Mandlik <[email protected]> wrote:
>
> Whenever the controller starts/stops monitoring a bt device, it sends
> MSFT Monitor Device event. Add handler to read this vendor event.
>
> Test performed:
> - Verified by logs that the MSFT Monitor Device event is received from
> the controller whenever it starts/stops monitoring a device.
I'd expect that we would be extending the emulator to perform such
tests, this is especially important for events since syzbot fuzes
events, so we better have a way to test these code path from CI so we
can properly validate these changes.
> Signed-off-by: Manish Mandlik <[email protected]>
> Reviewed-by: Miao-chen Chou <[email protected]>
> ---
> Hello Bt-Maintainers,
>
> As mentioned in the bluez patch series [1], we need to capture the 'MSFT
> Monitor Device' event from the controller and pass on the necessary
> information to the bluetoothd.
>
> This is required to further optimize the power consumption by avoiding
> handling of RSSI thresholds and timeouts in the user space and let the
> controller do the RSSI tracking.
>
> This patch series adds support to read the MSFT vendor event
> HCI_VS_MSFT_LE_Monitor_Device_Event and introduces new MGMT events
> MGMT_EV_ADV_MONITOR_DEVICE_FOUND and MGMT_EV_ADV_MONITOR_DEVICE_LOST to
> indicate that the controller has started/stopped tracking a particular
> device.
>
> Please let me know what you think about this or if you have any further
> questions.
>
> [1] https://patchwork.kernel.org/project/bluetooth/list/?series=583423
>
> Thanks,
> Manish.
>
> Changes in v9:
> - Fix compiler error.
>
> Changes in v8:
> - Fix use-after-free in msft_le_cancel_monitor_advertisement_cb().
> - Use skb_pull_data() instead of skb_pull().
>
> Changes in v6:
> - Fix compiler warning bt_dev_err() missing argument.
>
> Changes in v5:
> - Split v4 into two patches.
> - Buffer controller Device Found event and maintain the device tracking
> state in the kernel.
>
> Changes in v4:
> - Add Advertisement Monitor Device Found event and update addr type.
>
> Changes in v3:
> - Discard changes to the Device Found event and notify bluetoothd only
> when the controller stops monitoring the device via new Device Lost
> event.
>
> Changes in v2:
> - Instead of creating a new 'Device Tracking' event, add a flag 'Device
> Tracked' in the existing 'Device Found' event and add a new 'Device
> Lost' event to indicate that the controller has stopped tracking that
> device.
>
> include/net/bluetooth/hci_core.h | 11 +++
> net/bluetooth/hci_core.c | 1 +
> net/bluetooth/msft.c | 150 +++++++++++++++++++++++++++++--
> 3 files changed, 154 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 4d69dcfebd63..c2a8b1163c30 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -258,6 +258,15 @@ struct adv_info {
>
> #define HCI_ADV_TX_POWER_NO_PREFERENCE 0x7F
>
> +struct monitored_device {
> + struct list_head list;
> +
> + bdaddr_t bdaddr;
> + __u8 addr_type;
> + __u16 handle;
> + bool notified;
> +};
> +
> struct adv_pattern {
> struct list_head list;
> __u8 ad_type;
> @@ -590,6 +599,8 @@ struct hci_dev {
>
> struct delayed_work interleave_scan;
>
> + struct list_head monitored_devices;
> +
> #if IS_ENABLED(CONFIG_BT_LEDS)
> struct led_trigger *power_led;
> #endif
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 38063bf1fdc5..c67e6d40c97d 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2503,6 +2503,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv)
> INIT_LIST_HEAD(&hdev->conn_hash.list);
> INIT_LIST_HEAD(&hdev->adv_instances);
> INIT_LIST_HEAD(&hdev->blocked_keys);
> + INIT_LIST_HEAD(&hdev->monitored_devices);
>
> INIT_LIST_HEAD(&hdev->local_codecs);
> INIT_WORK(&hdev->rx_work, hci_rx_work);
> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> index 6a943634b31a..a2e1cfb5d5d0 100644
> --- a/net/bluetooth/msft.c
> +++ b/net/bluetooth/msft.c
> @@ -80,6 +80,14 @@ struct msft_rp_le_set_advertisement_filter_enable {
> __u8 sub_opcode;
> } __packed;
>
> +#define MSFT_EV_LE_MONITOR_DEVICE 0x02
> +struct msft_ev_le_monitor_device {
> + __u8 addr_type;
> + bdaddr_t bdaddr;
> + __u8 monitor_handle;
> + __u8 monitor_state;
> +} __packed;
> +
> struct msft_monitor_advertisement_handle_data {
> __u8 msft_handle;
> __u16 mgmt_handle;
> @@ -266,6 +274,7 @@ static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
> struct msft_data *msft = hdev->msft_data;
> int err;
> bool pending;
> + struct monitored_device *dev, *tmp;
>
> if (status)
> goto done;
> @@ -294,6 +303,15 @@ static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
> if (monitor && !msft->suspending)
> hci_free_adv_monitor(hdev, monitor);
>
> + /* Clear any monitored devices by this Adv Monitor */
> + list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices,
> + list) {
> + if (dev->handle == handle_data->mgmt_handle) {
> + list_del(&dev->list);
> + kfree(dev);
> + }
> + }
It might be a good idea to create a helper function from removing an
item like above using a special handle to clear everything e.g.
monitor_device_del(hdev, handle).
> list_del(&handle_data->list);
> kfree(handle_data);
> }
> @@ -538,6 +556,7 @@ void msft_do_close(struct hci_dev *hdev)
> struct msft_data *msft = hdev->msft_data;
> struct msft_monitor_advertisement_handle_data *handle_data, *tmp;
> struct adv_monitor *monitor;
> + struct monitored_device *dev, *tmp_dev;
>
> if (!msft)
> return;
> @@ -557,6 +576,16 @@ void msft_do_close(struct hci_dev *hdev)
> list_del(&handle_data->list);
> kfree(handle_data);
> }
> +
> + hci_dev_lock(hdev);
> +
> + /* Clear any devices that are being monitored */
> + list_for_each_entry_safe(dev, tmp_dev, &hdev->monitored_devices, list) {
> + list_del(&dev->list);
> + kfree(dev);
> + }
> +
> + hci_dev_unlock(hdev);
> }
>
> void msft_register(struct hci_dev *hdev)
> @@ -590,10 +619,103 @@ void msft_unregister(struct hci_dev *hdev)
> kfree(msft);
> }
>
> +/* This function requires the caller holds hdev->lock */
> +static void msft_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr,
> + __u8 addr_type, __u16 mgmt_handle)
> +{
> + struct monitored_device *dev;
> +
> + dev = kmalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev) {
> + bt_dev_err(hdev, "MSFT vendor event %u: no memory",
> + MSFT_EV_LE_MONITOR_DEVICE);
> + return;
> + }
> +
> + bacpy(&dev->bdaddr, bdaddr);
> + dev->addr_type = addr_type;
> + dev->handle = mgmt_handle;
> + dev->notified = false;
> +
> + INIT_LIST_HEAD(&dev->list);
> + list_add(&dev->list, &hdev->monitored_devices);
> +}
> +
> +/* This function requires the caller holds hdev->lock */
> +static void msft_device_lost(struct hci_dev *hdev, bdaddr_t *bdaddr,
> + __u8 addr_type, __u16 mgmt_handle)
> +{
> + struct monitored_device *dev, *tmp;
> +
> + list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, list) {
> + if (dev->handle == mgmt_handle) {
> + list_del(&dev->list);
> + kfree(dev);
> +
> + break;
> + }
> + }
> +}
> +
> +static void *msft_skb_pull(struct hci_dev *hdev, struct sk_buff *skb,
> + u8 ev, size_t len)
> +{
> + void *data;
> +
> + data = skb_pull_data(skb, len);
> + if (!data)
> + bt_dev_err(hdev, "Malformed MSFT vendor event: 0x%02x", ev);
> +
> + return data;
> +}
> +
> +/* This function requires the caller holds hdev->lock */
> +static void msft_monitor_device_evt(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> + struct msft_ev_le_monitor_device *ev;
> + struct msft_monitor_advertisement_handle_data *handle_data;
> + u8 addr_type;
> +
> + ev = msft_skb_pull(hdev, skb, MSFT_EV_LE_MONITOR_DEVICE, sizeof(*ev));
> + if (!ev)
> + return;
> +
> + bt_dev_dbg(hdev,
> + "MSFT vendor event 0x%02x: handle 0x%04x state %d addr %pMR",
> + MSFT_EV_LE_MONITOR_DEVICE, ev->monitor_handle,
> + ev->monitor_state, &ev->bdaddr);
> +
> + handle_data = msft_find_handle_data(hdev, ev->monitor_handle, false);
> +
> + switch (ev->addr_type) {
> + case ADDR_LE_DEV_PUBLIC:
> + addr_type = BDADDR_LE_PUBLIC;
> + break;
> +
> + case ADDR_LE_DEV_RANDOM:
> + addr_type = BDADDR_LE_RANDOM;
> + break;
> +
> + default:
> + bt_dev_err(hdev,
> + "MSFT vendor event 0x%02x: unknown addr type 0x%02x",
> + MSFT_EV_LE_MONITOR_DEVICE, ev->addr_type);
> + return;
> + }
> +
> + if (ev->monitor_state)
> + msft_device_found(hdev, &ev->bdaddr, addr_type,
> + handle_data->mgmt_handle);
> + else
> + msft_device_lost(hdev, &ev->bdaddr, addr_type,
> + handle_data->mgmt_handle);
> +}
> +
> void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
> {
> struct msft_data *msft = hdev->msft_data;
> - u8 event;
> + u8 *evt_prefix;
> + u8 *evt;
>
> if (!msft)
> return;
> @@ -602,13 +724,12 @@ void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
> * matches, and otherwise just return.
> */
> if (msft->evt_prefix_len > 0) {
> - if (skb->len < msft->evt_prefix_len)
> + evt_prefix = msft_skb_pull(hdev, skb, 0, msft->evt_prefix_len);
> + if (!evt_prefix)
> return;
>
> - if (memcmp(skb->data, msft->evt_prefix, msft->evt_prefix_len))
> + if (memcmp(evt_prefix, msft->evt_prefix, msft->evt_prefix_len))
> return;
> -
> - skb_pull(skb, msft->evt_prefix_len);
> }
>
> /* Every event starts at least with an event code and the rest of
> @@ -617,10 +738,23 @@ void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
> if (skb->len < 1)
> return;
>
> - event = *skb->data;
> - skb_pull(skb, 1);
> + hci_dev_lock(hdev);
> +
> + evt = msft_skb_pull(hdev, skb, 0, sizeof(*evt));
> + if (!evt)
> + return;
> +
> + switch (*evt) {
> + case MSFT_EV_LE_MONITOR_DEVICE:
> + msft_monitor_device_evt(hdev, skb);
> + break;
>
> - bt_dev_dbg(hdev, "MSFT vendor event %u", event);
> + default:
> + bt_dev_dbg(hdev, "MSFT vendor event 0x%02x", *evt);
> + break;
> + }
> +
> + hci_dev_unlock(hdev);
> }
>
> __u64 msft_get_features(struct hci_dev *hdev)
> --
> 2.34.1.173.g76aa8bc2d0-goog
>
--
Luiz Augusto von Dentz
Hi Manish,
On Thu, Dec 16, 2021 at 4:50 AM Manish Mandlik <[email protected]> wrote:
>
> Use correct sizeof() parameter while allocating skb.
>
> Signed-off-by: Manish Mandlik <[email protected]>
> ---
>
> (no changes since v8)
>
> Changes in v8:
> - New patch in the series.
>
> net/bluetooth/mgmt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index c65247b5896c..5fd29bd399f1 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -9709,7 +9709,7 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>
> /* Allocate skb. The 5 extra bytes are for the potential CoD field */
> skb = mgmt_alloc_skb(hdev, MGMT_EV_DEVICE_FOUND,
> - sizeof(ev) + eir_len + scan_rsp_len + 5);
> + sizeof(*ev) + eir_len + scan_rsp_len + 5);
> if (!skb)
> return;
>
> --
> 2.34.1.173.g76aa8bc2d0-goog
There is already a patch addressing this:
https://patchwork.kernel.org/project/bluetooth/patch/[email protected]/
Please use that instead and if that works for you reply adding Tested-by.
--
Luiz Augusto von Dentz
Hi Manish,
url: https://github.com/0day-ci/linux/commits/Manish-Mandlik/bluetooth-msft-Handle-MSFT-Monitor-Device-Event/20211216-205227
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: i386-randconfig-m021-20211216 (https://download.01.org/0day-ci/archive/20211217/[email protected]/config )
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
smatch warnings:
net/bluetooth/msft.c:757 msft_vendor_evt() warn: inconsistent returns '&hdev->lock'.
vim +757 net/bluetooth/msft.c
3e54c5890c87a30 Luiz Augusto von Dentz 2021-12-01 714 void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
145373cb1b1fcdb Miao-chen Chou 2020-04-03 715 {
145373cb1b1fcdb Miao-chen Chou 2020-04-03 716 struct msft_data *msft = hdev->msft_data;
e5af6a85decc8c1 Manish Mandlik 2021-12-16 717 u8 *evt_prefix;
e5af6a85decc8c1 Manish Mandlik 2021-12-16 718 u8 *evt;
145373cb1b1fcdb Miao-chen Chou 2020-04-03 719
145373cb1b1fcdb Miao-chen Chou 2020-04-03 720 if (!msft)
145373cb1b1fcdb Miao-chen Chou 2020-04-03 721 return;
145373cb1b1fcdb Miao-chen Chou 2020-04-03 722
145373cb1b1fcdb Miao-chen Chou 2020-04-03 723 /* When the extension has defined an event prefix, check that it
145373cb1b1fcdb Miao-chen Chou 2020-04-03 724 * matches, and otherwise just return.
145373cb1b1fcdb Miao-chen Chou 2020-04-03 725 */
145373cb1b1fcdb Miao-chen Chou 2020-04-03 726 if (msft->evt_prefix_len > 0) {
e5af6a85decc8c1 Manish Mandlik 2021-12-16 727 evt_prefix = msft_skb_pull(hdev, skb, 0, msft->evt_prefix_len);
e5af6a85decc8c1 Manish Mandlik 2021-12-16 728 if (!evt_prefix)
145373cb1b1fcdb Miao-chen Chou 2020-04-03 729 return;
145373cb1b1fcdb Miao-chen Chou 2020-04-03 730
e5af6a85decc8c1 Manish Mandlik 2021-12-16 731 if (memcmp(evt_prefix, msft->evt_prefix, msft->evt_prefix_len))
145373cb1b1fcdb Miao-chen Chou 2020-04-03 732 return;
145373cb1b1fcdb Miao-chen Chou 2020-04-03 733 }
145373cb1b1fcdb Miao-chen Chou 2020-04-03 734
145373cb1b1fcdb Miao-chen Chou 2020-04-03 735 /* Every event starts at least with an event code and the rest of
145373cb1b1fcdb Miao-chen Chou 2020-04-03 736 * the data is variable and depends on the event code.
145373cb1b1fcdb Miao-chen Chou 2020-04-03 737 */
145373cb1b1fcdb Miao-chen Chou 2020-04-03 738 if (skb->len < 1)
145373cb1b1fcdb Miao-chen Chou 2020-04-03 739 return;
145373cb1b1fcdb Miao-chen Chou 2020-04-03 740
e5af6a85decc8c1 Manish Mandlik 2021-12-16 741 hci_dev_lock(hdev);
145373cb1b1fcdb Miao-chen Chou 2020-04-03 742
e5af6a85decc8c1 Manish Mandlik 2021-12-16 743 evt = msft_skb_pull(hdev, skb, 0, sizeof(*evt));
e5af6a85decc8c1 Manish Mandlik 2021-12-16 744 if (!evt)
e5af6a85decc8c1 Manish Mandlik 2021-12-16 745 return;
Missing hci_dev_unlock(hdev);
e5af6a85decc8c1 Manish Mandlik 2021-12-16 746
e5af6a85decc8c1 Manish Mandlik 2021-12-16 747 switch (*evt) {
e5af6a85decc8c1 Manish Mandlik 2021-12-16 748 case MSFT_EV_LE_MONITOR_DEVICE:
e5af6a85decc8c1 Manish Mandlik 2021-12-16 749 msft_monitor_device_evt(hdev, skb);
e5af6a85decc8c1 Manish Mandlik 2021-12-16 750 break;
e5af6a85decc8c1 Manish Mandlik 2021-12-16 751
e5af6a85decc8c1 Manish Mandlik 2021-12-16 752 default:
e5af6a85decc8c1 Manish Mandlik 2021-12-16 753 bt_dev_dbg(hdev, "MSFT vendor event 0x%02x", *evt);
e5af6a85decc8c1 Manish Mandlik 2021-12-16 754 break;
e5af6a85decc8c1 Manish Mandlik 2021-12-16 755 }
e5af6a85decc8c1 Manish Mandlik 2021-12-16 756
e5af6a85decc8c1 Manish Mandlik 2021-12-16 @757 hci_dev_unlock(hdev);
145373cb1b1fcdb Miao-chen Chou 2020-04-03 758 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
_______________________________________________
kbuild mailing list -- [email protected]
To unsubscribe send an email to [email protected]
Hi Manish,
On Thu, Dec 16, 2021 at 11:18 PM Dan Carpenter <[email protected]> wrote:
>
> Hi Manish,
>
> url: https://github.com/0day-ci/linux/commits/Manish-Mandlik/bluetooth-msft-Handle-MSFT-Monitor-Device-Event/20211216-205227
> base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
> config: i386-randconfig-m021-20211216 (https://download.01.org/0day-ci/archive/20211217/[email protected]/config )
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
>
> smatch warnings:
> net/bluetooth/msft.c:757 msft_vendor_evt() warn: inconsistent returns '&hdev->lock'.
>
> vim +757 net/bluetooth/msft.c
>
> 3e54c5890c87a30 Luiz Augusto von Dentz 2021-12-01 714 void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 715 {
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 716 struct msft_data *msft = hdev->msft_data;
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 717 u8 *evt_prefix;
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 718 u8 *evt;
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 719
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 720 if (!msft)
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 721 return;
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 722
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 723 /* When the extension has defined an event prefix, check that it
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 724 * matches, and otherwise just return.
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 725 */
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 726 if (msft->evt_prefix_len > 0) {
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 727 evt_prefix = msft_skb_pull(hdev, skb, 0, msft->evt_prefix_len);
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 728 if (!evt_prefix)
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 729 return;
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 730
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 731 if (memcmp(evt_prefix, msft->evt_prefix, msft->evt_prefix_len))
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 732 return;
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 733 }
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 734
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 735 /* Every event starts at least with an event code and the rest of
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 736 * the data is variable and depends on the event code.
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 737 */
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 738 if (skb->len < 1)
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 739 return;
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 740
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 741 hci_dev_lock(hdev);
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 742
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 743 evt = msft_skb_pull(hdev, skb, 0, sizeof(*evt));
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 744 if (!evt)
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 745 return;
>
> Missing hci_dev_unlock(hdev);
>
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 746
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 747 switch (*evt) {
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 748 case MSFT_EV_LE_MONITOR_DEVICE:
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 749 msft_monitor_device_evt(hdev, skb);
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 750 break;
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 751
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 752 default:
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 753 bt_dev_dbg(hdev, "MSFT vendor event 0x%02x", *evt);
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 754 break;
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 755 }
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 756
> e5af6a85decc8c1 Manish Mandlik 2021-12-16 @757 hci_dev_unlock(hdev);
> 145373cb1b1fcdb Miao-chen Chou 2020-04-03 758 }
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/[email protected]
> _______________________________________________
> kbuild mailing list -- [email protected]
> To unsubscribe send an email to [email protected]
Are you working on fixing the above problems?
--
Luiz Augusto von Dentz