]> git.proxmox.com Git - mirror_frr.git/commitdiff
Merge pull request #2099 from qlyoung/fix-cpu-thread-hist-race
authorRuss White <russ@riw.us>
Tue, 24 Apr 2018 11:59:06 +0000 (07:59 -0400)
committerGitHub <noreply@github.com>
Tue, 24 Apr 2018 11:59:06 +0000 (07:59 -0400)
lib: fix data race in thread history collection

17 files changed:
bgpd/bgp_zebra.c
isisd/dict.c
lib/frr_zmq.c
lib/graph.c
lib/graph.h
lib/nexthop_group.c
lib/subdir.am
lib/table.h
lib/thread.c
pbrd/pbr_vty.c
pbrd/pbr_zebra.c
tests/.gitignore
tests/Makefile.am
tests/lib/test_graph.c [new file with mode: 0644]
tests/lib/test_graph.py [new file with mode: 0644]
tests/lib/test_graph.refout [new file with mode: 0644]
zebra/redistribute.c

index b564fccf436bf014e42f9507e63913dff36f7f81..1c3229174049ff1112f7dca7775f03e90a622d0b 100644 (file)
@@ -1379,25 +1379,6 @@ void bgp_zebra_withdraw(struct prefix *p, struct bgp_info *info,
                        struct bgp *bgp, safi_t safi)
 {
        struct zapi_route api;
-       struct peer *peer;
-
-       peer = info->peer;
-       assert(peer);
-
-       if (info->type == ZEBRA_ROUTE_BGP
-           && info->sub_type == BGP_ROUTE_IMPORTED) {
-
-               struct bgp_info *bi;
-
-               /*
-                * Look at parent chain for peer sort
-                */
-               for (bi = info; bi->extra && bi->extra->parent;
-                    bi = bi->extra->parent) {
-
-                       peer = ((struct bgp_info *)(bi->extra->parent))->peer;
-               }
-       }
 
        /* Don't try to install if we're not connected to Zebra or Zebra doesn't
         * know of this instance.
@@ -1416,16 +1397,6 @@ void bgp_zebra_withdraw(struct prefix *p, struct bgp_info *info,
        if (is_route_parent_evpn(info))
                SET_FLAG(api.flags, ZEBRA_FLAG_EVPN_ROUTE);
 
-       if (peer->sort == BGP_PEER_IBGP) {
-               SET_FLAG(api.flags, ZEBRA_FLAG_ALLOW_RECURSION);
-               SET_FLAG(api.flags, ZEBRA_FLAG_IBGP);
-       }
-
-       if ((peer->sort == BGP_PEER_EBGP && peer->ttl != 1)
-           || CHECK_FLAG(peer->flags, PEER_FLAG_DISABLE_CONNECTED_CHECK)
-           || bgp_flag_check(bgp, BGP_FLAG_DISABLE_NH_CONNECTED_CHK))
-               SET_FLAG(api.flags, ZEBRA_FLAG_ALLOW_RECURSION);
-
        if (bgp_debug_zebra(p)) {
                char buf[PREFIX_STRLEN];
 
index 2ea86d1b68bdb80655f4b450bd086cb0c831e74d..20a4c0ff7383fe31d7199482ff186d797b2bf2e1 100644 (file)
@@ -1323,9 +1323,8 @@ static void construct(dict_t *d)
                                free(val);
                                if (dn)
                                        dnode_destroy(dn);
-                       }
-
-                       dict_load_next(&dl, dn, key);
+                       } else
+                               dict_load_next(&dl, dn, key);
                        break;
                default:
                        putchar('?');
index d4df5130e7e33e40d04da993e9f4fec731896cb6..8f190a3a09c713c42ea5bbd135520962d3962b40 100644 (file)
@@ -338,6 +338,7 @@ void frrzmq_check_events(struct frrzmq_cb **cbp, struct cb_core *core,
        if (!cb || !cb->zmqsock)
                return;
 
+       len = sizeof(events);
        if (zmq_getsockopt(cb->zmqsock, ZMQ_EVENTS, &events, &len))
                return;
        if (events & event && core->thread && !core->cancelled) {
index a9cc43f7c1e5d4933256c32d1c3acbb33ea44ff6..a9e35b46ff3339315854af4b98eb79d35b15199f 100644 (file)
@@ -23,6 +23,7 @@
 #include <zebra.h>
 #include "graph.h"
 #include "memory.h"
+#include "buffer.h"
 
 DEFINE_MTYPE_STATIC(LIB, GRAPH, "Graph")
 DEFINE_MTYPE_STATIC(LIB, GRAPH_NODE, "Graph Node")
@@ -157,3 +158,73 @@ bool graph_has_edge(struct graph_node *from, struct graph_node *to)
 
        return false;
 }
+
+static void _graph_dfs(struct graph *graph, struct graph_node *start,
+                      vector visited,
+                      void (*dfs_cb)(struct graph_node *, void *), void *arg)
+{
+       /* check that we have not visited this node */
+       for (unsigned int i = 0; i < vector_active(visited); i++) {
+               if (start == vector_slot(visited, i))
+                       return;
+       }
+
+       /* put this node in visited stack */
+       vector_ensure(visited, vector_active(visited));
+       vector_set_index(visited, vector_active(visited), start);
+
+       /* callback */
+       dfs_cb(start, arg);
+
+       /* recurse into children */
+       for (unsigned int i = vector_active(start->to); i--; /**/) {
+               struct graph_node *c = vector_slot(start->to, i);
+
+               _graph_dfs(graph, c, visited, dfs_cb, arg);
+       }
+}
+
+void graph_dfs(struct graph *graph, struct graph_node *start,
+              void (*dfs_cb)(struct graph_node *, void *), void *arg)
+{
+       vector visited = vector_init(VECTOR_MIN_SIZE);
+
+       _graph_dfs(graph, start, visited, dfs_cb, arg);
+       vector_free(visited);
+}
+
+#ifndef BUILDING_CLIPPY
+
+void graph_dump_dot_default_print_cb(struct graph_node *gn, struct buffer *buf)
+{
+       char nbuf[64];
+
+       for (unsigned int i = 0; i < vector_active(gn->to); i++) {
+               struct graph_node *adj = vector_slot(gn->to, i);
+
+               snprintf(nbuf, sizeof(nbuf), "    n%p -> n%p;\n", gn, adj);
+               buffer_putstr(buf, nbuf);
+       }
+}
+
+char *graph_dump_dot(struct graph *graph, struct graph_node *start,
+                    void (*pcb)(struct graph_node *, struct buffer *))
+{
+       struct buffer *buf = buffer_new(0);
+       char *ret;
+
+       pcb = (pcb) ? pcb : graph_dump_dot_default_print_cb;
+       buffer_putstr(buf, "digraph {\n");
+
+       graph_dfs(graph, start, (void (*)(struct graph_node *, void *))pcb,
+                 buf);
+
+       buffer_putstr(buf, "}\n");
+
+       ret = buffer_getstr(buf);
+       buffer_free(buf);
+
+       return ret;
+}
+
+#endif /* BUILDING_CLIPPY */
index d6dfef5a636a65be815ca09328b25d3dbee478ac..87262a07b88757318ed1a1ce01d3a4e71c472a7f 100644 (file)
@@ -26,6 +26,7 @@
 
 #include <stdbool.h>
 #include "vector.h"
+#include "buffer.h"
 
 struct graph {
        vector nodes;
@@ -111,4 +112,56 @@ struct graph_node *graph_find_node(struct graph *graph, void *data);
  */
 bool graph_has_edge(struct graph_node *from, struct graph_node *to);
 
+/*
+ * Depth-first search.
+ *
+ * Performs a depth-first traversal of the given graph, visiting each node
+ * exactly once and calling the user-provided callback for each visit.
+ *
+ * @param graph the graph to operate on
+ * @param start the node to take as the root
+ * @param dfs_cb callback called for each node visited in the traversal
+ * @param arg argument to provide to dfs_cb
+ */
+void graph_dfs(struct graph *graph, struct graph_node *start,
+              void (*dfs_cb)(struct graph_node *, void *), void *arg);
+
+#ifndef BUILDING_CLIPPY
+/*
+ * Clippy relies on a small subset of sources in lib/, but it cannot link
+ * libfrr since clippy itself is required to build libfrr. Instead it directly
+ * includes the sources it needs. One of these is the command graph
+ * implementation, which wraps this graph implementation. Since we need to use
+ * the buffer.[ch] sources here, which indirectly rely on most of libfrr, we
+ * have to ignore them when compiling clippy to avoid build dependency issues.
+ *
+ * TODO: Fix clippy build.
+ */
+
+/*
+ * Default node printer for use with graph_dump_dot.
+ *
+ * @param gn the node to print
+ * @param buf the buffer to print into
+ */
+void graph_dump_dot_default_print_cb(struct graph_node *gn, struct buffer *buf);
+
+/*
+ * Prints a graph in the DOT language.
+ *
+ * The generated output is produced from a depth-first traversal of the graph.
+ *
+ * @param graph the graph to print
+ * @param start the node to take as the root
+ * @param pcb callback called for each node in the traversal that should
+ *        print the node in the DOT language. Passing NULL for this argument
+ *        will use the default printer. See graph_dump_dot_default_print_cb for
+ *        an example.
+ * @return representation of graph in DOT language, allocated with MTYPE_TMP.
+ *         Caller is responsible for freeing this string.
+ */
+char *graph_dump_dot(struct graph *graph, struct graph_node *start,
+                    void (*pcb)(struct graph_node *, struct buffer *buf));
+
+#endif /* BUILDING_CLIPPY */
 #endif /* _ZEBRA_COMMAND_GRAPH_H */
index 5ac38d66853cc91a09aa4ebca1ff008c8ba00ad5..937b84bdddf6c5af6d671b41a2cf3e08d5eb523c 100644 (file)
@@ -387,6 +387,13 @@ DEFPY(ecmp_nexthops, ecmp_nexthops_cmd,
        struct nexthop *nh;
        bool legal;
 
+       /*
+        * This is impossible to happen as that the cli parser refuses
+        * to let you get here without an addr, but the SA system
+        * does not understand this intricacy
+        */
+       assert(addr);
+
        legal = nexthop_group_parse_nexthop(&nhop, addr, intf, name);
 
        if (nhop.type == NEXTHOP_TYPE_IPV6
index 3b469d4524216d46df56019de6cc9b3f22a857b4..c5719786d62a02732dcdd0386bbd17316cd1dca7 100644 (file)
@@ -171,6 +171,7 @@ pkginclude_HEADERS += \
        lib/pbr.h \
        # end
 
+
 nodist_pkginclude_HEADERS += \
        lib/route_types.h \
        lib/version.h \
@@ -232,7 +233,7 @@ lib_grammar_sandbox_SOURCES = \
 lib_grammar_sandbox_LDADD = \
        lib/libfrr.la
 
-lib_clippy_CPPFLAGS = $(AM_CPPFLAGS) -D_GNU_SOURCE
+lib_clippy_CPPFLAGS = $(AM_CPPFLAGS) -D_GNU_SOURCE -DBUILDING_CLIPPY
 lib_clippy_CFLAGS = $(PYTHON_CFLAGS)
 lib_clippy_LDADD = $(PYTHON_LIBS)
 lib_clippy_SOURCES = \
index 9637fec1499ccb2e07a916cbab5e870adeaf0ded..a9d788b35a07d6970554a7e365ac43668afc0e28 100644 (file)
@@ -24,6 +24,7 @@
 
 #include "memory.h"
 #include "hash.h"
+#include "prefix.h"
 DECLARE_MTYPE(ROUTE_TABLE)
 DECLARE_MTYPE(ROUTE_NODE)
 
index 45bdbc71d3b2da05a748151803feb8770fa1ad51..f9ff16b7b3cd560b89642e944554f1e7ec88c441 100644 (file)
@@ -157,7 +157,7 @@ static void cpu_record_print(struct vty *vty, uint8_t filter)
 
                        char underline[strlen(name) + 1];
                        memset(underline, '-', sizeof(underline));
-                       underline[sizeof(underline)] = '\0';
+                       underline[sizeof(underline) - 1] = '\0';
 
                        vty_out(vty, "\n");
                        vty_out(vty, "Showing statistics for pthread %s\n",
index 475ad86b5885b8a77cd13b4df0bdc207b8aee312..03f2104835ff46463fb12dee2614a65d115a7536 100644 (file)
@@ -227,6 +227,11 @@ DEFPY(pbr_map_nexthop, pbr_map_nexthop_cmd,
        memset(&nhop, 0, sizeof(nhop));
        nhop.vrf_id = vrf->vrf_id;
 
+       /*
+        * Make SA happy.  CLIPPY is not going to give us a NULL
+        * addr.
+        */
+       assert(addr);
        if (addr->sa.sa_family == AF_INET) {
                nhop.gate.ipv4.s_addr = addr->sin.sin_addr.s_addr;
                if (intf) {
index 4e5b5f3dde90d1142a59935b77251190063f53b8..bc7dd2083213916b08d483741e65bec8795d5357 100644 (file)
@@ -60,7 +60,8 @@ struct pbr_interface *pbr_if_new(struct interface *ifp)
                return 0;
        }
 
-       return (pbr_ifp);
+       ifp->info = pbr_ifp;
+       return pbr_ifp;
 }
 
 /* Inteface addition message from zebra. */
@@ -74,12 +75,8 @@ static int interface_add(int command, struct zclient *zclient,
        if (!ifp)
                return 0;
 
-       if (!ifp->info) {
-               struct pbr_interface *pbr_ifp;
-
-               pbr_ifp = pbr_if_new(ifp);
-               ifp->info = pbr_ifp;
-       }
+       if (!ifp->info)
+               pbr_if_new(ifp);
 
        return 0;
 }
@@ -494,7 +491,7 @@ void pbr_send_pbr_map(struct pbr_map_sequence *pbrms,
 {
        struct pbr_map *pbrm = pbrms->parent;
        struct stream *s;
-       uint64_t is_installed = 1 << pmi->install_bit;
+       uint64_t is_installed = (uint64_t)1 << pmi->install_bit;
 
        is_installed &= pbrms->installed;
 
index c2fe9c8890298bb93016585834b7b20098421095..1708a4b7b064e9ce7c3bfb1db2a76faf2355bbf1 100644 (file)
@@ -50,5 +50,7 @@ __pycache__
 /lib/test_timer_performance
 /lib/test_ttable
 /lib/test_zmq
+/lib/test_zlog
+/lib/test_graph
 /ospf6d/test_lsdb
 /ospf6d/test_lsdb_clippy.c
index 0c9a5684dad2c18806882988c6a0a5fd93dfca36..703c1d05fc0c9280dc8fecb7808707ae353046b4 100644 (file)
@@ -74,6 +74,7 @@ check_PROGRAMS = \
        lib/test_timer_performance \
        lib/test_ttable \
        lib/test_zlog \
+       lib/test_graph \
        lib/cli/test_cli \
        lib/cli/test_commands \
        $(TESTS_BGPD) \
@@ -129,6 +130,7 @@ lib_test_timer_performance_SOURCES = lib/test_timer_performance.c \
                                      helpers/c/prng.c
 lib_test_ttable_SOURCES = lib/test_ttable.c
 lib_test_zlog_SOURCES = lib/test_zlog.c
+lib_test_graph_SOURCES = lib/test_graph.c
 lib_test_zmq_SOURCES = lib/test_zmq.c
 lib_test_zmq_CFLAGS = $(AM_CFLAGS) $(ZEROMQ_CFLAGS)
 lib_cli_test_cli_SOURCES = lib/cli/test_cli.c lib/cli/common_cli.c
@@ -170,6 +172,7 @@ lib_test_timer_correctness_LDADD = $(ALL_TESTS_LDADD)
 lib_test_timer_performance_LDADD = $(ALL_TESTS_LDADD)
 lib_test_ttable_LDADD = $(ALL_TESTS_LDADD)
 lib_test_zlog_LDADD = $(ALL_TESTS_LDADD)
+lib_test_graph_LDADD = $(ALL_TESTS_LDADD)
 lib_test_zmq_LDADD = ../lib/libfrrzmq.la $(ALL_TESTS_LDADD) $(ZEROMQ_LIBS)
 lib_cli_test_cli_LDADD = $(ALL_TESTS_LDADD)
 lib_cli_test_commands_LDADD = $(ALL_TESTS_LDADD)
@@ -211,6 +214,7 @@ EXTRA_DIST = \
     lib/test_ttable.py \
     lib/test_ttable.refout \
     lib/test_zlog.py \
+    lib/test_graph.py \
     ospf6d/test_lsdb.py \
     ospf6d/test_lsdb.in \
     ospf6d/test_lsdb.refout \
diff --git a/tests/lib/test_graph.c b/tests/lib/test_graph.c
new file mode 100644 (file)
index 0000000..f21f8b7
--- /dev/null
@@ -0,0 +1,77 @@
+/*
+ * Test graph data structure.
+ * Copyright (C) 2018  Cumulus Networks, Inc.
+ *                     Quentin Young
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; see the file COPYING; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+#include <zebra.h>
+#include <graph.h>
+#include <memory.h>
+#include <buffer.h>
+
+#define NUMNODES 32
+
+static void graph_custom_print_cb(struct graph_node *gn, struct buffer *buf)
+{
+       char nbuf[64];
+       char *gname = gn->data;
+
+       for (unsigned int i = 0; i < vector_active(gn->to); i++) {
+               struct graph_node *adj = vector_slot(gn->to, i);
+               char *name = adj->data;
+
+               snprintf(nbuf, sizeof(nbuf), "    n%s -> n%s;\n", gname, name);
+               buffer_putstr(buf, nbuf);
+       }
+}
+
+int main(int argc, char **argv)
+{
+       struct graph *g = graph_new();
+       struct graph_node *gn[NUMNODES];
+       char names[NUMNODES][16];
+
+       /* create vertices */
+       for (unsigned int i = 0; i < NUMNODES; i++) {
+               snprintf(names[i], sizeof(names[i]), "%d", i);
+               gn[i] = graph_new_node(g, names[i], NULL);
+       }
+
+       /* create edges */
+       for (unsigned int i = 1; i < NUMNODES - 1; i++) {
+               graph_add_edge(gn[0], gn[i]);
+               graph_add_edge(gn[i], gn[i + 1]);
+       }
+       graph_add_edge(gn[0], gn[NUMNODES - 1]);
+       graph_add_edge(gn[NUMNODES - 1], gn[1]);
+
+       /* print DOT */
+       char *dumped = graph_dump_dot(g, gn[0], graph_custom_print_cb);
+
+       fprintf(stdout, "%s", dumped);
+       XFREE(MTYPE_TMP, dumped);
+
+       /* remove some edges */
+       for (unsigned int i = NUMNODES - 1; i > NUMNODES / 2; --i)
+               for (unsigned int j = 0; j < NUMNODES; j++)
+                       graph_remove_edge(gn[i], gn[j]);
+
+       /* remove some nodes */
+       for (unsigned int i = 0; i < NUMNODES / 2; i++)
+               graph_delete_node(g, gn[i]);
+
+       graph_delete_graph(g);
+}
diff --git a/tests/lib/test_graph.py b/tests/lib/test_graph.py
new file mode 100644 (file)
index 0000000..697e56c
--- /dev/null
@@ -0,0 +1,4 @@
+import frrtest
+
+class TestGraph(frrtest.TestRefOut):
+    program = './test_graph'
diff --git a/tests/lib/test_graph.refout b/tests/lib/test_graph.refout
new file mode 100644 (file)
index 0000000..955f552
--- /dev/null
@@ -0,0 +1,64 @@
+digraph {
+    n0 -> n1;
+    n0 -> n2;
+    n0 -> n3;
+    n0 -> n4;
+    n0 -> n5;
+    n0 -> n6;
+    n0 -> n7;
+    n0 -> n8;
+    n0 -> n9;
+    n0 -> n10;
+    n0 -> n11;
+    n0 -> n12;
+    n0 -> n13;
+    n0 -> n14;
+    n0 -> n15;
+    n0 -> n16;
+    n0 -> n17;
+    n0 -> n18;
+    n0 -> n19;
+    n0 -> n20;
+    n0 -> n21;
+    n0 -> n22;
+    n0 -> n23;
+    n0 -> n24;
+    n0 -> n25;
+    n0 -> n26;
+    n0 -> n27;
+    n0 -> n28;
+    n0 -> n29;
+    n0 -> n30;
+    n0 -> n31;
+    n31 -> n1;
+    n1 -> n2;
+    n2 -> n3;
+    n3 -> n4;
+    n4 -> n5;
+    n5 -> n6;
+    n6 -> n7;
+    n7 -> n8;
+    n8 -> n9;
+    n9 -> n10;
+    n10 -> n11;
+    n11 -> n12;
+    n12 -> n13;
+    n13 -> n14;
+    n14 -> n15;
+    n15 -> n16;
+    n16 -> n17;
+    n17 -> n18;
+    n18 -> n19;
+    n19 -> n20;
+    n20 -> n21;
+    n21 -> n22;
+    n22 -> n23;
+    n23 -> n24;
+    n24 -> n25;
+    n25 -> n26;
+    n26 -> n27;
+    n27 -> n28;
+    n28 -> n29;
+    n29 -> n30;
+    n30 -> n31;
+}
index a51cd6ecc94342c5600e12497e4c0b02ddce8f13..1d66653e74580f47c5622a7c39d7f73b21480141 100644 (file)
@@ -113,18 +113,20 @@ static void zebra_redistribute(struct zserv *client, int type,
        for (rn = route_top(table); rn; rn = srcdest_route_next(rn))
                RNODE_FOREACH_RE (rn, newre) {
                        struct prefix *dst_p, *src_p;
+                       char buf[PREFIX_STRLEN];
+
                        srcdest_rnode_prefixes(rn, &dst_p, &src_p);
 
                        if (IS_ZEBRA_DEBUG_EVENT)
                                zlog_debug(
-                                       "%s: client %s vrf %d checking: selected=%d, type=%d, distance=%d, "
-                                       "zebra_check_addr=%d",
+                                       "%s: client %s %s(%d) checking: selected=%d, type=%d, distance=%d, metric=%d zebra_check_addr=%d",
                                        __func__,
                                        zebra_route_string(client->proto),
+                                       prefix2str(dst_p, buf, sizeof(buf)),
                                        vrf_id, CHECK_FLAG(newre->flags,
                                                           ZEBRA_FLAG_SELECTED),
                                        newre->type, newre->distance,
-                                       zebra_check_addr(dst_p));
+                                       newre->metric, zebra_check_addr(dst_p));
 
                        if (!CHECK_FLAG(newre->flags, ZEBRA_FLAG_SELECTED))
                                continue;
@@ -151,13 +153,13 @@ void redistribute_update(struct prefix *p, struct prefix *src_p,
        struct zserv *client;
        int send_redistribute;
        int afi;
-       char buf[INET6_ADDRSTRLEN];
+       char buf[PREFIX_STRLEN];
 
        if (IS_ZEBRA_DEBUG_RIB) {
-               inet_ntop(p->family, &p->u.prefix, buf, INET6_ADDRSTRLEN);
                zlog_debug(
-                       "%u:%s/%d: Redist update re %p (type %d), old %p (type %d)",
-                       re->vrf_id, buf, p->prefixlen, re, re->type, prev_re,
+                       "%u:%s: Redist update re %p (type %d), old %p (type %d)",
+                       re->vrf_id, prefix2str(p, buf, sizeof(buf)),
+                       re, re->type, prev_re,
                        prev_re ? prev_re->type : -1);
        }
 
@@ -187,6 +189,15 @@ void redistribute_update(struct prefix *p, struct prefix *src_p,
                        send_redistribute = 1;
 
                if (send_redistribute) {
+                       if (IS_ZEBRA_DEBUG_EVENT) {
+                               zlog_debug(
+                                          "%s: client %s %s(%d), type=%d, distance=%d, metric=%d",
+                                          __func__,
+                                          zebra_route_string(client->proto),
+                                          prefix2str(p, buf, sizeof(buf)),
+                                          re->vrf_id, re->type,
+                                          re->distance, re->metric);
+                       }
                        zsend_redistribute_route(ZEBRA_REDISTRIBUTE_ROUTE_ADD,
                                                 client, p, src_p, re);
                } else if (prev_re