[PATCH BlueZ] mesh: Fix IV recovery

[PATCH BlueZ] mesh: Fix IV recovery

[Rafał Gajda]

This patch fixes saving IV received in SNB to storage.                                              
                                                                                                    
Previously after creating new node with IV 0 (or loading node with                                  
IV > 0 but after long inactivity) first received SNB should update IV                               
(and reset sequence number to 0 if necessary).                                                      
                                                                                                    
The bug would prevent new IV being saved in storage                                                 
which resulted in sequence number being set to 0                                                    
on first SNB received after every daemon reset                                                      
but IV never being updated.                
---
 mesh/net.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/mesh/net.c b/mesh/net.c
index 5be95086d..1598933d4 100644
--- a/mesh/net.c
+++ b/mesh/net.c
@@ -2706,14 +2706,7 @@ static void update_kr_state(struct mesh_subnet *subnet, bool kr, uint32_t id)
 static void update_iv_ivu_state(struct mesh_net *net, uint32_t iv_index,
 								bool ivu)
 {
-	uint32_t local_iv_index;
-	bool local_ivu;
-
-	/* Save original settings to differentiate what has changed */
-	local_iv_index = net->iv_index;
-	local_ivu = net->iv_update;
-
-	if ((iv_index - ivu) > (local_iv_index - local_ivu)) {
+	if ((iv_index - ivu) > (net->iv_index - net->iv_update)) {
 		/* Don't accept IV_Index changes when performing SAR Out */
 		if (l_queue_length(net->sar_out))
 			return;
@@ -2737,26 +2730,27 @@ static void update_iv_ivu_state(struct mesh_net *net, uint32_t iv_index,
 		}
 	} else if (ivu) {
 		/* Ignore beacons with IVU if they come too soon */
-		if (!local_ivu && net->iv_upd_state == IV_UPD_NORMAL_HOLD) {
+		if (!net->iv_update &&
+				net->iv_upd_state == IV_UPD_NORMAL_HOLD) {
 			l_error("Update attempted too soon");
 			return;
 		}
 
-		if (!local_ivu) {
+		if (!net->iv_update) {
 			l_info("iv_upd_state = IV_UPD_UPDATING");
 			net->iv_upd_state = IV_UPD_UPDATING;
 			net->iv_update_timeout = l_timeout_create(
 					IV_IDX_UPD_MIN, iv_upd_to, net, NULL);
 		}
-	} else if (local_ivu) {
+	} else if (net->iv_update) {
 		l_error("IVU clear attempted too soon");
 		return;
 	}
 
-	if ((iv_index - ivu) > (local_iv_index - local_ivu))
+	if ((iv_index - ivu) > (net->iv_index - net->iv_update))
 		mesh_net_set_seq_num(net, 0);
 
-	if (ivu != net->iv_update || local_iv_index != net->iv_index) {
+	if (ivu != net->iv_update || iv_index != net->iv_index) {
 		struct mesh_config *cfg = node_config_get(net->node);
 
 		mesh_config_write_iv_index(cfg, iv_index, ivu);
-- 
2.22.0

Re: [PATCH BlueZ] mesh: Fix IV recovery

[Gix, Brian]

Hi Rafał,

On Wed, 2020-01-08 at 10:16 +0100, Rafał Gajda wrote:
> This patch fixes saving IV received in SNB to storage.                                              
>                                                                                                     
> Previously after creating new node with IV 0 (or loading node with                                  
> IV > 0 but after long inactivity) first received SNB should update IV                               
> (and reset sequence number to 0 if necessary).                                                      
>                                                                                                     
> The bug would prevent new IV being saved in storage                                                 
> which resulted in sequence number being set to 0                                                    
> on first SNB received after every daemon reset                                                      
> but IV never being updated.                
> ---
>  mesh/net.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/mesh/net.c b/mesh/net.c
> index 5be95086d..1598933d4 100644
> --- a/mesh/net.c
> +++ b/mesh/net.c
> 

<...>

> @@ -2737,26 +2730,27 @@ static void update_iv_ivu_state(struct mesh_net *net, uint32_t iv_index,
>  		}
>  	} else if (ivu) {
>  		/* Ignore beacons with IVU if they come too soon */
> -		if (!local_ivu && net->iv_upd_state == IV_UPD_NORMAL_HOLD) {
> +		if (!net->iv_update &&
> +				net->iv_upd_state == IV_UPD_NORMAL_HOLD) {
>  			l_error("Update attempted too soon");
>  			return;
>  		}
>  
> -		if (!local_ivu) {
> +		if (!net->iv_update) {
>  			l_info("iv_upd_state = IV_UPD_UPDATING");
>  			net->iv_upd_state = IV_UPD_UPDATING;
>  			net->iv_update_timeout = l_timeout_create(
>  					IV_IDX_UPD_MIN, iv_upd_to, net, NULL);
>  		}
> -	} else if (local_ivu) {
> +	} else if (net->iv_update) {
>  		l_error("IVU clear attempted too soon");
>  		return;
>  	}
>  
> -	if ((iv_index - ivu) > (local_iv_index - local_ivu))
> +	if ((iv_index - ivu) > (net->iv_index - net->iv_update))
>  		mesh_net_set_seq_num(net, 0);
>  
> -	if (ivu != net->iv_update || local_iv_index != net->iv_index) {
> +	if (ivu != net->iv_update || iv_index != net->iv_index) {

Would it be fair to say that this is the *only* change to the code that fixes the logic that was
malfunctioning?

If so, I would like to shrink this patch to just:

-	if (ivu != net->iv_update || local_iv_index != net->iv_index) {
+	if (ivu != local_ivu || iv_index != local_iv_index) {


or, breaking this into a 2-patch patchset, where one of the patches removes the local caching of ivu and ivi,
and the second patch fixes the actual bug.

>  		struct mesh_config *cfg = node_config_get(net->node);
>  
>  		mesh_config_write_iv_index(cfg, iv_index, ivu);

Re: [PATCH BlueZ] mesh: Fix IV recovery

[Michał Lowas-Rzechonek]

Brian, Rafał,

On 01/08, Gix, Brian wrote:
> > -	if (ivu != net->iv_update || local_iv_index != net->iv_index) {
> > +	if (ivu != net->iv_update || iv_index != net->iv_index) {
> 
> Would it be fair to say that this is the *only* change to the code that fixes the logic that was
> malfunctioning?

That's correct.

> 
> If so, I would like to shrink this patch to just:
> 
> -	if (ivu != net->iv_update || local_iv_index != net->iv_index) {
> +	if (ivu != local_ivu || iv_index != local_iv_index) {
>
> or, breaking this into a 2-patch patchset, where one of the patches removes the local caching of ivu and ivi,
> and the second patch fixes the actual bug.

Ok, let's split this into 2 separate patches. I still think this "local
caching" was the underlying cause of the mistake.

regards
-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

Re: [PATCH BlueZ] mesh: Fix IV recovery

[Gix, Brian]

On Wed, 2020-01-08 at 19:17 +0100, Michał Lowas-Rzechonek wrote:
> Brian, Rafał,
> 
> On 01/08, Gix, Brian wrote:
> > > -	if (ivu != net->iv_update || local_iv_index != net->iv_index) {
> > > +	if (ivu != net->iv_update || iv_index != net->iv_index) {
> > 
> > Would it be fair to say that this is the *only* change to the code that fixes the logic that was
> > malfunctioning?
> 
> That's correct.
> 
> > If so, I would like to shrink this patch to just:
> > 
> > -	if (ivu != net->iv_update || local_iv_index != net->iv_index) {
> > +	if (ivu != local_ivu || iv_index != local_iv_index) {
> > 
> > or, breaking this into a 2-patch patchset, where one of the patches removes the local caching of ivu and
> > ivi,
> > and the second patch fixes the actual bug.
> 
> Ok, let's split this into 2 separate patches. I still think this "local
> caching" was the underlying cause of the mistake.

It probably makes more sense for the *first* patch of the set to fix the logic error,
and with the second patch, remove the local caching...  Otherwise the cache removal
patch will include the absurdity of comparing net->iv_index against itself. 

> 
> regards