[Bluez PATCH 0/3] Bluetooth: Fix scannable broadcast advertising on extended APIs

[Bluez PATCH 0/3] Bluetooth: Fix scannable broadcast advertising on extended APIs

[Daniel Winkler]

Hello Maintainers,

We have discovered that when userspace registers a broadcast
(non-connectable) advertisement with scan response data, it exposes a
limitation in the new extended MGMT APIs. At the time that the
parameters are registered with the controller, kernel does not yet have
the advertising data and scan response (coming in a separate MGMT call),
and will default to a non-scannable PDU. When the MGMT call for
data/scan response is received, the controller will either fail when we
request to set the scan response, or return success and not use it.

This series along with another in kernel will allow userspace to pass a
flag with the params request indicating if the advertisement contains a
scan response. This allows kernel to register the parameters correctly
with the controller.

The patch is tested with a scannable broadcast advertisement on Hatch
and Kukui chromebooks (ext and non-ext capabilities) and ensuring a
peripheral device can detect the scan response.

Best,
Daniel


Daniel Winkler (3):
  advertising: Generate advertising data earlier in pipeline
  advertising: Create and use scannable adv param flag
  doc/mgmt-api: Update documentation for scan_rsp param flag

 doc/mgmt-api.txt  |  5 +++
 lib/mgmt.h        |  1 +
 src/advertising.c | 83 ++++++++++++++++++++++++++---------------------
 3 files changed, 52 insertions(+), 37 deletions(-)

-- 
2.30.1.766.gb4fecdf3b7-goog

[Bluez PATCH 1/3] advertising: Generate advertising data earlier in pipeline

[Daniel Winkler]

This change moves the advertising data generation to the beginning of
the registration pipeline. This is necessary for the following patch,
which will need to know whether the scan response data is existent so
that the parameter request can be populated correctly.

Reviewed-by: Alain Michaud <alainm@chromium.org>
Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>

---

 src/advertising.c | 79 +++++++++++++++++++++++++----------------------
 1 file changed, 42 insertions(+), 37 deletions(-)

diff --git a/src/advertising.c b/src/advertising.c
index 15a343e52..f3dc357a1 100644
--- a/src/advertising.c
+++ b/src/advertising.c
@@ -80,6 +80,10 @@ struct btd_adv_client {
 	uint32_t flags;
 	struct bt_ad *data;
 	struct bt_ad *scan;
+	uint8_t *adv_data;
+	uint8_t *scan_rsp;
+	size_t adv_data_len;
+	size_t scan_rsp_len;
 	uint8_t instance;
 	uint32_t min_interval;
 	uint32_t max_interval;
@@ -141,6 +145,16 @@ static void client_free(void *data)
 	bt_ad_unref(client->data);
 	bt_ad_unref(client->scan);
 
+	if (client->adv_data) {
+		free(client->adv_data);
+		client->adv_data = NULL;
+	}
+
+	if (client->scan_rsp) {
+		free(client->scan_rsp);
+		client->scan_rsp = NULL;
+	}
+
 	g_dbus_proxy_unref(client->proxy);
 
 	if (client->owner)
@@ -915,6 +929,22 @@ static int refresh_extended_adv(struct btd_adv_client *client,
 		flags |= MGMT_ADV_PARAM_TX_POWER;
 	}
 
+	client->adv_data = generate_adv_data(client, &flags,
+						&client->adv_data_len);
+	if (!client->adv_data ||
+		(client->adv_data_len > calc_max_adv_len(client, flags))) {
+		error("Advertising data too long or couldn't be generated.");
+		return -EINVAL;
+	}
+
+	client->scan_rsp = generate_scan_rsp(client, &flags,
+						&client->scan_rsp_len);
+	if (!client->scan_rsp && client->scan_rsp_len) {
+		error("Scan data couldn't be generated.");
+		free(client->adv_data);
+		return -EINVAL;
+	}
+
 	cp.flags = htobl(flags);
 
 	mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS,
@@ -1222,11 +1252,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length,
 	const struct mgmt_rp_add_ext_adv_params *rp = param;
 	struct mgmt_cp_add_ext_adv_data *cp = NULL;
 	uint8_t param_len;
-	uint8_t *adv_data = NULL;
-	size_t adv_data_len;
-	uint8_t *scan_rsp = NULL;
-	size_t scan_rsp_len = -1;
-	uint32_t flags = 0;
 	unsigned int mgmt_ret;
 	dbus_int16_t tx_power;
 
@@ -1248,23 +1273,8 @@ static void add_adv_params_callback(uint8_t status, uint16_t length,
 
 	client->instance = rp->instance;
 
-	flags = get_adv_flags(client);
-
-	adv_data = generate_adv_data(client, &flags, &adv_data_len);
-	if (!adv_data || (adv_data_len > rp->max_adv_data_len)) {
-		error("Advertising data too long or couldn't be generated.");
-		goto fail;
-	}
-
-	scan_rsp = generate_scan_rsp(client, &flags, &scan_rsp_len);
-	if ((!scan_rsp && scan_rsp_len) ||
-			scan_rsp_len > rp->max_scan_rsp_len) {
-		error("Scan data couldn't be generated.");
-		goto fail;
-	}
-
-	param_len = sizeof(struct mgmt_cp_add_advertising) + adv_data_len +
-							scan_rsp_len;
+	param_len = sizeof(struct mgmt_cp_add_advertising) +
+			client->adv_data_len + client->scan_rsp_len;
 
 	cp = malloc0(param_len);
 	if (!cp) {
@@ -1273,15 +1283,11 @@ static void add_adv_params_callback(uint8_t status, uint16_t length,
 	}
 
 	cp->instance = client->instance;
-	cp->adv_data_len = adv_data_len;
-	cp->scan_rsp_len = scan_rsp_len;
-	memcpy(cp->data, adv_data, adv_data_len);
-	memcpy(cp->data + adv_data_len, scan_rsp, scan_rsp_len);
-
-	free(adv_data);
-	free(scan_rsp);
-	adv_data = NULL;
-	scan_rsp = NULL;
+	cp->adv_data_len = client->adv_data_len;
+	cp->scan_rsp_len = client->scan_rsp_len;
+	memcpy(cp->data, client->adv_data, client->adv_data_len);
+	memcpy(cp->data + client->adv_data_len, client->scan_rsp,
+			client->scan_rsp_len);
 
 	/* Submit request to update instance data */
 	mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_DATA,
@@ -1305,12 +1311,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length,
 	return;
 
 fail:
-	if (adv_data)
-		free(adv_data);
-
-	if (scan_rsp)
-		free(scan_rsp);
-
 	if (cp)
 		free(cp);
 
@@ -1454,6 +1454,11 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager,
 	if (!client->scan)
 		goto fail;
 
+	client->adv_data = NULL;
+	client->scan_rsp = NULL;
+	client->adv_data_len = 0;
+	client->scan_rsp_len = 0;
+
 	client->manager = manager;
 	client->appearance = UINT16_MAX;
 	client->tx_power = ADV_TX_POWER_NO_PREFERENCE;
-- 
2.30.1.766.gb4fecdf3b7-goog

[Bluez PATCH 2/3] advertising: Create and use scannable adv param flag

[Daniel Winkler]

In order for the advertising parameters hci request to indicate that an
advertising set uses a scannable PDU, we pass a scannable flag along
with the initial parameters MGMT request.

Without this patch, a broadcast advertisement with a scan response will
either be rejected by the controller, or will ignore the requested scan
response. The patch is tested by performing the above and confirming
that the scan response is retrievable from a peer as expected.

Reviewed-by: Alain Michaud <alainm@chromium.org>
Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>

---

 lib/mgmt.h        | 1 +
 src/advertising.c | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/lib/mgmt.h b/lib/mgmt.h
index 76a03c9c2..7b1b9ab54 100644
--- a/lib/mgmt.h
+++ b/lib/mgmt.h
@@ -507,6 +507,7 @@ struct mgmt_rp_add_advertising {
 #define MGMT_ADV_PARAM_TIMEOUT		(1 << 13)
 #define MGMT_ADV_PARAM_INTERVALS	(1 << 14)
 #define MGMT_ADV_PARAM_TX_POWER		(1 << 15)
+#define MGMT_ADV_PARAM_SCAN_RSP		(1 << 16)
 
 #define MGMT_OP_REMOVE_ADVERTISING	0x003F
 struct mgmt_cp_remove_advertising {
diff --git a/src/advertising.c b/src/advertising.c
index f3dc357a1..38cef565f 100644
--- a/src/advertising.c
+++ b/src/advertising.c
@@ -945,6 +945,10 @@ static int refresh_extended_adv(struct btd_adv_client *client,
 		return -EINVAL;
 	}
 
+	/* Indicate that this instance will be configured as scannable */
+	if (client->scan_rsp_len)
+		flags |= MGMT_ADV_PARAM_SCAN_RSP;
+
 	cp.flags = htobl(flags);
 
 	mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS,
-- 
2.30.1.766.gb4fecdf3b7-goog

Re: [Bluez PATCH 2/3] advertising: Create and use scannable adv param flag

[Luiz Augusto von Dentz]

Hi Daniel,

On Wed, Mar 3, 2021 at 11:20 AM Daniel Winkler <danielwinkler@google.com> wrote:
>
> In order for the advertising parameters hci request to indicate that an
> advertising set uses a scannable PDU, we pass a scannable flag along
> with the initial parameters MGMT request.
>
> Without this patch, a broadcast advertisement with a scan response will
> either be rejected by the controller, or will ignore the requested scan
> response. The patch is tested by performing the above and confirming
> that the scan response is retrievable from a peer as expected.
>
> Reviewed-by: Alain Michaud <alainm@chromium.org>
> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
>
> ---
>
>  lib/mgmt.h        | 1 +
>  src/advertising.c | 4 ++++
>  2 files changed, 5 insertions(+)
>
> diff --git a/lib/mgmt.h b/lib/mgmt.h
> index 76a03c9c2..7b1b9ab54 100644
> --- a/lib/mgmt.h
> +++ b/lib/mgmt.h
> @@ -507,6 +507,7 @@ struct mgmt_rp_add_advertising {
>  #define MGMT_ADV_PARAM_TIMEOUT         (1 << 13)
>  #define MGMT_ADV_PARAM_INTERVALS       (1 << 14)
>  #define MGMT_ADV_PARAM_TX_POWER                (1 << 15)
> +#define MGMT_ADV_PARAM_SCAN_RSP                (1 << 16)
>
>  #define MGMT_OP_REMOVE_ADVERTISING     0x003F
>  struct mgmt_cp_remove_advertising {
> diff --git a/src/advertising.c b/src/advertising.c
> index f3dc357a1..38cef565f 100644
> --- a/src/advertising.c
> +++ b/src/advertising.c
> @@ -945,6 +945,10 @@ static int refresh_extended_adv(struct btd_adv_client *client,
>                 return -EINVAL;
>         }
>
> +       /* Indicate that this instance will be configured as scannable */
> +       if (client->scan_rsp_len)
> +               flags |= MGMT_ADV_PARAM_SCAN_RSP;
> +

Don't we need to check if the flag is actually supported by the kernel?

>         cp.flags = htobl(flags);

For new code it is prefered to use the function from src/shared/util.h
(cpu_to_*).

>         mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS,
> --
> 2.30.1.766.gb4fecdf3b7-goog
>


-- 
Luiz Augusto von Dentz

Re: [Bluez PATCH 2/3] advertising: Create and use scannable adv param flag

[Daniel Winkler]

Hello Luiz,

Thank you for the catch and suggestion. I have just sent out a v2 to
address your recommendations.

Thanks!
Daniel


On Thu, Mar 4, 2021 at 10:59 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Daniel,
>
> On Wed, Mar 3, 2021 at 11:20 AM Daniel Winkler <danielwinkler@google.com> wrote:
> >
> > In order for the advertising parameters hci request to indicate that an
> > advertising set uses a scannable PDU, we pass a scannable flag along
> > with the initial parameters MGMT request.
> >
> > Without this patch, a broadcast advertisement with a scan response will
> > either be rejected by the controller, or will ignore the requested scan
> > response. The patch is tested by performing the above and confirming
> > that the scan response is retrievable from a peer as expected.
> >
> > Reviewed-by: Alain Michaud <alainm@chromium.org>
> > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> >
> > ---
> >
> >  lib/mgmt.h        | 1 +
> >  src/advertising.c | 4 ++++
> >  2 files changed, 5 insertions(+)
> >
> > diff --git a/lib/mgmt.h b/lib/mgmt.h
> > index 76a03c9c2..7b1b9ab54 100644
> > --- a/lib/mgmt.h
> > +++ b/lib/mgmt.h
> > @@ -507,6 +507,7 @@ struct mgmt_rp_add_advertising {
> >  #define MGMT_ADV_PARAM_TIMEOUT         (1 << 13)
> >  #define MGMT_ADV_PARAM_INTERVALS       (1 << 14)
> >  #define MGMT_ADV_PARAM_TX_POWER                (1 << 15)
> > +#define MGMT_ADV_PARAM_SCAN_RSP                (1 << 16)
> >
> >  #define MGMT_OP_REMOVE_ADVERTISING     0x003F
> >  struct mgmt_cp_remove_advertising {
> > diff --git a/src/advertising.c b/src/advertising.c
> > index f3dc357a1..38cef565f 100644
> > --- a/src/advertising.c
> > +++ b/src/advertising.c
> > @@ -945,6 +945,10 @@ static int refresh_extended_adv(struct btd_adv_client *client,
> >                 return -EINVAL;
> >         }
> >
> > +       /* Indicate that this instance will be configured as scannable */
> > +       if (client->scan_rsp_len)
> > +               flags |= MGMT_ADV_PARAM_SCAN_RSP;
> > +
>
> Don't we need to check if the flag is actually supported by the kernel?
>
> >         cp.flags = htobl(flags);
>
> For new code it is prefered to use the function from src/shared/util.h
> (cpu_to_*).
>
> >         mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS,
> > --
> > 2.30.1.766.gb4fecdf3b7-goog
> >
>
>
> --
> Luiz Augusto von Dentz

[Bluez PATCH 3/3] doc/mgmt-api: Update documentation for scan_rsp param flag

[Daniel Winkler]

This patch adds the new scannable flag to the Add Extended Advertising
Parameters MGMT API documentation.

Reviewed-by: Alain Michaud <alainm@chromium.org>
Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>

---

 doc/mgmt-api.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt
index 1736ef009..cab1fffc5 100644
--- a/doc/mgmt-api.txt
+++ b/doc/mgmt-api.txt
@@ -3632,6 +3632,7 @@ Add Extended Advertising Parameters Command
 		13	The Timeout parameter should be used
 		14	The Interval parameters should be used
 		15	The Tx Power parameter should be used
+		16	The advertisement will contain a scan response
 
 	When the connectable flag is set, then the controller will use
 	undirected connectable advertising. The value of the connectable
@@ -3708,6 +3709,10 @@ Add Extended Advertising Parameters Command
 	chosen by the controller. If the requested Tx Power is outside
 	the valid range, the registration will fail.
 
+	When flag bit 16 is enabled, it indicates that the subsequent request
+	to set advertising data will contain a scan response, and that the
+	parameters should set a PDU type that is scannable.
+
 	Re-adding an already existing instance (i.e. issuing the Add Extended
 	Advertising Parameters command with an Instance identifier of an
 	existing instance) will update that instance's configuration. In this
-- 
2.30.1.766.gb4fecdf3b7-goog