[PATCH BlueZ 0/4] Adjust tests for sequential conn establishing

[PATCH BlueZ 0/4] Adjust tests for sequential conn establishing

[Jonas Dreßler]

We're going to change the logic to establish connections to ACL
devices in the kernel so that they only happen sequentially,
similar to what we're already doing for "LE Create Connection".

This needs a change in mgmt-tester, and while at it, let's also
introduce a test verifying the new behavior.

Also included in this series is a small drive-by fix for running
mgmt-tester with address sanititzer.

Kernel series: https://lore.kernel.org/linux-bluetooth/20240108224614.56900-1-verdre@v0yd.nl/

Jonas Dreßler (4):
  mgmt-tester: Add a 0-opcode to expect_hci_list lists
  mgmt-tester: Adjust a test for recent kernel changes
  emulator/btdev: Send page timeout after 2 secs delay
  mgmt-tester: Add a test for connecting sequentially

 emulator/btdev.c    |  30 ++++++++++-
 tools/mgmt-tester.c | 129 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 157 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH BlueZ 1/4] mgmt-tester: Add a 0-opcode to expect_hci_list lists

[Jonas Dreßler]

In add_expect_hci_list() we iterate through the entries of the
expect_hci_list as long as there is an opcode, which means currently
this relies on overflowing the buffer to detect the end of the list.

This is not great and when running with address sanitizer, the
out-of-bounds read gets detected and mgmt-tester aborts. Fix it by
adding a trailing 0-opcode to all those lists.
---
 tools/mgmt-tester.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tools/mgmt-tester.c b/tools/mgmt-tester.c
index 7dfd1b0c7..ee12ed7d5 100644
--- a/tools/mgmt-tester.c
+++ b/tools/mgmt-tester.c
@@ -8798,6 +8798,9 @@ static const struct hci_cmd_data multi_ext_adv_add_second_hci_cmds[] = {
 		.len = sizeof(le_set_ext_adv_enable_inst_2),
 		.param = le_set_ext_adv_enable_inst_2,
 	},
+	{
+		.opcode = 0,
+	},
 };
 
 static const struct generic_data multi_ext_advertising_add_second_2 = {
@@ -8845,6 +8848,9 @@ static const struct hci_cmd_data multi_ext_adv_remove_adv_hci_cmds[] = {
 		.len = sizeof(advertising_instance1_param),
 		.param = advertising_instance1_param,
 	},
+	{
+		.opcode = 0,
+	},
 };
 
 static const struct generic_data multi_ext_advertising_remove = {
@@ -8877,6 +8883,9 @@ static const struct hci_cmd_data multi_ext_adv_remove_all_adv_hci_cmds[] = {
 	{
 		.opcode = BT_HCI_CMD_LE_CLEAR_ADV_SETS,
 	},
+	{
+		.opcode = 0,
+	},
 };
 
 static const struct generic_data multi_ext_advertising_remove_all = {
@@ -8913,6 +8922,9 @@ static const struct hci_cmd_data multi_ext_adv_add_2_advs_hci_cmds[] = {
 		.len = sizeof(set_ext_adv_data_test1),
 		.param = set_ext_adv_data_test1,
 	},
+	{
+		.opcode = 0,
+	},
 };
 
 static const struct generic_data multi_ext_advertising_add_no_power = {
@@ -10378,6 +10390,9 @@ static const struct hci_cmd_data ll_privacy_add_device_3_hci_list[] = {
 		.param = set_resolv_on_param,
 		.len = sizeof(set_resolv_on_param),
 	},
+	{
+		.opcode = 0,
+	},
 };
 
 static const struct generic_data ll_privacy_add_device_3 = {
@@ -10495,6 +10510,9 @@ static const struct hci_cmd_data ll_privacy_add_device_9_hci_list[] = {
 		.len = sizeof(le_add_to_resolv_list_param),
 		.param = le_add_to_resolv_list_param
 	},
+	{
+		.opcode = 0,
+	},
 };
 
 static const struct generic_data ll_privacy_add_device_9 = {
@@ -10823,6 +10841,9 @@ static const struct hci_cmd_data ll_privacy_set_device_flags_1_hci_list[] = {
 		.param = set_resolv_on_param,
 		.len = sizeof(set_resolv_on_param),
 	},
+	{
+		.opcode = 0,
+	},
 };
 
 static const uint8_t device_flags_changed_params_1[] = {
-- 
2.43.0

Re: [PATCH BlueZ 1/4] mgmt-tester: Add a 0-opcode to expect_hci_list lists

[Luiz Augusto von Dentz]

Hi Jonas,

On Mon, Jan 29, 2024 at 6:49 AM Jonas Dreßler <verdre@v0yd.nl> wrote:
>
> In add_expect_hci_list() we iterate through the entries of the
> expect_hci_list as long as there is an opcode, which means currently
> this relies on overflowing the buffer to detect the end of the list.
>
> This is not great and when running with address sanitizer, the
> out-of-bounds read gets detected and mgmt-tester aborts. Fix it by
> adding a trailing 0-opcode to all those lists.
> ---
>  tools/mgmt-tester.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/tools/mgmt-tester.c b/tools/mgmt-tester.c
> index 7dfd1b0c7..ee12ed7d5 100644
> --- a/tools/mgmt-tester.c
> +++ b/tools/mgmt-tester.c
> @@ -8798,6 +8798,9 @@ static const struct hci_cmd_data multi_ext_adv_add_second_hci_cmds[] = {
>                 .len = sizeof(le_set_ext_adv_enable_inst_2),
>                 .param = le_set_ext_adv_enable_inst_2,
>         },
> +       {
> +               .opcode = 0,
> +       },

Normally the compiler would put a NULL term when last member has ',',
but we should either use {} to properly terminate the list or perhaps
it would have been better to have a something like
.expect_hci_list_len = ARRAY_SIZE(list) to ensure we never access past
the end of the list.

>  };
>
>  static const struct generic_data multi_ext_advertising_add_second_2 = {
> @@ -8845,6 +8848,9 @@ static const struct hci_cmd_data multi_ext_adv_remove_adv_hci_cmds[] = {
>                 .len = sizeof(advertising_instance1_param),
>                 .param = advertising_instance1_param,
>         },
> +       {
> +               .opcode = 0,
> +       },
>  };
>
>  static const struct generic_data multi_ext_advertising_remove = {
> @@ -8877,6 +8883,9 @@ static const struct hci_cmd_data multi_ext_adv_remove_all_adv_hci_cmds[] = {
>         {
>                 .opcode = BT_HCI_CMD_LE_CLEAR_ADV_SETS,
>         },
> +       {
> +               .opcode = 0,
> +       },
>  };
>
>  static const struct generic_data multi_ext_advertising_remove_all = {
> @@ -8913,6 +8922,9 @@ static const struct hci_cmd_data multi_ext_adv_add_2_advs_hci_cmds[] = {
>                 .len = sizeof(set_ext_adv_data_test1),
>                 .param = set_ext_adv_data_test1,
>         },
> +       {
> +               .opcode = 0,
> +       },
>  };
>
>  static const struct generic_data multi_ext_advertising_add_no_power = {
> @@ -10378,6 +10390,9 @@ static const struct hci_cmd_data ll_privacy_add_device_3_hci_list[] = {
>                 .param = set_resolv_on_param,
>                 .len = sizeof(set_resolv_on_param),
>         },
> +       {
> +               .opcode = 0,
> +       },
>  };
>
>  static const struct generic_data ll_privacy_add_device_3 = {
> @@ -10495,6 +10510,9 @@ static const struct hci_cmd_data ll_privacy_add_device_9_hci_list[] = {
>                 .len = sizeof(le_add_to_resolv_list_param),
>                 .param = le_add_to_resolv_list_param
>         },
> +       {
> +               .opcode = 0,
> +       },
>  };
>
>  static const struct generic_data ll_privacy_add_device_9 = {
> @@ -10823,6 +10841,9 @@ static const struct hci_cmd_data ll_privacy_set_device_flags_1_hci_list[] = {
>                 .param = set_resolv_on_param,
>                 .len = sizeof(set_resolv_on_param),
>         },
> +       {
> +               .opcode = 0,
> +       },
>  };
>
>  static const uint8_t device_flags_changed_params_1[] = {
> --
> 2.43.0
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/4] mgmt-tester: Add a 0-opcode to expect_hci_list lists

[Jonas Dreßler]

Hi Luiz,

On 29.01.24 14:40, Luiz Augusto von Dentz wrote:
> Hi Jonas,
> 
> On Mon, Jan 29, 2024 at 6:49 AM Jonas Dreßler <verdre@v0yd.nl> wrote:
>>
>> In add_expect_hci_list() we iterate through the entries of the
>> expect_hci_list as long as there is an opcode, which means currently
>> this relies on overflowing the buffer to detect the end of the list.
>>
>> This is not great and when running with address sanitizer, the
>> out-of-bounds read gets detected and mgmt-tester aborts. Fix it by
>> adding a trailing 0-opcode to all those lists.
>> ---
>>   tools/mgmt-tester.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/tools/mgmt-tester.c b/tools/mgmt-tester.c
>> index 7dfd1b0c7..ee12ed7d5 100644
>> --- a/tools/mgmt-tester.c
>> +++ b/tools/mgmt-tester.c
>> @@ -8798,6 +8798,9 @@ static const struct hci_cmd_data multi_ext_adv_add_second_hci_cmds[] = {
>>                  .len = sizeof(le_set_ext_adv_enable_inst_2),
>>                  .param = le_set_ext_adv_enable_inst_2,
>>          },
>> +       {
>> +               .opcode = 0,
>> +       },
> 
> Normally the compiler would put a NULL term when last member has ',',
> but we should either use {} to properly terminate the list or perhaps
> it would have been better to have a something like
> .expect_hci_list_len = ARRAY_SIZE(list) to ensure we never access past
> the end of the list.

Ahh good point, I'll add an {} entry to the lists instead. Yeah I also thought
a bit about adding expect_hci_list_len, but decided against it because that
could cause weird situations where the list is updated with a new HCI command
but increasing the expect_hci_list_len is forgotten. Then we silently wouldn't
test the new command, which seems to be a lot worse compared to a failing
address sanitizer.

Cheers,
Jonas

> 
>>   };
>>
>>   static const struct generic_data multi_ext_advertising_add_second_2 = {
>> @@ -8845,6 +8848,9 @@ static const struct hci_cmd_data multi_ext_adv_remove_adv_hci_cmds[] = {
>>                  .len = sizeof(advertising_instance1_param),
>>                  .param = advertising_instance1_param,
>>          },
>> +       {
>> +               .opcode = 0,
>> +       },
>>   };
>>
>>   static const struct generic_data multi_ext_advertising_remove = {
>> @@ -8877,6 +8883,9 @@ static const struct hci_cmd_data multi_ext_adv_remove_all_adv_hci_cmds[] = {
>>          {
>>                  .opcode = BT_HCI_CMD_LE_CLEAR_ADV_SETS,
>>          },
>> +       {
>> +               .opcode = 0,
>> +       },
>>   };
>>
>>   static const struct generic_data multi_ext_advertising_remove_all = {
>> @@ -8913,6 +8922,9 @@ static const struct hci_cmd_data multi_ext_adv_add_2_advs_hci_cmds[] = {
>>                  .len = sizeof(set_ext_adv_data_test1),
>>                  .param = set_ext_adv_data_test1,
>>          },
>> +       {
>> +               .opcode = 0,
>> +       },
>>   };
>>
>>   static const struct generic_data multi_ext_advertising_add_no_power = {
>> @@ -10378,6 +10390,9 @@ static const struct hci_cmd_data ll_privacy_add_device_3_hci_list[] = {
>>                  .param = set_resolv_on_param,
>>                  .len = sizeof(set_resolv_on_param),
>>          },
>> +       {
>> +               .opcode = 0,
>> +       },
>>   };
>>
>>   static const struct generic_data ll_privacy_add_device_3 = {
>> @@ -10495,6 +10510,9 @@ static const struct hci_cmd_data ll_privacy_add_device_9_hci_list[] = {
>>                  .len = sizeof(le_add_to_resolv_list_param),
>>                  .param = le_add_to_resolv_list_param
>>          },
>> +       {
>> +               .opcode = 0,
>> +       },
>>   };
>>
>>   static const struct generic_data ll_privacy_add_device_9 = {
>> @@ -10823,6 +10841,9 @@ static const struct hci_cmd_data ll_privacy_set_device_flags_1_hci_list[] = {
>>                  .param = set_resolv_on_param,
>>                  .len = sizeof(set_resolv_on_param),
>>          },
>> +       {
>> +               .opcode = 0,
>> +       },
>>   };
>>
>>   static const uint8_t device_flags_changed_params_1[] = {
>> --
>> 2.43.0
>>
> 
> 

RE: Adjust tests for sequential conn establishing

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 5626 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=820846

---Test result---

Test Summary:
CheckPatch                    FAIL      1.60 seconds
GitLint                       FAIL      1.10 seconds
BuildEll                      PASS      23.92 seconds
BluezMake                     PASS      715.16 seconds
MakeCheck                     PASS      11.73 seconds
MakeDistcheck                 PASS      163.35 seconds
CheckValgrind                 PASS      226.37 seconds
CheckSmatch                   WARNING   334.43 seconds
bluezmakeextell               PASS      109.33 seconds
IncrementalBuild              PASS      2785.53 seconds
ScanBuild                     WARNING   941.74 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,2/4] mgmt-tester: Adjust a test for recent kernel changes
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#58: 
sequentially", https://lore.kernel.org/linux-bluetooth/20240108224614.56900-1-verdre@v0yd.nl/),

/github/workspace/src/src/13535450.patch total: 0 errors, 1 warnings, 8 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13535450.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[BlueZ,3/4] emulator/btdev: Send page timeout after 2 secs delay
WARNING:LONG_LINE: line length of 86 exceeds 80 columns
#102: FILE: emulator/btdev.c:1322:
+		struct page_timeout_data *pt_data = new0(struct page_timeout_data, 1);

WARNING:LINE_SPACING: Missing a blank line after declarations
#103: FILE: emulator/btdev.c:1323:
+		struct page_timeout_data *pt_data = new0(struct page_timeout_data, 1);
+		pt_data->btdev = dev;

/github/workspace/src/src/13535451.patch total: 0 errors, 2 warnings, 42 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13535451.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[BlueZ,4/4] mgmt-tester: Add a test for connecting sequentially
WARNING:LONG_LINE: line length of 92 exceeds 80 columns
#125: FILE: tools/mgmt-tester.c:12836:
+		if (pd_data->n_connect_failed_evts != pd_data->n_create_conn_commands - 1) {

WARNING:LONG_LINE_COMMENT: line length of 93 exceeds 80 columns
#143: FILE: tools/mgmt-tester.c:12854:
+		0x31, 0xAB, 0xCD, 0x32, 0x34, 0x73, /* random bdaddr so we fail to connect */

/github/workspace/src/src/13535452.patch total: 0 errors, 2 warnings, 117 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13535452.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,2/4] mgmt-tester: Adjust a test for recent kernel changes

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
5: B1 Line exceeds max length (95>80): "sequentially", https://lore.kernel.org/linux-bluetooth/20240108224614.56900-1-verdre@v0yd.nl/),"
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
emulator/btdev.c:420:29: warning: Variable length array is used.
##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
emulator/btdev.c:1084:10: warning: Although the value stored to 'conn' is used in the enclosing expression, the value is never actually read from 'conn'
        while ((conn = queue_find(dev->conns, match_handle,
                ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
emulator/btdev.c:1363:24: warning: Access to field 'link' results in a dereference of a null pointer (loaded from variable 'conn')
        pending_conn_del(dev, conn->link->dev);
                              ^~~~~~~~~~
emulator/btdev.c:1485:13: warning: Access to field 'dev' results in a dereference of a null pointer (loaded from variable 'conn')
        send_event(conn->dev, BT_HCI_EVT_AUTH_COMPLETE, &ev, sizeof(ev));
                   ^~~~~~~~~
3 warnings generated.



---
Regards,
Linux Bluetooth

[PATCH BlueZ 2/4] mgmt-tester: Adjust a test for recent kernel changes

[Jonas Dreßler]

With the changes in the kernel to move to hci_sync for connecting ACL
devices (see kernel commit "Bluetooth: hci_conn: Only do ACL connections
sequentially", https://lore.kernel.org/linux-bluetooth/20240108224614.56900-1-verdre@v0yd.nl/),
the "ETIMEDOUT" error path for the "HCI Create Connection" command was
changed, sending a "HCI Create Connection Cancel" after the timeout.

This leads to the returned error in the "Pair Device - Power off 1"
test to change from NOT_POWERED to DISCONNECTED, so adjust for that.
---
 tools/mgmt-tester.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/mgmt-tester.c b/tools/mgmt-tester.c
index ee12ed7d5..7c9e63cbb 100644
--- a/tools/mgmt-tester.c
+++ b/tools/mgmt-tester.c
@@ -3216,7 +3216,7 @@ static const struct generic_data pair_device_power_off_test_1 = {
 	.send_opcode = MGMT_OP_PAIR_DEVICE,
 	.send_func = pair_device_send_param_func,
 	.force_power_off = true,
-	.expect_status = MGMT_STATUS_NOT_POWERED,
+	.expect_status = MGMT_STATUS_DISCONNECTED,
 	.expect_func = pair_device_expect_param_func,
 };
 
-- 
2.43.0

Re: [PATCH BlueZ 2/4] mgmt-tester: Adjust a test for recent kernel changes

[Luiz Augusto von Dentz]

Hi Jonas,

On Mon, Jan 29, 2024 at 6:49 AM Jonas Dreßler <verdre@v0yd.nl> wrote:
>
> With the changes in the kernel to move to hci_sync for connecting ACL
> devices (see kernel commit "Bluetooth: hci_conn: Only do ACL connections
> sequentially", https://lore.kernel.org/linux-bluetooth/20240108224614.56900-1-verdre@v0yd.nl/),
> the "ETIMEDOUT" error path for the "HCI Create Connection" command was
> changed, sending a "HCI Create Connection Cancel" after the timeout.
>
> This leads to the returned error in the "Pair Device - Power off 1"
> test to change from NOT_POWERED to DISCONNECTED, so adjust for that.
> ---
>  tools/mgmt-tester.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/mgmt-tester.c b/tools/mgmt-tester.c
> index ee12ed7d5..7c9e63cbb 100644
> --- a/tools/mgmt-tester.c
> +++ b/tools/mgmt-tester.c
> @@ -3216,7 +3216,7 @@ static const struct generic_data pair_device_power_off_test_1 = {
>         .send_opcode = MGMT_OP_PAIR_DEVICE,
>         .send_func = pair_device_send_param_func,
>         .force_power_off = true,
> -       .expect_status = MGMT_STATUS_NOT_POWERED,
> +       .expect_status = MGMT_STATUS_DISCONNECTED,
>         .expect_func = pair_device_expect_param_func,
>  };
>
> --
> 2.43.0

We haven't applied all the changes from that set though, are you
planning to resend them?

-- 
Luiz Augusto von Dentz

[PATCH BlueZ 3/4] emulator/btdev: Send page timeout after 2 secs delay

[Jonas Dreßler]

Real bluetooth adapters wouldn't send the page timeout immediately
when trying to page a device, instead it would take a few seconds.

Try to behave more realistically in the emulator and send the page
timeout after two seconds.
---
 emulator/btdev.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/emulator/btdev.c b/emulator/btdev.c
index da94f29d1..a40117400 100644
--- a/emulator/btdev.c
+++ b/emulator/btdev.c
@@ -1281,6 +1281,27 @@ static void conn_complete(struct btdev *btdev,
 	send_event(btdev, BT_HCI_EVT_CONN_COMPLETE, &cc, sizeof(cc));
 }
 
+struct page_timeout_data {
+	struct btdev *btdev;
+	uint8_t bdaddr[6];
+	unsigned int timeout_id;
+};
+
+static bool page_timeout(void *user_data)
+{
+	struct page_timeout_data *pt_data = user_data;
+	struct btdev *btdev = pt_data->btdev;
+	const uint8_t *bdaddr = pt_data->bdaddr;
+
+	timeout_remove(pt_data->timeout_id);
+	pt_data->timeout_id = 0;
+
+	conn_complete(btdev, bdaddr, BT_HCI_ERR_PAGE_TIMEOUT);
+
+	free(pt_data);
+	return false;
+}
+
 static int cmd_create_conn_complete(struct btdev *dev, const void *data,
 						uint8_t len)
 {
@@ -1298,7 +1319,14 @@ static int cmd_create_conn_complete(struct btdev *dev, const void *data,
 
 		send_event(remote, BT_HCI_EVT_CONN_REQUEST, &cr, sizeof(cr));
 	} else {
-		conn_complete(dev, cmd->bdaddr, BT_HCI_ERR_PAGE_TIMEOUT);
+		struct page_timeout_data *pt_data = new0(struct page_timeout_data, 1);
+		pt_data->btdev = dev;
+		memcpy(pt_data->bdaddr, cmd->bdaddr, 6);
+
+		/* Send page timeout after 2 seconds to emulate real paging */
+		pt_data->timeout_id = timeout_add(2000,
+						  page_timeout,
+						  pt_data, NULL);
 	}
 
 	return 0;
-- 
2.43.0

Re: [PATCH BlueZ 3/4] emulator/btdev: Send page timeout after 2 secs delay

[Luiz Augusto von Dentz]

Hi Jonas,

On Mon, Jan 29, 2024 at 6:49 AM Jonas Dreßler <verdre@v0yd.nl> wrote:
>
> Real bluetooth adapters wouldn't send the page timeout immediately
> when trying to page a device, instead it would take a few seconds.
>
> Try to behave more realistically in the emulator and send the page
> timeout after two seconds.
> ---
>  emulator/btdev.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/emulator/btdev.c b/emulator/btdev.c
> index da94f29d1..a40117400 100644
> --- a/emulator/btdev.c
> +++ b/emulator/btdev.c
> @@ -1281,6 +1281,27 @@ static void conn_complete(struct btdev *btdev,
>         send_event(btdev, BT_HCI_EVT_CONN_COMPLETE, &cc, sizeof(cc));
>  }
>
> +struct page_timeout_data {
> +       struct btdev *btdev;
> +       uint8_t bdaddr[6];
> +       unsigned int timeout_id;
> +};
> +
> +static bool page_timeout(void *user_data)
> +{
> +       struct page_timeout_data *pt_data = user_data;
> +       struct btdev *btdev = pt_data->btdev;
> +       const uint8_t *bdaddr = pt_data->bdaddr;
> +
> +       timeout_remove(pt_data->timeout_id);
> +       pt_data->timeout_id = 0;
> +
> +       conn_complete(btdev, bdaddr, BT_HCI_ERR_PAGE_TIMEOUT);
> +
> +       free(pt_data);
> +       return false;
> +}
> +
>  static int cmd_create_conn_complete(struct btdev *dev, const void *data,
>                                                 uint8_t len)
>  {
> @@ -1298,7 +1319,14 @@ static int cmd_create_conn_complete(struct btdev *dev, const void *data,
>
>                 send_event(remote, BT_HCI_EVT_CONN_REQUEST, &cr, sizeof(cr));
>         } else {
> -               conn_complete(dev, cmd->bdaddr, BT_HCI_ERR_PAGE_TIMEOUT);
> +               struct page_timeout_data *pt_data = new0(struct page_timeout_data, 1);
> +               pt_data->btdev = dev;
> +               memcpy(pt_data->bdaddr, cmd->bdaddr, 6);
> +
> +               /* Send page timeout after 2 seconds to emulate real paging */
> +               pt_data->timeout_id = timeout_add(2000,
> +                                                 page_timeout,
> +                                                 pt_data, NULL);
>         }
>
>         return 0;
> --
> 2.43.0

We currently don't set a specific page timeout which means we are
using the default value which is 5.12 sec, so I'd replace 2000 with
5120, we might have to do something similar to LE Audio scanning
though since during this period the remote instance could enable
connections which should trigger the connection request event as well.


-- 
Luiz Augusto von Dentz

[PATCH BlueZ 4/4] mgmt-tester: Add a test for connecting sequentially

[Jonas Dreßler]

The kernel was recently changed to only connect to ACL devices
sequentially (see kernel commit "Bluetooth: hci_conn: Only do
ACL connections sequentially"), similar to how it already behaves
for LE devices.

Add some testing for this behavior to mgmt-tester and make sure
that new "HCI Create Connection" commands only get sent after
the previous "Pair Device" MGMT command has failed.
---
 tools/mgmt-tester.c | 106 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/tools/mgmt-tester.c b/tools/mgmt-tester.c
index 7c9e63cbb..39a3b59dd 100644
--- a/tools/mgmt-tester.c
+++ b/tools/mgmt-tester.c
@@ -12781,6 +12781,103 @@ static void test_hci_devcd(const void *test_data)
 	tester_wait(test->dump_data->timeout + 1, verify_devcd, NULL);
 }
 
+static const struct generic_data sequential_connect = {
+	.setup_settings = settings_powered_bondable,
+	.pin = pair_device_pin,
+	.pin_len = sizeof(pair_device_pin),
+	.client_pin = pair_device_pin,
+	.client_pin_len = sizeof(pair_device_pin),
+};
+
+struct pair_devices_data {
+	struct test_data *test_data;
+	unsigned int n_connect_failed_evts;
+	unsigned int n_create_conn_commands;
+};
+
+static void pair_device_command_callback(uint8_t status, uint16_t length,
+					const void *param, void *user_data)
+{
+	struct pair_devices_data *pd_data = user_data;
+
+	if (status != MGMT_STATUS_CONNECT_FAILED) {
+		tester_warn("Unexpected status got %d expected %d",
+			    status, MGMT_STATUS_CONNECT_FAILED);
+		tester_test_failed();
+		free(pd_data);
+		return;
+	}
+
+	tester_print("Connect failed for Pair Device");
+
+	pd_data->n_connect_failed_evts++;
+	if (pd_data->n_connect_failed_evts != pd_data->n_create_conn_commands) {
+		tester_test_failed();
+		free(pd_data);
+		return;
+	}
+
+	if (pd_data->n_connect_failed_evts == 3) {
+		test_condition_complete(pd_data->test_data);
+		free(pd_data);
+	}
+}
+
+static bool connect_multiple_create_conn(const void *data, uint16_t len,
+					void *user_data)
+{
+	struct pair_devices_data *pd_data = user_data;
+	const uint8_t *status = data;
+
+	if (*status == 0) {
+		tester_print("Create connection finished");
+
+		pd_data->n_create_conn_commands++;
+		if (pd_data->n_connect_failed_evts != pd_data->n_create_conn_commands - 1) {
+			tester_test_failed();
+			free(pd_data);
+		}
+	} else {
+		tester_print("Create connection failed: 0x%02x", *status);
+		tester_test_failed();
+		free(pd_data);
+	}
+
+	return true;
+}
+
+static void test_sequential_connect(const void *test_data)
+{
+	struct test_data *data = tester_get_data();
+	struct pair_devices_data *pd_data;
+	static uint8_t param[8] = {
+		0x31, 0xAB, 0xCD, 0x32, 0x34, 0x73, /* random bdaddr so we fail to connect */
+		BDADDR_BREDR,
+		0x03, /* NoInputNoOutput */
+	 };
+
+	pd_data = new0(struct pair_devices_data, 1);
+	pd_data->test_data = data;
+	pd_data->n_connect_failed_evts = 0;
+	pd_data->n_create_conn_commands = 0;
+
+	hciemu_add_hook(data->hciemu, HCIEMU_HOOK_POST_CMD,
+			BT_HCI_CMD_CREATE_CONN,
+			connect_multiple_create_conn, pd_data);
+
+	mgmt_send_nowait(data->mgmt, MGMT_OP_PAIR_DEVICE, data->mgmt_index,
+			 sizeof(param), param,
+			 pair_device_command_callback, pd_data, NULL);
+	param[2] = 0x09; /* change bdaddr a bit */
+	mgmt_send_nowait(data->mgmt, MGMT_OP_PAIR_DEVICE, data->mgmt_index,
+			 sizeof(param), param,
+			 pair_device_command_callback, pd_data, NULL);
+	param[2] = 0x10; /* change bdaddr a bit */
+	mgmt_send_nowait(data->mgmt, MGMT_OP_PAIR_DEVICE, data->mgmt_index,
+			 sizeof(param), param,
+			 pair_device_command_callback, pd_data, NULL);
+}
+
 int main(int argc, char *argv[])
 {
 	tester_init(&argc, &argv);
@@ -14946,5 +15043,14 @@ int main(int argc, char *argv[])
 	test_bredrle_full("HCI Devcoredump - Dump Timeout", &dump_timeout, NULL,
 				test_hci_devcd, 3);
 
+	/* Sequential connect
+	 * Setup: Power on
+	 * Run: Try connecting to multiple devices
+	 * Expect: Connects time out sequentially
+	 */
+	test_bredrle_full("Sequential connect",
+				&sequential_connect, NULL,
+				test_sequential_connect, 7);
+
 	return tester_run();
 }
-- 
2.43.0

[PATCH BlueZ v2 1/4] mgmt-tester: Add a 0-entry to expect_hci_list lists

[Jonas Dreßler]

In add_expect_hci_list() we iterate through the entries of the
expect_hci_list as long as there is an opcode, which means currently
this relies on overflowing the buffer to detect the end of the list.

This is not great and when running with address sanitizer, the
out-of-bounds read gets detected and mgmt-tester aborts. Fix it by
adding a trailing zero-entry to all those lists.
---
 tools/mgmt-tester.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/mgmt-tester.c b/tools/mgmt-tester.c
index 7dfd1b0c7..7d884bbf6 100644
--- a/tools/mgmt-tester.c
+++ b/tools/mgmt-tester.c
@@ -8798,6 +8798,7 @@ static const struct hci_cmd_data multi_ext_adv_add_second_hci_cmds[] = {
 		.len = sizeof(le_set_ext_adv_enable_inst_2),
 		.param = le_set_ext_adv_enable_inst_2,
 	},
+	{},
 };
 
 static const struct generic_data multi_ext_advertising_add_second_2 = {
@@ -8845,6 +8846,7 @@ static const struct hci_cmd_data multi_ext_adv_remove_adv_hci_cmds[] = {
 		.len = sizeof(advertising_instance1_param),
 		.param = advertising_instance1_param,
 	},
+	{},
 };
 
 static const struct generic_data multi_ext_advertising_remove = {
@@ -8877,6 +8879,7 @@ static const struct hci_cmd_data multi_ext_adv_remove_all_adv_hci_cmds[] = {
 	{
 		.opcode = BT_HCI_CMD_LE_CLEAR_ADV_SETS,
 	},
+	{},
 };
 
 static const struct generic_data multi_ext_advertising_remove_all = {
@@ -8913,6 +8916,7 @@ static const struct hci_cmd_data multi_ext_adv_add_2_advs_hci_cmds[] = {
 		.len = sizeof(set_ext_adv_data_test1),
 		.param = set_ext_adv_data_test1,
 	},
+	{},
 };
 
 static const struct generic_data multi_ext_advertising_add_no_power = {
@@ -10378,6 +10382,7 @@ static const struct hci_cmd_data ll_privacy_add_device_3_hci_list[] = {
 		.param = set_resolv_on_param,
 		.len = sizeof(set_resolv_on_param),
 	},
+	{},
 };
 
 static const struct generic_data ll_privacy_add_device_3 = {
@@ -10495,6 +10500,7 @@ static const struct hci_cmd_data ll_privacy_add_device_9_hci_list[] = {
 		.len = sizeof(le_add_to_resolv_list_param),
 		.param = le_add_to_resolv_list_param
 	},
+	{},
 };
 
 static const struct generic_data ll_privacy_add_device_9 = {
@@ -10823,6 +10829,7 @@ static const struct hci_cmd_data ll_privacy_set_device_flags_1_hci_list[] = {
 		.param = set_resolv_on_param,
 		.len = sizeof(set_resolv_on_param),
 	},
+	{},
 };
 
 static const uint8_t device_flags_changed_params_1[] = {
-- 
2.43.0

RE: Adjust tests for sequential conn establishing

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3918 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=823536

---Test result---

Test Summary:
CheckPatch                    FAIL      1.54 seconds
GitLint                       PASS      0.90 seconds
BuildEll                      PASS      23.89 seconds
BluezMake                     PASS      695.39 seconds
MakeCheck                     PASS      11.91 seconds
MakeDistcheck                 PASS      162.67 seconds
CheckValgrind                 PASS      226.45 seconds
CheckSmatch                   WARNING   331.05 seconds
bluezmakeextell               PASS      108.44 seconds
IncrementalBuild              PASS      2662.64 seconds
ScanBuild                     WARNING   988.49 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,v2,3/4] emulator/btdev: Send page timeout after 5.12 secs delay
WARNING:LONG_LINE_COMMENT: line length of 81 exceeds 80 columns
#106: FILE: emulator/btdev.c:1328:
+		/* Send page timeout after 5.12 seconds to emulate real paging */

/github/workspace/src/src/13547070.patch total: 0 errors, 1 warnings, 44 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13547070.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[BlueZ,v2,4/4] mgmt-tester: Add a test for connecting sequentially
WARNING:LONG_LINE: line length of 85 exceeds 80 columns
#128: FILE: tools/mgmt-tester.c:12822:
+		if (pd_data->n_conn_failed_evts != pd_data->n_create_conn_cmds - 1) {

WARNING:LONG_LINE_COMMENT: line length of 93 exceeds 80 columns
#146: FILE: tools/mgmt-tester.c:12840:
+		0x31, 0xAB, 0xCD, 0x32, 0x34, 0x73, /* random bdaddr so we fail to connect */

/github/workspace/src/src/13547071.patch total: 0 errors, 2 warnings, 117 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13547071.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
emulator/btdev.c:420:29: warning: Variable length array is used.
##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
emulator/btdev.c:1084:10: warning: Although the value stored to 'conn' is used in the enclosing expression, the value is never actually read from 'conn'
        while ((conn = queue_find(dev->conns, match_handle,
                ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
emulator/btdev.c:1365:24: warning: Access to field 'link' results in a dereference of a null pointer (loaded from variable 'conn')
        pending_conn_del(dev, conn->link->dev);
                              ^~~~~~~~~~
emulator/btdev.c:1487:13: warning: Access to field 'dev' results in a dereference of a null pointer (loaded from variable 'conn')
        send_event(conn->dev, BT_HCI_EVT_AUTH_COMPLETE, &ev, sizeof(ev));
                   ^~~~~~~~~
3 warnings generated.



---
Regards,
Linux Bluetooth