From 5cc74ec1b87f0ebf7e0c38c4da9fe5d9b3966ff9 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sat, 28 Oct 2017 17:40:13 -0400 Subject: [PATCH 1/8] eigrpd: Create a function to return a string of prefix state Signed-off-by: Donald Sharp --- eigrpd/eigrp_fsm.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/eigrpd/eigrp_fsm.c b/eigrpd/eigrp_fsm.c index 29357c2b24..86914316b8 100644 --- a/eigrpd/eigrp_fsm.c +++ b/eigrpd/eigrp_fsm.c @@ -170,6 +170,24 @@ struct { }, }; +static const char *prefix_state2str(enum eigrp_fsm_states state) +{ + switch (state) { + case EIGRP_FSM_STATE_PASSIVE: + return "Passive"; + case EIGRP_FSM_STATE_ACTIVE_0: + return "Active oij0"; + case EIGRP_FSM_STATE_ACTIVE_1: + return "Active oij1"; + case EIGRP_FSM_STATE_ACTIVE_2: + return "Active oij2"; + case EIGRP_FSM_STATE_ACTIVE_3: + return "Active oij3"; + } + + return "Unknown"; +} + /* * Main function in which are make decisions which event occurred. * msg - argument of type struct eigrp_fsm_action_message contain @@ -331,9 +349,9 @@ static int eigrp_get_fsm_event(struct eigrp_fsm_action_message *msg) int eigrp_fsm_event(struct eigrp_fsm_action_message *msg) { int event = eigrp_get_fsm_event(msg); - zlog_info("EIGRP AS: %d State: %d Event: %d Network: %s", - msg->eigrp->AS, msg->prefix->state, event, - eigrp_topology_ip_string(msg->prefix)); + zlog_info("EIGRP AS: %d State: %s Event: %d Network: %s", + msg->eigrp->AS, prefix_state2str(msg->prefix->state), + event, eigrp_topology_ip_string(msg->prefix)); (*(NSM[msg->prefix->state][event].func))(msg); return 1; From 94ccb30979b7a289da6f4fb95fc52307e8057162 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sat, 28 Oct 2017 18:04:19 -0400 Subject: [PATCH 2/8] eigrpd: Create enum for states and string name for display Create an enum for the different states and create a string name display handler. Signed-off-by: Donald Sharp --- eigrpd/eigrp_const.h | 39 +++++++++++++++++++++++++++++++-------- eigrpd/eigrp_fsm.c | 34 ++++++++++++++++++++++++++++++---- 2 files changed, 61 insertions(+), 12 deletions(-) diff --git a/eigrpd/eigrp_const.h b/eigrpd/eigrp_const.h index 3fa59756b7..c5f6e64d0b 100644 --- a/eigrpd/eigrp_const.h +++ b/eigrpd/eigrp_const.h @@ -153,14 +153,37 @@ enum eigrp_fsm_states { #define EIGRP_FSM_NEED_QUERY 2 /*EIGRP FSM events*/ -#define EIGRP_FSM_EVENT_NQ_FCN 0 /*input event other than query from succ, FC not satisfied*/ -#define EIGRP_FSM_EVENT_LR 1 /*last reply, FD is reset*/ -#define EIGRP_FSM_EVENT_Q_FCN 2 /*query from succ, FC not satisfied*/ -#define EIGRP_FSM_EVENT_LR_FCS 3 /*last reply, FC satisfied with current value of FDij*/ -#define EIGRP_FSM_EVENT_DINC 4 /*distance increase while in active state*/ -#define EIGRP_FSM_EVENT_QACT 5 /*query from succ while in active state*/ -#define EIGRP_FSM_EVENT_LR_FCN 6 /*last reply, FC not satisfied with current value of FDij*/ -#define EIGRP_FSM_KEEP_STATE 7 /*state not changed, usually by receiving not last reply */ +enum eigrp_fsm_events { + /* + * Input event other than query from succ, + * FC is not satisified + */ + EIGRP_FSM_EVENT_NQ_FCN, + + /* last reply, FD is reset */ + EIGRP_FSM_EVENT_LR, + + /* Query from succ, FC not satisfied */ + EIGRP_FSM_EVENT_Q_FCN, + + /* last reply, FC satisifed with current value of FDij */ + EIGRP_FSM_EVENT_LR_FCS, + + /* distance increase while in a active state */ + EIGRP_FSM_EVENT_DINC, + + /* Query from succ while in active state */ + EIGRP_FSM_EVENT_QACT, + + /* last reply, FC not satisified */ + EIGRP_FSM_EVENT_LR_FCN, + + /* + * state not changed + * usually by receiving not last reply + */ + EIGRP_FSM_KEEP_STATE, +}; /** * External routes originate from some other protocol - these are them diff --git a/eigrpd/eigrp_fsm.c b/eigrpd/eigrp_fsm.c index 86914316b8..84f21013e4 100644 --- a/eigrpd/eigrp_fsm.c +++ b/eigrpd/eigrp_fsm.c @@ -188,6 +188,29 @@ static const char *prefix_state2str(enum eigrp_fsm_states state) return "Unknown"; } +static const char *fsm_state2str(enum eigrp_fsm_events event) +{ + switch (event) { + case EIGRP_FSM_KEEP_STATE: + return "Keep State Event"; + case EIGRP_FSM_EVENT_NQ_FCN: + return "Non Query Event Feasability not satisfied"; + case EIGRP_FSM_EVENT_LR: + return "Last Reply Event"; + case EIGRP_FSM_EVENT_Q_FCN: + return "Query Event Feasability not satisified"; + case EIGRP_FSM_EVENT_LR_FCS: + return "Last Reply Event Feasability satisified"; + case EIGRP_FSM_EVENT_DINC: + return "Distance Increase Event"; + case EIGRP_FSM_EVENT_QACT: + return "Query from Successor while in active state"; + case EIGRP_FSM_EVENT_LR_FCN: + return "Last Reply Event, Feasibility not satisfied"; + }; + + return "Unknown"; +} /* * Main function in which are make decisions which event occurred. * msg - argument of type struct eigrp_fsm_action_message contain @@ -196,7 +219,8 @@ static const char *prefix_state2str(enum eigrp_fsm_states state) * Return number of occurred event (arrow in diagram). * */ -static int eigrp_get_fsm_event(struct eigrp_fsm_action_message *msg) +static enum eigrp_fsm_events eigrp_get_fsm_event( + struct eigrp_fsm_action_message *msg) { // Loading base information from message // struct eigrp *eigrp = msg->eigrp; @@ -348,10 +372,12 @@ static int eigrp_get_fsm_event(struct eigrp_fsm_action_message *msg) */ int eigrp_fsm_event(struct eigrp_fsm_action_message *msg) { - int event = eigrp_get_fsm_event(msg); - zlog_info("EIGRP AS: %d State: %s Event: %d Network: %s", + enum eigrp_fsm_events event = eigrp_get_fsm_event(msg); + + zlog_info("EIGRP AS: %d State: %s Event: %s Network: %s", msg->eigrp->AS, prefix_state2str(msg->prefix->state), - event, eigrp_topology_ip_string(msg->prefix)); + fsm_state2str(event), + eigrp_topology_ip_string(msg->prefix)); (*(NSM[msg->prefix->state][event].func))(msg); return 1; From 895722db4e7d09750006e6e17099b15b42a0470f Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sat, 28 Oct 2017 18:35:56 -0400 Subject: [PATCH 3/8] eigrpd: Add ability to show packet type in log Allow us to examine the packet type that caused us to change state. Signed-off-by: Donald Sharp --- eigrpd/eigrp_fsm.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/eigrpd/eigrp_fsm.c b/eigrpd/eigrp_fsm.c index 84f21013e4..44fdbf7d1b 100644 --- a/eigrpd/eigrp_fsm.c +++ b/eigrpd/eigrp_fsm.c @@ -170,6 +170,30 @@ struct { }, }; +static const char *packet_type2str(u_char packet_type) +{ + if (packet_type == EIGRP_OPC_UPDATE) + return "Update"; + if (packet_type == EIGRP_OPC_REQUEST) + return "Request"; + if (packet_type == EIGRP_OPC_QUERY) + return "Query"; + if (packet_type == EIGRP_OPC_REPLY) + return "Reply"; + if (packet_type == EIGRP_OPC_HELLO) + return "Hello"; + if (packet_type == EIGRP_OPC_IPXSAP) + return "IPXSAP"; + if (packet_type == EIGRP_OPC_ACK) + return "Ack"; + if (packet_type == EIGRP_OPC_SIAQUERY) + return "SIA Query"; + if (packet_type == EIGRP_OPC_SIAREPLY) + return "SIA Reply"; + + return "Unknown"; +} + static const char *prefix_state2str(enum eigrp_fsm_states state) { switch (state) { @@ -374,10 +398,12 @@ int eigrp_fsm_event(struct eigrp_fsm_action_message *msg) { enum eigrp_fsm_events event = eigrp_get_fsm_event(msg); - zlog_info("EIGRP AS: %d State: %s Event: %s Network: %s", + zlog_info("EIGRP AS: %d State: %s Event: %s Network: %s Packet Type: %s Reply RIJ Count: %d", msg->eigrp->AS, prefix_state2str(msg->prefix->state), fsm_state2str(event), - eigrp_topology_ip_string(msg->prefix)); + eigrp_topology_ip_string(msg->prefix), + packet_type2str(msg->packet_type), + msg->prefix->rij->count); (*(NSM[msg->prefix->state][event].func))(msg); return 1; From 377f30c31fa07db75b316088bcd172d3e1182eaf Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sat, 28 Oct 2017 18:45:08 -0400 Subject: [PATCH 4/8] eigrpd: Fix an issue found with metric change A past commit modified the change value to an enum but did not bother to fix all the places where change was used. Fix this. Additionally add some more output to the fsm prefix string about the change. Signed-off-by: Donald Sharp --- eigrpd/eigrp_fsm.c | 28 ++++++++++++++++++++++++---- eigrpd/eigrp_structs.h | 1 + 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/eigrpd/eigrp_fsm.c b/eigrpd/eigrp_fsm.c index 44fdbf7d1b..b4978bc06f 100644 --- a/eigrpd/eigrp_fsm.c +++ b/eigrpd/eigrp_fsm.c @@ -235,6 +235,20 @@ static const char *fsm_state2str(enum eigrp_fsm_events event) return "Unknown"; } + +static const char *change2str(enum metric_change change) +{ + switch (change) { + case METRIC_DECREASE: + return "Decrease"; + case METRIC_SAME: + return "Same"; + case METRIC_INCREASE: + return "Increase"; + } + + return "Unknown"; +} /* * Main function in which are make decisions which event occurred. * msg - argument of type struct eigrp_fsm_action_message contain @@ -267,6 +281,9 @@ static enum eigrp_fsm_events eigrp_get_fsm_event( */ change = eigrp_topology_update_distance(msg); + /* Store for display later */ + msg->change = change; + switch (actual_state) { case EIGRP_FSM_STATE_PASSIVE: { struct eigrp_nexthop_entry *head = @@ -331,7 +348,8 @@ static enum eigrp_fsm_events eigrp_get_fsm_event( zlog_info("All reply received\n"); return EIGRP_FSM_EVENT_LR; } - } else if (msg->packet_type == EIGRP_OPC_UPDATE && change == 1 + } else if (msg->packet_type == EIGRP_OPC_UPDATE + && change == METRIC_INCREASE && (entry->flags & EIGRP_NEXTHOP_ENTRY_SUCCESSOR_FLAG)) { return EIGRP_FSM_EVENT_DINC; @@ -376,7 +394,8 @@ static enum eigrp_fsm_events eigrp_get_fsm_event( zlog_info("All reply received\n"); return EIGRP_FSM_EVENT_LR; } - } else if (msg->packet_type == EIGRP_OPC_UPDATE && change == 1 + } else if (msg->packet_type == EIGRP_OPC_UPDATE + && change == METRIC_INCREASE && (entry->flags & EIGRP_NEXTHOP_ENTRY_SUCCESSOR_FLAG)) { return EIGRP_FSM_EVENT_DINC; @@ -398,12 +417,13 @@ int eigrp_fsm_event(struct eigrp_fsm_action_message *msg) { enum eigrp_fsm_events event = eigrp_get_fsm_event(msg); - zlog_info("EIGRP AS: %d State: %s Event: %s Network: %s Packet Type: %s Reply RIJ Count: %d", + zlog_info("EIGRP AS: %d State: %s Event: %s Network: %s Packet Type: %s Reply RIJ Count: %d change: %s", msg->eigrp->AS, prefix_state2str(msg->prefix->state), fsm_state2str(event), eigrp_topology_ip_string(msg->prefix), packet_type2str(msg->packet_type), - msg->prefix->rij->count); + msg->prefix->rij->count, + change2str(msg->change)); (*(NSM[msg->prefix->state][event].func))(msg); return 1; diff --git a/eigrpd/eigrp_structs.h b/eigrpd/eigrp_structs.h index 324181c21e..aae56c8ffe 100644 --- a/eigrpd/eigrp_structs.h +++ b/eigrpd/eigrp_structs.h @@ -499,6 +499,7 @@ struct eigrp_fsm_action_message { struct eigrp_prefix_entry *prefix; msg_data_t data_type; // internal or external tlv type struct eigrp_metrics metrics; + enum metric_change change; }; #endif /* _ZEBRA_EIGRP_STRUCTURES_H_ */ From 052fe05405f5a6df2573a296c89f42a1d703eca7 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sat, 28 Oct 2017 18:56:34 -0400 Subject: [PATCH 5/8] eigrpd: On shutdown, delete list after we've cleared prefixes Signed-off-by: Donald Sharp --- eigrpd/eigrpd.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/eigrpd/eigrpd.c b/eigrpd/eigrpd.c index 42d398458e..fd7a233238 100644 --- a/eigrpd/eigrpd.c +++ b/eigrpd/eigrpd.c @@ -284,14 +284,15 @@ void eigrp_finish_final(struct eigrp *eigrp) list_delete_and_null(&eigrp->eiflist); list_delete_and_null(&eigrp->oi_write_q); - list_delete_and_null(&eigrp->topology_changes_externalIPV4); - list_delete_and_null(&eigrp->topology_changes_internalIPV4); eigrp_topology_cleanup(eigrp->topology_table); eigrp_topology_free(eigrp->topology_table); eigrp_nbr_delete(eigrp->neighbor_self); + list_delete_and_null(&eigrp->topology_changes_externalIPV4); + list_delete_and_null(&eigrp->topology_changes_internalIPV4); + eigrp_delete(eigrp); XFREE(MTYPE_EIGRP_TOP, eigrp); From 68b7dd07d5676967259c116d6187ec095d8bc278 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 29 Oct 2017 08:27:08 -0400 Subject: [PATCH 6/8] eigrpd: Query Send is not incrementing the sequence number When we send a query make sure we increment the query sequence number. Signed-off-by: Donald Sharp --- eigrpd/eigrp_query.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/eigrpd/eigrp_query.c b/eigrpd/eigrp_query.c index 88a592e6c9..58f3d2b53c 100644 --- a/eigrpd/eigrp_query.c +++ b/eigrpd/eigrp_query.c @@ -183,13 +183,14 @@ void eigrp_send_query(struct eigrp_interface *ei) has_tlv = 0; for (ALL_LIST_ELEMENTS(ei->eigrp->topology_changes_internalIPV4, node, nnode, pe)) { - if (pe->req_action & EIGRP_FSM_NEED_QUERY) { - length += eigrp_add_internalTLV_to_stream(ep->s, pe); - for (ALL_LIST_ELEMENTS(ei->nbrs, node2, nnode2, nbr)) { - if (nbr->state == EIGRP_NEIGHBOR_UP) { - listnode_add(pe->rij, nbr); - has_tlv = 1; - } + if (!(pe->req_action & EIGRP_FSM_NEED_QUERY)) + continue; + + length += eigrp_add_internalTLV_to_stream(ep->s, pe); + for (ALL_LIST_ELEMENTS(ei->nbrs, node2, nnode2, nbr)) { + if (nbr->state == EIGRP_NEIGHBOR_UP) { + listnode_add(pe->rij, nbr); + has_tlv = 1; } } } @@ -212,6 +213,7 @@ void eigrp_send_query(struct eigrp_interface *ei) /*This ack number we await from neighbor*/ ep->sequence_number = ei->eigrp->sequence_number; + ei->eigrp->sequence_number++; for (ALL_LIST_ELEMENTS(ei->nbrs, node2, nnode2, nbr)) { if (nbr->state == EIGRP_NEIGHBOR_UP) { From b42a4a099a105c0e12b30c27c820b27afbcf1871 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 29 Oct 2017 08:28:01 -0400 Subject: [PATCH 7/8] eigrpd: Fix crash in reply receive packet. When we receive a reply for a prefix we no longer have we should note the issue and move on instead of crashing eigrp. Signed-off-by: Donald Sharp --- eigrpd/eigrp_reply.c | 81 ++++++++++++++++++++++++-------------------- 1 file changed, 44 insertions(+), 37 deletions(-) diff --git a/eigrpd/eigrp_reply.c b/eigrpd/eigrp_reply.c index 8dbd1a5b35..0ccffde72b 100644 --- a/eigrpd/eigrp_reply.c +++ b/eigrpd/eigrp_reply.c @@ -149,49 +149,56 @@ void eigrp_reply_receive(struct eigrp *eigrp, struct ip *iph, while (s->endp > s->getp) { type = stream_getw(s); - if (type == EIGRP_TLV_IPv4_INT) { - struct prefix dest_addr; - stream_set_getp(s, s->getp - sizeof(u_int16_t)); + if (type != EIGRP_TLV_IPv4_INT) + continue; - tlv = eigrp_read_ipv4_tlv(s); + struct prefix dest_addr; - dest_addr.family = AF_INET; - dest_addr.u.prefix4 = tlv->destination; - dest_addr.prefixlen = tlv->prefix_length; - struct eigrp_prefix_entry *dest = - eigrp_topology_table_lookup_ipv4( - eigrp->topology_table, &dest_addr); - /* - * Destination must exists - */ - assert(dest); + stream_set_getp(s, s->getp - sizeof(u_int16_t)); - struct eigrp_fsm_action_message msg; - struct eigrp_nexthop_entry *entry = - eigrp_prefix_entry_lookup(dest->entries, nbr); + tlv = eigrp_read_ipv4_tlv(s); - if (eigrp_update_prefix_apply(eigrp, ei, - EIGRP_FILTER_IN, - &dest_addr)) { - tlv->metric.delay = EIGRP_MAX_METRIC; - } - /* - * End of filtering - */ - - msg.packet_type = EIGRP_OPC_REPLY; - msg.eigrp = eigrp; - msg.data_type = EIGRP_INT; - msg.adv_router = nbr; - msg.metrics = tlv->metric; - msg.entry = entry; - msg.prefix = dest; - eigrp_fsm_event(&msg); - - - eigrp_IPv4_InternalTLV_free(tlv); + dest_addr.family = AF_INET; + dest_addr.u.prefix4 = tlv->destination; + dest_addr.prefixlen = tlv->prefix_length; + struct eigrp_prefix_entry *dest = + eigrp_topology_table_lookup_ipv4( + eigrp->topology_table, &dest_addr); + /* + * Destination must exists + */ + if (!dest) { + char buf[PREFIX_STRLEN]; + zlog_err("%s: Received prefix %s which we do not know about", + __PRETTY_FUNCTION__, + prefix2str(&dest_addr, buf, strlen(buf))); + continue; } + + struct eigrp_fsm_action_message msg; + struct eigrp_nexthop_entry *entry = + eigrp_prefix_entry_lookup(dest->entries, nbr); + + if (eigrp_update_prefix_apply(eigrp, ei, + EIGRP_FILTER_IN, + &dest_addr)) { + tlv->metric.delay = EIGRP_MAX_METRIC; + } + /* + * End of filtering + */ + + msg.packet_type = EIGRP_OPC_REPLY; + msg.eigrp = eigrp; + msg.data_type = EIGRP_INT; + msg.adv_router = nbr; + msg.metrics = tlv->metric; + msg.entry = entry; + msg.prefix = dest; + eigrp_fsm_event(&msg); + + eigrp_IPv4_InternalTLV_free(tlv); } eigrp_hello_send_ack(nbr); } From d4395853e8af1e21c86d4d9227e702640af58ab1 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 29 Oct 2017 08:51:57 -0400 Subject: [PATCH 8/8] eigrpd: When writing packet don't crash in some cases When we are writing a packet if we have gotten ourselves into a bad situation, note it and move on. Hopefully dumping enough information so that we can find the offending reason. Signed-off-by: Donald Sharp --- eigrpd/eigrp_packet.c | 15 +++++++++++++-- eigrpd/eigrp_query.c | 5 ++--- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/eigrpd/eigrp_packet.c b/eigrpd/eigrp_packet.c index 83ff194729..5b54f81326 100644 --- a/eigrpd/eigrp_packet.c +++ b/eigrpd/eigrp_packet.c @@ -344,8 +344,18 @@ int eigrp_write(struct thread *thread) /* Get one packet from queue. */ ep = eigrp_fifo_next(ei->obuf); - assert(ep); - assert(ep->length >= EIGRP_HEADER_LEN); + if (!ep) { + zlog_err("%s: Interface %s no packet on queue?", + __PRETTY_FUNCTION__, ei->ifp->name); + goto out; + } + if (ep->length < EIGRP_HEADER_LEN) { + zlog_err("%s: Packet just has a header?", + __PRETTY_FUNCTION__); + eigrp_header_dump((struct eigrp_header *)ep->s->data); + eigrp_packet_delete(ei); + goto out; + } if (ep->dst.s_addr == htonl(EIGRP_MULTICAST_ADDRESS)) eigrp_if_ipmulticast(eigrp, ei->address, ei->ifp->ifindex); @@ -442,6 +452,7 @@ int eigrp_write(struct thread *thread) /* Now delete packet from queue. */ eigrp_packet_delete(ei); +out: if (eigrp_fifo_next(ei->obuf) == NULL) { ei->on_write_q = 0; list_delete_node(eigrp->oi_write_q, node); diff --git a/eigrpd/eigrp_query.c b/eigrpd/eigrp_query.c index 58f3d2b53c..caa37870a1 100644 --- a/eigrpd/eigrp_query.c +++ b/eigrpd/eigrp_query.c @@ -165,7 +165,7 @@ void eigrp_send_query(struct eigrp_interface *ei) struct listnode *node, *nnode, *node2, *nnode2; struct eigrp_neighbor *nbr; struct eigrp_prefix_entry *pe; - char has_tlv; + bool has_tlv = false; bool ep_saved = false; ep = eigrp_packet_new(ei->ifp->mtu, NULL); @@ -180,7 +180,6 @@ void eigrp_send_query(struct eigrp_interface *ei) length += eigrp_add_authTLV_MD5_to_stream(ep->s, ei); } - has_tlv = 0; for (ALL_LIST_ELEMENTS(ei->eigrp->topology_changes_internalIPV4, node, nnode, pe)) { if (!(pe->req_action & EIGRP_FSM_NEED_QUERY)) @@ -190,7 +189,7 @@ void eigrp_send_query(struct eigrp_interface *ei) for (ALL_LIST_ELEMENTS(ei->nbrs, node2, nnode2, nbr)) { if (nbr->state == EIGRP_NEIGHBOR_UP) { listnode_add(pe->rij, nbr); - has_tlv = 1; + has_tlv = true;; } } }