[PATCH BlueZ 0/6] Mesh: Fixes for PTS issues

[PATCH BlueZ 0/6] Mesh: Fixes for PTS issues

[Isak Westin]

Hi,

Here is the last patch set with fixes for issues found during PTS
testing. This covers following tests:
 - MESH/NODE/PROV/*
 - MESH/NODE/TNPT/*
 - MESH/NODE/RLY/*

Best regards,
Isak

Isak Westin (6):
  mesh: Update Key Refresh flag after provision
  mesh: provisionee: Handle unknown PDUs
  mesh: provisionee: Handle failed provisioning
  mesh: provisionee: Check prov start parameters
  mesh: Keep canceled SAR data for at least 10 sec
  mesh: Fix msg cache ring buffer

 mesh/net.c           | 33 +++++++++++++++++---
 mesh/node.c          |  4 +--
 mesh/prov-acceptor.c | 73 +++++++++++++++++++++++++++++++++++---------
 3 files changed, 90 insertions(+), 20 deletions(-)

-- 
2.20.1

[PATCH BlueZ 1/6] mesh: Update Key Refresh flag after provision

[Isak Westin]

The Key Refresh flag in the Secure Network beacon is now correctly
updated based on provisioning data after being successfully provisioned.
---
 mesh/node.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mesh/node.c b/mesh/node.c
index e81aa82fe..cf4ed140e 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -1270,8 +1270,8 @@ static bool add_local_node(struct mesh_node *node, uint16_t unicast, bool kr,
 							MESH_STATUS_SUCCESS)
 			return false;
 
-		if (!mesh_config_net_key_set_phase(node->cfg, net_key_idx,
-							KEY_REFRESH_PHASE_TWO))
+		if (mesh_net_key_refresh_phase_set(node->net, net_key_idx,
+				KEY_REFRESH_PHASE_TWO) != MESH_STATUS_SUCCESS)
 			return false;
 	}
 
-- 
2.20.1

[PATCH BlueZ 2/6] mesh: provisionee: Handle unknown PDUs

[Isak Westin]

If an unknown PDU is received during provisioning, the provisioning
should fail with "Invalid PDU".
---
 mesh/prov-acceptor.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/mesh/prov-acceptor.c b/mesh/prov-acceptor.c
index f579a143b..ac257b170 100644
--- a/mesh/prov-acceptor.c
+++ b/mesh/prov-acceptor.c
@@ -399,6 +399,12 @@ static void acp_prov_rx(void *user_data, const uint8_t *data, uint16_t len)
 	l_debug("Provisioning packet received type: %2.2x (%u octets)",
 								type, len);
 
+	if (type >= L_ARRAY_SIZE(expected_pdu_size)) {
+		l_error("Unknown PDU type: %2.2x", type);
+		fail.reason = PROV_ERR_INVALID_PDU;
+		goto failure;
+	}
+
 	if (type == prov->previous) {
 		l_error("Ignore repeated %2.2x packet", type);
 		return;
@@ -408,8 +414,7 @@ static void acp_prov_rx(void *user_data, const uint8_t *data, uint16_t len)
 		goto failure;
 	}
 
-	if (type >= L_ARRAY_SIZE(expected_pdu_size) ||
-					len != expected_pdu_size[type]) {
+	if (len != expected_pdu_size[type]) {
 		l_error("Expected PDU size %d, Got %d (type: %2.2x)",
 			len, expected_pdu_size[type], type);
 		fail.reason = PROV_ERR_INVALID_FORMAT;
-- 
2.20.1

[PATCH BlueZ 3/6] mesh: provisionee: Handle failed provisioning

[Isak Westin]

When a provisioning fails, all additionally received PDU should be
unexpected until link is closed by provisioner. See MshPRFv1.0.1 section
5.4.4.
---
 mesh/prov-acceptor.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mesh/prov-acceptor.c b/mesh/prov-acceptor.c
index ac257b170..0cefb2fa9 100644
--- a/mesh/prov-acceptor.c
+++ b/mesh/prov-acceptor.c
@@ -70,6 +70,7 @@ struct mesh_prov_acceptor {
 	uint8_t material;
 	uint8_t expected;
 	int8_t previous;
+	bool failed;
 	struct conf_input conf_inputs;
 	uint8_t calc_key[16];
 	uint8_t salt[16];
@@ -408,7 +409,8 @@ static void acp_prov_rx(void *user_data, const uint8_t *data, uint16_t len)
 	if (type == prov->previous) {
 		l_error("Ignore repeated %2.2x packet", type);
 		return;
-	} else if (type > prov->expected || type < prov->previous) {
+	} else if (prov->failed || type > prov->expected ||
+							type < prov->previous) {
 		l_error("Expected %2.2x, Got:%2.2x", prov->expected, type);
 		fail.reason = PROV_ERR_UNEXPECTED_PDU;
 		goto failure;
@@ -648,6 +650,8 @@ static void acp_prov_rx(void *user_data, const uint8_t *data, uint16_t len)
 failure:
 	fail.opcode = PROV_FAILED;
 	prov_send(prov, &fail, sizeof(fail));
+	prov->failed = true;
+	prov->previous = -1;
 	if (prov->cmplt)
 		prov->cmplt(prov->caller_data, fail.reason, NULL);
 	prov->cmplt = NULL;
@@ -707,6 +711,7 @@ bool acceptor_start(uint8_t num_ele, uint8_t uuid[16],
 	prov->cmplt = complete_cb;
 	prov->ob = l_queue_new();
 	prov->previous = -1;
+	prov->failed = false;
 	prov->out_opcode = PROV_NONE;
 	prov->caller_data = caller_data;
 
-- 
2.20.1

[PATCH BlueZ 4/6] mesh: provisionee: Check prov start parameters

[Isak Westin]

Verify that all parameters in a Provisioning Start PDU are valid, also
compared to the capabilities that has been sent.
---
 mesh/prov-acceptor.c | 57 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/mesh/prov-acceptor.c b/mesh/prov-acceptor.c
index 0cefb2fa9..bf8c573da 100644
--- a/mesh/prov-acceptor.c
+++ b/mesh/prov-acceptor.c
@@ -384,6 +384,47 @@ static void send_rand(struct mesh_prov_acceptor *prov)
 	prov_send(prov, &msg, sizeof(msg));
 }
 
+static bool prov_start_check(struct prov_start *start,
+						struct mesh_net_prov_caps *caps)
+{
+	if (start->algorithm || start->pub_key > 1 || start->auth_method > 3)
+		return false;
+
+	if (start->pub_key && !caps->pub_type)
+		return false;
+
+	switch (start->auth_method) {
+	case 0: /* No OOB */
+		if (start->auth_action != 0 || start->auth_size != 0)
+			return false;
+
+		break;
+
+	case 1: /* Static OOB */
+		if (!caps->static_type || start->auth_action != 0 ||
+							start->auth_size != 0)
+			return false;
+
+		break;
+
+	case 2: /* Output OOB */
+		if (!(caps->output_action & (1 << start->auth_action)) ||
+							start->auth_size == 0)
+			return false;
+
+		break;
+
+	case 3: /* Input OOB */
+		if (!(caps->input_action & (1 << start->auth_action)) ||
+							start->auth_size == 0)
+			return false;
+
+		break;
+	}
+
+	return true;
+}
+
 static void acp_prov_rx(void *user_data, const uint8_t *data, uint16_t len)
 {
 	struct mesh_prov_acceptor *rx_prov = user_data;
@@ -433,22 +474,16 @@ static void acp_prov_rx(void *user_data, const uint8_t *data, uint16_t len)
 		memcpy(&prov->conf_inputs.start, data,
 				sizeof(prov->conf_inputs.start));
 
-		if (prov->conf_inputs.start.algorithm ||
-				prov->conf_inputs.start.pub_key > 1 ||
-				prov->conf_inputs.start.auth_method > 3) {
+		if (!prov_start_check(&prov->conf_inputs.start,
+						&prov->conf_inputs.caps)) {
 			fail.reason = PROV_ERR_INVALID_FORMAT;
 			goto failure;
 		}
 
 		if (prov->conf_inputs.start.pub_key) {
-			if (prov->conf_inputs.caps.pub_type) {
-				/* Prompt Agent for Private Key of OOB */
-				mesh_agent_request_private_key(prov->agent,
-							priv_key_cb, prov);
-			} else {
-				fail.reason = PROV_ERR_INVALID_PDU;
-				goto failure;
-			}
+			/* Prompt Agent for Private Key of OOB */
+			mesh_agent_request_private_key(prov->agent,
+						priv_key_cb, prov);
 		} else {
 			/* Ephemeral Public Key requested */
 			ecc_make_key(prov->conf_inputs.dev_pub_key,
-- 
2.20.1

[PATCH BlueZ 5/6] mesh: Keep canceled SAR data for at least 10 sec

[Isak Westin]

When a SAR transmission has been completed or canceled, the recipent
should store the block authentication values for at least 10 seconds
and ignore new segments with the same values during this period. See
MshPRFv1.0.1 section 3.5.3.4.
---
 mesh/net.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/mesh/net.c b/mesh/net.c
index 379a6e250..e95ae5114 100644
--- a/mesh/net.c
+++ b/mesh/net.c
@@ -46,6 +46,7 @@
 
 #define SEG_TO	2
 #define MSG_TO	60
+#define SAR_DEL	10
 
 #define DEFAULT_TRANSMIT_COUNT		1
 #define DEFAULT_TRANSMIT_INTERVAL	100
@@ -166,6 +167,7 @@ struct mesh_sar {
 	bool segmented;
 	bool frnd;
 	bool frnd_cred;
+	bool delete;
 	uint8_t ttl;
 	uint8_t last_seg;
 	uint8_t key_aid;
@@ -1493,13 +1495,27 @@ static void inseg_to(struct l_timeout *seg_timeout, void *user_data)
 static void inmsg_to(struct l_timeout *msg_timeout, void *user_data)
 {
 	struct mesh_net *net = user_data;
-	struct mesh_sar *sar = l_queue_remove_if(net->sar_in,
+	struct mesh_sar *sar = l_queue_find(net->sar_in,
 			match_msg_timeout, msg_timeout);
 
 	l_timeout_remove(msg_timeout);
 	if (!sar)
 		return;
 
+	if (!sar->delete) {
+		/*
+		 * Incomplete timer expired, cancel SAR and start
+		 * delete timer
+		 */
+		sar->delete = true;
+		l_timeout_remove(sar->seg_timeout);
+		sar->seg_timeout = NULL;
+		sar->msg_timeout = l_timeout_create(SAR_DEL,
+					inmsg_to, net, NULL);
+		return;
+	}
+
+	l_queue_remove(net->sar_in, sar);
 	sar->msg_timeout = NULL;
 	mesh_sar_free(sar);
 }
@@ -1956,7 +1972,9 @@ static bool seg_rxed(struct mesh_net *net, bool frnd, uint32_t iv_index,
 			/* Re-Send ACK for full msg */
 			send_net_ack(net, sar_in, expected);
 			return true;
-		}
+		} else if (sar_in->delete)
+			/* Ignore canceled */
+			return false;
 	} else {
 		uint16_t len = MAX_SEG_TO_LEN(segN);
 
@@ -1996,6 +2014,9 @@ static bool seg_rxed(struct mesh_net *net, bool frnd, uint32_t iv_index,
 		sar_in->len = segN * MAX_SEG_LEN + size;
 
 	if (sar_in->flags == expected) {
+		/* Remove message incomplete timer */
+		l_timeout_remove(sar_in->msg_timeout);
+
 		/* Got it all */
 		send_net_ack(net, sar_in, expected);
 
@@ -2006,6 +2027,11 @@ static bool seg_rxed(struct mesh_net *net, bool frnd, uint32_t iv_index,
 		/* Kill Inter-Seg timeout */
 		l_timeout_remove(sar_in->seg_timeout);
 		sar_in->seg_timeout = NULL;
+
+		/* Start delete timer */
+		sar_in->delete = true;
+		sar_in->msg_timeout = l_timeout_create(SAR_DEL,
+				inmsg_to, net, NULL);
 		return true;
 	}
 
-- 
2.20.1

[PATCH BlueZ 6/6] mesh: Fix msg cache ring buffer

[Isak Westin]

The message cache should be a strict ring buffer, suppressed message
should not move to the front of the queue.
---
 mesh/net.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mesh/net.c b/mesh/net.c
index e95ae5114..8be45e61a 100644
--- a/mesh/net.c
+++ b/mesh/net.c
@@ -1028,12 +1028,11 @@ static bool msg_in_cache(struct mesh_net *net, uint16_t src, uint32_t seq,
 		.mic = mic,
 	};
 
-	msg = l_queue_remove_if(net->msg_cache, match_cache, &tst);
+	msg = l_queue_find(net->msg_cache, match_cache, &tst);
 
 	if (msg) {
 		l_debug("Supressing duplicate %4.4x + %6.6x + %8.8x",
 							src, seq, mic);
-		l_queue_push_head(net->msg_cache, msg);
 		return true;
 	}
 
-- 
2.20.1

RE: Mesh: Fixes for PTS issues

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1049 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=683513

---Test result---

Test Summary:
CheckPatch                    PASS      6.47 seconds
GitLint                       PASS      4.32 seconds
Prep - Setup ELL              PASS      27.73 seconds
Build - Prep                  PASS      0.73 seconds
Build - Configure             PASS      8.76 seconds
Build - Make                  PASS      867.47 seconds
Make Check                    PASS      11.56 seconds
Make Check w/Valgrind         PASS      295.55 seconds
Make Distcheck                PASS      242.61 seconds
Build w/ext ELL - Configure   PASS      8.79 seconds
Build w/ext ELL - Make        PASS      85.61 seconds
Incremental Build w/ patches  PASS      602.70 seconds
Scan Build                    PASS      518.24 seconds



---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ 6/6] mesh: Fix msg cache ring buffer

[Gix, Brian]

Hi Isak,

On Thu, 2022-10-06 at 16:59 +0200, Isak Westin wrote:
> The message cache should be a strict ring buffer, suppressed message
> should not move to the front of the queue.
> ---
>  mesh/net.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/mesh/net.c b/mesh/net.c
> index e95ae5114..8be45e61a 100644
> --- a/mesh/net.c
> +++ b/mesh/net.c
> @@ -1028,12 +1028,11 @@ static bool msg_in_cache(struct mesh_net
> *net, uint16_t src, uint32_t seq,
>                 .mic = mic,
>         };
>  
> -       msg = l_queue_remove_if(net->msg_cache, match_cache, &tst);
> +       msg = l_queue_find(net->msg_cache, match_cache, &tst);
>  
>         if (msg) {
>                 l_debug("Supressing duplicate %4.4x + %6.6x + %8.8x",
>                                                         src, seq,
> mic);
> -               l_queue_push_head(net->msg_cache, msg);

The purpose of this bit of code was to maintain a cache of the X most
recently received packets in the order most recently seen, which was
why the re-ordering took place. Was this causing incorrect behavior, or
are you doing this as a streamline?

>                 return true;
>         }
>  

Re: [PATCH BlueZ 5/6] mesh: Keep canceled SAR data for at least 10 sec

[Gix, Brian]

Hi Isak,

Have you run this patch through any runtime analysis (like valgrind
etc) to ensure it has introduced no memory leaks?

I am particularily concerned with any l_timeout_remove() calls that
don't immediately set the freed timer pointer to NULL, and any new
l_timeout_create() calls that are not preceded with a check that
there is not already a valid pointer occupying the seg_timeout or
msg_timeout members. 

On Thu, 2022-10-06 at 16:59 +0200, Isak Westin wrote:
> When a SAR transmission has been completed or canceled, the recipent
> should store the block authentication values for at least 10 seconds
> and ignore new segments with the same values during this period. See
> MshPRFv1.0.1 section 3.5.3.4.
> ---
>  mesh/net.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/mesh/net.c b/mesh/net.c
> index 379a6e250..e95ae5114 100644
> --- a/mesh/net.c
> +++ b/mesh/net.c
> @@ -46,6 +46,7 @@
>  
>  #define SEG_TO 2
>  #define MSG_TO 60
> +#define SAR_DEL        10
>  
>  #define DEFAULT_TRANSMIT_COUNT         1
>  #define DEFAULT_TRANSMIT_INTERVAL      100
> @@ -166,6 +167,7 @@ struct mesh_sar {
>         bool segmented;
>         bool frnd;
>         bool frnd_cred;
> +       bool delete;
>         uint8_t ttl;
>         uint8_t last_seg;
>         uint8_t key_aid;
> @@ -1493,13 +1495,27 @@ static void inseg_to(struct l_timeout
> *seg_timeout, void *user_data)
>  static void inmsg_to(struct l_timeout *msg_timeout, void *user_data)
>  {
>         struct mesh_net *net = user_data;
> -       struct mesh_sar *sar = l_queue_remove_if(net->sar_in,
> +       struct mesh_sar *sar = l_queue_find(net->sar_in,
>                         match_msg_timeout, msg_timeout);
>  
>         l_timeout_remove(msg_timeout);
>         if (!sar)
>                 return;
>  
> +       if (!sar->delete) {
> +               /*
> +                * Incomplete timer expired, cancel SAR and start
> +                * delete timer
> +                */
> +               sar->delete = true;
> +               l_timeout_remove(sar->seg_timeout);
> +               sar->seg_timeout = NULL;
> +               sar->msg_timeout = l_timeout_create(SAR_DEL,
> +                                       inmsg_to, net, NULL);
> +               return;
> +       }
> +
> +       l_queue_remove(net->sar_in, sar);
>         sar->msg_timeout = NULL;
>         mesh_sar_free(sar);
>  }
> @@ -1956,7 +1972,9 @@ static bool seg_rxed(struct mesh_net *net, bool
> frnd, uint32_t iv_index,
>                         /* Re-Send ACK for full msg */
>                         send_net_ack(net, sar_in, expected);
>                         return true;
> -               }
> +               } else if (sar_in->delete)
> +                       /* Ignore canceled */
> +                       return false;
>         } else {
>                 uint16_t len = MAX_SEG_TO_LEN(segN);
>  
> @@ -1996,6 +2014,9 @@ static bool seg_rxed(struct mesh_net *net, bool
> frnd, uint32_t iv_index,
>                 sar_in->len = segN * MAX_SEG_LEN + size;
>  
>         if (sar_in->flags == expected) {
> +               /* Remove message incomplete timer */
> +               l_timeout_remove(sar_in->msg_timeout);
> +
>                 /* Got it all */
>                 send_net_ack(net, sar_in, expected);
>  
> @@ -2006,6 +2027,11 @@ static bool seg_rxed(struct mesh_net *net,
> bool frnd, uint32_t iv_index,
>                 /* Kill Inter-Seg timeout */
>                 l_timeout_remove(sar_in->seg_timeout);
>                 sar_in->seg_timeout = NULL;
> +
> +               /* Start delete timer */
> +               sar_in->delete = true;
> +               sar_in->msg_timeout = l_timeout_create(SAR_DEL,
> +                               inmsg_to, net, NULL);
>                 return true;
>         }
>  

Re: [PATCH BlueZ 0/6] Mesh: Fixes for PTS issues

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluez.git (master)
by Brian Gix <brian.gix@intel.com>:

On Thu,  6 Oct 2022 16:59:21 +0200 you wrote:
> Hi,
> 
> Here is the last patch set with fixes for issues found during PTS
> testing. This covers following tests:
>  - MESH/NODE/PROV/*
>  - MESH/NODE/TNPT/*
>  - MESH/NODE/RLY/*
> 
> [...]

Here is the summary with links:
  - [BlueZ,1/6] mesh: Update Key Refresh flag after provision
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=95bf980b015e
  - [BlueZ,2/6] mesh: provisionee: Handle unknown PDUs
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=c1f1a49aeb15
  - [BlueZ,3/6] mesh: provisionee: Handle failed provisioning
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=77da94eb7a8c
  - [BlueZ,4/6] mesh: provisionee: Check prov start parameters
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=838ddc931263
  - [BlueZ,5/6] mesh: Keep canceled SAR data for at least 10 sec
    (no matching commit)
  - [BlueZ,6/6] mesh: Fix msg cache ring buffer
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH BlueZ 0/6] Mesh: Fixes for PTS issues

[Gix, Brian]

Hi Isak,

Patches 1-4 of patchset applied.

Patches 5 and 6 have outstanding comments.

Thanks,
--Brian

On Thu, 2022-10-06 at 16:59 +0200, Isak Westin wrote:
> Hi,
> 
> Here is the last patch set with fixes for issues found during PTS
> testing. This covers following tests:
>  - MESH/NODE/PROV/*
>  - MESH/NODE/TNPT/*
>  - MESH/NODE/RLY/*
> 
> Best regards,
> Isak
> 
> Isak Westin (6):
>   mesh: Update Key Refresh flag after provision
>   mesh: provisionee: Handle unknown PDUs
>   mesh: provisionee: Handle failed provisioning
>   mesh: provisionee: Check prov start parameters
>   mesh: Keep canceled SAR data for at least 10 sec
>   mesh: Fix msg cache ring buffer
> 
>  mesh/net.c           | 33 +++++++++++++++++---
>  mesh/node.c          |  4 +--
>  mesh/prov-acceptor.c | 73 +++++++++++++++++++++++++++++++++++-------
> --
>  3 files changed, 90 insertions(+), 20 deletions(-)
> 

RE: [PATCH BlueZ 6/6] mesh: Fix msg cache ring buffer

[Isak Westin]

Hi Brian,

> Hi Isak,
>
> On Thu, 2022-10-06 at 16:59 +0200, Isak Westin wrote:
> > The message cache should be a strict ring buffer, suppressed message
> > should not move to the front of the queue.
> > ---
> >  mesh/net.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/mesh/net.c b/mesh/net.c
> > index e95ae5114..8be45e61a 100644
> > --- a/mesh/net.c
> > +++ b/mesh/net.c
> > @@ -1028,12 +1028,11 @@ static bool msg_in_cache(struct mesh_net *net,
> > uint16_t src, uint32_t seq,
> >                 .mic = mic,
> >         };
> >
> > -       msg = l_queue_remove_if(net->msg_cache, match_cache, &tst);
> > +       msg = l_queue_find(net->msg_cache, match_cache, &tst);
> >
> >         if (msg) {
> >                 l_debug("Supressing duplicate %4.4x + %6.6x + %8.8x",
> >                                                         src, seq,
> > mic);
> > -               l_queue_push_head(net->msg_cache, msg);
>
> The purpose of this bit of code was to maintain a cache of the X most recently
> received packets in the order most recently seen, which was why the re-ordering
> took place. Was this causing incorrect behavior, or are you doing this as a
> streamline?

PTS is testing specifically that the message cache is a ring buffer (in test
MESH/NODE/RLY/BV-02-C).

The test goes like this (specified cache size = 4):

Send msg 1      <-- relay expected
Send msg 2      <-- relay expected
Send msg 3      <-- relay expected
Send msg 4      <-- relay expected
Send msg 1      <-- relay *not* expected
Send msg 5      <-- relay expected
Send msg 1      <-- relay expected

So the test failed with current behaviour. That is however the only reason for the patch.

>
> >                 return true;
> >         }
> >
>
>

RE: [PATCH BlueZ 5/6] mesh: Keep canceled SAR data for at least 10 sec

[Isak Westin]

Hi Brian,

> Hi Isak,
>
> Have you run this patch through any runtime analysis (like valgrind
> etc) to ensure it has introduced no memory leaks?
>
> I am particularily concerned with any l_timeout_remove() calls that don't immediately set the freed timer pointer to NULL, and any new
> l_timeout_create() calls that are not preceded with a check that there is not already a valid pointer occupying the seg_timeout or msg_timeout members.
>

I tested it with valgrind and found no memory leaks. However, it is perhaps anyway a better practice to use l_timeout_modify() ?
If so, I will update the patch.

> On Thu, 2022-10-06 at 16:59 +0200, Isak Westin wrote:
> > When a SAR transmission has been completed or canceled, the recipent
> > should store the block authentication values for at least 10 seconds
> > and ignore new segments with the same values during this period. See
> > MshPRFv1.0.1 section 3.5.3.4.
> > ---
> >  mesh/net.c | 30 ++++++++++++++++++++++++++++--
> >  1 file changed, 28 insertions(+), 2 deletions(-)
> >
> > diff --git a/mesh/net.c b/mesh/net.c
> > index 379a6e250..e95ae5114 100644
> > --- a/mesh/net.c
> > +++ b/mesh/net.c
> > @@ -46,6 +46,7 @@
> >
> >  #define SEG_TO 2
> >  #define MSG_TO 60
> > +#define SAR_DEL        10
> >
> >  #define DEFAULT_TRANSMIT_COUNT         1
> >  #define DEFAULT_TRANSMIT_INTERVAL      100
> > @@ -166,6 +167,7 @@ struct mesh_sar {
> >         bool segmented;
> >         bool frnd;
> >         bool frnd_cred;
> > +       bool delete;
> >         uint8_t ttl;
> >         uint8_t last_seg;
> >         uint8_t key_aid;
> > @@ -1493,13 +1495,27 @@ static void inseg_to(struct l_timeout
> > *seg_timeout, void *user_data)  static void inmsg_to(struct l_timeout
> > *msg_timeout, void *user_data)  {
> >         struct mesh_net *net = user_data;
> > -       struct mesh_sar *sar = l_queue_remove_if(net->sar_in,
> > +       struct mesh_sar *sar = l_queue_find(net->sar_in,
> >                         match_msg_timeout, msg_timeout);
> >
> >         l_timeout_remove(msg_timeout);
> >         if (!sar)
> >                 return;
> >
> > +       if (!sar->delete) {
> > +               /*
> > +                * Incomplete timer expired, cancel SAR and start
> > +                * delete timer
> > +                */
> > +               sar->delete = true;
> > +               l_timeout_remove(sar->seg_timeout);
> > +               sar->seg_timeout = NULL;
> > +               sar->msg_timeout = l_timeout_create(SAR_DEL,
> > +                                       inmsg_to, net, NULL);
> > +               return;
> > +       }
> > +
> > +       l_queue_remove(net->sar_in, sar);
> >         sar->msg_timeout = NULL;
> >         mesh_sar_free(sar);
> >  }
> > @@ -1956,7 +1972,9 @@ static bool seg_rxed(struct mesh_net *net, bool
> > frnd, uint32_t iv_index,
> >                         /* Re-Send ACK for full msg */
> >                         send_net_ack(net, sar_in, expected);
> >                         return true;
> > -               }
> > +               } else if (sar_in->delete)
> > +                       /* Ignore canceled */
> > +                       return false;
> >         } else {
> >                 uint16_t len = MAX_SEG_TO_LEN(segN);
> >
> > @@ -1996,6 +2014,9 @@ static bool seg_rxed(struct mesh_net *net, bool
> > frnd, uint32_t iv_index,
> >                 sar_in->len = segN * MAX_SEG_LEN + size;
> >
> >         if (sar_in->flags == expected) {
> > +               /* Remove message incomplete timer */
> > +               l_timeout_remove(sar_in->msg_timeout);
> > +
> >                 /* Got it all */
> >                 send_net_ack(net, sar_in, expected);
> >
> > @@ -2006,6 +2027,11 @@ static bool seg_rxed(struct mesh_net *net, bool
> > frnd, uint32_t iv_index,
> >                 /* Kill Inter-Seg timeout */
> >                 l_timeout_remove(sar_in->seg_timeout);
> >                 sar_in->seg_timeout = NULL;
> > +
> > +               /* Start delete timer */
> > +               sar_in->delete = true;
> > +               sar_in->msg_timeout = l_timeout_create(SAR_DEL,
> > +                               inmsg_to, net, NULL);
> >                 return true;
> >         }
> >
>
>

Re: [PATCH BlueZ 5/6] mesh: Keep canceled SAR data for at least 10 sec

[Gix, Brian]

Hi Isak,

On Fri, 2022-10-07 at 13:33 +0000, Isak Westin wrote:
> Hi Brian,
> 
> > Hi Isak,
> > 
> > Have you run this patch through any runtime analysis (like valgrind
> > etc) to ensure it has introduced no memory leaks?
> > 
> > I am particularily concerned with any l_timeout_remove() calls that
> > don't immediately set the freed timer pointer to NULL, and any new
> > l_timeout_create() calls that are not preceded with a check that
> > there is not already a valid pointer occupying the seg_timeout or
> > msg_timeout members.
> > 
> 
> I tested it with valgrind and found no memory leaks. However, it is
> perhaps anyway a better practice to use l_timeout_modify() ?
> If so, I will update the patch.

What's important to remember about the l_timeout* functions is that
they do not clean-up after themselves, you must remove them when you
are done, and you need to be careful to not double-free.

You can free and then create if you are more comfortable with that then
l_timeout_modify, but follow the rule of thumb to set the pointers to
NULL after you have freed the timer, and make sure you free the timers
before creating a new one. And free the timers that have fired which
you do not intend to restart. Many of the timers in the SAR system 
never trigger if the messages are flowing as they should, and so some
potential memory leaks don't get caught by valgrind *unless* an
"abnormal" timer fire occurs.  We've had to address a lot of those.

> 
> > On Thu, 2022-10-06 at 16:59 +0200, Isak Westin wrote:
> > > When a SAR transmission has been completed or canceled, the
> > > recipent
> > > should store the block authentication values for at least 10
> > > seconds
> > > and ignore new segments with the same values during this period.
> > > See
> > > MshPRFv1.0.1 section 3.5.3.4.
> > > ---
> > >  mesh/net.c | 30 ++++++++++++++++++++++++++++--
> > >  1 file changed, 28 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/mesh/net.c b/mesh/net.c
> > > index 379a6e250..e95ae5114 100644
> > > --- a/mesh/net.c
> > > +++ b/mesh/net.c
> > > @@ -46,6 +46,7 @@
> > > 
> > >  #define SEG_TO 2
> > >  #define MSG_TO 60
> > > +#define SAR_DEL        10
> > > 
> > >  #define DEFAULT_TRANSMIT_COUNT         1
> > >  #define DEFAULT_TRANSMIT_INTERVAL      100
> > > @@ -166,6 +167,7 @@ struct mesh_sar {
> > >         bool segmented;
> > >         bool frnd;
> > >         bool frnd_cred;
> > > +       bool delete;
> > >         uint8_t ttl;
> > >         uint8_t last_seg;
> > >         uint8_t key_aid;
> > > @@ -1493,13 +1495,27 @@ static void inseg_to(struct l_timeout
> > > *seg_timeout, void *user_data)  static void inmsg_to(struct
> > > l_timeout
> > > *msg_timeout, void *user_data)  {
> > >         struct mesh_net *net = user_data;
> > > -       struct mesh_sar *sar = l_queue_remove_if(net->sar_in,
> > > +       struct mesh_sar *sar = l_queue_find(net->sar_in,
> > >                         match_msg_timeout, msg_timeout);
> > > 
> > >         l_timeout_remove(msg_timeout);
> > >         if (!sar)
> > >                 return;
> > > 
> > > +       if (!sar->delete) {
> > > +               /*
> > > +                * Incomplete timer expired, cancel SAR and start
> > > +                * delete timer
> > > +                */
> > > +               sar->delete = true;
> > > +               l_timeout_remove(sar->seg_timeout);
> > > +               sar->seg_timeout = NULL;
> > > +               sar->msg_timeout = l_timeout_create(SAR_DEL,
> > > +                                       inmsg_to, net, NULL);
> > > +               return;
> > > +       }
> > > +
> > > +       l_queue_remove(net->sar_in, sar);
> > >         sar->msg_timeout = NULL;
> > >         mesh_sar_free(sar);
> > >  }
> > > @@ -1956,7 +1972,9 @@ static bool seg_rxed(struct mesh_net *net,
> > > bool
> > > frnd, uint32_t iv_index,
> > >                         /* Re-Send ACK for full msg */
> > >                         send_net_ack(net, sar_in, expected);
> > >                         return true;
> > > -               }
> > > +               } else if (sar_in->delete)
> > > +                       /* Ignore canceled */
> > > +                       return false;
> > >         } else {
> > >                 uint16_t len = MAX_SEG_TO_LEN(segN);
> > > 
> > > @@ -1996,6 +2014,9 @@ static bool seg_rxed(struct mesh_net *net,
> > > bool
> > > frnd, uint32_t iv_index,
> > >                 sar_in->len = segN * MAX_SEG_LEN + size;
> > > 
> > >         if (sar_in->flags == expected) {
> > > +               /* Remove message incomplete timer */
> > > +               l_timeout_remove(sar_in->msg_timeout);
> > > +
> > >                 /* Got it all */
> > >                 send_net_ack(net, sar_in, expected);
> > > 
> > > @@ -2006,6 +2027,11 @@ static bool seg_rxed(struct mesh_net *net,
> > > bool
> > > frnd, uint32_t iv_index,
> > >                 /* Kill Inter-Seg timeout */
> > >                 l_timeout_remove(sar_in->seg_timeout);
> > >                 sar_in->seg_timeout = NULL;
> > > +
> > > +               /* Start delete timer */
> > > +               sar_in->delete = true;
> > > +               sar_in->msg_timeout = l_timeout_create(SAR_DEL,
> > > +                               inmsg_to, net, NULL);
> > >                 return true;
> > >         }
> > > 
> > 
> > 

Re: [PATCH BlueZ 6/6] mesh: Fix msg cache ring buffer

[Gix, Brian]

6th patch of 6 applied. Still considering patch 5.

On Thu, 2022-10-06 at 16:59 +0200, Isak Westin wrote:
> The message cache should be a strict ring buffer, suppressed message
> should not move to the front of the queue.
> ---
>  mesh/net.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/mesh/net.c b/mesh/net.c
> index e95ae5114..8be45e61a 100644
> --- a/mesh/net.c
> +++ b/mesh/net.c
> @@ -1028,12 +1028,11 @@ static bool msg_in_cache(struct mesh_net
> *net, uint16_t src, uint32_t seq,
>                 .mic = mic,
>         };
>  
> -       msg = l_queue_remove_if(net->msg_cache, match_cache, &tst);
> +       msg = l_queue_find(net->msg_cache, match_cache, &tst);
>  
>         if (msg) {
>                 l_debug("Supressing duplicate %4.4x + %6.6x + %8.8x",
>                                                         src, seq,
> mic);
> -               l_queue_push_head(net->msg_cache, msg);
>                 return true;
>         }
>  

Re: [PATCH BlueZ 0/6] Mesh: Fixes for PTS issues

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluez.git (master)
by Brian Gix <brian.gix@intel.com>:

On Thu,  6 Oct 2022 16:59:21 +0200 you wrote:
> Hi,
> 
> Here is the last patch set with fixes for issues found during PTS
> testing. This covers following tests:
>  - MESH/NODE/PROV/*
>  - MESH/NODE/TNPT/*
>  - MESH/NODE/RLY/*
> 
> [...]

Here is the summary with links:
  - [BlueZ,1/6] mesh: Update Key Refresh flag after provision
    (no matching commit)
  - [BlueZ,2/6] mesh: provisionee: Handle unknown PDUs
    (no matching commit)
  - [BlueZ,3/6] mesh: provisionee: Handle failed provisioning
    (no matching commit)
  - [BlueZ,4/6] mesh: provisionee: Check prov start parameters
    (no matching commit)
  - [BlueZ,5/6] mesh: Keep canceled SAR data for at least 10 sec
    (no matching commit)
  - [BlueZ,6/6] mesh: Fix msg cache ring buffer
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=dabf32b313c1

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

RE: [PATCH BlueZ 5/6] mesh: Keep canceled SAR data for at least 10 sec

[Isak Westin]

Hi Brian,

> Hi Isak,
>
> On Fri, 2022-10-07 at 13:33 +0000, Isak Westin wrote:
> > Hi Brian,
> >
> > > Hi Isak,
> > >
> > > Have you run this patch through any runtime analysis (like valgrind
> > > etc) to ensure it has introduced no memory leaks?
> > >
> > > I am particularily concerned with any l_timeout_remove() calls that
> > > don't immediately set the freed timer pointer to NULL, and any new
> > > l_timeout_create() calls that are not preceded with a check that
> > > there is not already a valid pointer occupying the seg_timeout or
> > > msg_timeout members.
> > >
> >
> > I tested it with valgrind and found no memory leaks. However, it is
> > perhaps anyway a better practice to use l_timeout_modify() ?
> > If so, I will update the patch.
>
> What's important to remember about the l_timeout* functions is
> that they do not clean-up after themselves, you must remove them
> when you are done, and you need to be careful to not double-free.
>
> You can free and then create if you are more comfortable with that
> then l_timeout_modify, but follow the rule of thumb to set the
> pointers to NULL after you have freed the timer, and make sure
> you free the timers before creating a new one. And free the timers
> that have fired which you do not intend to restart. Many of the
> timers in the SAR system never trigger if the messages are flowing
> as they should, and so some potential memory leaks don't get
> caught by valgrind *unless* an "abnormal" timer fire occurs.
> We've had to address a lot of those.
>

I submitted a v3 (fixed some spelling from v2) patch now where I changed to use l_timeout_modify instead. That makes more sense imo, since the timeout is in fact being reused.
I tested this patch with valgrind in PTS but also in "normal" operation with a running application. If there is something additional you would wish me to test, please let me know.

> >
> > > On Thu, 2022-10-06 at 16:59 +0200, Isak Westin wrote:
> > > > When a SAR transmission has been completed or canceled, the
> > > > recipent should store the block authentication values for at least
> > > > 10 seconds and ignore new segments with the same values during
> > > > this period.
> > > > See
> > > > MshPRFv1.0.1 section 3.5.3.4.
> > > > ---
> > > >  mesh/net.c | 30 ++++++++++++++++++++++++++++--
> > > >  1 file changed, 28 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/mesh/net.c b/mesh/net.c index 379a6e250..e95ae5114
> > > > 100644
> > > > --- a/mesh/net.c
> > > > +++ b/mesh/net.c
> > > > @@ -46,6 +46,7 @@
> > > >
> > > >  #define SEG_TO 2
> > > >  #define MSG_TO 60
> > > > +#define SAR_DEL        10
> > > >
> > > >  #define DEFAULT_TRANSMIT_COUNT         1
> > > >  #define DEFAULT_TRANSMIT_INTERVAL      100
> > > > @@ -166,6 +167,7 @@ struct mesh_sar {
> > > >         bool segmented;
> > > >         bool frnd;
> > > >         bool frnd_cred;
> > > > +       bool delete;
> > > >         uint8_t ttl;
> > > >         uint8_t last_seg;
> > > >         uint8_t key_aid;
> > > > @@ -1493,13 +1495,27 @@ static void inseg_to(struct l_timeout
> > > > *seg_timeout, void *user_data)  static void inmsg_to(struct
> > > > l_timeout *msg_timeout, void *user_data)  {
> > > >         struct mesh_net *net = user_data;
> > > > -       struct mesh_sar *sar = l_queue_remove_if(net->sar_in,
> > > > +       struct mesh_sar *sar = l_queue_find(net->sar_in,
> > > >                         match_msg_timeout, msg_timeout);
> > > >
> > > >         l_timeout_remove(msg_timeout);
> > > >         if (!sar)
> > > >                 return;
> > > >
> > > > +       if (!sar->delete) {
> > > > +               /*
> > > > +                * Incomplete timer expired, cancel SAR and start
> > > > +                * delete timer
> > > > +                */
> > > > +               sar->delete = true;
> > > > +               l_timeout_remove(sar->seg_timeout);
> > > > +               sar->seg_timeout = NULL;
> > > > +               sar->msg_timeout = l_timeout_create(SAR_DEL,
> > > > +                                       inmsg_to, net, NULL);
> > > > +               return;
> > > > +       }
> > > > +
> > > > +       l_queue_remove(net->sar_in, sar);
> > > >         sar->msg_timeout = NULL;
> > > >         mesh_sar_free(sar);
> > > >  }
> > > > @@ -1956,7 +1972,9 @@ static bool seg_rxed(struct mesh_net *net,
> > > > bool frnd, uint32_t iv_index,
> > > >                         /* Re-Send ACK for full msg */
> > > >                         send_net_ack(net, sar_in, expected);
> > > >                         return true;
> > > > -               }
> > > > +               } else if (sar_in->delete)
> > > > +                       /* Ignore canceled */
> > > > +                       return false;
> > > >         } else {
> > > >                 uint16_t len = MAX_SEG_TO_LEN(segN);
> > > >
> > > > @@ -1996,6 +2014,9 @@ static bool seg_rxed(struct mesh_net *net,
> > > > bool frnd, uint32_t iv_index,
> > > >                 sar_in->len = segN * MAX_SEG_LEN + size;
> > > >
> > > >         if (sar_in->flags == expected) {
> > > > +               /* Remove message incomplete timer */
> > > > +               l_timeout_remove(sar_in->msg_timeout);
> > > > +
> > > >                 /* Got it all */
> > > >                 send_net_ack(net, sar_in, expected);
> > > >
> > > > @@ -2006,6 +2027,11 @@ static bool seg_rxed(struct mesh_net *net,
> > > > bool frnd, uint32_t iv_index,
> > > >                 /* Kill Inter-Seg timeout */
> > > >                 l_timeout_remove(sar_in->seg_timeout);
> > > >                 sar_in->seg_timeout = NULL;
> > > > +
> > > > +               /* Start delete timer */
> > > > +               sar_in->delete = true;
> > > > +               sar_in->msg_timeout = l_timeout_create(SAR_DEL,
> > > > +                               inmsg_to, net, NULL);
> > > >                 return true;
> > > >         }
> > > >
> > >
> > >
>
>