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
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
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
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
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
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;
> }
>
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(-)
>
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;
> }
>
Hello:
This series was applied to bluetooth/bluez.git (master)
by Brian Gix <[email protected]>:
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
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;
> > }
> >
>
>
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;
> > }
> >
>
>
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;
> > > }
> > >
> >
> >
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;
> }
>
Hello:
This series was applied to bluetooth/bluez.git (master)
by Brian Gix <[email protected]>:
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
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;
> > > > }
> > > >
> > >
> > >
>
>