diff mbox series

[v9,1/3] bluetooth: msft: Handle MSFT Monitor Device Event

Message ID 20211216044839.v9.1.Ic0a40b84dee3825302890aaea690e73165c71820@changeid
State Superseded
Headers show
Series [v9,1/3] bluetooth: msft: Handle MSFT Monitor Device Event | expand

Commit Message

Manish Mandlik Dec. 16, 2021, 12:49 p.m. UTC 
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 <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(-)

Comments

Dan Carpenter Dec. 17, 2021, 7:17 a.m. UTC | #1 
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
Luiz Augusto von Dentz Dec. 21, 2021, 9:56 p.m. UTC | #2 
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?
diff mbox series

Patch

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)