Re: [tipc-discussion] [PATCH v1 1/1] tipc: fix unexpected link reset due to discovery messages

[Jon M. <jm...@re...>]

On 1/3/23 21:42, Tung Nguyen wrote:
> This unexpected behavior is observed:
>
> node 1                    | node 2
> ------                    | ------
> link is established       | link is established
> reboot                    | link is reset
> up                        | send discovery message
> receive discovery message |
> link is established       | link is established
> send discovery message    |
>                            | receive discovery message
>                            | link is reset (unexpected)
>                            | send reset message
> link is reset             |
>
> It is due to delayed re-discovery as described in function
> tipc_node_check_dest(): "this link endpoint has already reset
> and re-established contact with the peer, before receiving a
> discovery message from that node."
>
> However, commit 598411d70f85 has changed the condition for calling
> tipc_node_link_down() which was the acceptance of new media address.
>
> This commit fixes this by restoring the old and correct behavior.
>
> Fixes: 598411d70f85 ("tipc: make link implementation independent from struct tipc_bearer")
> Signed-off-by: Tung Nguyen <tun...@de...>
> ---
>   net/tipc/node.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index 49ddc484c4fe..5e000fde8067 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -1179,8 +1179,9 @@ void tipc_node_check_dest(struct net *net, u32 addr,
>   	bool addr_match = false;
>   	bool sign_match = false;
>   	bool link_up = false;
> +	bool link_is_reset = false;
>   	bool accept_addr = false;
> -	bool reset = true;
> +	bool reset = false;
>   	char *if_name;
>   	unsigned long intv;
>   	u16 session;
> @@ -1200,14 +1201,14 @@ void tipc_node_check_dest(struct net *net, u32 addr,
>   	/* Prepare to validate requesting node's signature and media address */
>   	l = le->link;
>   	link_up = l && tipc_link_is_up(l);
> +	link_is_reset = l && tipc_link_is_reset(l);
>   	addr_match = l && !memcmp(&le->maddr, maddr, sizeof(*maddr));
>   	sign_match = (signature == n->signature);
>   
>   	/* These three flags give us eight permutations: */
>   
>   	if (sign_match && addr_match && link_up) {
> -		/* All is fine. Do nothing. */
> -		reset = false;
> +		/* All is fine. Ignore requests. */
>   		/* Peer node is not a container/local namespace */
>   		if (!n->peer_hash_mix)
>   			n->peer_hash_mix = hash_mixes;
> @@ -1232,6 +1233,7 @@ void tipc_node_check_dest(struct net *net, u32 addr,
>   		 */
>   		accept_addr = true;
>   		*respond = true;
> +		reset = true;
>   	} else if (!sign_match && addr_match && link_up) {
>   		/* Peer node rebooted. Two possibilities:
>   		 *  - Delayed re-discovery; this link endpoint has already
> @@ -1263,6 +1265,7 @@ void tipc_node_check_dest(struct net *net, u32 addr,
>   		n->signature = signature;
>   		accept_addr = true;
>   		*respond = true;
> +		reset = true;
>   	}
>   
>   	if (!accept_addr)
> @@ -1291,6 +1294,7 @@ void tipc_node_check_dest(struct net *net, u32 addr,
>   		tipc_link_fsm_evt(l, LINK_RESET_EVT);
>   		if (n->state == NODE_FAILINGOVER)
>   			tipc_link_fsm_evt(l, LINK_FAILOVER_BEGIN_EVT);
> +		link_is_reset = tipc_link_is_reset(l);
>   		le->link = l;
>   		n->link_cnt++;
>   		tipc_node_calculate_timer(n, l);
> @@ -1303,7 +1307,7 @@ void tipc_node_check_dest(struct net *net, u32 addr,
>   	memcpy(&le->maddr, maddr, sizeof(*maddr));
>   exit:
>   	tipc_node_write_unlock(n);
> -	if (reset && l && !tipc_link_is_reset(l))
> +	if (reset && !link_is_reset)
>   		tipc_node_link_down(n, b->identity, false);
>   	tipc_node_put(n);
>   }

Acked-by: Jon Maloy <jm...@re...>
Nice job!