Re: [tipc-discussion] [net] tipc: fix link session and re-establish issues

[Amit J. <ami...@te...>]

Thanks a lot Tuong, for your detailed analysis, feedback and solution.

Regards,
Amit



On Mon, 11 Feb 2019, 12:00 Tuong Lien <tuo...@de... wrote:

> When a link endpoint is re-created (e.g. after a node reboot or
> interface reset), the link session number is varied by random, the peer
> endpoint will be synced with this new session number before the link is
> re-established.
>
> However, there is a shortcoming in this mechanism that can lead to the
> link never re-established or faced with a failure then. It happens when
> the peer endpoint is ready in ESTABLISHING state, the 'peer_session' as
> well as the 'in_session' flag have been set, but suddenly this link
> endpoint leaves. When it comes back with a random session number, there
> are two situations possible:
>
> 1/ If the random session number is larger than (or equal to) the
> previous one, the peer endpoint will be updated with this new session
> upon receipt of a RESET_MSG from this endpoint, and the link can be re-
> established as normal. Otherwise, all the RESET_MSGs from this endpoint
> will be rejected by the peer. In turn, when this link endpoint receives
> one ACTIVATE_MSG from the peer, it will move to ESTABLISHED and start
> to send STATE_MSGs, but again these messages will be dropped by the
> peer due to wrong session.
> The peer link endpoint can still become ESTABLISHED after receiving a
> traffic message from this endpoint (e.g. a BCAST_PROTOCOL or
> NAME_DISTRIBUTOR), but since all the STATE_MSGs are invalid, the link
> will be forced down sooner or later!
>
> Even in case the random session number is larger than the previous one,
> it can be that the ACTIVATE_MSG from the peer arrives first, and this
> link endpoint moves quickly to ESTABLISHED without sending out any
> RESET_MSG yet. Consequently, the peer link will not be updated with the
> new session number, and the same link failure scenario as above will
> happen.
>
> 2/ Another situation can be that, the peer link endpoint was reset due
> to any reasons in the meantime, its link state was set to RESET from
> ESTABLISHING but still in session, i.e. the 'in_session' flag is not
> reset...
> Now, if the random session number from this endpoint is less than the
> previous one, all the RESET_MSGs from this endpoint will be rejected by
> the peer. In the other direction, when this link endpoint receives a
> RESET_MSG from the peer, it moves to ESTABLISHING and starts to send
> ACTIVATE_MSGs, but all these messages will be rejected by the peer too.
> As a result, the link cannot be re-established but gets stuck with this
> link endpoint in state ESTABLISHING and the peer in RESET!
>
> Solution:
> ===========
>
> This link endpoint should not go directly to ESTABLISHED when getting
> ACTIVATE_MSG from the peer which may belong to the old session if the
> link was re-created. To ensure the session to be correct before the
> link is re-established, the peer endpoint in ESTABLISHING state will
> send back the last session number in ACTIVATE_MSG for a verification at
> this endpoint. Then, if needed, a new and more appropriate session
> number will be regenerated to force a re-synch first.
>
> In addition, when a link in ESTABLISHING state is reset, its state will
> move to RESET according to the link FSM, along with resetting the
> 'in_session' flag (and the other data) as a normal link reset, it will
> also be deleted if requested.
>
> The solution is backward compatible.
>
> Acked-by: Jon Maloy <jon...@er...>
> Acked-by: Ying Xue <yin...@wi...>
> Signed-off-by: Tuong Lien <tuo...@de...>
> ---
>  net/tipc/link.c | 15 +++++++++++++++
>  net/tipc/msg.h  | 22 ++++++++++++++++++++++
>  net/tipc/node.c | 11 ++++++-----
>  3 files changed, 43 insertions(+), 5 deletions(-)
>
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index ac306d17f8ad..631e21cd4256 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -1425,6 +1425,10 @@ static void tipc_link_build_proto_msg(struct
> tipc_link *l, int mtyp, bool probe,
>                 l->rcv_unacked = 0;
>         } else {
>                 /* RESET_MSG or ACTIVATE_MSG */
> +               if (mtyp == ACTIVATE_MSG) {
> +                       msg_set_dest_session_valid(hdr, 1);
> +                       msg_set_dest_session(hdr, l->peer_session);
> +               }
>                 msg_set_max_pkt(hdr, l->advertised_mtu);
>                 strcpy(data, l->if_name);
>                 msg_set_size(hdr, INT_H_SIZE + TIPC_MAX_IF_NAME);
> @@ -1642,6 +1646,17 @@ static int tipc_link_proto_rcv(struct tipc_link *l,
> struct sk_buff *skb,
>                         rc = tipc_link_fsm_evt(l, LINK_FAILURE_EVT);
>                         break;
>                 }
> +
> +               /* If this endpoint was re-created while peer was
> ESTABLISHING
> +                * it doesn't know current session number. Force re-synch.
> +                */
> +               if (mtyp == ACTIVATE_MSG && msg_dest_session_valid(hdr) &&
> +                   l->session != msg_dest_session(hdr)) {
> +                       if (less(l->session, msg_dest_session(hdr)))
> +                               l->session = msg_dest_session(hdr) + 1;
> +                       break;
> +               }
> +
>                 /* ACTIVATE_MSG serves as PEER_RESET if link is already
> down */
>                 if (mtyp == RESET_MSG || !link_is_up(l))
>                         rc = tipc_link_fsm_evt(l, LINK_PEER_RESET_EVT);
> diff --git a/net/tipc/msg.h b/net/tipc/msg.h
> index a0924956bb61..d7e4b8b93f9d 100644
> --- a/net/tipc/msg.h
> +++ b/net/tipc/msg.h
> @@ -360,6 +360,28 @@ static inline void msg_set_bcast_ack(struct tipc_msg
> *m, u16 n)
>         msg_set_bits(m, 1, 0, 0xffff, n);
>  }
>
> +/* Note: reusing bits in word 1 for ACTIVATE_MSG only, to re-synch
> + * link peer session number
> + */
> +static inline bool msg_dest_session_valid(struct tipc_msg *m)
> +{
> +       return msg_bits(m, 1, 16, 0x1);
> +}
> +
> +static inline void msg_set_dest_session_valid(struct tipc_msg *m, bool
> valid)
> +{
> +       msg_set_bits(m, 1, 16, 0x1, valid);
> +}
> +
> +static inline u16 msg_dest_session(struct tipc_msg *m)
> +{
> +       return msg_bits(m, 1, 0, 0xffff);
> +}
> +
> +static inline void msg_set_dest_session(struct tipc_msg *m, u16 n)
> +{
> +       msg_set_bits(m, 1, 0, 0xffff, n);
> +}
>
>  /*
>   * Word 2
> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index db2a6c3e0be9..2dc4919ab23c 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -830,15 +830,16 @@ static void tipc_node_link_down(struct tipc_node *n,
> int bearer_id, bool delete)
>         tipc_node_write_lock(n);
>         if (!tipc_link_is_establishing(l)) {
>                 __tipc_node_link_down(n, &bearer_id, &xmitq, &maddr);
> -               if (delete) {
> -                       kfree(l);
> -                       le->link = NULL;
> -                       n->link_cnt--;
> -               }
>         } else {
>                 /* Defuse pending tipc_node_link_up() */
> +               tipc_link_reset(l);
>                 tipc_link_fsm_evt(l, LINK_RESET_EVT);
>         }
> +       if (delete) {
> +               kfree(l);
> +               le->link = NULL;
> +               n->link_cnt--;
> +       }
>         trace_tipc_node_link_down(n, true, "node link down or deleted!");
>         tipc_node_write_unlock(n);
>         if (delete)
> --
> 2.13.7
>
>
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>