[PATCH v2 1/2] Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag

[PATCH v2 1/2] Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This introduces HCI_CONN_FLAG_DEVICE_PRIVACY which can be used by
userspace to indicate to the controller to use Device Privacy Mode to a
specific device.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
v2: Fix marking Device Privacy Flag even when adapter is not capable of
handling Set Privacy Mode.

 include/net/bluetooth/hci_core.h |  4 ++++
 net/bluetooth/mgmt.c             | 24 ++++++++++++++++++++----
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index b5f061882c10..07d2d099dc2a 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -160,6 +160,7 @@ struct bdaddr_list_with_flags {
 
 enum hci_conn_flags {
 	HCI_CONN_FLAG_REMOTE_WAKEUP,
+	HCI_CONN_FLAG_DEVICE_PRIVACY,
 	HCI_CONN_FLAG_MAX
 };
 
@@ -1468,6 +1469,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
 #define use_ll_privacy(dev) (ll_privacy_capable(dev) && \
 			     hci_dev_test_flag(dev, HCI_ENABLE_LL_PRIVACY))
 
+#define privacy_mode_capable(dev) (use_ll_privacy(dev) && \
+				   (hdev->commands[39] & 0x04))
+
 /* Use enhanced synchronous connection if command is supported */
 #define enhanced_sco_capable(dev) ((dev)->commands[29] & 0x08)
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 06384d761928..8a8376d32be3 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4350,7 +4350,16 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
 			       MGMT_STATUS_NOT_SUPPORTED);
 }
 
-#define SUPPORTED_DEVICE_FLAGS() ((1U << HCI_CONN_FLAG_MAX) - 1)
+static u32 supported_device_flags(struct hci_dev *hdev)
+{
+	u32 flags = BIT(HCI_CONN_FLAG_MAX) - 1;
+
+	/* Check if Privacy Mode can be set */
+	if (!privacy_mode_capable(hdev))
+		flags &= ~BIT(HCI_CONN_FLAG_DEVICE_PRIVACY);
+
+	return flags;
+}
 
 static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 			    u16 data_len)
@@ -4359,7 +4368,7 @@ static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 	struct mgmt_rp_get_device_flags rp;
 	struct bdaddr_list_with_flags *br_params;
 	struct hci_conn_params *params;
-	u32 supported_flags = SUPPORTED_DEVICE_FLAGS();
+	u32 supported_flags = supported_device_flags(hdev);
 	u32 current_flags = 0;
 	u8 status = MGMT_STATUS_INVALID_PARAMS;
 
@@ -4423,7 +4432,7 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 	struct bdaddr_list_with_flags *br_params;
 	struct hci_conn_params *params;
 	u8 status = MGMT_STATUS_INVALID_PARAMS;
-	u32 supported_flags = SUPPORTED_DEVICE_FLAGS();
+	u32 supported_flags = supported_device_flags(hdev);
 	u32 current_flags = __le32_to_cpu(cp->current_flags);
 
 	bt_dev_dbg(hdev, "Set device flags %pMR (type 0x%x) = 0x%x",
@@ -4456,6 +4465,13 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 		if (params) {
 			params->current_flags = current_flags;
 			status = MGMT_STATUS_SUCCESS;
+
+			/* Update passive scan if HCI_CONN_FLAG_DEVICE_PRIVACY
+			 * has been set.
+			 */
+			if (hci_conn_test_flag(HCI_CONN_FLAG_DEVICE_PRIVACY,
+					       params->current_flags))
+				hci_update_passive_scan(hdev);
 		} else {
 			bt_dev_warn(hdev, "No such LE device %pMR (0x%x)",
 				    &cp->addr.bdaddr,
@@ -7061,7 +7077,7 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
 added:
 	device_added(sk, hdev, &cp->addr.bdaddr, cp->addr.type, cp->action);
 	device_flags_changed(NULL, hdev, &cp->addr.bdaddr, cp->addr.type,
-			     SUPPORTED_DEVICE_FLAGS(), current_flags);
+			     supported_device_flags(hdev), current_flags);
 
 	err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
 				MGMT_STATUS_SUCCESS, &cp->addr,
-- 
2.31.1

[PATCH v2 2/2] Bluetooth: hci_sync: Set Privacy Mode when updating the resolving list

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This adds support for Set Privacy Mode when updating the resolving list
when HCI_CONN_FLAG_DEVICE_PRIVACY so the controller shall use Device
Mode for devices programmed in the resolving list, Device Mode is
actually required when the remote device are not able to use RPA as
otherwise the default mode is Network Privacy Mode in which only
allows RPAs thus the controller would filter out advertisement using
identity addresses for which there is an IRK.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 include/net/bluetooth/hci.h      | 10 ++++++
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_event.c        | 29 +++++++++++++++++
 net/bluetooth/hci_sync.c         | 53 ++++++++++++++++++++++++++++----
 4 files changed, 87 insertions(+), 6 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 63065bc01b76..979da5179ff4 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1930,6 +1930,16 @@ struct hci_rp_le_read_transmit_power {
 	__s8  max_le_tx_power;
 } __packed;
 
+#define HCI_NETWORK_PRIVACY		0x00
+#define HCI_DEVICE_PRIVACY		0x01
+
+#define HCI_OP_LE_SET_PRIVACY_MODE	0x204e
+struct hci_cp_le_set_privacy_mode {
+	__u8  bdaddr_type;
+	bdaddr_t  bdaddr;
+	__u8  mode;
+} __packed;
+
 #define HCI_OP_LE_READ_BUFFER_SIZE_V2	0x2060
 struct hci_rp_le_read_buffer_size_v2 {
 	__u8    status;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 07d2d099dc2a..cb5684da3ed4 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -758,6 +758,7 @@ struct hci_conn_params {
 
 	struct hci_conn *conn;
 	bool explicit_connect;
+	uint8_t privacy_mode;
 	u32 current_flags;
 };
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index d4b75a6cfeee..9cadc543abcb 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1300,6 +1300,31 @@ static void hci_cc_le_read_transmit_power(struct hci_dev *hdev,
 	hdev->max_le_tx_power = rp->max_le_tx_power;
 }
 
+static void hci_cc_le_set_privacy_mode(struct hci_dev *hdev,
+				       struct sk_buff *skb)
+{
+	__u8 status = *((__u8 *)skb->data);
+	struct hci_cp_le_set_privacy_mode *cp;
+	struct hci_conn_params *params;
+
+	bt_dev_dbg(hdev, "status 0x%2.2x", status);
+
+	if (status)
+		return;
+
+	cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_PRIVACY_MODE);
+	if (!cp)
+		return;
+
+	hci_dev_lock(hdev);
+
+	params = hci_conn_params_lookup(hdev, &cp->bdaddr, cp->bdaddr_type);
+	if (params)
+		params->privacy_mode = cp->mode;
+
+	hci_dev_unlock(hdev);
+}
+
 static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	__u8 *sent, status = *((__u8 *) skb->data);
@@ -3812,6 +3837,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb,
 		hci_cc_le_read_transmit_power(hdev, skb);
 		break;
 
+	case HCI_OP_LE_SET_PRIVACY_MODE:
+		hci_cc_le_set_privacy_mode(hdev, skb);
+		break;
+
 	default:
 		BT_DBG("%s opcode 0x%4.4x", hdev->name, *opcode);
 		break;
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index b794605dc882..32ed7da3b6dd 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -1580,8 +1580,42 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev,
 				     sizeof(cp), &cp, HCI_CMD_TIMEOUT);
 }
 
+/* Set Device Privacy Mode. */
+static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev,
+					struct hci_conn_params *params)
+{
+	struct hci_cp_le_set_privacy_mode cp;
+	struct smp_irk *irk;
+
+	/* If device privacy mode has already been set there is nothing to do */
+	if (params->privacy_mode == HCI_DEVICE_PRIVACY)
+		return 0;
+
+	/* Set Privacy Mode requires the use of resolving list (aka. LL Privacy)
+	 * by default Network Mode is used so only really send the command if
+	 * Device Mode is required (HCI_CONN_FLAG_DEVICE_PRIVACY).
+	 */
+	if (!privacy_mode_capable(hdev) ||
+	    !hci_conn_test_flag(HCI_CONN_FLAG_DEVICE_PRIVACY,
+				params->current_flags))
+		return 0;
+
+	irk = hci_find_irk_by_addr(hdev, &params->addr, params->addr_type);
+	if (!irk)
+		return 0;
+
+	memset(&cp, 0, sizeof(cp));
+	cp.bdaddr_type = irk->addr_type;
+	bacpy(&cp.bdaddr, &irk->bdaddr);
+	cp.mode = HCI_DEVICE_PRIVACY;
+
+	return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_PRIVACY_MODE,
+				     sizeof(cp), &cp, HCI_CMD_TIMEOUT);
+}
+
 /* Adds connection to allow list if needed, if the device uses RPA (has IRK)
- * this attempts to program the device in the resolving list as well.
+ * this attempts to program the device in the resolving list as well and
+ * properly set the privacy mode.
  */
 static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
 				       struct hci_conn_params *params,
@@ -1590,11 +1624,6 @@ static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
 	struct hci_cp_le_add_to_accept_list cp;
 	int err;
 
-	/* Already in accept list */
-	if (hci_bdaddr_list_lookup(&hdev->le_accept_list, &params->addr,
-				   params->addr_type))
-		return 0;
-
 	/* Select filter policy to accept all advertising */
 	if (*num_entries >= hdev->le_accept_list_size)
 		return -ENOSPC;
@@ -1620,6 +1649,18 @@ static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
 		return err;
 	}
 
+	/* Set Privacy Mode */
+	err = hci_le_set_privacy_mode_sync(hdev, params);
+	if (err) {
+		bt_dev_err(hdev, "Unable to set privacy mode: %d", err);
+		return err;
+	}
+
+	/* Check if already in accept list */
+	if (hci_bdaddr_list_lookup(&hdev->le_accept_list, &params->addr,
+				   params->addr_type))
+		return 0;
+
 	*num_entries += 1;
 	cp.bdaddr_type = params->addr_type;
 	bacpy(&cp.bdaddr, &params->addr);
-- 
2.31.1

Re: [PATCH v2 2/2] Bluetooth: hci_sync: Set Privacy Mode when updating the resolving list

[Marcel Holtmann]

Hi Luiz,

> This adds support for Set Privacy Mode when updating the resolving list
> when HCI_CONN_FLAG_DEVICE_PRIVACY so the controller shall use Device
> Mode for devices programmed in the resolving list, Device Mode is
> actually required when the remote device are not able to use RPA as
> otherwise the default mode is Network Privacy Mode in which only
> allows RPAs thus the controller would filter out advertisement using
> identity addresses for which there is an IRK.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> include/net/bluetooth/hci.h      | 10 ++++++
> include/net/bluetooth/hci_core.h |  1 +
> net/bluetooth/hci_event.c        | 29 +++++++++++++++++
> net/bluetooth/hci_sync.c         | 53 ++++++++++++++++++++++++++++----
> 4 files changed, 87 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 63065bc01b76..979da5179ff4 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1930,6 +1930,16 @@ struct hci_rp_le_read_transmit_power {
> 	__s8  max_le_tx_power;
> } __packed;
> 
> +#define HCI_NETWORK_PRIVACY		0x00
> +#define HCI_DEVICE_PRIVACY		0x01
> +
> +#define HCI_OP_LE_SET_PRIVACY_MODE	0x204e
> +struct hci_cp_le_set_privacy_mode {
> +	__u8  bdaddr_type;
> +	bdaddr_t  bdaddr;
> +	__u8  mode;
> +} __packed;
> +
> #define HCI_OP_LE_READ_BUFFER_SIZE_V2	0x2060
> struct hci_rp_le_read_buffer_size_v2 {
> 	__u8    status;
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 07d2d099dc2a..cb5684da3ed4 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -758,6 +758,7 @@ struct hci_conn_params {
> 
> 	struct hci_conn *conn;
> 	bool explicit_connect;
> +	uint8_t privacy_mode;

actually u8 please.

> 	u32 current_flags;
> };
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index d4b75a6cfeee..9cadc543abcb 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1300,6 +1300,31 @@ static void hci_cc_le_read_transmit_power(struct hci_dev *hdev,
> 	hdev->max_le_tx_power = rp->max_le_tx_power;
> }
> 
> +static void hci_cc_le_set_privacy_mode(struct hci_dev *hdev,
> +				       struct sk_buff *skb)
> +{
> +	__u8 status = *((__u8 *)skb->data);
> +	struct hci_cp_le_set_privacy_mode *cp;
> +	struct hci_conn_params *params;
> +
> +	bt_dev_dbg(hdev, "status 0x%2.2x", status);
> +
> +	if (status)
> +		return;
> +
> +	cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_PRIVACY_MODE);
> +	if (!cp)
> +		return;
> +
> +	hci_dev_lock(hdev);
> +
> +	params = hci_conn_params_lookup(hdev, &cp->bdaddr, cp->bdaddr_type);
> +	if (params)
> +		params->privacy_mode = cp->mode;
> +
> +	hci_dev_unlock(hdev);
> +}
> +
> static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
> {
> 	__u8 *sent, status = *((__u8 *) skb->data);
> @@ -3812,6 +3837,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb,
> 		hci_cc_le_read_transmit_power(hdev, skb);
> 		break;
> 
> +	case HCI_OP_LE_SET_PRIVACY_MODE:
> +		hci_cc_le_set_privacy_mode(hdev, skb);
> +		break;
> +
> 	default:
> 		BT_DBG("%s opcode 0x%4.4x", hdev->name, *opcode);
> 		break;
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index b794605dc882..32ed7da3b6dd 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -1580,8 +1580,42 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev,
> 				     sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> }
> 
> +/* Set Device Privacy Mode. */
> +static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev,
> +					struct hci_conn_params *params)
> +{
> +	struct hci_cp_le_set_privacy_mode cp;
> +	struct smp_irk *irk;
> +
> +	/* If device privacy mode has already been set there is nothing to do */
> +	if (params->privacy_mode == HCI_DEVICE_PRIVACY)
> +		return 0;
> +
> +	/* Set Privacy Mode requires the use of resolving list (aka. LL Privacy)
> +	 * by default Network Mode is used so only really send the command if
> +	 * Device Mode is required (HCI_CONN_FLAG_DEVICE_PRIVACY).
> +	 */
> +	if (!privacy_mode_capable(hdev) ||
> +	    !hci_conn_test_flag(HCI_CONN_FLAG_DEVICE_PRIVACY,
> +				params->current_flags))
> +		return 0;
> +
> +	irk = hci_find_irk_by_addr(hdev, &params->addr, params->addr_type);
> +	if (!irk)
> +		return 0;
> +
> +	memset(&cp, 0, sizeof(cp));
> +	cp.bdaddr_type = irk->addr_type;
> +	bacpy(&cp.bdaddr, &irk->bdaddr);
> +	cp.mode = HCI_DEVICE_PRIVACY;
> +
> +	return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_PRIVACY_MODE,
> +				     sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> +}
> +
> /* Adds connection to allow list if needed, if the device uses RPA (has IRK)
> - * this attempts to program the device in the resolving list as well.
> + * this attempts to program the device in the resolving list as well and
> + * properly set the privacy mode.
>  */
> static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
> 				       struct hci_conn_params *params,
> @@ -1590,11 +1624,6 @@ static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
> 	struct hci_cp_le_add_to_accept_list cp;
> 	int err;
> 
> -	/* Already in accept list */
> -	if (hci_bdaddr_list_lookup(&hdev->le_accept_list, &params->addr,
> -				   params->addr_type))
> -		return 0;
> -
> 	/* Select filter policy to accept all advertising */
> 	if (*num_entries >= hdev->le_accept_list_size)
> 		return -ENOSPC;
> @@ -1620,6 +1649,18 @@ static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
> 		return err;
> 	}
> 
> +	/* Set Privacy Mode */
> +	err = hci_le_set_privacy_mode_sync(hdev, params);
> +	if (err) {
> +		bt_dev_err(hdev, "Unable to set privacy mode: %d", err);
> +		return err;
> +	}
> +
> +	/* Check if already in accept list */
> +	if (hci_bdaddr_list_lookup(&hdev->le_accept_list, &params->addr,
> +				   params->addr_type))
> +		return 0;
> +
> 	*num_entries += 1;
> 	cp.bdaddr_type = params->addr_type;
> 	bacpy(&cp.bdaddr, &params->addr);

Regards

Marcel

Re: [PATCH v2 2/2] Bluetooth: hci_sync: Set Privacy Mode when updating the resolving list

[Luiz Augusto von Dentz]

Hi Marcel,

On Wed, Nov 17, 2021 at 8:46 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > This adds support for Set Privacy Mode when updating the resolving list
> > when HCI_CONN_FLAG_DEVICE_PRIVACY so the controller shall use Device
> > Mode for devices programmed in the resolving list, Device Mode is
> > actually required when the remote device are not able to use RPA as
> > otherwise the default mode is Network Privacy Mode in which only
> > allows RPAs thus the controller would filter out advertisement using
> > identity addresses for which there is an IRK.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > include/net/bluetooth/hci.h      | 10 ++++++
> > include/net/bluetooth/hci_core.h |  1 +
> > net/bluetooth/hci_event.c        | 29 +++++++++++++++++
> > net/bluetooth/hci_sync.c         | 53 ++++++++++++++++++++++++++++----
> > 4 files changed, 87 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 63065bc01b76..979da5179ff4 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -1930,6 +1930,16 @@ struct hci_rp_le_read_transmit_power {
> >       __s8  max_le_tx_power;
> > } __packed;
> >
> > +#define HCI_NETWORK_PRIVACY          0x00
> > +#define HCI_DEVICE_PRIVACY           0x01
> > +
> > +#define HCI_OP_LE_SET_PRIVACY_MODE   0x204e
> > +struct hci_cp_le_set_privacy_mode {
> > +     __u8  bdaddr_type;
> > +     bdaddr_t  bdaddr;
> > +     __u8  mode;
> > +} __packed;
> > +
> > #define HCI_OP_LE_READ_BUFFER_SIZE_V2 0x2060
> > struct hci_rp_le_read_buffer_size_v2 {
> >       __u8    status;
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 07d2d099dc2a..cb5684da3ed4 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -758,6 +758,7 @@ struct hci_conn_params {
> >
> >       struct hci_conn *conn;
> >       bool explicit_connect;
> > +     uint8_t privacy_mode;
>
> actually u8 please.

Ah nice catch, I really wish that was a way to catch this sort of
mistake with CI but checkpatch doesn't seem to have a rule for it.

> >       u32 current_flags;
> > };
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index d4b75a6cfeee..9cadc543abcb 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -1300,6 +1300,31 @@ static void hci_cc_le_read_transmit_power(struct hci_dev *hdev,
> >       hdev->max_le_tx_power = rp->max_le_tx_power;
> > }
> >
> > +static void hci_cc_le_set_privacy_mode(struct hci_dev *hdev,
> > +                                    struct sk_buff *skb)
> > +{
> > +     __u8 status = *((__u8 *)skb->data);
> > +     struct hci_cp_le_set_privacy_mode *cp;
> > +     struct hci_conn_params *params;
> > +
> > +     bt_dev_dbg(hdev, "status 0x%2.2x", status);
> > +
> > +     if (status)
> > +             return;
> > +
> > +     cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_PRIVACY_MODE);
> > +     if (!cp)
> > +             return;
> > +
> > +     hci_dev_lock(hdev);
> > +
> > +     params = hci_conn_params_lookup(hdev, &cp->bdaddr, cp->bdaddr_type);
> > +     if (params)
> > +             params->privacy_mode = cp->mode;
> > +
> > +     hci_dev_unlock(hdev);
> > +}
> > +
> > static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
> > {
> >       __u8 *sent, status = *((__u8 *) skb->data);
> > @@ -3812,6 +3837,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb,
> >               hci_cc_le_read_transmit_power(hdev, skb);
> >               break;
> >
> > +     case HCI_OP_LE_SET_PRIVACY_MODE:
> > +             hci_cc_le_set_privacy_mode(hdev, skb);
> > +             break;
> > +
> >       default:
> >               BT_DBG("%s opcode 0x%4.4x", hdev->name, *opcode);
> >               break;
> > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> > index b794605dc882..32ed7da3b6dd 100644
> > --- a/net/bluetooth/hci_sync.c
> > +++ b/net/bluetooth/hci_sync.c
> > @@ -1580,8 +1580,42 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev,
> >                                    sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> > }
> >
> > +/* Set Device Privacy Mode. */
> > +static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev,
> > +                                     struct hci_conn_params *params)
> > +{
> > +     struct hci_cp_le_set_privacy_mode cp;
> > +     struct smp_irk *irk;
> > +
> > +     /* If device privacy mode has already been set there is nothing to do */
> > +     if (params->privacy_mode == HCI_DEVICE_PRIVACY)
> > +             return 0;
> > +
> > +     /* Set Privacy Mode requires the use of resolving list (aka. LL Privacy)
> > +      * by default Network Mode is used so only really send the command if
> > +      * Device Mode is required (HCI_CONN_FLAG_DEVICE_PRIVACY).
> > +      */
> > +     if (!privacy_mode_capable(hdev) ||
> > +         !hci_conn_test_flag(HCI_CONN_FLAG_DEVICE_PRIVACY,
> > +                             params->current_flags))
> > +             return 0;
> > +
> > +     irk = hci_find_irk_by_addr(hdev, &params->addr, params->addr_type);
> > +     if (!irk)
> > +             return 0;
> > +
> > +     memset(&cp, 0, sizeof(cp));
> > +     cp.bdaddr_type = irk->addr_type;
> > +     bacpy(&cp.bdaddr, &irk->bdaddr);
> > +     cp.mode = HCI_DEVICE_PRIVACY;
> > +
> > +     return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_PRIVACY_MODE,
> > +                                  sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> > +}
> > +
> > /* Adds connection to allow list if needed, if the device uses RPA (has IRK)
> > - * this attempts to program the device in the resolving list as well.
> > + * this attempts to program the device in the resolving list as well and
> > + * properly set the privacy mode.
> >  */
> > static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
> >                                      struct hci_conn_params *params,
> > @@ -1590,11 +1624,6 @@ static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
> >       struct hci_cp_le_add_to_accept_list cp;
> >       int err;
> >
> > -     /* Already in accept list */
> > -     if (hci_bdaddr_list_lookup(&hdev->le_accept_list, &params->addr,
> > -                                params->addr_type))
> > -             return 0;
> > -
> >       /* Select filter policy to accept all advertising */
> >       if (*num_entries >= hdev->le_accept_list_size)
> >               return -ENOSPC;
> > @@ -1620,6 +1649,18 @@ static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
> >               return err;
> >       }
> >
> > +     /* Set Privacy Mode */
> > +     err = hci_le_set_privacy_mode_sync(hdev, params);
> > +     if (err) {
> > +             bt_dev_err(hdev, "Unable to set privacy mode: %d", err);
> > +             return err;
> > +     }
> > +
> > +     /* Check if already in accept list */
> > +     if (hci_bdaddr_list_lookup(&hdev->le_accept_list, &params->addr,
> > +                                params->addr_type))
> > +             return 0;
> > +
> >       *num_entries += 1;
> >       cp.bdaddr_type = params->addr_type;
> >       bacpy(&cp.bdaddr, &params->addr);
>
> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v2 1/2] Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag

[Marcel Holtmann]

Hi Luiz,

> This introduces HCI_CONN_FLAG_DEVICE_PRIVACY which can be used by
> userspace to indicate to the controller to use Device Privacy Mode to a
> specific device.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> v2: Fix marking Device Privacy Flag even when adapter is not capable of
> handling Set Privacy Mode.
> 
> include/net/bluetooth/hci_core.h |  4 ++++
> net/bluetooth/mgmt.c             | 24 ++++++++++++++++++++----
> 2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index b5f061882c10..07d2d099dc2a 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -160,6 +160,7 @@ struct bdaddr_list_with_flags {
> 
> enum hci_conn_flags {
> 	HCI_CONN_FLAG_REMOTE_WAKEUP,
> +	HCI_CONN_FLAG_DEVICE_PRIVACY,
> 	HCI_CONN_FLAG_MAX
> };
> 
> @@ -1468,6 +1469,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> #define use_ll_privacy(dev) (ll_privacy_capable(dev) && \
> 			     hci_dev_test_flag(dev, HCI_ENABLE_LL_PRIVACY))
> 
> +#define privacy_mode_capable(dev) (use_ll_privacy(dev) && \
> +				   (hdev->commands[39] & 0x04))
> +
> /* Use enhanced synchronous connection if command is supported */
> #define enhanced_sco_capable(dev) ((dev)->commands[29] & 0x08)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 06384d761928..8a8376d32be3 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -4350,7 +4350,16 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> 			       MGMT_STATUS_NOT_SUPPORTED);
> }
> 
> -#define SUPPORTED_DEVICE_FLAGS() ((1U << HCI_CONN_FLAG_MAX) - 1)
> +static u32 supported_device_flags(struct hci_dev *hdev)
> +{
> +	u32 flags = BIT(HCI_CONN_FLAG_MAX) - 1;
> +
> +	/* Check if Privacy Mode can be set */
> +	if (!privacy_mode_capable(hdev))
> +		flags &= ~BIT(HCI_CONN_FLAG_DEVICE_PRIVACY);
> +
> +	return flags;
> +}

I am lost on what we are doing, I know that SUPPORTED_DEVICE_FLAGS was introduced by 4c54bf2b093bb from Abhishek, but I fail to reason now why it is correct.

If we really set all bits on the supported device flags, then that is a bug. Or is it too early for me to grok this code?

> 
> static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> 			    u16 data_len)
> @@ -4359,7 +4368,7 @@ static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> 	struct mgmt_rp_get_device_flags rp;
> 	struct bdaddr_list_with_flags *br_params;
> 	struct hci_conn_params *params;
> -	u32 supported_flags = SUPPORTED_DEVICE_FLAGS();
> +	u32 supported_flags = supported_device_flags(hdev);
> 	u32 current_flags = 0;
> 	u8 status = MGMT_STATUS_INVALID_PARAMS;
> 
> @@ -4423,7 +4432,7 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> 	struct bdaddr_list_with_flags *br_params;
> 	struct hci_conn_params *params;
> 	u8 status = MGMT_STATUS_INVALID_PARAMS;
> -	u32 supported_flags = SUPPORTED_DEVICE_FLAGS();
> +	u32 supported_flags = supported_device_flags(hdev);
> 	u32 current_flags = __le32_to_cpu(cp->current_flags);
> 
> 	bt_dev_dbg(hdev, "Set device flags %pMR (type 0x%x) = 0x%x",
> @@ -4456,6 +4465,13 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> 		if (params) {
> 			params->current_flags = current_flags;
> 			status = MGMT_STATUS_SUCCESS;
> +
> +			/* Update passive scan if HCI_CONN_FLAG_DEVICE_PRIVACY
> +			 * has been set.
> +			 */
> +			if (hci_conn_test_flag(HCI_CONN_FLAG_DEVICE_PRIVACY,
> +					       params->current_flags))
> +				hci_update_passive_scan(hdev);
> 		} else {
> 			bt_dev_warn(hdev, "No such LE device %pMR (0x%x)",
> 				    &cp->addr.bdaddr,
> @@ -7061,7 +7077,7 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
> added:
> 	device_added(sk, hdev, &cp->addr.bdaddr, cp->addr.type, cp->action);
> 	device_flags_changed(NULL, hdev, &cp->addr.bdaddr, cp->addr.type,
> -			     SUPPORTED_DEVICE_FLAGS(), current_flags);
> +			     supported_device_flags(hdev), current_flags);

Regards

Marcel

Re: [PATCH v2 1/2] Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag

[Luiz Augusto von Dentz]

Hi Marcel,

On Wed, Nov 17, 2021 at 8:45 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > This introduces HCI_CONN_FLAG_DEVICE_PRIVACY which can be used by
> > userspace to indicate to the controller to use Device Privacy Mode to a
> > specific device.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > v2: Fix marking Device Privacy Flag even when adapter is not capable of
> > handling Set Privacy Mode.
> >
> > include/net/bluetooth/hci_core.h |  4 ++++
> > net/bluetooth/mgmt.c             | 24 ++++++++++++++++++++----
> > 2 files changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index b5f061882c10..07d2d099dc2a 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -160,6 +160,7 @@ struct bdaddr_list_with_flags {
> >
> > enum hci_conn_flags {
> >       HCI_CONN_FLAG_REMOTE_WAKEUP,
> > +     HCI_CONN_FLAG_DEVICE_PRIVACY,
> >       HCI_CONN_FLAG_MAX
> > };
> >
> > @@ -1468,6 +1469,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> > #define use_ll_privacy(dev) (ll_privacy_capable(dev) && \
> >                            hci_dev_test_flag(dev, HCI_ENABLE_LL_PRIVACY))
> >
> > +#define privacy_mode_capable(dev) (use_ll_privacy(dev) && \
> > +                                (hdev->commands[39] & 0x04))
> > +
> > /* Use enhanced synchronous connection if command is supported */
> > #define enhanced_sco_capable(dev) ((dev)->commands[29] & 0x08)
> >
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index 06384d761928..8a8376d32be3 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -4350,7 +4350,16 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> >                              MGMT_STATUS_NOT_SUPPORTED);
> > }
> >
> > -#define SUPPORTED_DEVICE_FLAGS() ((1U << HCI_CONN_FLAG_MAX) - 1)
> > +static u32 supported_device_flags(struct hci_dev *hdev)
> > +{
> > +     u32 flags = BIT(HCI_CONN_FLAG_MAX) - 1;
> > +
> > +     /* Check if Privacy Mode can be set */
> > +     if (!privacy_mode_capable(hdev))
> > +             flags &= ~BIT(HCI_CONN_FLAG_DEVICE_PRIVACY);
> > +
> > +     return flags;
> > +}
>
> I am lost on what we are doing, I know that SUPPORTED_DEVICE_FLAGS was introduced by 4c54bf2b093bb from Abhishek, but I fail to reason now why it is correct.

But we don't set the HCI_CONN_FLAG_DEVICE_PRIVACY is hdev is not
capable of setting it, anyway the general idea of the supported_flags
is to indicate to userspace what flags the kernel is capable of
settings, so yeah I would expected it to be capable of setting every
flag except for those the controller don't have proper support for.
Maybe you got confused by the logic of first enabling everything and
then toggle back the bits that are not supported by the hdev.

> If we really set all bits on the supported device flags, then that is a bug. Or is it too early for me to grok this code?
>
> >
> > static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> >                           u16 data_len)
> > @@ -4359,7 +4368,7 @@ static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> >       struct mgmt_rp_get_device_flags rp;
> >       struct bdaddr_list_with_flags *br_params;
> >       struct hci_conn_params *params;
> > -     u32 supported_flags = SUPPORTED_DEVICE_FLAGS();
> > +     u32 supported_flags = supported_device_flags(hdev);
> >       u32 current_flags = 0;
> >       u8 status = MGMT_STATUS_INVALID_PARAMS;
> >
> > @@ -4423,7 +4432,7 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> >       struct bdaddr_list_with_flags *br_params;
> >       struct hci_conn_params *params;
> >       u8 status = MGMT_STATUS_INVALID_PARAMS;
> > -     u32 supported_flags = SUPPORTED_DEVICE_FLAGS();
> > +     u32 supported_flags = supported_device_flags(hdev);
> >       u32 current_flags = __le32_to_cpu(cp->current_flags);
> >
> >       bt_dev_dbg(hdev, "Set device flags %pMR (type 0x%x) = 0x%x",
> > @@ -4456,6 +4465,13 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> >               if (params) {
> >                       params->current_flags = current_flags;
> >                       status = MGMT_STATUS_SUCCESS;
> > +
> > +                     /* Update passive scan if HCI_CONN_FLAG_DEVICE_PRIVACY
> > +                      * has been set.
> > +                      */
> > +                     if (hci_conn_test_flag(HCI_CONN_FLAG_DEVICE_PRIVACY,
> > +                                            params->current_flags))
> > +                             hci_update_passive_scan(hdev);
> >               } else {
> >                       bt_dev_warn(hdev, "No such LE device %pMR (0x%x)",
> >                                   &cp->addr.bdaddr,
> > @@ -7061,7 +7077,7 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
> > added:
> >       device_added(sk, hdev, &cp->addr.bdaddr, cp->addr.type, cp->action);
> >       device_flags_changed(NULL, hdev, &cp->addr.bdaddr, cp->addr.type,
> > -                          SUPPORTED_DEVICE_FLAGS(), current_flags);
> > +                          supported_device_flags(hdev), current_flags);
>
> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v2 1/2] Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag

[Marcel Holtmann]

Hi Luiz,

>>> This introduces HCI_CONN_FLAG_DEVICE_PRIVACY which can be used by
>>> userspace to indicate to the controller to use Device Privacy Mode to a
>>> specific device.
>>> 
>>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>> ---
>>> v2: Fix marking Device Privacy Flag even when adapter is not capable of
>>> handling Set Privacy Mode.
>>> 
>>> include/net/bluetooth/hci_core.h |  4 ++++
>>> net/bluetooth/mgmt.c             | 24 ++++++++++++++++++++----
>>> 2 files changed, 24 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>> index b5f061882c10..07d2d099dc2a 100644
>>> --- a/include/net/bluetooth/hci_core.h
>>> +++ b/include/net/bluetooth/hci_core.h
>>> @@ -160,6 +160,7 @@ struct bdaddr_list_with_flags {
>>> 
>>> enum hci_conn_flags {
>>>      HCI_CONN_FLAG_REMOTE_WAKEUP,
>>> +     HCI_CONN_FLAG_DEVICE_PRIVACY,
>>>      HCI_CONN_FLAG_MAX
>>> };
>>> 
>>> @@ -1468,6 +1469,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
>>> #define use_ll_privacy(dev) (ll_privacy_capable(dev) && \
>>>                           hci_dev_test_flag(dev, HCI_ENABLE_LL_PRIVACY))
>>> 
>>> +#define privacy_mode_capable(dev) (use_ll_privacy(dev) && \
>>> +                                (hdev->commands[39] & 0x04))
>>> +
>>> /* Use enhanced synchronous connection if command is supported */
>>> #define enhanced_sco_capable(dev) ((dev)->commands[29] & 0x08)
>>> 
>>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>>> index 06384d761928..8a8376d32be3 100644
>>> --- a/net/bluetooth/mgmt.c
>>> +++ b/net/bluetooth/mgmt.c
>>> @@ -4350,7 +4350,16 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
>>>                             MGMT_STATUS_NOT_SUPPORTED);
>>> }
>>> 
>>> -#define SUPPORTED_DEVICE_FLAGS() ((1U << HCI_CONN_FLAG_MAX) - 1)
>>> +static u32 supported_device_flags(struct hci_dev *hdev)
>>> +{
>>> +     u32 flags = BIT(HCI_CONN_FLAG_MAX) - 1;
>>> +
>>> +     /* Check if Privacy Mode can be set */
>>> +     if (!privacy_mode_capable(hdev))
>>> +             flags &= ~BIT(HCI_CONN_FLAG_DEVICE_PRIVACY);
>>> +
>>> +     return flags;
>>> +}
>> 
>> I am lost on what we are doing, I know that SUPPORTED_DEVICE_FLAGS was introduced by 4c54bf2b093bb from Abhishek, but I fail to reason now why it is correct.
> 
> But we don't set the HCI_CONN_FLAG_DEVICE_PRIVACY is hdev is not
> capable of setting it, anyway the general idea of the supported_flags
> is to indicate to userspace what flags the kernel is capable of
> settings, so yeah I would expected it to be capable of setting every
> flag except for those the controller don't have proper support for.
> Maybe you got confused by the logic of first enabling everything and
> then toggle back the bits that are not supported by the hdev.

and I think that is wrong. We should only set the bits that the hardware and/or kernel supports. I have no idea on how I missed this in the review back then. I am trying to figure it out, but I am failing to follow this logic.

Regards

Marcel

Re: [PATCH v2 1/2] Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag

[Luiz Augusto von Dentz]

Hi Marcel,

On Thu, Nov 18, 2021 at 9:19 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> >>> This introduces HCI_CONN_FLAG_DEVICE_PRIVACY which can be used by
> >>> userspace to indicate to the controller to use Device Privacy Mode to a
> >>> specific device.
> >>>
> >>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >>> ---
> >>> v2: Fix marking Device Privacy Flag even when adapter is not capable of
> >>> handling Set Privacy Mode.
> >>>
> >>> include/net/bluetooth/hci_core.h |  4 ++++
> >>> net/bluetooth/mgmt.c             | 24 ++++++++++++++++++++----
> >>> 2 files changed, 24 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >>> index b5f061882c10..07d2d099dc2a 100644
> >>> --- a/include/net/bluetooth/hci_core.h
> >>> +++ b/include/net/bluetooth/hci_core.h
> >>> @@ -160,6 +160,7 @@ struct bdaddr_list_with_flags {
> >>>
> >>> enum hci_conn_flags {
> >>>      HCI_CONN_FLAG_REMOTE_WAKEUP,
> >>> +     HCI_CONN_FLAG_DEVICE_PRIVACY,
> >>>      HCI_CONN_FLAG_MAX
> >>> };
> >>>
> >>> @@ -1468,6 +1469,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> >>> #define use_ll_privacy(dev) (ll_privacy_capable(dev) && \
> >>>                           hci_dev_test_flag(dev, HCI_ENABLE_LL_PRIVACY))
> >>>
> >>> +#define privacy_mode_capable(dev) (use_ll_privacy(dev) && \
> >>> +                                (hdev->commands[39] & 0x04))
> >>> +
> >>> /* Use enhanced synchronous connection if command is supported */
> >>> #define enhanced_sco_capable(dev) ((dev)->commands[29] & 0x08)
> >>>
> >>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> >>> index 06384d761928..8a8376d32be3 100644
> >>> --- a/net/bluetooth/mgmt.c
> >>> +++ b/net/bluetooth/mgmt.c
> >>> @@ -4350,7 +4350,16 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> >>>                             MGMT_STATUS_NOT_SUPPORTED);
> >>> }
> >>>
> >>> -#define SUPPORTED_DEVICE_FLAGS() ((1U << HCI_CONN_FLAG_MAX) - 1)
> >>> +static u32 supported_device_flags(struct hci_dev *hdev)
> >>> +{
> >>> +     u32 flags = BIT(HCI_CONN_FLAG_MAX) - 1;
> >>> +
> >>> +     /* Check if Privacy Mode can be set */
> >>> +     if (!privacy_mode_capable(hdev))
> >>> +             flags &= ~BIT(HCI_CONN_FLAG_DEVICE_PRIVACY);
> >>> +
> >>> +     return flags;
> >>> +}
> >>
> >> I am lost on what we are doing, I know that SUPPORTED_DEVICE_FLAGS was introduced by 4c54bf2b093bb from Abhishek, but I fail to reason now why it is correct.
> >
> > But we don't set the HCI_CONN_FLAG_DEVICE_PRIVACY is hdev is not
> > capable of setting it, anyway the general idea of the supported_flags
> > is to indicate to userspace what flags the kernel is capable of
> > settings, so yeah I would expected it to be capable of setting every
> > flag except for those the controller don't have proper support for.
> > Maybe you got confused by the logic of first enabling everything and
> > then toggle back the bits that are not supported by the hdev.
>
> and I think that is wrong. We should only set the bits that the hardware and/or kernel supports. I have no idea on how I missed this in the review back then. I am trying to figure it out, but I am failing to follow this logic.

There is only 1 flag currently though, HCI_CONN_FLAG_REMOTE_WAKEUP, so
I wonder if that really requires a check? Or perhaps you want to
change the way we supported_device_flags works to:

index fe52b7eefb56..3ad09ce6eaeb 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4351,11 +4351,15 @@ static int set_exp_feature(struct sock *sk,
struct hci_dev *hdev,

 static u32 supported_device_flags(struct hci_dev *hdev)
 {
-       u32 flags = BIT(HCI_CONN_FLAG_MAX) - 1;
+       u32 flags = 0;
+
+       /* Check if adapter can wakeup the system */
+       if (hdev->wakeup && hdev->wakeup(hdev))
+               flags |= HCI_CONN_FLAG_REMOTE_WAKEUP;

        /* Check if Privacy Mode can be set */
-       if (!privacy_mode_capable(hdev))
-               flags &= ~BIT(HCI_CONN_FLAG_DEVICE_PRIVACY);
+       if (privacy_mode_capable(hdev))
+               flags |= HCI_CONN_FLAG_DEVICE_PRIVACY;

        return flags;
 }

We will need to adjust some of the tests though to reflect this change
in behavior:

et Device Flags - Success                           Failed       0.016 seconds
Set Device Flags - Success                           Not Run

Re: [PATCH v2 1/2] Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag

[Marcel Holtmann]

Hi Luiz,

>>>>> This introduces HCI_CONN_FLAG_DEVICE_PRIVACY which can be used by
>>>>> userspace to indicate to the controller to use Device Privacy Mode to a
>>>>> specific device.
>>>>> 
>>>>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>> ---
>>>>> v2: Fix marking Device Privacy Flag even when adapter is not capable of
>>>>> handling Set Privacy Mode.
>>>>> 
>>>>> include/net/bluetooth/hci_core.h |  4 ++++
>>>>> net/bluetooth/mgmt.c             | 24 ++++++++++++++++++++----
>>>>> 2 files changed, 24 insertions(+), 4 deletions(-)
>>>>> 
>>>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>>>> index b5f061882c10..07d2d099dc2a 100644
>>>>> --- a/include/net/bluetooth/hci_core.h
>>>>> +++ b/include/net/bluetooth/hci_core.h
>>>>> @@ -160,6 +160,7 @@ struct bdaddr_list_with_flags {
>>>>> 
>>>>> enum hci_conn_flags {
>>>>>     HCI_CONN_FLAG_REMOTE_WAKEUP,
>>>>> +     HCI_CONN_FLAG_DEVICE_PRIVACY,
>>>>>     HCI_CONN_FLAG_MAX
>>>>> };
>>>>> 
>>>>> @@ -1468,6 +1469,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
>>>>> #define use_ll_privacy(dev) (ll_privacy_capable(dev) && \
>>>>>                          hci_dev_test_flag(dev, HCI_ENABLE_LL_PRIVACY))
>>>>> 
>>>>> +#define privacy_mode_capable(dev) (use_ll_privacy(dev) && \
>>>>> +                                (hdev->commands[39] & 0x04))
>>>>> +
>>>>> /* Use enhanced synchronous connection if command is supported */
>>>>> #define enhanced_sco_capable(dev) ((dev)->commands[29] & 0x08)
>>>>> 
>>>>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>>>>> index 06384d761928..8a8376d32be3 100644
>>>>> --- a/net/bluetooth/mgmt.c
>>>>> +++ b/net/bluetooth/mgmt.c
>>>>> @@ -4350,7 +4350,16 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
>>>>>                            MGMT_STATUS_NOT_SUPPORTED);
>>>>> }
>>>>> 
>>>>> -#define SUPPORTED_DEVICE_FLAGS() ((1U << HCI_CONN_FLAG_MAX) - 1)
>>>>> +static u32 supported_device_flags(struct hci_dev *hdev)
>>>>> +{
>>>>> +     u32 flags = BIT(HCI_CONN_FLAG_MAX) - 1;
>>>>> +
>>>>> +     /* Check if Privacy Mode can be set */
>>>>> +     if (!privacy_mode_capable(hdev))
>>>>> +             flags &= ~BIT(HCI_CONN_FLAG_DEVICE_PRIVACY);
>>>>> +
>>>>> +     return flags;
>>>>> +}
>>>> 
>>>> I am lost on what we are doing, I know that SUPPORTED_DEVICE_FLAGS was introduced by 4c54bf2b093bb from Abhishek, but I fail to reason now why it is correct.
>>> 
>>> But we don't set the HCI_CONN_FLAG_DEVICE_PRIVACY is hdev is not
>>> capable of setting it, anyway the general idea of the supported_flags
>>> is to indicate to userspace what flags the kernel is capable of
>>> settings, so yeah I would expected it to be capable of setting every
>>> flag except for those the controller don't have proper support for.
>>> Maybe you got confused by the logic of first enabling everything and
>>> then toggle back the bits that are not supported by the hdev.
>> 
>> and I think that is wrong. We should only set the bits that the hardware and/or kernel supports. I have no idea on how I missed this in the review back then. I am trying to figure it out, but I am failing to follow this logic.
> 
> There is only 1 flag currently though, HCI_CONN_FLAG_REMOTE_WAKEUP, so
> I wonder if that really requires a check? Or perhaps you want to
> change the way we supported_device_flags works to:
> 
> index fe52b7eefb56..3ad09ce6eaeb 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -4351,11 +4351,15 @@ static int set_exp_feature(struct sock *sk,
> struct hci_dev *hdev,
> 
> static u32 supported_device_flags(struct hci_dev *hdev)
> {
> -       u32 flags = BIT(HCI_CONN_FLAG_MAX) - 1;
> +       u32 flags = 0;
> +
> +       /* Check if adapter can wakeup the system */
> +       if (hdev->wakeup && hdev->wakeup(hdev))
> +               flags |= HCI_CONN_FLAG_REMOTE_WAKEUP;
> 
>        /* Check if Privacy Mode can be set */
> -       if (!privacy_mode_capable(hdev))
> -               flags &= ~BIT(HCI_CONN_FLAG_DEVICE_PRIVACY);
> +       if (privacy_mode_capable(hdev))
> +               flags |= HCI_CONN_FLAG_DEVICE_PRIVACY;
> 
>        return flags;
> }

yes, that is how I would expected it to be.

Regards

Marcel

[PATCH v2 1/2] Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This introduces HCI_CONN_FLAG_DEVICE_PRIVACY which can be used by
userspace to indicate to the controller to use Device Privacy Mode to a
specific device.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
v2: Fix supported flags not actually checking if the hdev really
supports the flags.

 include/net/bluetooth/hci_core.h |  4 ++++
 net/bluetooth/mgmt.c             | 30 ++++++++++++++++++++++++++----
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 2560cfe80db8..42ba40df6e20 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -160,6 +160,7 @@ struct bdaddr_list_with_flags {
 
 enum hci_conn_flags {
 	HCI_CONN_FLAG_REMOTE_WAKEUP,
+	HCI_CONN_FLAG_DEVICE_PRIVACY,
 	HCI_CONN_FLAG_MAX
 };
 
@@ -1465,6 +1466,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
 #define use_ll_privacy(dev) (ll_privacy_capable(dev) && \
 			     hci_dev_test_flag(dev, HCI_ENABLE_LL_PRIVACY))
 
+#define privacy_mode_capable(dev) (use_ll_privacy(dev) && \
+				   (hdev->commands[39] & 0x04))
+
 /* Use enhanced synchronous connection if command is supported */
 #define enhanced_sco_capable(dev) ((dev)->commands[29] & 0x08)
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index f8f74d344297..d82d1a62754a 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4349,7 +4349,22 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
 			       MGMT_STATUS_NOT_SUPPORTED);
 }
 
-#define SUPPORTED_DEVICE_FLAGS() ((1U << HCI_CONN_FLAG_MAX) - 1)
+static u32 supported_device_flags(struct hci_dev *hdev)
+{
+	u32 flags = 0;
+
+	/* Check if adapter can wakeup the system */
+	if (hdev->wakeup && hdev->wakeup(hdev))
+		flags |= BIT(HCI_CONN_FLAG_REMOTE_WAKEUP);
+
+	/* Check if Privacy Mode can be set */
+	if (privacy_mode_capable(hdev))
+		flags |= BIT(HCI_CONN_FLAG_DEVICE_PRIVACY);
+
+	bt_dev_err(hdev, "flag 0x%8x", flags);
+
+	return flags;
+}
 
 static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 			    u16 data_len)
@@ -4358,7 +4373,7 @@ static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 	struct mgmt_rp_get_device_flags rp;
 	struct bdaddr_list_with_flags *br_params;
 	struct hci_conn_params *params;
-	u32 supported_flags = SUPPORTED_DEVICE_FLAGS();
+	u32 supported_flags = supported_device_flags(hdev);
 	u32 current_flags = 0;
 	u8 status = MGMT_STATUS_INVALID_PARAMS;
 
@@ -4422,7 +4437,7 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 	struct bdaddr_list_with_flags *br_params;
 	struct hci_conn_params *params;
 	u8 status = MGMT_STATUS_INVALID_PARAMS;
-	u32 supported_flags = SUPPORTED_DEVICE_FLAGS();
+	u32 supported_flags = supported_device_flags(hdev);
 	u32 current_flags = __le32_to_cpu(cp->current_flags);
 
 	bt_dev_dbg(hdev, "Set device flags %pMR (type 0x%x) = 0x%x",
@@ -4455,6 +4470,13 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 		if (params) {
 			params->current_flags = current_flags;
 			status = MGMT_STATUS_SUCCESS;
+
+			/* Update passive scan if HCI_CONN_FLAG_DEVICE_PRIVACY
+			 * has been set.
+			 */
+			if (hci_conn_test_flag(HCI_CONN_FLAG_DEVICE_PRIVACY,
+					       params->current_flags))
+				hci_update_passive_scan(hdev);
 		} else {
 			bt_dev_warn(hdev, "No such LE device %pMR (0x%x)",
 				    &cp->addr.bdaddr,
@@ -7060,7 +7082,7 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
 added:
 	device_added(sk, hdev, &cp->addr.bdaddr, cp->addr.type, cp->action);
 	device_flags_changed(NULL, hdev, &cp->addr.bdaddr, cp->addr.type,
-			     SUPPORTED_DEVICE_FLAGS(), current_flags);
+			     supported_device_flags(hdev), current_flags);
 
 	err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
 				MGMT_STATUS_SUCCESS, &cp->addr,
-- 
2.33.1

[PATCH v2 2/2] Bluetooth: hci_sync: Set Privacy Mode when updating the resolving list

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This adds support for Set Privacy Mode when updating the resolving list
when HCI_CONN_FLAG_DEVICE_PRIVACY so the controller shall use Device
Mode for devices programmed in the resolving list, Device Mode is
actually required when the remote device are not able to use RPA as
otherwise the default mode is Network Privacy Mode in which only
allows RPAs thus the controller would filter out advertisement using
identity addresses for which there is an IRK.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 include/net/bluetooth/hci.h      | 10 ++++++
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_event.c        | 29 +++++++++++++++++
 net/bluetooth/hci_sync.c         | 53 ++++++++++++++++++++++++++++----
 4 files changed, 87 insertions(+), 6 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 84db6b275231..7444d286e6be 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1931,6 +1931,16 @@ struct hci_rp_le_read_transmit_power {
 	__s8  max_le_tx_power;
 } __packed;
 
+#define HCI_NETWORK_PRIVACY		0x00
+#define HCI_DEVICE_PRIVACY		0x01
+
+#define HCI_OP_LE_SET_PRIVACY_MODE	0x204e
+struct hci_cp_le_set_privacy_mode {
+	__u8  bdaddr_type;
+	bdaddr_t  bdaddr;
+	__u8  mode;
+} __packed;
+
 #define HCI_OP_LE_READ_BUFFER_SIZE_V2	0x2060
 struct hci_rp_le_read_buffer_size_v2 {
 	__u8    status;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 42ba40df6e20..0b3de5411948 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -755,6 +755,7 @@ struct hci_conn_params {
 
 	struct hci_conn *conn;
 	bool explicit_connect;
+	u8  privacy_mode;
 	u32 current_flags;
 };
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index efc5458b1345..51c88f4f1274 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1300,6 +1300,31 @@ static void hci_cc_le_read_transmit_power(struct hci_dev *hdev,
 	hdev->max_le_tx_power = rp->max_le_tx_power;
 }
 
+static void hci_cc_le_set_privacy_mode(struct hci_dev *hdev,
+				       struct sk_buff *skb)
+{
+	__u8 status = *((__u8 *)skb->data);
+	struct hci_cp_le_set_privacy_mode *cp;
+	struct hci_conn_params *params;
+
+	bt_dev_dbg(hdev, "status 0x%2.2x", status);
+
+	if (status)
+		return;
+
+	cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_PRIVACY_MODE);
+	if (!cp)
+		return;
+
+	hci_dev_lock(hdev);
+
+	params = hci_conn_params_lookup(hdev, &cp->bdaddr, cp->bdaddr_type);
+	if (params)
+		params->privacy_mode = cp->mode;
+
+	hci_dev_unlock(hdev);
+}
+
 static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	__u8 *sent, status = *((__u8 *) skb->data);
@@ -3812,6 +3837,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb,
 		hci_cc_le_read_transmit_power(hdev, skb);
 		break;
 
+	case HCI_OP_LE_SET_PRIVACY_MODE:
+		hci_cc_le_set_privacy_mode(hdev, skb);
+		break;
+
 	default:
 		BT_DBG("%s opcode 0x%4.4x", hdev->name, *opcode);
 		break;
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index ad86caf41f91..08acd664590b 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -1580,8 +1580,42 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev,
 				     sizeof(cp), &cp, HCI_CMD_TIMEOUT);
 }
 
+/* Set Device Privacy Mode. */
+static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev,
+					struct hci_conn_params *params)
+{
+	struct hci_cp_le_set_privacy_mode cp;
+	struct smp_irk *irk;
+
+	/* If device privacy mode has already been set there is nothing to do */
+	if (params->privacy_mode == HCI_DEVICE_PRIVACY)
+		return 0;
+
+	/* Set Privacy Mode requires the use of resolving list (aka. LL Privacy)
+	 * by default Network Mode is used so only really send the command if
+	 * Device Mode is required (HCI_CONN_FLAG_DEVICE_PRIVACY).
+	 */
+	if (!privacy_mode_capable(hdev) ||
+	    !hci_conn_test_flag(HCI_CONN_FLAG_DEVICE_PRIVACY,
+				params->current_flags))
+		return 0;
+
+	irk = hci_find_irk_by_addr(hdev, &params->addr, params->addr_type);
+	if (!irk)
+		return 0;
+
+	memset(&cp, 0, sizeof(cp));
+	cp.bdaddr_type = irk->addr_type;
+	bacpy(&cp.bdaddr, &irk->bdaddr);
+	cp.mode = HCI_DEVICE_PRIVACY;
+
+	return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_PRIVACY_MODE,
+				     sizeof(cp), &cp, HCI_CMD_TIMEOUT);
+}
+
 /* Adds connection to allow list if needed, if the device uses RPA (has IRK)
- * this attempts to program the device in the resolving list as well.
+ * this attempts to program the device in the resolving list as well and
+ * properly set the privacy mode.
  */
 static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
 				       struct hci_conn_params *params,
@@ -1590,11 +1624,6 @@ static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
 	struct hci_cp_le_add_to_accept_list cp;
 	int err;
 
-	/* Already in accept list */
-	if (hci_bdaddr_list_lookup(&hdev->le_accept_list, &params->addr,
-				   params->addr_type))
-		return 0;
-
 	/* Select filter policy to accept all advertising */
 	if (*num_entries >= hdev->le_accept_list_size)
 		return -ENOSPC;
@@ -1620,6 +1649,18 @@ static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
 		return err;
 	}
 
+	/* Set Privacy Mode */
+	err = hci_le_set_privacy_mode_sync(hdev, params);
+	if (err) {
+		bt_dev_err(hdev, "Unable to set privacy mode: %d", err);
+		return err;
+	}
+
+	/* Check if already in accept list */
+	if (hci_bdaddr_list_lookup(&hdev->le_accept_list, &params->addr,
+				   params->addr_type))
+		return 0;
+
 	*num_entries += 1;
 	cp.bdaddr_type = params->addr_type;
 	bacpy(&cp.bdaddr, &params->addr);
-- 
2.33.1

Re: [PATCH v2 2/2] Bluetooth: hci_sync: Set Privacy Mode when updating the resolving list

[Marcel Holtmann]

Hi Luiz,

> This adds support for Set Privacy Mode when updating the resolving list
> when HCI_CONN_FLAG_DEVICE_PRIVACY so the controller shall use Device
> Mode for devices programmed in the resolving list, Device Mode is
> actually required when the remote device are not able to use RPA as
> otherwise the default mode is Network Privacy Mode in which only
> allows RPAs thus the controller would filter out advertisement using
> identity addresses for which there is an IRK.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> include/net/bluetooth/hci.h      | 10 ++++++
> include/net/bluetooth/hci_core.h |  1 +
> net/bluetooth/hci_event.c        | 29 +++++++++++++++++
> net/bluetooth/hci_sync.c         | 53 ++++++++++++++++++++++++++++----
> 4 files changed, 87 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 84db6b275231..7444d286e6be 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1931,6 +1931,16 @@ struct hci_rp_le_read_transmit_power {
> 	__s8  max_le_tx_power;
> } __packed;
> 
> +#define HCI_NETWORK_PRIVACY		0x00
> +#define HCI_DEVICE_PRIVACY		0x01
> +
> +#define HCI_OP_LE_SET_PRIVACY_MODE	0x204e
> +struct hci_cp_le_set_privacy_mode {
> +	__u8  bdaddr_type;
> +	bdaddr_t  bdaddr;
> +	__u8  mode;
> +} __packed;
> +
> #define HCI_OP_LE_READ_BUFFER_SIZE_V2	0x2060
> struct hci_rp_le_read_buffer_size_v2 {
> 	__u8    status;
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 42ba40df6e20..0b3de5411948 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -755,6 +755,7 @@ struct hci_conn_params {
> 
> 	struct hci_conn *conn;
> 	bool explicit_connect;
> +	u8  privacy_mode;
> 	u32 current_flags;
> };
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index efc5458b1345..51c88f4f1274 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1300,6 +1300,31 @@ static void hci_cc_le_read_transmit_power(struct hci_dev *hdev,
> 	hdev->max_le_tx_power = rp->max_le_tx_power;
> }
> 
> +static void hci_cc_le_set_privacy_mode(struct hci_dev *hdev,
> +				       struct sk_buff *skb)
> +{
> +	__u8 status = *((__u8 *)skb->data);
> +	struct hci_cp_le_set_privacy_mode *cp;
> +	struct hci_conn_params *params;
> +
> +	bt_dev_dbg(hdev, "status 0x%2.2x", status);
> +
> +	if (status)
> +		return;
> +
> +	cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_PRIVACY_MODE);
> +	if (!cp)
> +		return;
> +
> +	hci_dev_lock(hdev);
> +
> +	params = hci_conn_params_lookup(hdev, &cp->bdaddr, cp->bdaddr_type);
> +	if (params)
> +		params->privacy_mode = cp->mode;
> +
> +	hci_dev_unlock(hdev);
> +}
> +
> static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
> {
> 	__u8 *sent, status = *((__u8 *) skb->data);
> @@ -3812,6 +3837,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb,
> 		hci_cc_le_read_transmit_power(hdev, skb);
> 		break;
> 
> +	case HCI_OP_LE_SET_PRIVACY_MODE:
> +		hci_cc_le_set_privacy_mode(hdev, skb);
> +		break;
> +
> 	default:
> 		BT_DBG("%s opcode 0x%4.4x", hdev->name, *opcode);
> 		break;
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index ad86caf41f91..08acd664590b 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -1580,8 +1580,42 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev,
> 				     sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> }
> 
> +/* Set Device Privacy Mode. */
> +static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev,
> +					struct hci_conn_params *params)
> +{
> +	struct hci_cp_le_set_privacy_mode cp;
> +	struct smp_irk *irk;
> +
> +	/* If device privacy mode has already been set there is nothing to do */
> +	if (params->privacy_mode == HCI_DEVICE_PRIVACY)
> +		return 0;
> +
> +	/* Set Privacy Mode requires the use of resolving list (aka. LL Privacy)
> +	 * by default Network Mode is used so only really send the command if
> +	 * Device Mode is required (HCI_CONN_FLAG_DEVICE_PRIVACY).
> +	 */
> +	if (!privacy_mode_capable(hdev) ||
> +	    !hci_conn_test_flag(HCI_CONN_FLAG_DEVICE_PRIVACY,
> +				params->current_flags))
> +		return 0;

does this also account for the fact that LL Privacy is behind an experimental option. I think in the previous patch it is more important to check if LL Privacy is actually enabled via experimental setting.

Regards

Marcel

Re: [PATCH v2 2/2] Bluetooth: hci_sync: Set Privacy Mode when updating the resolving list

[Luiz Augusto von Dentz]

Hi Marcel,

On Fri, Nov 19, 2021 at 2:02 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > This adds support for Set Privacy Mode when updating the resolving list
> > when HCI_CONN_FLAG_DEVICE_PRIVACY so the controller shall use Device
> > Mode for devices programmed in the resolving list, Device Mode is
> > actually required when the remote device are not able to use RPA as
> > otherwise the default mode is Network Privacy Mode in which only
> > allows RPAs thus the controller would filter out advertisement using
> > identity addresses for which there is an IRK.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > include/net/bluetooth/hci.h      | 10 ++++++
> > include/net/bluetooth/hci_core.h |  1 +
> > net/bluetooth/hci_event.c        | 29 +++++++++++++++++
> > net/bluetooth/hci_sync.c         | 53 ++++++++++++++++++++++++++++----
> > 4 files changed, 87 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 84db6b275231..7444d286e6be 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -1931,6 +1931,16 @@ struct hci_rp_le_read_transmit_power {
> >       __s8  max_le_tx_power;
> > } __packed;
> >
> > +#define HCI_NETWORK_PRIVACY          0x00
> > +#define HCI_DEVICE_PRIVACY           0x01
> > +
> > +#define HCI_OP_LE_SET_PRIVACY_MODE   0x204e
> > +struct hci_cp_le_set_privacy_mode {
> > +     __u8  bdaddr_type;
> > +     bdaddr_t  bdaddr;
> > +     __u8  mode;
> > +} __packed;
> > +
> > #define HCI_OP_LE_READ_BUFFER_SIZE_V2 0x2060
> > struct hci_rp_le_read_buffer_size_v2 {
> >       __u8    status;
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 42ba40df6e20..0b3de5411948 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -755,6 +755,7 @@ struct hci_conn_params {
> >
> >       struct hci_conn *conn;
> >       bool explicit_connect;
> > +     u8  privacy_mode;
> >       u32 current_flags;
> > };
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index efc5458b1345..51c88f4f1274 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -1300,6 +1300,31 @@ static void hci_cc_le_read_transmit_power(struct hci_dev *hdev,
> >       hdev->max_le_tx_power = rp->max_le_tx_power;
> > }
> >
> > +static void hci_cc_le_set_privacy_mode(struct hci_dev *hdev,
> > +                                    struct sk_buff *skb)
> > +{
> > +     __u8 status = *((__u8 *)skb->data);
> > +     struct hci_cp_le_set_privacy_mode *cp;
> > +     struct hci_conn_params *params;
> > +
> > +     bt_dev_dbg(hdev, "status 0x%2.2x", status);
> > +
> > +     if (status)
> > +             return;
> > +
> > +     cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_PRIVACY_MODE);
> > +     if (!cp)
> > +             return;
> > +
> > +     hci_dev_lock(hdev);
> > +
> > +     params = hci_conn_params_lookup(hdev, &cp->bdaddr, cp->bdaddr_type);
> > +     if (params)
> > +             params->privacy_mode = cp->mode;
> > +
> > +     hci_dev_unlock(hdev);
> > +}
> > +
> > static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
> > {
> >       __u8 *sent, status = *((__u8 *) skb->data);
> > @@ -3812,6 +3837,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb,
> >               hci_cc_le_read_transmit_power(hdev, skb);
> >               break;
> >
> > +     case HCI_OP_LE_SET_PRIVACY_MODE:
> > +             hci_cc_le_set_privacy_mode(hdev, skb);
> > +             break;
> > +
> >       default:
> >               BT_DBG("%s opcode 0x%4.4x", hdev->name, *opcode);
> >               break;
> > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> > index ad86caf41f91..08acd664590b 100644
> > --- a/net/bluetooth/hci_sync.c
> > +++ b/net/bluetooth/hci_sync.c
> > @@ -1580,8 +1580,42 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev,
> >                                    sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> > }
> >
> > +/* Set Device Privacy Mode. */
> > +static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev,
> > +                                     struct hci_conn_params *params)
> > +{
> > +     struct hci_cp_le_set_privacy_mode cp;
> > +     struct smp_irk *irk;
> > +
> > +     /* If device privacy mode has already been set there is nothing to do */
> > +     if (params->privacy_mode == HCI_DEVICE_PRIVACY)
> > +             return 0;
> > +
> > +     /* Set Privacy Mode requires the use of resolving list (aka. LL Privacy)
> > +      * by default Network Mode is used so only really send the command if
> > +      * Device Mode is required (HCI_CONN_FLAG_DEVICE_PRIVACY).
> > +      */
> > +     if (!privacy_mode_capable(hdev) ||
> > +         !hci_conn_test_flag(HCI_CONN_FLAG_DEVICE_PRIVACY,
> > +                             params->current_flags))
> > +             return 0;
>
> does this also account for the fact that LL Privacy is behind an experimental option. I think in the previous patch it is more important to check if LL Privacy is actually enabled via experimental setting.

Yep, it does check it:

#define privacy_mode_capable(dev) (use_ll_privacy(dev) && \
   (hdev->commands[39] & 0x04))

That said, maybe we should check it again since one wouldn't be able
to set HCI_CONN_FLAG_DEVICE_PRIVACY as it wouldn't be supported.

> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v2 2/2] Bluetooth: hci_sync: Set Privacy Mode when updating the resolving list

[Marcel Holtmann]

Hi Luiz,

>>> This adds support for Set Privacy Mode when updating the resolving list
>>> when HCI_CONN_FLAG_DEVICE_PRIVACY so the controller shall use Device
>>> Mode for devices programmed in the resolving list, Device Mode is
>>> actually required when the remote device are not able to use RPA as
>>> otherwise the default mode is Network Privacy Mode in which only
>>> allows RPAs thus the controller would filter out advertisement using
>>> identity addresses for which there is an IRK.
>>> 
>>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>> ---
>>> include/net/bluetooth/hci.h      | 10 ++++++
>>> include/net/bluetooth/hci_core.h |  1 +
>>> net/bluetooth/hci_event.c        | 29 +++++++++++++++++
>>> net/bluetooth/hci_sync.c         | 53 ++++++++++++++++++++++++++++----
>>> 4 files changed, 87 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>> index 84db6b275231..7444d286e6be 100644
>>> --- a/include/net/bluetooth/hci.h
>>> +++ b/include/net/bluetooth/hci.h
>>> @@ -1931,6 +1931,16 @@ struct hci_rp_le_read_transmit_power {
>>>      __s8  max_le_tx_power;
>>> } __packed;
>>> 
>>> +#define HCI_NETWORK_PRIVACY          0x00
>>> +#define HCI_DEVICE_PRIVACY           0x01
>>> +
>>> +#define HCI_OP_LE_SET_PRIVACY_MODE   0x204e
>>> +struct hci_cp_le_set_privacy_mode {
>>> +     __u8  bdaddr_type;
>>> +     bdaddr_t  bdaddr;
>>> +     __u8  mode;
>>> +} __packed;
>>> +
>>> #define HCI_OP_LE_READ_BUFFER_SIZE_V2 0x2060
>>> struct hci_rp_le_read_buffer_size_v2 {
>>>      __u8    status;
>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>> index 42ba40df6e20..0b3de5411948 100644
>>> --- a/include/net/bluetooth/hci_core.h
>>> +++ b/include/net/bluetooth/hci_core.h
>>> @@ -755,6 +755,7 @@ struct hci_conn_params {
>>> 
>>>      struct hci_conn *conn;
>>>      bool explicit_connect;
>>> +     u8  privacy_mode;
>>>      u32 current_flags;
>>> };
>>> 
>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>> index efc5458b1345..51c88f4f1274 100644
>>> --- a/net/bluetooth/hci_event.c
>>> +++ b/net/bluetooth/hci_event.c
>>> @@ -1300,6 +1300,31 @@ static void hci_cc_le_read_transmit_power(struct hci_dev *hdev,
>>>      hdev->max_le_tx_power = rp->max_le_tx_power;
>>> }
>>> 
>>> +static void hci_cc_le_set_privacy_mode(struct hci_dev *hdev,
>>> +                                    struct sk_buff *skb)
>>> +{
>>> +     __u8 status = *((__u8 *)skb->data);
>>> +     struct hci_cp_le_set_privacy_mode *cp;
>>> +     struct hci_conn_params *params;
>>> +
>>> +     bt_dev_dbg(hdev, "status 0x%2.2x", status);
>>> +
>>> +     if (status)
>>> +             return;
>>> +
>>> +     cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_PRIVACY_MODE);
>>> +     if (!cp)
>>> +             return;
>>> +
>>> +     hci_dev_lock(hdev);
>>> +
>>> +     params = hci_conn_params_lookup(hdev, &cp->bdaddr, cp->bdaddr_type);
>>> +     if (params)
>>> +             params->privacy_mode = cp->mode;
>>> +
>>> +     hci_dev_unlock(hdev);
>>> +}
>>> +
>>> static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
>>> {
>>>      __u8 *sent, status = *((__u8 *) skb->data);
>>> @@ -3812,6 +3837,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb,
>>>              hci_cc_le_read_transmit_power(hdev, skb);
>>>              break;
>>> 
>>> +     case HCI_OP_LE_SET_PRIVACY_MODE:
>>> +             hci_cc_le_set_privacy_mode(hdev, skb);
>>> +             break;
>>> +
>>>      default:
>>>              BT_DBG("%s opcode 0x%4.4x", hdev->name, *opcode);
>>>              break;
>>> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
>>> index ad86caf41f91..08acd664590b 100644
>>> --- a/net/bluetooth/hci_sync.c
>>> +++ b/net/bluetooth/hci_sync.c
>>> @@ -1580,8 +1580,42 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev,
>>>                                   sizeof(cp), &cp, HCI_CMD_TIMEOUT);
>>> }
>>> 
>>> +/* Set Device Privacy Mode. */
>>> +static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev,
>>> +                                     struct hci_conn_params *params)
>>> +{
>>> +     struct hci_cp_le_set_privacy_mode cp;
>>> +     struct smp_irk *irk;
>>> +
>>> +     /* If device privacy mode has already been set there is nothing to do */
>>> +     if (params->privacy_mode == HCI_DEVICE_PRIVACY)
>>> +             return 0;
>>> +
>>> +     /* Set Privacy Mode requires the use of resolving list (aka. LL Privacy)
>>> +      * by default Network Mode is used so only really send the command if
>>> +      * Device Mode is required (HCI_CONN_FLAG_DEVICE_PRIVACY).
>>> +      */
>>> +     if (!privacy_mode_capable(hdev) ||
>>> +         !hci_conn_test_flag(HCI_CONN_FLAG_DEVICE_PRIVACY,
>>> +                             params->current_flags))
>>> +             return 0;
>> 
>> does this also account for the fact that LL Privacy is behind an experimental option. I think in the previous patch it is more important to check if LL Privacy is actually enabled via experimental setting.
> 
> Yep, it does check it:
> 
> #define privacy_mode_capable(dev) (use_ll_privacy(dev) && \
>   (hdev->commands[39] & 0x04))
> 
> That said, maybe we should check it again since one wouldn't be able
> to set HCI_CONN_FLAG_DEVICE_PRIVACY as it wouldn't be supported.

I think we should just store the supported device flags in hdev determined during power on. And then just allow setting or unsetting of it based on the fact that it is supported.

On a different note, while checking above, I found this:

	changed = !hci_dev_test_flag(hdev, HCI_ENABLE_LL_PRIVACY);
	hci_dev_set_flag(hdev, HCI_ENABLE_LL_PRIVACY);

This is not ok. It is fully racy. We need to have hci_dev_test_and_set_flag() and hci_dev_test_and_clear_flag() helpers.

Regards

Marcel

Re: [PATCH v2 2/2] Bluetooth: hci_sync: Set Privacy Mode when updating the resolving list

[Luiz Augusto von Dentz]

Hi Marcel,

On Fri, Nov 19, 2021 at 11:59 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> >>> This adds support for Set Privacy Mode when updating the resolving list
> >>> when HCI_CONN_FLAG_DEVICE_PRIVACY so the controller shall use Device
> >>> Mode for devices programmed in the resolving list, Device Mode is
> >>> actually required when the remote device are not able to use RPA as
> >>> otherwise the default mode is Network Privacy Mode in which only
> >>> allows RPAs thus the controller would filter out advertisement using
> >>> identity addresses for which there is an IRK.
> >>>
> >>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >>> ---
> >>> include/net/bluetooth/hci.h      | 10 ++++++
> >>> include/net/bluetooth/hci_core.h |  1 +
> >>> net/bluetooth/hci_event.c        | 29 +++++++++++++++++
> >>> net/bluetooth/hci_sync.c         | 53 ++++++++++++++++++++++++++++----
> >>> 4 files changed, 87 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> >>> index 84db6b275231..7444d286e6be 100644
> >>> --- a/include/net/bluetooth/hci.h
> >>> +++ b/include/net/bluetooth/hci.h
> >>> @@ -1931,6 +1931,16 @@ struct hci_rp_le_read_transmit_power {
> >>>      __s8  max_le_tx_power;
> >>> } __packed;
> >>>
> >>> +#define HCI_NETWORK_PRIVACY          0x00
> >>> +#define HCI_DEVICE_PRIVACY           0x01
> >>> +
> >>> +#define HCI_OP_LE_SET_PRIVACY_MODE   0x204e
> >>> +struct hci_cp_le_set_privacy_mode {
> >>> +     __u8  bdaddr_type;
> >>> +     bdaddr_t  bdaddr;
> >>> +     __u8  mode;
> >>> +} __packed;
> >>> +
> >>> #define HCI_OP_LE_READ_BUFFER_SIZE_V2 0x2060
> >>> struct hci_rp_le_read_buffer_size_v2 {
> >>>      __u8    status;
> >>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >>> index 42ba40df6e20..0b3de5411948 100644
> >>> --- a/include/net/bluetooth/hci_core.h
> >>> +++ b/include/net/bluetooth/hci_core.h
> >>> @@ -755,6 +755,7 @@ struct hci_conn_params {
> >>>
> >>>      struct hci_conn *conn;
> >>>      bool explicit_connect;
> >>> +     u8  privacy_mode;
> >>>      u32 current_flags;
> >>> };
> >>>
> >>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >>> index efc5458b1345..51c88f4f1274 100644
> >>> --- a/net/bluetooth/hci_event.c
> >>> +++ b/net/bluetooth/hci_event.c
> >>> @@ -1300,6 +1300,31 @@ static void hci_cc_le_read_transmit_power(struct hci_dev *hdev,
> >>>      hdev->max_le_tx_power = rp->max_le_tx_power;
> >>> }
> >>>
> >>> +static void hci_cc_le_set_privacy_mode(struct hci_dev *hdev,
> >>> +                                    struct sk_buff *skb)
> >>> +{
> >>> +     __u8 status = *((__u8 *)skb->data);
> >>> +     struct hci_cp_le_set_privacy_mode *cp;
> >>> +     struct hci_conn_params *params;
> >>> +
> >>> +     bt_dev_dbg(hdev, "status 0x%2.2x", status);
> >>> +
> >>> +     if (status)
> >>> +             return;
> >>> +
> >>> +     cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_PRIVACY_MODE);
> >>> +     if (!cp)
> >>> +             return;
> >>> +
> >>> +     hci_dev_lock(hdev);
> >>> +
> >>> +     params = hci_conn_params_lookup(hdev, &cp->bdaddr, cp->bdaddr_type);
> >>> +     if (params)
> >>> +             params->privacy_mode = cp->mode;
> >>> +
> >>> +     hci_dev_unlock(hdev);
> >>> +}
> >>> +
> >>> static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
> >>> {
> >>>      __u8 *sent, status = *((__u8 *) skb->data);
> >>> @@ -3812,6 +3837,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb,
> >>>              hci_cc_le_read_transmit_power(hdev, skb);
> >>>              break;
> >>>
> >>> +     case HCI_OP_LE_SET_PRIVACY_MODE:
> >>> +             hci_cc_le_set_privacy_mode(hdev, skb);
> >>> +             break;
> >>> +
> >>>      default:
> >>>              BT_DBG("%s opcode 0x%4.4x", hdev->name, *opcode);
> >>>              break;
> >>> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> >>> index ad86caf41f91..08acd664590b 100644
> >>> --- a/net/bluetooth/hci_sync.c
> >>> +++ b/net/bluetooth/hci_sync.c
> >>> @@ -1580,8 +1580,42 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev,
> >>>                                   sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> >>> }
> >>>
> >>> +/* Set Device Privacy Mode. */
> >>> +static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev,
> >>> +                                     struct hci_conn_params *params)
> >>> +{
> >>> +     struct hci_cp_le_set_privacy_mode cp;
> >>> +     struct smp_irk *irk;
> >>> +
> >>> +     /* If device privacy mode has already been set there is nothing to do */
> >>> +     if (params->privacy_mode == HCI_DEVICE_PRIVACY)
> >>> +             return 0;
> >>> +
> >>> +     /* Set Privacy Mode requires the use of resolving list (aka. LL Privacy)
> >>> +      * by default Network Mode is used so only really send the command if
> >>> +      * Device Mode is required (HCI_CONN_FLAG_DEVICE_PRIVACY).
> >>> +      */
> >>> +     if (!privacy_mode_capable(hdev) ||
> >>> +         !hci_conn_test_flag(HCI_CONN_FLAG_DEVICE_PRIVACY,
> >>> +                             params->current_flags))
> >>> +             return 0;
> >>
> >> does this also account for the fact that LL Privacy is behind an experimental option. I think in the previous patch it is more important to check if LL Privacy is actually enabled via experimental setting.
> >
> > Yep, it does check it:
> >
> > #define privacy_mode_capable(dev) (use_ll_privacy(dev) && \
> >   (hdev->commands[39] & 0x04))
> >
> > That said, maybe we should check it again since one wouldn't be able
> > to set HCI_CONN_FLAG_DEVICE_PRIVACY as it wouldn't be supported.
>
> I think we should just store the supported device flags in hdev determined during power on. And then just allow setting or unsetting of it based on the fact that it is supported.

I'm afraid we can't do that all at power on though since some of the
flag support can change e.g. by enabling LL Privacy, but I guess we
can detect when things like LL Privacy is enabled via Set Experimental
Features and then just enable Device Privacy Mode based on that.

> On a different note, while checking above, I found this:
>
>         changed = !hci_dev_test_flag(hdev, HCI_ENABLE_LL_PRIVACY);
>         hci_dev_set_flag(hdev, HCI_ENABLE_LL_PRIVACY);
>
> This is not ok. It is fully racy. We need to have hci_dev_test_and_set_flag() and hci_dev_test_and_clear_flag() helpers.

Yep, actually they already exist but haven't been used for some reason.

> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz