December 2016 - dev - lists.frrouting.org

[cmaster-next] [PATCH] bgpd: Fix route node unlock when clearing adj-out
by Donald Sharp 02 Dec '16

02 Dec '16

From: vivek <vivek(a)cumulusnetworks.com> When clearing the adj-out for a subgroup (e.g., upon peer going down), ensure that the adj-out is removed before unlocking the route node that it points to, otherwise, there is a possibility that the route node may be prematurely freed. Signed-off-by: Vivek Venkatraman <vivek(a)cumulusnetworks.com> Ticket: CM-13690 Reviewed By: None Testing Done: bgp-smoke, resilient-hash tests (cherry picked from commit c3d7d35f3b7152c9e50e52c3cfd60b0dc52df703) --- bgpd/bgp_updgrp_adv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c index c485e61..a95a11b 100644 --- a/bgpd/bgp_updgrp_adv.c +++ b/bgpd/bgp_updgrp_adv.c @@ -573,8 +573,8 @@ subgroup_clear_table (struct update_subgroup *subgrp) SUBGRP_FOREACH_ADJ_SAFE (subgrp, aout, taout) { - bgp_unlock_node (aout->rn); bgp_adj_out_remove_subgroup (aout->rn, aout, subgrp); + bgp_unlock_node (aout->rn); } } -- 2.5.5

1 0

[cmaster-next] [PATCH] ospfd: Fix json Crash with inactive timer
by Donald Sharp 02 Dec '16

02 Dec '16

When nbr->t_inactivity is not active, and you do a show json over the neighbor it will crash ospfd. Fix the code so it prints out -1 when the timer is inactive. Ticket:CM-13835 Signed-off-by: Donald Sharp <sharpd(a)cumulusnetworks.com> Reviewed-by: Don Slice <dslice(a)cumulusnetworks.com> Reviewed-by: Daniel Walton <dwalton(a)cumulusnetworks.com> (cherry picked from commit 78d8fcb9623ab4d9cebf6187a451448e056a84bf) --- ospfd/ospf_vty.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/ospfd/ospf_vty.c b/ospfd/ospf_vty.c index 43a4434..4eeb5c1 100644 --- a/ospfd/ospf_vty.c +++ b/ospfd/ospf_vty.c @@ -4590,10 +4590,15 @@ show_ip_ospf_neighbor_detail_sub (struct vty *vty, struct ospf_interface *oi, /* Show Router Dead interval timer. */ if (use_json) { - struct timeval res = tv_sub (nbr->t_inactivity->u.sands, recent_relative_time ()); - unsigned long time_store = 0; - time_store = (1000 * res.tv_sec) + (res.tv_usec / 1000); - json_object_int_add(json_sub, "routerDeadIntervalTimerDueMsec", time_store); + if (nbr->t_inactivity) + { + struct timeval res = tv_sub (nbr->t_inactivity->u.sands, recent_relative_time ()); + unsigned long time_store = 0; + time_store = (1000 * res.tv_sec) + (res.tv_usec / 1000); + json_object_int_add(json_sub, "routerDeadIntervalTimerDueMsec", time_store); + } + else + json_object_int_add(json_sub, "routerDeadIntervalTimerDueMsec", -1); } else vty_out (vty, " Dead timer due in %s%s", -- 2.5.5

1 0

[cmaster-next] CLI plans
by David Lamparter 01 Dec '16

01 Dec '16

Hi all, Quentin and I just had a chat about future CLI plans. Here's a summary for everyone else: - the parser for CLI commands currently merges them straight into the common graph of all CLI commands. This'll be split off into an extra step: (pseudocode) graph = graph_new() command_parse_format(graph, cmd->str) command_merge_graph(graph, target_graph) The idea here is that the separate merge step makes it easier to * handle errors * undo the installation * do other graph transformations before merging - I'm finishing up on keyword specification {keyword OPTION|other A.B.C.D} - it's mostly done and works. There's nothing special for keywords, they're just selections that loop on themselves, allowing multiple uses. - for keyword arg lists one can use by-name references to *fixed* args (e.g. argv_find("keyword")). - by-name references to *variables* (WORD, A.B.C.D) will be pending until we get the merge split off, after which this is a new graph transformation step. - Quentin is looking at tests/, the testcli case is exhibiting some "interesting" behaviour. They're mostly help-string related and some of these may end up just being changes in design. - I have autocomplete for {interface, route-map, etc.} names mostly done, but only in-daemon, not vtysh. I'll push that out; adding vtysh support is a matter of adding a communication channel for completions, e.g. a hidden command. Whoever wants to can do that :) - Help string bloat reduction is complicated and we don't quite have a plan there yet. - Improving the way "no" commands work is on the long-term horizon. For now they need another DEFUN in most cases. Cheers, -David

1 0

[cmaster-next] [PATCH] bgpd: fix invalid memory access in peer_free()
by Renato Westphal 01 Dec '16

01 Dec '16

We shoult not call bgp_unlock() before calling bgp_delete_connected_nexthop() in the peer_free() function. Otherwise, if bgp->lock reaches zero, bgp_free() is called and peer->bgp becomes an invalid pointer in the bgp_delete_connected_nexthop() function. To fix this, move the call to bgp_unlock() to the end of peer_free(). Bug exposed by commit 37d361e ("bgpd: plug several memleaks"). Signed-off-by: Renato Westphal <renato(a)opensourcerouting.org> --- bgpd/bgpd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 22d4dd8..d5aff84 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -1019,8 +1019,6 @@ peer_free (struct peer *peer) { assert (peer->status == Deleted); - bgp_unlock(peer->bgp); - /* this /ought/ to have been done already through bgp_stop earlier, * but just to be sure.. */ @@ -1092,6 +1090,8 @@ peer_free (struct peer *peer) bfd_info_free(&(peer->bfd_info)); + bgp_unlock(peer->bgp); + memset (peer, 0, sizeof (struct peer)); XFREE (MTYPE_BGP_PEER, peer); -- 1.9.1

3 3

[cmaster-next] [PATCH 0/2] Minor stable/2.0 patches
by Donald Sharp 01 Dec '16

01 Dec '16

These 2 patches fix issue #7. Donald Sharp (2): ospfd: Fix memory leak when not using json pimd: Fix large integer display of drpriority ospfd/ospf_vty.c | 6 ++++-- pimd/pim_vty.c | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) -- 2.5.5

2 1

[cmaster-next] link-params sub mode
by Donald Sharp 01 Dec '16

01 Dec '16

Olivier - We've noticed that link-params creates allot of config lines when you configure any part of it. Would you mind if I change the behavior to this: When link-params is configured it auto starts displaying 6000-02# conf t dell-s6000-02(config)# int swp1 dell-s6000-02(config-if)# link-params dell-s6000-02(config-link-params)# admin-grp 0x12345678 dell-s6000-02(config-link-params)# end dell-s6000-02# show run interface swp1 link-params enable metric 0 <----Remove the metric and bw lines max-bw 1.25e+06 max-rsv-bw 1.25e+06 unrsv-bw 0 1.25e+06 unrsv-bw 1 1.25e+06 unrsv-bw 2 1.25e+06 unrsv-bw 3 1.25e+06 unrsv-bw 4 1.25e+06 unrsv-bw 5 1.25e+06 unrsv-bw 6 1.25e+06 unrsv-bw 7 1.25e+06 admin-grp 305419896 exit-link-params ! I'd like to reduce this to: interface swp1 link-params enable admin-grp 0x12345678 <----- Fix this to be what we entered exit-link-params ! Finally do we need to display the enable line as well? donald

3 8

[cmaster-next] Topology Tests added to CI - and new BGPd crash discovered
by Martin Winter 01 Dec '16

01 Dec '16

(Testing the new mailing list as well) First of all: I finally got around and added some framework for Topology Tests based on Mininet to the CI testbed. As Mininet is all based on Python, the whole framework is based on pytest. At this time, there are 2 tests in it: - OSPFv3 simple topology - BGP Multiview topology The git for this setup is here: https://git.netdef.org/scm/netdef/topotests.git There is no proprietary code in it - all can be easy replicated on a Ubuntu 16.04 (I have not tested any other Distros and Mininet requires Linux). Just follow the directions in the README inside the git and you should be able to replicate the setup. The idea is to have some test setup which can be shared and anyone can add tests (The CI system runs all the tests in this git) Happy to hear any feedback on this setup… As a side note, the BGP Multiview test crashes BGPd on startup. Because of this, I have NOT YET enabled the test results parsing in the CI system (or all builds would fail at this time) If you look at the the latest build on our CI system, you’ll see a crash in BGPd See https://ci1.netdef.org/browse/QUAGGA-CMASTERNEXT-TOPOU1604-106 Seems to be a crash in the VNC code. (cc’ing Lou to make sure he see this…) Again, please provide feedback. The idea is to add many more topologies to the CI system, but would like to hear some feedback on the setup. - Martin

3 2

[cmaster-next] [PATCH] bgpd: Fix multiple bgp view vnc crash
by Donald Sharp 01 Dec '16

01 Dec '16

When configuring multiple bgp views without a default bgp session, the code will crash. This code change only fixes the crash from happening. I'm not entirely sure that the code will not misbehave with multiple bgp views and configuring vnc in this case. Signed-off-by: Donald Sharp <sharpd(a)cumulusnetworks.com> --- bgpd/rfapi/vnc_import_bgp.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c index 4215ce2..441bdee 100644 --- a/bgpd/rfapi/vnc_import_bgp.c +++ b/bgpd/rfapi/vnc_import_bgp.c @@ -1798,6 +1798,9 @@ vnc_import_bgp_exterior_add_route_it ( struct bgp *bgp_default = bgp_get_default (); afi_t afi = family2afi (prefix->family); + if (!bgp_default) + return; + h = bgp_default->rfapi; hc = bgp_default->rfapi_cfg; @@ -1992,6 +1995,9 @@ vnc_import_bgp_exterior_del_route ( afi_t afi = family2afi (prefix->family); struct bgp *bgp_default = bgp_get_default (); + if (!bgp_default) + return; + memset (&pfx_orig_nexthop, 0, sizeof (struct prefix)); /* keep valgrind happy */ h = bgp_default->rfapi; -- 2.5.5

3 4