* [PATCH v9 2/3] bluetooth: mgmt: Add MGMT Adv Monitor Device Found/Lost events
2021-12-16 12:49 [PATCH v9 1/3] bluetooth: msft: Handle MSFT Monitor Device Event Manish Mandlik
@ 2021-12-16 12:49 ` Manish Mandlik
2021-12-16 12:49 ` [PATCH v9 3/3] bluetooth: mgmt: Fix sizeof in mgmt_device_found() Manish Mandlik
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Manish Mandlik @ 2021-12-16 12:49 UTC (permalink / raw)
To: marcel, luiz.dentz
Cc: chromeos-bluetooth-upstreaming, linux-bluetooth, Manish Mandlik,
Miao-chen Chou, David S. Miller, Jakub Kicinski, Johan Hedberg,
linux-kernel, netdev
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 <mmandlik@google.com>
Reviewed-by: Miao-chen Chou <mcchou@google.com>
---
(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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v9 3/3] bluetooth: mgmt: Fix sizeof in mgmt_device_found()
2021-12-16 12:49 [PATCH v9 1/3] bluetooth: msft: Handle MSFT Monitor Device Event Manish Mandlik
2021-12-16 12:49 ` [PATCH v9 2/3] bluetooth: mgmt: Add MGMT Adv Monitor Device Found/Lost events Manish Mandlik
@ 2021-12-16 12:49 ` Manish Mandlik
2021-12-16 19:49 ` Luiz Augusto von Dentz
2021-12-16 19:35 ` [PATCH v9 1/3] bluetooth: msft: Handle MSFT Monitor Device Event Luiz Augusto von Dentz
2021-12-17 7:17 ` [kbuild] " Dan Carpenter
3 siblings, 1 reply; 7+ messages in thread
From: Manish Mandlik @ 2021-12-16 12:49 UTC (permalink / raw)
To: marcel, luiz.dentz
Cc: chromeos-bluetooth-upstreaming, linux-bluetooth, Manish Mandlik,
David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel,
netdev
Use correct sizeof() parameter while allocating skb.
Signed-off-by: Manish Mandlik <mmandlik@google.com>
---
(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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v9 3/3] bluetooth: mgmt: Fix sizeof in mgmt_device_found()
2021-12-16 12:49 ` [PATCH v9 3/3] bluetooth: mgmt: Fix sizeof in mgmt_device_found() Manish Mandlik
@ 2021-12-16 19:49 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 7+ messages in thread
From: Luiz Augusto von Dentz @ 2021-12-16 19:49 UTC (permalink / raw)
To: Manish Mandlik
Cc: Marcel Holtmann, ChromeOS Bluetooth Upstreaming, linux-bluetooth,
David S. Miller, Jakub Kicinski, Johan Hedberg,
Linux Kernel Mailing List, open list:NETWORKING [GENERAL]
Hi Manish,
On Thu, Dec 16, 2021 at 4:50 AM Manish Mandlik <mmandlik@google.com> wrote:
>
> Use correct sizeof() parameter while allocating skb.
>
> Signed-off-by: Manish Mandlik <mmandlik@google.com>
> ---
>
> (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/20211213212650.2067066-1-luiz.dentz@gmail.com/
Please use that instead and if that works for you reply adding Tested-by.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v9 1/3] bluetooth: msft: Handle MSFT Monitor Device Event
2021-12-16 12:49 [PATCH v9 1/3] bluetooth: msft: Handle MSFT Monitor Device Event Manish Mandlik
2021-12-16 12:49 ` [PATCH v9 2/3] bluetooth: mgmt: Add MGMT Adv Monitor Device Found/Lost events Manish Mandlik
2021-12-16 12:49 ` [PATCH v9 3/3] bluetooth: mgmt: Fix sizeof in mgmt_device_found() Manish Mandlik
@ 2021-12-16 19:35 ` Luiz Augusto von Dentz
2021-12-17 7:17 ` [kbuild] " Dan Carpenter
3 siblings, 0 replies; 7+ messages in thread
From: Luiz Augusto von Dentz @ 2021-12-16 19:35 UTC (permalink / raw)
To: Manish Mandlik
Cc: Marcel Holtmann, ChromeOS Bluetooth Upstreaming, linux-bluetooth,
Miao-chen Chou, David S. Miller, Jakub Kicinski, Johan Hedberg,
Linux Kernel Mailing List, open list:NETWORKING [GENERAL]
Hi Manish,
On Thu, Dec 16, 2021 at 4:50 AM Manish Mandlik <mmandlik@google.com> 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 <mmandlik@google.com>
> Reviewed-by: Miao-chen Chou <mcchou@google.com>
> ---
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [kbuild] Re: [PATCH v9 1/3] bluetooth: msft: Handle MSFT Monitor Device Event
2021-12-16 12:49 [PATCH v9 1/3] bluetooth: msft: Handle MSFT Monitor Device Event Manish Mandlik
` (2 preceding siblings ...)
2021-12-16 19:35 ` [PATCH v9 1/3] bluetooth: msft: Handle MSFT Monitor Device Event Luiz Augusto von Dentz
@ 2021-12-17 7:17 ` Dan Carpenter
2021-12-21 21:56 ` Luiz Augusto von Dentz
3 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2021-12-17 7:17 UTC (permalink / raw)
To: kbuild, Manish Mandlik, marcel, luiz.dentz
Cc: lkp, kbuild-all, chromeos-bluetooth-upstreaming, linux-bluetooth,
Manish Mandlik, Miao-chen Chou, Jakub Kicinski, Johan Hedberg,
linux-kernel, netdev
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/202112171439.KaggScQN-lkp@intel.com/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 <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
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/kbuild-all@lists.01.org
_______________________________________________
kbuild mailing list -- kbuild@lists.01.org
To unsubscribe send an email to kbuild-leave@lists.01.org
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [kbuild] Re: [PATCH v9 1/3] bluetooth: msft: Handle MSFT Monitor Device Event
2021-12-17 7:17 ` [kbuild] " Dan Carpenter
@ 2021-12-21 21:56 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 7+ messages in thread
From: Luiz Augusto von Dentz @ 2021-12-21 21:56 UTC (permalink / raw)
To: Dan Carpenter
Cc: kbuild, Manish Mandlik, Marcel Holtmann, kbuild test robot,
kbuild-all, ChromeOS Bluetooth Upstreaming, linux-bluetooth,
Miao-chen Chou, Jakub Kicinski, Johan Hedberg,
Linux Kernel Mailing List, open list:NETWORKING [GENERAL]
Hi Manish,
On Thu, Dec 16, 2021 at 11:18 PM Dan Carpenter <dan.carpenter@oracle.com> 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/202112171439.KaggScQN-lkp@intel.com/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 <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> 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/kbuild-all@lists.01.org
> _______________________________________________
> kbuild mailing list -- kbuild@lists.01.org
> To unsubscribe send an email to kbuild-leave@lists.01.org
Are you working on fixing the above problems?
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 7+ messages in thread